diff --git a/internal/harness/compose.go b/internal/harness/compose.go index 9270f5e06..dbcdbad89 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 and scripts (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,41 @@ 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, 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 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. + // + // 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 != "" { + 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, allowlist, 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) @@ -553,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 @@ -583,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 { @@ -597,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 } @@ -608,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 } @@ -632,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 { @@ -645,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 @@ -657,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 @@ -687,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 == "" { @@ -706,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 { @@ -721,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) @@ -741,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 == "" { @@ -756,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 b3556d66e..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{} @@ -3950,3 +4071,775 @@ 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) +} + +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") +}