fix(#5305): resolve child skills via SourceURL after base composition#5306
fix(#5305): resolve child skills via SourceURL after base composition#5306fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
When a URL-sourced harness has a base: field, LoadWithBase enters the base composition path. The base's resources (agent, policy, skills, scripts) are correctly resolved against the base URL. However, after mergeBaseIntoChild, the child's own relative resource paths (skills, agent, policy) were left unresolved — they remained as relative paths like "skills/pr-review" instead of being fetched from the source URL. This caused skill directories from the agents repo to be resolved against the local workspace (via ResolveRelativeTo) instead of being fetched from the source repo with their full directory trees. When the local workspace had only partial content (e.g., SKILL.md without sub-agents/ or meta-prompt.md), the agent received incomplete skills. The fix adds a post-composition resolution step that calls resolveBaseResources and resolveBaseHostFiles against opts.SourceURL after base merging. Both functions already skip absolute paths and URLs (fields already resolved from the base), so they are safe to call on the merged harness. resolveBaseScripts is intentionally omitted because it rejects absolute paths as defense-in-depth — scripts inherited from the base are already resolved to cache paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
|
🤖 Finished Review · ✅ Success · Started 8:49 AM UTC · Completed 9:01 AM UTC |
Site previewPreview: https://f51f214f-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsLow
Labels: PR modifies internal/harness/ base composition logic |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| // that rejection. Scripts that the child overrides will be resolved | ||
| // later by ResolveRelativeTo (local paths are fine for scripts). | ||
| if opts.SourceURL != "" { | ||
| resourceDeps, err := resolveBaseResources(ctx, child, opts.SourceURL, opts.OrgAllowlist, opts) |
There was a problem hiding this comment.
[low] Allowlist inconsistency
The post-composition SourceURL resolution passes opts.OrgAllowlist to resolveBaseResources and resolveBaseHostFiles, but the loadBaseChain call on line 163 uses the local allowlist variable, which may have been augmented via allowSelfAllowlist (lines 152-153). When allowSelfAllowlist is true and OrgAllowlist is empty, allowlist is set to child.AllowedRemoteResources, but the new code would pass an empty allowlist. This is test-only and currently untriggerable since all tests provide a non-empty OrgAllowlist, but introduces an asymmetry that could cause confusing failures in future tests.
Suggested fix: Use the local allowlist variable (already computed at line 147) instead of opts.OrgAllowlist in both resolveBaseResources and resolveBaseHostFiles calls in the new block, matching what loadBaseChain uses on line 163.
fullsend-ai-review
Bot
added
ready-for-merge
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review squad pass (3 independent review agents). 2 findings survived verification and cross-checking against existing review history: one HIGH (the post-merge resolution step covers resources and host_files but not scripts, so a child's own script fields can still fall back to local resolution under base + SourceURL — the same bug class #5305 fixes, just for scripts) and one MEDIUM (test coverage gaps for host_files and mixed base+child skills lists in the new post-merge path). Full details inline. The existing allowlist-inconsistency comment on this PR already covers a related finding all three agents also independently surfaced, so it wasn't reposted.
| // resolveBaseScripts rejects absolute paths as defense-in-depth for | ||
| // URL-sourced bases — we skip it here since scripts inherited from | ||
| // the base are already resolved to cache paths and would trigger | ||
| // that rejection. Scripts that the child overrides will be resolved | ||
| // later by ResolveRelativeTo (local paths are fine for scripts). | ||
| if opts.SourceURL != "" { | ||
| resourceDeps, err := resolveBaseResources(ctx, child, opts.SourceURL, opts.OrgAllowlist, opts) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("resolving URL-sourced resources after base composition: %w", err) | ||
| } | ||
| deps = append(deps, resourceDeps...) | ||
|
|
||
| hostFileDeps, err := resolveBaseHostFiles(ctx, child, opts.SourceURL, opts.OrgAllowlist, opts) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("resolving URL-sourced host_files after base composition: %w", err) | ||
| } | ||
| deps = append(deps, hostFileDeps...) | ||
| } |
There was a problem hiding this comment.
[HIGH] Child-declared script fields bypass SourceURL resolution after base composition (reintroduces the #5305 bug class)
Finding: This block resolves resolveBaseResources and resolveBaseHostFiles against opts.SourceURL, but resolveBaseScripts is never called here. If the child itself (not the base) declares its own pre_script, post_script, validation_loop.script/schema, or a forge.<platform>.* script field in a harness that has both base: and a non-empty SourceURL, mergeBaseIntoChild leaves that value as the child's own relative path (child overrides base when non-zero), and this block never touches it. It falls through to Harness.ResolveRelativeTo, which joins any still-relative field against the local workspace directory, not SourceURL. That's the same class of bug #5305 describes, just for scripts instead of skills: either a confusing "no such file" failure, or, if a file coincidentally exists at that relative path in the local workspace being processed, silent execution of the wrong script.
The comment above explains why resolveBaseScripts can't simply be reused here — it rejects absolute paths via validateBaseRelPath before any IsURL/IsAbs check, unlike resolveBaseResources/resolveBaseHostFiles, which skip already-resolved fields before validating — but that only explains why the current implementation can't be called as-is; it doesn't address the resulting gap for child-declared scripts.
Neither new test exercises "base + SourceURL + child's own new script field." TestLoadWithBase_SourceURL_WithBase_AlreadyResolvedSkipped only checks that an already-absolute, base-inherited pre_script is left alone, which passes regardless of whether this gap exists.
Flagged independently by all 3 review agents dispatched for this PR (severity assessed as CRITICAL by one, HIGH by two — posting at HIGH).
Suggestion: Add the same if *f.ptr == "" || IsURL(*f.ptr) || filepath.IsAbs(*f.ptr) { continue } guard to resolveBaseScripts's per-field and forge-level loops (matching resolveBaseResources), then call it here too, symmetric with the no-base branch earlier in this function. Add a regression test with a URL-sourced, base-composed child declaring its own pre_script, asserting it resolves against SourceURL rather than staying relative.
| } | ||
| assert.True(t, fieldNames["base"], "should have base dep") | ||
| assert.True(t, fieldNames["skills[0]"], "should have skill dep") | ||
| } |
There was a problem hiding this comment.
[MEDIUM] New post-merge resolution path has no test coverage for host_files, and no mixed base+child skills case
Finding: The two new tests cover a child's own relative skills entry resolving via SourceURL post-merge, and already-absolute agent/pre_script being skipped. resolveBaseHostFiles — one of the field categories this fix targets — has zero coverage here: neither test declares a host_files entry, so the post-merge resolveBaseHostFiles call only ever exercises its empty-loop no-op. There's also no test for a child overriding agent/policy directly (only skills gets that treatment), and no test for a merged Skills slice containing both an already-resolved base entry and a still-relative child entry together, which is the actual shape produced whenever a base with its own skills is combined with a child that adds more.
Flagged independently by 2 of 3 review agents dispatched for this PR.
Suggestion: Add a test where the child declares its own host_files entry alongside a URL base (asserting it resolves to a cache path), and a test with a mixed base+child Skills slice to confirm the per-index IsAbs skip doesn't disturb already-resolved base entries.
1 participant
Summary
Related Issue
Closes #5305
Changes
internal/harness/compose.goLoadWithBase: aftermergeBaseIntoChild, callresolveBaseResourcesandresolveBaseHostFilesagainstopts.SourceURLto resolve the child's own relative pathsresolveBaseScriptsis intentionally omitted since it rejects absolute paths as defense-in-depth; scripts inherited from the base are already resolvedinternal/harness/compose_test.goTestLoadWithBase_SourceURL_WithBase_ResolvesChildSkills: verifies that a URL-sourced harness with a base field has its child skills resolved to cache paths with full directory trees (SKILL.md, meta-prompt.md, sub-agents/)TestLoadWithBase_SourceURL_WithBase_AlreadyResolvedSkipped: verifies that already-resolved absolute cache paths from the base are not re-processedTesting
go test ./internal/harness/passes (all 170+ tests)go test ./internal/...passes (pre-existingTestPortRestrictionfailure in sandbox is unrelated)go vet ./internal/harness/cleanscan-secretscleanChecklist
Generated by code agent from #5305
Closes #5305
Post-script verification
agent/5305-recursive-skill-directory-fetch)146ddfb63749aece5027def6e3a3a277b044a784..HEAD)