From b040c764baf2b1c5865f8b7502a8658d7804c990 Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Sun, 19 Jul 2026 08:46:40 +0000 Subject: [PATCH 1/2] fix(#5305): resolve child skills via SourceURL after base composition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/harness/compose.go | 40 ++++++- internal/harness/compose_test.go | 185 +++++++++++++++++++++++++++++++ 2 files changed, 222 insertions(+), 3 deletions(-) diff --git a/internal/harness/compose.go b/internal/harness/compose.go index 9270f5e06..cedf4010e 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -89,10 +89,11 @@ type ComposeOpts struct { // // Pipeline: // 1. LoadRaw(path) — preserves forge map -// 2. If base absent: ResolveForge → Validate → return +// 2. If base absent: resolve URL-sourced resources → ResolveForge → Validate → return // 3. If base present: loadBaseChain recursively, then mergeBaseIntoChild -// 4. ResolveForge once on final merged result -// 5. Validate +// 4. Resolve remaining URL-sourced resources (child's own relative paths) +// 5. ResolveForge once on final merged result +// 6. Validate // // When base is absent, this behaves identically to LoadWithOpts. func LoadWithBase(ctx context.Context, path string, opts ComposeOpts) (*Harness, []Dependency, error) { @@ -171,6 +172,39 @@ func LoadWithBase(ctx context.Context, path string, opts ComposeOpts) (*Harness, // Clear the base field (consumed) child.Base = "" + // When the harness was fetched from a URL, the child may still have + // relative resource paths (skills, agent, policy, host_files) that + // originated in the child harness — not the base. After merge, base + // resources are absolute cache paths but the child's own relative + // paths remain unresolved. Resolve them against the SourceURL now, + // the same way the no-base path does. + // + // Without this, relative skill paths like "skills/pr-review" would + // be resolved against the local workspace by ResolveRelativeTo, + // missing companion files (sub-agents/, meta-prompt.md) that only + // exist at the source URL. See #5305. + // + // resolveBaseResources and resolveBaseHostFiles already skip absolute + // paths and URLs, so they are safe to call on the merged harness. + // 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...) + } + // ResolveForge once on the merged result if err := child.validateForge(); err != nil { return nil, nil, fmt.Errorf("invalid harness: %w", err) diff --git a/internal/harness/compose_test.go b/internal/harness/compose_test.go index b3556d66e..b5565df44 100644 --- a/internal/harness/compose_test.go +++ b/internal/harness/compose_test.go @@ -3950,3 +3950,188 @@ func TestIsTransientFetchError(t *testing.T) { }) } } + +func TestLoadWithBase_SourceURL_WithBase_ResolvesChildSkills(t *testing.T) { + // When a URL-sourced harness has a base: field, the child's own + // relative skills should be resolved against the SourceURL after + // base composition — not left as relative paths for local resolution. + // This is the fix for #5305: without it, skills/pr-review would + // resolve against the local workspace, missing companion files + // (sub-agents/, meta-prompt.md) that only exist at the source URL. + + baseAgentContent := []byte("# base agent definition") + + baseContent := []byte(` +agent: agents/base.md +role: review +model: opus +`) + baseHash := computeHash(baseContent) + + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + + // Test server serves the base harness and its agent file. + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/base.yaml": + w.Write(baseContent) + case "/agents/base.md": + w.Write(baseAgentContent) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + policy := fetch.NewTestPolicy( + server.Client().Transport.(*http.Transport).TLSClientConfig, + []string{"127.0.0.1"}, + []string{server.Listener.Addr().String()[len("127.0.0.1:"):]}, + ) + + baseURL := server.URL + "/base.yaml#sha256=" + baseHash + + // The child harness: fetched from a URL, has a base field, and + // declares its own skills (relative paths to the source repo). + // It does NOT override agent — inherits from the base, which is + // already resolved to a cache path. + childHarness := fmt.Sprintf(` +base: %s +role: review +skills: + - skills/pr-review +`, baseURL) + + path := writeTestHarness(t, dir, "review.yaml", childHarness) + + // The source URL uses raw.githubusercontent.com format — this is + // what the agents repo fallback and config-registered agent paths + // produce. Skills are resolved relative to this URL's grandparent. + sourceURL := "https://raw.githubusercontent.com/org/agents/abc123/harness/review.yaml" + + // TreeFetcher returns skill files including subdirectories — + // exactly what git sparse checkout would return for a real repo. + fetcher := fakeTreeFetcher(map[string][]byte{ + "SKILL.md": []byte("# PR Review Skill"), + "meta-prompt.md": []byte("meta prompt content"), + "sub-agents/correctness.md": []byte("# correctness sub-agent"), + "sub-agents/security.md": []byte("# security sub-agent"), + }) + + allowlist := []string{ + server.URL + "/", + "https://raw.githubusercontent.com/org/agents/", + } + + h, deps, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: allowlist, + SourceURL: sourceURL, + TreeFetcher: fetcher, + allowSelfAllowlist: true, + }) + require.NoError(t, err) + + // The merged harness should have the review role from the child. + assert.Equal(t, "review", h.Role) + + // The child's skill should be resolved to a local cache path (not + // the relative "skills/pr-review" that would need local resolution). + require.Len(t, h.Skills, 1) + assert.True(t, filepath.IsAbs(h.Skills[0]), + "skill should be resolved to an absolute cache path, got %q", h.Skills[0]) + + // The cached skill directory should contain all files, including + // subdirectories (the fix for #5305). + skillDir := h.Skills[0] + assert.FileExists(t, filepath.Join(skillDir, "SKILL.md")) + assert.FileExists(t, filepath.Join(skillDir, "meta-prompt.md")) + assert.FileExists(t, filepath.Join(skillDir, "sub-agents", "correctness.md")) + assert.FileExists(t, filepath.Join(skillDir, "sub-agents", "security.md")) + + // Verify the sub-agent content is correct. + correctnessContent, err := os.ReadFile(filepath.Join(skillDir, "sub-agents", "correctness.md")) + require.NoError(t, err) + assert.Equal(t, "# correctness sub-agent", string(correctnessContent)) + + // Dependencies should include both the base fetch and the skill fetch. + assert.NotEmpty(t, deps) + fieldNames := map[string]bool{} + for _, d := range deps { + fieldNames[d.Field] = true + } + assert.True(t, fieldNames["base"], "should have base dep") + assert.True(t, fieldNames["skills[0]"], "should have skill dep") +} + +func TestLoadWithBase_SourceURL_WithBase_AlreadyResolvedSkipped(t *testing.T) { + // After base composition, some resources (e.g., agent, policy, + // scripts) may already be resolved to absolute cache paths from the + // base. The post-composition SourceURL resolution should skip these + // — they must not be re-fetched or rejected by validateBaseRelPath. + + agentContent := []byte("# base agent (will be resolved to cache path by base composition)") + + baseContent := []byte(` +agent: agents/base.md +role: review +model: opus +pre_script: scripts/pre.sh +`) + baseHash := computeHash(baseContent) + preScriptContent := []byte("#!/bin/bash\necho pre") + + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/base.yaml": + w.Write(baseContent) + case "/agents/base.md": + w.Write(agentContent) + case "/scripts/pre.sh": + w.Write(preScriptContent) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + policy := fetch.NewTestPolicy( + server.Client().Transport.(*http.Transport).TLSClientConfig, + []string{"127.0.0.1"}, + []string{server.Listener.Addr().String()[len("127.0.0.1:"):]}, + ) + + baseURL := server.URL + "/base.yaml#sha256=" + baseHash + + // The child declares no resources of its own — everything comes + // from the base. After composition, agent and pre_script are absolute + // cache paths. The SourceURL resolution must skip them. + childHarness := fmt.Sprintf(` +base: %s +role: review +`, baseURL) + + path := writeTestHarness(t, dir, "review.yaml", childHarness) + sourceURL := server.URL + "/harness/review.yaml" + allowlist := []string{server.URL + "/"} + + h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: allowlist, + SourceURL: sourceURL, + allowSelfAllowlist: true, + }) + require.NoError(t, err) + + // Agent and pre_script should be absolute cache paths from base resolution. + assert.True(t, filepath.IsAbs(h.Agent), + "agent should be an absolute cache path from base resolution, got %q", h.Agent) + assert.True(t, filepath.IsAbs(h.PreScript), + "pre_script should be an absolute cache path from base resolution, got %q", h.PreScript) +} From 55c26048ed52f177d24ed98b3c809080a51e8a22 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Wed, 22 Jul 2026 19:07:16 +0000 Subject: [PATCH 2/2] fix(harness): resolve child scripts via SourceURL after base composition Add skip guards to resolveBaseScripts so it can safely be called on merged harnesses where base-inherited scripts are already absolute cache paths. Call resolveBaseScripts in the post-merge SourceURL resolution block, closing the gap where child-declared script fields (pre_script, post_script, validation_loop) bypassed SourceURL resolution. Also fix allowlist inconsistency by using the local allowlist variable (which includes allowSelfAllowlist augmentation) instead of opts.OrgAllowlist. The initial skip guard used a bare filepath.IsAbs() check, matching resolveBaseResources's existing pattern for declarative fields (agent, policy, skills). For resolveBaseScripts's exec fields -- pre_script, post_script, validation_loop.script/.schema, and their forge. equivalents, all run directly via exec.Command on the host -- that let a URL/base-sourced harness declare an arbitrary absolute host path and have it silently accepted instead of rejected by validateBaseRelPath. Replace the shape-based check with isFullsendCachePath, which only skips a path already resolved inside fullsend's own content-addressed cache (/.fullsend-cache/...); any other absolute value still falls through to validateBaseRelPath and is rejected. resolveBaseResources (agent, policy, skills) and resolveBaseHostFiles (host_files.src) had the same bare filepath.IsAbs() skip and are reachable through this same post-merge block for the with-base case. Their fields aren't exec'd, but agent/policy content is read on the host and becomes the literal agent definition and sandbox policy, and host_files are read on the host and uploaded into the sandbox -- an arbitrary host path there is a disclosure risk, not just a resolution bug. Apply the same isFullsendCachePath check to both functions. Add regression tests for child scripts, child host_files, and mixed base+child Skills slices in the post-merge resolution path, plus coverage for the skip-vs-reject boundary in all three resolve functions (isFullsendCachePath unit tests, an absolute-cache-path skip test per field family) and for a child skill overriding a same-basename base skill (#5408) alongside post-merge SourceURL resolution. Existing tests that simulated "already resolved" fields with arbitrary (non-cache) absolute paths are updated to use genuine cache-shaped paths instead, since that shortcut is no longer valid input under the tightened check. Addresses review feedback on #5306 Assisted-by: Claude (fix, review), Grok (review) Signed-off-by: Wayne Sun --- internal/harness/compose.go | 102 ++-- internal/harness/compose_test.go | 776 +++++++++++++++++++++++++++++-- 2 files changed, 811 insertions(+), 67 deletions(-) diff --git a/internal/harness/compose.go b/internal/harness/compose.go index cedf4010e..dbcdbad89 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -91,7 +91,7 @@ type ComposeOpts struct { // 1. LoadRaw(path) — preserves forge map // 2. If base absent: resolve URL-sourced resources → ResolveForge → Validate → return // 3. If base present: loadBaseChain recursively, then mergeBaseIntoChild -// 4. Resolve remaining URL-sourced resources (child's own relative paths) +// 4. Resolve remaining URL-sourced resources and scripts (child's own relative paths) // 5. ResolveForge once on final merged result // 6. Validate // @@ -173,32 +173,34 @@ func LoadWithBase(ctx context.Context, path string, opts ComposeOpts) (*Harness, child.Base = "" // When the harness was fetched from a URL, the child may still have - // relative resource paths (skills, agent, policy, host_files) that - // originated in the child harness — not the base. After merge, base - // resources are absolute cache paths but the child's own relative + // relative resource paths (skills, agent, policy, host_files, scripts) + // that originated in the child harness — not the base. After merge, + // base resources are absolute cache paths but the child's own relative // paths remain unresolved. Resolve them against the SourceURL now, // the same way the no-base path does. // - // Without this, relative skill paths like "skills/pr-review" would - // be resolved against the local workspace by ResolveRelativeTo, - // missing companion files (sub-agents/, meta-prompt.md) that only - // exist at the source URL. See #5305. + // Without this, relative paths like "skills/pr-review" or + // "scripts/pre.sh" would be resolved against the local workspace by + // ResolveRelativeTo, missing companion files (sub-agents/, + // meta-prompt.md) that only exist at the source URL. See #5305. // - // resolveBaseResources and resolveBaseHostFiles already skip absolute - // paths and URLs, so they are safe to call on the merged harness. - // 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). + // All three resolve functions skip absolute paths and URLs, so they + // are safe to call on the merged harness where base-inherited fields + // are already resolved to cache paths. if opts.SourceURL != "" { - resourceDeps, err := resolveBaseResources(ctx, child, opts.SourceURL, opts.OrgAllowlist, opts) + scriptDeps, err := resolveBaseScripts(ctx, child, opts.SourceURL, allowlist, opts) + if err != nil { + return nil, nil, fmt.Errorf("resolving URL-sourced scripts after base composition: %w", err) + } + deps = append(deps, scriptDeps...) + + resourceDeps, err := resolveBaseResources(ctx, child, opts.SourceURL, allowlist, 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) + hostFileDeps, err := resolveBaseHostFiles(ctx, child, opts.SourceURL, allowlist, opts) if err != nil { return nil, nil, fmt.Errorf("resolving URL-sourced host_files after base composition: %w", err) } @@ -587,12 +589,39 @@ func mergeBaseIntoChild(base, child *Harness) { } } +// isFullsendCachePath reports whether p is an absolute path already inside +// fullsend's own content-addressed cache (/.fullsend-cache/...), +// as opposed to an absolute path written directly into untrusted harness +// content. Shape alone (filepath.IsAbs) can't tell these apart. Used by +// resolveBaseScripts, resolveBaseResources, and resolveBaseHostFiles to +// decide which absolute values are safe to skip (already resolved by an +// earlier step) versus which must still be rejected by validateBaseRelPath: +// an arbitrary host path in an exec field runs on the host via exec.Command, +// and one in agent/policy/host_files is read on the host and becomes the +// literal agent definition, sandbox policy, or an uploaded sandbox file — +// letting either come from an untrusted absolute path is a code-execution +// or disclosure risk, not just a resolution bug. +func isFullsendCachePath(p, workspaceRoot string) bool { + if !filepath.IsAbs(p) || workspaceRoot == "" { + return false + } + rel, err := filepath.Rel(filepath.Join(workspaceRoot, ".fullsend-cache"), p) + return err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + // resolveBaseScripts fetches script fields from a URL-referenced base harness. // For each script field (pre_script, post_script, validation_loop.script) that // is a non-empty relative path, the script is fetched from the base URL's // directory, cached content-addressed, and the field is rewritten to the local // cache path. Forge-level scripts are also resolved. agent_input is excluded // because runtime treats it as a directory (uploaded recursively). +// +// A field is skipped (left unchanged) when it's a URL or already resolved to +// a path inside fullsend's own cache (see isFullsendCachePath) — both cases +// mean an earlier step already handled it. Any other absolute path is +// untrusted and is rejected by validateBaseRelPath, since these fields are +// executed on the host, not just read as data. +// // Returns additional dependencies for the fetched scripts. func resolveBaseScripts(ctx context.Context, base *Harness, baseURL string, allowlist []string, opts ComposeOpts) ([]Dependency, error) { // Script paths in harness YAMLs are relative to the scaffold root (the @@ -617,7 +646,7 @@ func resolveBaseScripts(ctx context.Context, base *Harness, baseURL string, allo } for _, f := range scriptFields { - if *f.ptr == "" { + if *f.ptr == "" || IsURL(*f.ptr) || isFullsendCachePath(*f.ptr, opts.WorkspaceRoot) { continue } if err := validateBaseRelPath(f.name, *f.ptr); err != nil { @@ -631,7 +660,7 @@ func resolveBaseScripts(ctx context.Context, base *Harness, baseURL string, allo deps = append(deps, dep) } - if base.ValidationLoop != nil && base.ValidationLoop.Script != "" { + if base.ValidationLoop != nil && base.ValidationLoop.Script != "" && !IsURL(base.ValidationLoop.Script) && !isFullsendCachePath(base.ValidationLoop.Script, opts.WorkspaceRoot) { if err := validateBaseRelPath("validation_loop.script", base.ValidationLoop.Script); err != nil { return nil, err } @@ -642,7 +671,7 @@ func resolveBaseScripts(ctx context.Context, base *Harness, baseURL string, allo base.ValidationLoop.Script = cachePath deps = append(deps, dep) } - if base.ValidationLoop != nil && base.ValidationLoop.Schema != "" { + if base.ValidationLoop != nil && base.ValidationLoop.Schema != "" && !IsURL(base.ValidationLoop.Schema) && !isFullsendCachePath(base.ValidationLoop.Schema, opts.WorkspaceRoot) { if err := validateBaseRelPath("validation_loop.schema", base.ValidationLoop.Schema); err != nil { return nil, err } @@ -666,7 +695,7 @@ func resolveBaseScripts(ctx context.Context, base *Harness, baseURL string, allo {fmt.Sprintf("forge.%s.post_script", platform), &fc.PostScript}, } for _, f := range forgeScripts { - if *f.ptr == "" { + if *f.ptr == "" || IsURL(*f.ptr) || isFullsendCachePath(*f.ptr, opts.WorkspaceRoot) { continue } if err := validateBaseRelPath(f.name, *f.ptr); err != nil { @@ -679,7 +708,7 @@ func resolveBaseScripts(ctx context.Context, base *Harness, baseURL string, allo *f.ptr = cachePath deps = append(deps, dep) } - if fc.ValidationLoop != nil && fc.ValidationLoop.Script != "" { + if fc.ValidationLoop != nil && fc.ValidationLoop.Script != "" && !IsURL(fc.ValidationLoop.Script) && !isFullsendCachePath(fc.ValidationLoop.Script, opts.WorkspaceRoot) { fieldName := fmt.Sprintf("forge.%s.validation_loop.script", platform) if err := validateBaseRelPath(fieldName, fc.ValidationLoop.Script); err != nil { return nil, err @@ -691,7 +720,7 @@ func resolveBaseScripts(ctx context.Context, base *Harness, baseURL string, allo fc.ValidationLoop.Script = cachePath deps = append(deps, dep) } - if fc.ValidationLoop != nil && fc.ValidationLoop.Schema != "" { + if fc.ValidationLoop != nil && fc.ValidationLoop.Schema != "" && !IsURL(fc.ValidationLoop.Schema) && !isFullsendCachePath(fc.ValidationLoop.Schema, opts.WorkspaceRoot) { fieldName := fmt.Sprintf("forge.%s.validation_loop.schema", platform) if err := validateBaseRelPath(fieldName, fc.ValidationLoop.Schema); err != nil { return nil, err @@ -721,8 +750,12 @@ func resolveBaseScripts(ctx context.Context, base *Harness, baseURL string, allo // rewritten to the local cache path. For skills (directories), SKILL.md is // fetched and cached as a directory via CachePutDir, and the field is // rewritten to the cache tree directory. Fields that are already URLs or -// absolute paths are left unchanged — they will be handled by -// ResolveHarness in the caller. +// already resolved to a path inside fullsend's own cache (see +// isFullsendCachePath) are left unchanged — an earlier step already handled +// them. Any other absolute path is untrusted and is rejected by +// validateBaseRelPath: agent/policy content is read on the host and used as +// the literal agent definition / sandbox policy, so an arbitrary host path +// here is a disclosure risk, not just a resolution bug. func resolveBaseResources(ctx context.Context, base *Harness, baseURL string, allowlist []string, opts ComposeOpts) ([]Dependency, error) { baseURLDir := urlParentDirPrefix(baseURL) if baseURLDir == "" { @@ -740,7 +773,7 @@ func resolveBaseResources(ctx context.Context, base *Harness, baseURL string, al } for _, f := range fileFields { - if *f.ptr == "" || IsURL(*f.ptr) || filepath.IsAbs(*f.ptr) { + if *f.ptr == "" || IsURL(*f.ptr) || isFullsendCachePath(*f.ptr, opts.WorkspaceRoot) { continue } if err := validateBaseRelPath(f.name, *f.ptr); err != nil { @@ -755,7 +788,7 @@ func resolveBaseResources(ctx context.Context, base *Harness, baseURL string, al } for i, skill := range base.Skills { - if skill == "" || IsURL(skill) || filepath.IsAbs(skill) { + if skill == "" || IsURL(skill) || isFullsendCachePath(skill, opts.WorkspaceRoot) { continue } fieldName := fmt.Sprintf("skills[%d]", i) @@ -775,11 +808,14 @@ func resolveBaseResources(ctx context.Context, base *Harness, baseURL string, al // resolveBaseHostFiles fetches host_files with relative src paths from a // URL-referenced base harness. For each host_files entry whose src is a -// non-empty relative path (not a ${VAR} reference, URL, or absolute path), -// the file is fetched from the base URL's directory, cached content-addressed, -// and the src field is rewritten to the local cache path. This ensures -// host_files inherited through base: composition resolve correctly at sandbox -// setup time, the same way scripts and resources do. +// non-empty relative path (not a ${VAR} reference, URL, or a path already +// resolved into fullsend's own cache — see isFullsendCachePath), the file is +// fetched from the base URL's directory, cached content-addressed, and the +// src field is rewritten to the local cache path. This ensures host_files +// inherited through base: composition resolve correctly at sandbox setup +// time, the same way scripts and resources do. Any other absolute src is +// untrusted and rejected: host_files are read on the host and uploaded into +// the sandbox, so an arbitrary host path here is a disclosure risk. func resolveBaseHostFiles(ctx context.Context, base *Harness, baseURL string, allowlist []string, opts ComposeOpts) ([]Dependency, error) { baseURLDir := urlParentDirPrefix(baseURL) if baseURLDir == "" { @@ -790,7 +826,7 @@ func resolveBaseHostFiles(ctx context.Context, base *Harness, baseURL string, al for i := range base.HostFiles { src := base.HostFiles[i].Src - if src == "" || strings.Contains(src, "${") || IsURL(src) || filepath.IsAbs(src) { + if src == "" || strings.Contains(src, "${") || IsURL(src) || isFullsendCachePath(src, opts.WorkspaceRoot) { continue } fieldName := fmt.Sprintf("host_files[%d].src", i) diff --git a/internal/harness/compose_test.go b/internal/harness/compose_test.go index b5565df44..1f182225d 100644 --- a/internal/harness/compose_test.go +++ b/internal/harness/compose_test.go @@ -2277,11 +2277,56 @@ base: `+baseURL+` } func TestResolveBaseScripts_RejectsAbsolutePath(t *testing.T) { + // Absolute paths that aren't already inside fullsend's own cache are + // untrusted content and must be rejected — pre_script/post_script run + // directly on the host via exec.Command, so this is the only guard + // preventing a URL/base-sourced harness from pointing at an arbitrary + // host path. base := &Harness{PreScript: "/etc/passwd"} _, err := resolveBaseScripts(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{}) require.Error(t, err) assert.Contains(t, err.Error(), "must be a relative path, not an absolute path") - assert.Contains(t, err.Error(), "pre_script") +} + +func TestResolveBaseScripts_SkipsCacheAbsolutePath(t *testing.T) { + // Absolute paths already inside fullsend's own content-addressed cache + // (as left behind by an earlier resolve step, e.g. base resolution + // before this function runs again on the merged child) are left + // unchanged rather than re-fetched or rejected. + workspaceRoot := t.TempDir() + cachePath, err := fetch.CachePath(workspaceRoot, strings.Repeat("a", 64)) + require.NoError(t, err) + + base := &Harness{PreScript: cachePath} + _, err = resolveBaseScripts(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{WorkspaceRoot: workspaceRoot}) + require.NoError(t, err) + assert.Equal(t, cachePath, base.PreScript, "already-cached absolute path should be left unchanged") +} + +func TestIsFullsendCachePath(t *testing.T) { + workspaceRoot := filepath.Join(string(filepath.Separator), "workspace", "repo") + cachePath, err := fetch.CachePath(workspaceRoot, strings.Repeat("a", 64)) + require.NoError(t, err) + + tests := []struct { + name string + path string + workspaceRoot string + want bool + }{ + {"cache path under workspace root", cachePath, workspaceRoot, true}, + {"relative path", "scripts/pre.sh", workspaceRoot, false}, + {"empty path", "", workspaceRoot, false}, + {"empty workspace root", cachePath, "", false}, + {"absolute path outside cache root", filepath.Join(string(filepath.Separator), "etc", "passwd"), workspaceRoot, false}, + {"absolute path under an unrelated sibling directory", filepath.Join(workspaceRoot, "other", ".fullsend-cache", "x"), workspaceRoot, false}, + {"absolute path with cache dir name as a prefix, not a parent", filepath.Join(workspaceRoot, ".fullsend-cache-evil", "x"), workspaceRoot, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isFullsendCachePath(tt.path, tt.workspaceRoot)) + }) + } } func TestResolveBaseScripts_RejectsPathTraversal(t *testing.T) { @@ -2292,11 +2337,14 @@ func TestResolveBaseScripts_RejectsPathTraversal(t *testing.T) { assert.Contains(t, err.Error(), "post_script") } -func TestResolveBaseScripts_RejectsURLInScriptField(t *testing.T) { +func TestResolveBaseScripts_SkipsURLInScriptField(t *testing.T) { + // URL-valued script fields are skipped, matching resolveBaseResources + // behavior. Standalone script URLs remain rejected by ADR-0038 via + // ValidateResourceTypes; this function only handles base composition. base := &Harness{PreScript: "https://evil.com/malware.sh"} _, err := resolveBaseScripts(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{}) - require.Error(t, err) - assert.Contains(t, err.Error(), "must be a relative path, not a URL") + require.NoError(t, err) + assert.Equal(t, "https://evil.com/malware.sh", base.PreScript, "URL should be left unchanged") } func TestResolveBaseScripts_RejectsAbsoluteValidationLoopScript(t *testing.T) { @@ -2306,7 +2354,19 @@ func TestResolveBaseScripts_RejectsAbsoluteValidationLoopScript(t *testing.T) { _, err := resolveBaseScripts(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{}) require.Error(t, err) assert.Contains(t, err.Error(), "must be a relative path, not an absolute path") - assert.Contains(t, err.Error(), "validation_loop.script") +} + +func TestResolveBaseScripts_SkipsURLValidationLoopScript(t *testing.T) { + // URL-valued ValidationLoop.Script fields are skipped, matching the + // URL skip behavior for top-level script fields. This exercises the + // !IsURL() branch of the compound guard on the ValidationLoop.Script + // condition. + base := &Harness{ + ValidationLoop: &ValidationLoop{Script: "https://example.com/loop.sh"}, + } + _, err := resolveBaseScripts(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{}) + require.NoError(t, err) + assert.Equal(t, "https://example.com/loop.sh", base.ValidationLoop.Script, "URL should be left unchanged") } func TestResolveBaseScripts_RejectsAbsoluteForgeScript(t *testing.T) { @@ -2318,7 +2378,6 @@ func TestResolveBaseScripts_RejectsAbsoluteForgeScript(t *testing.T) { _, err := resolveBaseScripts(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{}) require.Error(t, err) assert.Contains(t, err.Error(), "must be a relative path, not an absolute path") - assert.Contains(t, err.Error(), "forge.github.pre_script") } func TestResolveBaseScripts_RejectsTraversalInForgeScript(t *testing.T) { @@ -2344,7 +2403,6 @@ func TestResolveBaseScripts_RejectsAbsoluteForgeValidationLoop(t *testing.T) { _, err := resolveBaseScripts(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{}) require.Error(t, err) assert.Contains(t, err.Error(), "must be a relative path, not an absolute path") - assert.Contains(t, err.Error(), "forge.gitlab.validation_loop.script") } func TestResolveBaseScripts_RejectsTraversalInValidationLoopSchema(t *testing.T) { @@ -2384,7 +2442,6 @@ func TestResolveBaseScripts_RejectsAbsoluteValidationLoopSchema(t *testing.T) { _, err := resolveBaseScripts(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{}) require.Error(t, err) assert.Contains(t, err.Error(), "must be a relative path, not an absolute path") - assert.Contains(t, err.Error(), "validation_loop.schema") } func TestResolveBaseScripts_RejectsNullBytes(t *testing.T) { @@ -2763,28 +2820,53 @@ base: `+baseURL+` assert.Contains(t, err.Error(), "skills[0]") } -func TestResolveBaseResources_SkipsURLAndAbsFields(t *testing.T) { +func TestResolveBaseResources_SkipsURLFields(t *testing.T) { base := &Harness{ Agent: "https://example.com/agents/remote.md", - Policy: "/absolute/path/policy.yaml", + Policy: "https://example.com/policies/remote.yaml", Skills: []string{"https://example.com/skills/foo"}, } deps, err := resolveBaseResources(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{}) require.NoError(t, err) - // URL and absolute path fields are skipped — no deps, no modification + // URL fields are skipped — no deps, no modification. assert.Empty(t, deps) assert.Equal(t, "https://example.com/agents/remote.md", base.Agent) - assert.Equal(t, "/absolute/path/policy.yaml", base.Policy) + assert.Equal(t, "https://example.com/policies/remote.yaml", base.Policy) assert.Equal(t, "https://example.com/skills/foo", base.Skills[0]) } func TestResolveBaseResources_RejectsAbsolutePath(t *testing.T) { + // Absolute paths that aren't already inside fullsend's own cache are + // untrusted content and must be rejected — agent/policy content is read + // on the host and used as the literal agent definition / sandbox + // policy, so an arbitrary host path here is a disclosure risk. base := &Harness{Agent: "/etc/passwd"} _, err := resolveBaseResources(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "must be a relative path, not an absolute path") +} + +func TestResolveBaseResources_RejectsAbsoluteSkillPath(t *testing.T) { + base := &Harness{Skills: []string{"/etc/passwd"}} + _, err := resolveBaseResources(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "must be a relative path, not an absolute path") +} + +func TestResolveBaseResources_SkipsCacheAbsolutePath(t *testing.T) { + // Absolute paths already inside fullsend's own cache (left behind by an + // earlier resolve step, e.g. base resolution before this function runs + // again on the merged child) are left unchanged rather than re-fetched + // or rejected. + workspaceRoot := t.TempDir() + cachePath, err := fetch.CachePath(workspaceRoot, strings.Repeat("b", 64)) + require.NoError(t, err) + + base := &Harness{Agent: cachePath} + _, err = resolveBaseResources(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{WorkspaceRoot: workspaceRoot}) require.NoError(t, err) - // Absolute paths are skipped (not an error — they may be set by earlier resolution) - assert.Equal(t, "/etc/passwd", base.Agent) + assert.Equal(t, cachePath, base.Agent, "already-cached absolute path should be left unchanged") } func TestResolveBaseResources_RejectsPathTraversal(t *testing.T) { @@ -3487,16 +3569,37 @@ func TestResolveBaseHostFiles_SkipsEnvVarPaths(t *testing.T) { assert.Equal(t, "${HOME}/file.txt", base.HostFiles[0].Src) } -func TestResolveBaseHostFiles_SkipsAbsolutePaths(t *testing.T) { +func TestResolveBaseHostFiles_RejectsAbsolutePath(t *testing.T) { + // Absolute paths that aren't already inside fullsend's own cache are + // untrusted content and must be rejected — host_files are read on the + // host and uploaded into the sandbox, so an arbitrary host path here is + // a disclosure risk. base := &Harness{ HostFiles: []HostFile{ - {Src: "/absolute/path/file.txt", Dest: "/sandbox/file.txt"}, + {Src: "/etc/passwd", Dest: "/sandbox/file.txt"}, }, } - deps, err := resolveBaseHostFiles(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{}) + _, err := resolveBaseHostFiles(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "must be a relative path, not an absolute path") +} + +func TestResolveBaseHostFiles_SkipsCacheAbsolutePath(t *testing.T) { + // Absolute paths already inside fullsend's own cache (left behind by an + // earlier resolve step) are left unchanged rather than re-fetched or + // rejected. + workspaceRoot := t.TempDir() + cachePath, err := fetch.CachePath(workspaceRoot, strings.Repeat("c", 64)) require.NoError(t, err) - assert.Empty(t, deps) - assert.Equal(t, "/absolute/path/file.txt", base.HostFiles[0].Src) + + base := &Harness{ + HostFiles: []HostFile{ + {Src: cachePath, Dest: "/sandbox/file.txt"}, + }, + } + _, err = resolveBaseHostFiles(context.Background(), base, "https://example.com/harness/triage.yaml#sha256=abc", nil, ComposeOpts{WorkspaceRoot: workspaceRoot}) + require.NoError(t, err) + assert.Equal(t, cachePath, base.HostFiles[0].Src, "already-cached absolute path should be left unchanged") } func TestResolveBaseHostFiles_SkipsEmptySrc(t *testing.T) { @@ -3648,17 +3751,26 @@ post_script: scripts/post-triage.sh func TestLoadWithBase_SourceURL_NoRelativePaths(t *testing.T) { // A URL-sourced harness with no relative paths should be a no-op. - harnessContent := []byte(` + // The agent field must be a genuine cache path (not an arbitrary + // absolute one) for isFullsendCachePath to treat it as already + // resolved rather than untrusted. + dir := t.TempDir() + agentPath, err := fetch.CachePath(dir, strings.Repeat("f", 64)) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(filepath.Dir(agentPath), 0o755)) + require.NoError(t, os.WriteFile(agentPath, []byte("# agent"), 0o644)) + + harnessContent := fmt.Sprintf(` role: test slug: test-agent -agent: /absolute/path/agent.md -`) +agent: %s +`, agentPath) - dir := t.TempDir() - path := writeTestHarness(t, dir, "test.yaml", string(harnessContent)) + path := writeTestHarness(t, dir, "test.yaml", harnessContent) h, deps, err := LoadWithBase(context.Background(), path, ComposeOpts{ - SourceURL: "https://example.com/harness/test.yaml", + WorkspaceRoot: dir, + SourceURL: "https://example.com/harness/test.yaml", }) require.NoError(t, err) assert.Empty(t, deps) @@ -3712,7 +3824,19 @@ func TestLoadWithBase_SourceURL_HostFiles(t *testing.T) { agentContent := []byte("# triage agent definition") - harnessContent := []byte(` + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + + // A host_files src already inside fullsend's own cache (simulating a + // prior resolve step) is left unchanged; isFullsendCachePath rejects + // any other absolute path as untrusted, so this must be a genuine + // cache path rather than an arbitrary absolute one. + absHostFileSrc, err := fetch.CachePath(cacheDir, strings.Repeat("a", 64)) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(filepath.Dir(absHostFileSrc), 0o755)) + require.NoError(t, os.WriteFile(absHostFileSrc, []byte("cached content"), 0o644)) + + harnessContent := []byte(fmt.Sprintf(` role: triage slug: triage agent: agents/triage.md @@ -3721,9 +3845,9 @@ host_files: dest: /sandbox/.env - src: ${HOME}/.config/app.env dest: /sandbox/app.env - - src: /absolute/path/file.env + - src: %s dest: /sandbox/abs.env -`) +`, absHostFileSrc)) server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { @@ -3745,9 +3869,6 @@ host_files: []string{server.Listener.Addr().String()[len("127.0.0.1:"):]}, ) - dir := t.TempDir() - cacheDir := filepath.Join(dir, "cache") - path := writeTestHarness(t, dir, "triage.yaml", string(harnessContent)) sourceURL := server.URL + "/harness/triage.yaml" @@ -3773,9 +3894,9 @@ host_files: assert.Equal(t, "${HOME}/.config/app.env", h.HostFiles[1].Src, "host_files with ${VAR} should be left unchanged") - // Absolute paths should be left unchanged - assert.Equal(t, "/absolute/path/file.env", h.HostFiles[2].Src, - "host_files with absolute paths should be left unchanged") + // Already-cached absolute paths should be left unchanged + assert.Equal(t, absHostFileSrc, h.HostFiles[2].Src, + "host_files already inside fullsend's cache should be left unchanged") // Dependencies should include the host file fieldNames := map[string]bool{} @@ -4135,3 +4256,590 @@ role: review assert.True(t, filepath.IsAbs(h.PreScript), "pre_script should be an absolute cache path from base resolution, got %q", h.PreScript) } + +func TestLoadWithBase_SourceURL_WithBase_ResolvesChildScripts(t *testing.T) { + // When a URL-sourced harness has a base: field and the child declares + // its own pre_script, that script should be resolved against the + // SourceURL after base composition — not left as a relative path for + // local resolution. This is the same bug class as #5305, but for + // scripts instead of skills. + + baseAgentContent := []byte("# base agent definition") + + baseContent := []byte(` +agent: agents/base.md +role: review +model: opus +`) + baseHash := computeHash(baseContent) + + childPreScriptContent := []byte("#!/bin/bash\necho child-pre") + + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/base.yaml": + w.Write(baseContent) + case "/agents/base.md": + w.Write(baseAgentContent) + case "/scripts/child-pre.sh": + w.Write(childPreScriptContent) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + policy := fetch.NewTestPolicy( + server.Client().Transport.(*http.Transport).TLSClientConfig, + []string{"127.0.0.1"}, + []string{server.Listener.Addr().String()[len("127.0.0.1:"):]}, + ) + + baseURL := server.URL + "/base.yaml#sha256=" + baseHash + + // The child declares its own pre_script (relative to the source repo). + // After base composition, the child's pre_script must be resolved + // against the SourceURL, not left relative. + childHarness := fmt.Sprintf(` +base: %s +role: review +pre_script: scripts/child-pre.sh +`, baseURL) + + path := writeTestHarness(t, dir, "review.yaml", childHarness) + sourceURL := server.URL + "/harness/review.yaml" + allowlist := []string{server.URL + "/"} + + h, deps, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: allowlist, + SourceURL: sourceURL, + allowSelfAllowlist: true, + }) + require.NoError(t, err) + + // The child's pre_script should be resolved to an absolute cache path. + assert.True(t, filepath.IsAbs(h.PreScript), + "pre_script should be resolved to an absolute cache path, got %q", h.PreScript) + + // Verify the cached script content is correct. + scriptContent, err := os.ReadFile(h.PreScript) + require.NoError(t, err) + assert.Equal(t, "#!/bin/bash\necho child-pre", string(scriptContent)) + + // Dependencies should include both the base fetch and the script fetch. + assert.NotEmpty(t, deps) + fieldNames := map[string]bool{} + for _, d := range deps { + fieldNames[d.Field] = true + } + assert.True(t, fieldNames["base"], "should have base dep") + assert.True(t, fieldNames["pre_script"], "should have pre_script dep") +} + +func TestLoadWithBase_SourceURL_WithBase_ResolvesChildHostFiles(t *testing.T) { + // When a URL-sourced harness has a base: field and the child declares + // its own host_files entry with a relative src, that file should be + // resolved against the SourceURL after base composition. + + baseAgentContent := []byte("# base agent definition") + + baseContent := []byte(` +agent: agents/base.md +role: review +model: opus +`) + baseHash := computeHash(baseContent) + + hostFileContent := []byte("host file content from source repo") + + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/base.yaml": + w.Write(baseContent) + case "/agents/base.md": + w.Write(baseAgentContent) + case "/configs/settings.json": + w.Write(hostFileContent) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + policy := fetch.NewTestPolicy( + server.Client().Transport.(*http.Transport).TLSClientConfig, + []string{"127.0.0.1"}, + []string{server.Listener.Addr().String()[len("127.0.0.1:"):]}, + ) + + baseURL := server.URL + "/base.yaml#sha256=" + baseHash + + // The child declares its own host_files entry (relative to the source repo). + childHarness := fmt.Sprintf(` +base: %s +role: review +host_files: + - src: configs/settings.json + dest: /sandbox/.config/settings.json +`, baseURL) + + path := writeTestHarness(t, dir, "review.yaml", childHarness) + sourceURL := server.URL + "/harness/review.yaml" + allowlist := []string{server.URL + "/"} + + h, deps, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: allowlist, + SourceURL: sourceURL, + allowSelfAllowlist: true, + }) + require.NoError(t, err) + + // The child's host_files entry should have its src resolved to a cache path. + require.Len(t, h.HostFiles, 1) + assert.True(t, filepath.IsAbs(h.HostFiles[0].Src), + "host_files[0].src should be resolved to an absolute cache path, got %q", h.HostFiles[0].Src) + assert.Equal(t, "/sandbox/.config/settings.json", h.HostFiles[0].Dest) + + // Verify the cached file content is correct. + cachedContent, err := os.ReadFile(h.HostFiles[0].Src) + require.NoError(t, err) + assert.Equal(t, "host file content from source repo", string(cachedContent)) + + // Dependencies should include the host_files fetch. + fieldNames := map[string]bool{} + for _, d := range deps { + fieldNames[d.Field] = true + } + assert.True(t, fieldNames["host_files[0].src"], "should have host_files dep") +} + +func TestLoadWithBase_SourceURL_WithBase_MixedBaseChildSkills(t *testing.T) { + // After base composition, the merged Skills slice can contain both + // already-resolved absolute cache paths (from the base's URL + // resolution) and still-relative child entries. The post-merge + // SourceURL resolution must resolve the relative entries without + // disturbing the already-resolved absolute entries. + // + // This test simulates the merged state by having the base contribute + // a pre-resolved absolute skill path (via a base that declares a + // skill with an absolute path) and the child contribute a relative + // skill path that must be resolved via SourceURL. + + baseAgentContent := []byte("# base agent definition") + + // Create a pre-existing skill directory, anchored under the workspace's + // own cache root, to act as the base's already-resolved skill + // (simulating what loadBaseChain produces for a URL base's skills). It + // must be a genuine cache path — isFullsendCachePath rejects any other + // absolute path as untrusted, so a plain temp-dir path here would now + // be (correctly) rejected instead of treated as pre-resolved. + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + baseSkillDir, err := fetch.CachePath(cacheDir, strings.Repeat("d", 64)) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(baseSkillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(baseSkillDir, "SKILL.md"), + []byte("# Base Skill"), 0o644)) + + // The base harness contributes the pre-resolved skill as an absolute path. + // In production, this is the result of resolveBaseResources during + // loadBaseChain for a URL base. Here we shortcut by putting the + // absolute path directly in the base YAML. + baseContent := []byte(fmt.Sprintf(` +agent: agents/base.md +role: review +model: opus +skills: + - %s +`, baseSkillDir)) + baseHash := computeHash(baseContent) + + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/base.yaml": + w.Write(baseContent) + case "/agents/base.md": + w.Write(baseAgentContent) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + policy := fetch.NewTestPolicy( + server.Client().Transport.(*http.Transport).TLSClientConfig, + []string{"127.0.0.1"}, + []string{server.Listener.Addr().String()[len("127.0.0.1:"):]}, + ) + + baseURL := server.URL + "/base.yaml#sha256=" + baseHash + + // The child adds its own relative skill alongside the base's skill. + childHarness := fmt.Sprintf(` +base: %s +role: review +skills: + - skills/child-skill +`, baseURL) + + path := writeTestHarness(t, dir, "review.yaml", childHarness) + sourceURL := "https://raw.githubusercontent.com/org/agents/abc123/harness/review.yaml" + + fetcher := fakeTreeFetcher(map[string][]byte{ + "SKILL.md": []byte("# Child Skill"), + "meta-prompt.md": []byte("child meta prompt"), + }) + + allowlist := []string{ + server.URL + "/", + "https://raw.githubusercontent.com/org/agents/", + } + + h, deps, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: allowlist, + SourceURL: sourceURL, + TreeFetcher: fetcher, + allowSelfAllowlist: true, + }) + require.NoError(t, err) + + // Merged Skills slice should have 2 entries: base skill + child skill. + // After mergeBaseIntoChild: [baseSkillDir, "skills/child-skill"] + // After post-merge resolveBaseResources: [baseSkillDir, ] + require.Len(t, h.Skills, 2, "should have base skill + child skill") + + // Both should be absolute paths. + for i, skill := range h.Skills { + assert.True(t, filepath.IsAbs(skill), + "skills[%d] should be an absolute path, got %q", i, skill) + } + + // The base skill (index 0) should be the pre-resolved path, untouched. + assert.Equal(t, baseSkillDir, h.Skills[0], + "base skill should remain at its pre-resolved absolute path") + assert.FileExists(t, filepath.Join(h.Skills[0], "SKILL.md")) + baseSkillContent, err := os.ReadFile(filepath.Join(h.Skills[0], "SKILL.md")) + require.NoError(t, err) + assert.Equal(t, "# Base Skill", string(baseSkillContent)) + + // The child skill (index 1) should be resolved to a cache path. + assert.NotEqual(t, "skills/child-skill", h.Skills[1], + "child skill should be resolved, not remain relative") + assert.FileExists(t, filepath.Join(h.Skills[1], "SKILL.md")) + childSkillContent, err := os.ReadFile(filepath.Join(h.Skills[1], "SKILL.md")) + require.NoError(t, err) + assert.Equal(t, "# Child Skill", string(childSkillContent)) + assert.FileExists(t, filepath.Join(h.Skills[1], "meta-prompt.md")) + + // Dependencies should include the child skill fetch (but not the + // base skill, which was already an absolute path). + skillDeps := 0 + for _, d := range deps { + if strings.HasPrefix(d.Field, "skills[") { + skillDeps++ + } + } + assert.Equal(t, 1, skillDeps, "should have 1 skill dependency (child only; base was pre-resolved)") +} + +func TestLoadWithBase_SourceURL_WithBase_SkillOverrideByBasename(t *testing.T) { + // mergeSkills (#5408) makes a child skill override a base skill in + // place when their basenames match, rather than appending it + // alongside. This locks in that this PR's post-merge SourceURL + // resolution composes correctly with that override: the surviving + // entry must resolve to the child's content from SourceURL, and + // there must be exactly one resulting skill, not two. + + baseAgentContent := []byte("# base agent definition") + + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + + // The base's skill directory shares a basename ("pr-review") with the + // child's skill declaration below, so mergeSkills overrides it in + // place. It must also be a genuine cache path — isFullsendCachePath + // rejects any other absolute path as untrusted — so it's nested under + // a cache-hash directory rather than a plain temp-dir path, with + // "pr-review" as its basename (mirroring how fetchBaseSkill names the + // resolved directory after the skill's own basename). + cacheHashDir, err := fetch.CachePath(cacheDir, strings.Repeat("e", 64)) + require.NoError(t, err) + baseSkillDir := filepath.Join(cacheHashDir, "pr-review") + require.NoError(t, os.MkdirAll(baseSkillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(baseSkillDir, "SKILL.md"), + []byte("# Base Skill"), 0o644)) + + baseContent := []byte(fmt.Sprintf(` +agent: agents/base.md +role: review +model: opus +skills: + - %s +`, baseSkillDir)) + baseHash := computeHash(baseContent) + + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/base.yaml": + w.Write(baseContent) + case "/agents/base.md": + w.Write(baseAgentContent) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + policy := fetch.NewTestPolicy( + server.Client().Transport.(*http.Transport).TLSClientConfig, + []string{"127.0.0.1"}, + []string{server.Listener.Addr().String()[len("127.0.0.1:"):]}, + ) + + baseURL := server.URL + "/base.yaml#sha256=" + baseHash + + // The child declares its own skill with the SAME basename as the + // base's, so mergeSkills replaces the base's entry instead of + // appending alongside it. + childHarness := fmt.Sprintf(` +base: %s +role: review +skills: + - skills/pr-review +`, baseURL) + + path := writeTestHarness(t, dir, "review.yaml", childHarness) + sourceURL := "https://raw.githubusercontent.com/org/agents/abc123/harness/review.yaml" + + fetcher := fakeTreeFetcher(map[string][]byte{ + "SKILL.md": []byte("# Child Skill"), + "meta-prompt.md": []byte("child meta prompt"), + }) + + allowlist := []string{ + server.URL + "/", + "https://raw.githubusercontent.com/org/agents/", + } + + h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: allowlist, + SourceURL: sourceURL, + TreeFetcher: fetcher, + allowSelfAllowlist: true, + }) + require.NoError(t, err) + + require.Len(t, h.Skills, 1, "child's same-basename skill should override the base's, not sit alongside it") + assert.True(t, filepath.IsAbs(h.Skills[0]), "overriding skill should be resolved to an absolute path, got %q", h.Skills[0]) + assert.NotEqual(t, baseSkillDir, h.Skills[0], "should not resolve to the base's skill directory") + + content, err := os.ReadFile(filepath.Join(h.Skills[0], "SKILL.md")) + require.NoError(t, err) + assert.Equal(t, "# Child Skill", string(content), "overriding skill should fetch the child's content, not the base's") + assert.FileExists(t, filepath.Join(h.Skills[0], "meta-prompt.md")) +} + +func TestLoadWithBase_SourceURL_WithBase_ScriptResolutionError(t *testing.T) { + // When a URL-sourced harness has a base: field and the child declares + // its own pre_script, the post-composition resolveBaseScripts call + // must propagate errors (e.g., SourceURL not in allowlist). + // This exercises the error path at the first resolve call in the + // post-merge SourceURL resolution block. + + baseAgentContent := []byte("# base agent definition") + + baseContent := []byte(` +agent: agents/base.md +role: review +model: opus +`) + baseHash := computeHash(baseContent) + + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/base.yaml": + w.Write(baseContent) + case "/agents/base.md": + w.Write(baseAgentContent) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + policy := fetch.NewTestPolicy( + server.Client().Transport.(*http.Transport).TLSClientConfig, + []string{"127.0.0.1"}, + []string{server.Listener.Addr().String()[len("127.0.0.1:"):]}, + ) + + baseURL := server.URL + "/base.yaml#sha256=" + baseHash + + // The child declares its own pre_script. After base composition, + // this remains a relative path and must be resolved against the + // SourceURL. The SourceURL is NOT in the allowlist, so resolution + // fails. + childHarness := fmt.Sprintf(` +base: %s +role: review +pre_script: scripts/pre.sh +`, baseURL) + + path := writeTestHarness(t, dir, "review.yaml", childHarness) + + // SourceURL is NOT in the allowlist — only the base server URL is. + _, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: []string{server.URL + "/"}, + SourceURL: "https://not-allowed.example.com/harness/review.yaml", + allowSelfAllowlist: true, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "resolving URL-sourced scripts after base composition") +} + +func TestLoadWithBase_SourceURL_WithBase_ResourceResolutionError(t *testing.T) { + // When a URL-sourced harness has a base: field and the child declares + // its own skills, the post-composition resolveBaseResources call must + // propagate errors. The child declares no scripts (so resolveBaseScripts + // succeeds on the merged harness), but the child's skill path cannot be + // resolved because the SourceURL is not in the allowlist. + + baseAgentContent := []byte("# base agent definition") + + baseContent := []byte(` +agent: agents/base.md +role: review +model: opus +`) + baseHash := computeHash(baseContent) + + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/base.yaml": + w.Write(baseContent) + case "/agents/base.md": + w.Write(baseAgentContent) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + policy := fetch.NewTestPolicy( + server.Client().Transport.(*http.Transport).TLSClientConfig, + []string{"127.0.0.1"}, + []string{server.Listener.Addr().String()[len("127.0.0.1:"):]}, + ) + + baseURL := server.URL + "/base.yaml#sha256=" + baseHash + + // The child declares its own skills (no scripts). After base + // composition, the agent is an absolute cache path (from the base), + // but the child's skill path remains relative. + childHarness := fmt.Sprintf(` +base: %s +role: review +skills: + - skills/my-skill +`, baseURL) + + path := writeTestHarness(t, dir, "review.yaml", childHarness) + + // SourceURL is NOT in the allowlist. + _, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: []string{server.URL + "/"}, + SourceURL: "https://not-allowed.example.com/harness/review.yaml", + allowSelfAllowlist: true, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "resolving URL-sourced resources after base composition") +} + +func TestLoadWithBase_SourceURL_WithBase_HostFilesResolutionError(t *testing.T) { + // When a URL-sourced harness has a base: field and the child declares + // its own host_files, the post-composition resolveBaseHostFiles call + // must propagate errors. The child declares no scripts or resources + // (so the first two resolve calls succeed on the merged harness), + // but the child's host_files src cannot be resolved because the + // SourceURL is not in the allowlist. + + baseAgentContent := []byte("# base agent definition") + + baseContent := []byte(` +agent: agents/base.md +role: review +model: opus +`) + baseHash := computeHash(baseContent) + + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/base.yaml": + w.Write(baseContent) + case "/agents/base.md": + w.Write(baseAgentContent) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + policy := fetch.NewTestPolicy( + server.Client().Transport.(*http.Transport).TLSClientConfig, + []string{"127.0.0.1"}, + []string{server.Listener.Addr().String()[len("127.0.0.1:"):]}, + ) + + baseURL := server.URL + "/base.yaml#sha256=" + baseHash + + // The child declares its own host_files (no scripts, no skills/agent). + // After base composition, the agent is an absolute cache path (from + // the base) but the child's host_files src remains relative. + childHarness := fmt.Sprintf(` +base: %s +role: review +host_files: + - src: configs/settings.json + dest: /sandbox/.config/settings.json +`, baseURL) + + path := writeTestHarness(t, dir, "review.yaml", childHarness) + + // SourceURL is NOT in the allowlist. + _, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: []string{server.URL + "/"}, + SourceURL: "https://not-allowed.example.com/harness/review.yaml", + allowSelfAllowlist: true, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "resolving URL-sourced host_files after base composition") +}