From 8ef07180e0561a2c87b753c9c923774c596e4a9b Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Wed, 24 Jun 2026 10:31:15 -0400 Subject: [PATCH] fix(harness): resolve URL base scripts relative to scaffold root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveBaseScripts used urlDirPrefix to resolve script relative paths against the YAML file's URL directory. But script paths in harness YAMLs are relative to the scaffold root (the parent of harness/), not the YAML file itself — matching local resolution where ResolveRelativeTo is called with absFullsendDir (the workspace root). Add urlParentDirPrefix that goes up one additional directory level and use it in resolveBaseScripts. Fix existing tests that encoded the bug by mounting scripts under /harness/scripts/ instead of /scripts/. Fixes #2610 Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/harness/compose.go | 30 ++++++- internal/harness/compose_test.go | 130 ++++++++++++++++++++++++++----- 2 files changed, 141 insertions(+), 19 deletions(-) diff --git a/internal/harness/compose.go b/internal/harness/compose.go index c56270a39a..e448523660 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -484,7 +484,11 @@ func mergeBaseIntoChild(base, child *Harness) { // because runtime treats it as a directory (uploaded recursively). // Returns additional dependencies for the fetched scripts. func resolveBaseScripts(ctx context.Context, base *Harness, baseURL string, allowlist []string, opts ComposeOpts) ([]Dependency, error) { - baseURLDir := urlDirPrefix(baseURL) + // Script paths in harness YAMLs are relative to the scaffold root (the + // parent of the harness/ directory), not the YAML file. Use + // urlParentDirPrefix to match the local resolution behavior where + // ResolveRelativeTo is called with absFullsendDir (the workspace root). + baseURLDir := urlParentDirPrefix(baseURL) if baseURLDir == "" { return nil, fmt.Errorf("cannot determine directory from base URL") } @@ -724,6 +728,30 @@ func urlDirPrefix(rawURL string) string { return parsed.String() } +// urlParentDirPrefix returns the parent of the directory containing the URL's +// file. Script paths in harness YAMLs are relative to the scaffold root (the +// parent of the harness/ directory), not the YAML file itself. This matches +// local resolution where ResolveRelativeTo uses absFullsendDir (the workspace +// root), which is the parent of the harness/ directory. +func urlParentDirPrefix(rawURL string) string { + cleanURL, _, _ := ParseIntegrityHash(rawURL) + parsed, err := url.Parse(cleanURL) + if err != nil { + return "" + } + dir := path.Dir(path.Dir(parsed.Path)) + if dir == "." || dir == "" { + return "" + } + if !strings.HasSuffix(dir, "/") { + dir += "/" + } + parsed.Path = dir + parsed.RawPath = "" + parsed.Fragment = "" + return parsed.String() +} + // urlIndexPath returns the path to the URL-to-hash index file. func urlIndexPath(workspaceRoot string) string { return filepath.Join(workspaceRoot, ".fullsend-cache", "url-index.json") diff --git a/internal/harness/compose_test.go b/internal/harness/compose_test.go index 3f69026897..ff0e3e368b 100644 --- a/internal/harness/compose_test.go +++ b/internal/harness/compose_test.go @@ -1168,6 +1168,35 @@ func TestURLDirPrefix(t *testing.T) { } } +func TestURLParentDirPrefix(t *testing.T) { + tests := []struct { + input string + want string + }{ + { + "https://raw.githubusercontent.com/org/repo/sha/harness/triage.yaml#sha256=abc123", + "https://raw.githubusercontent.com/org/repo/sha/", + }, + { + "https://example.com/path/to/file.yaml", + "https://example.com/path/", + }, + { + // File at domain root: parent of "/" is still "/" + "https://example.com/file.yaml#sha256=0000000000000000000000000000000000000000000000000000000000000000", + "https://example.com/", + }, + { + "not-a-url", + "", + }, + } + for _, tt := range tests { + got := urlParentDirPrefix(tt.input) + assert.Equal(t, tt.want, got, "urlParentDirPrefix(%q)", tt.input) + } +} + func setupScriptTestServer(t *testing.T, harnessContent []byte, scripts map[string][]byte) (*httptest.Server, fetch.FetchPolicy) { t.Helper() server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -1205,9 +1234,10 @@ pre_script: scripts/pre.sh post_script: scripts/post.sh `) + // Scripts at /scripts/ (sibling to /harness/), matching real scaffold layout. server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ - "/harness/scripts/pre.sh": preScript, - "/harness/scripts/post.sh": postScript, + "/scripts/pre.sh": preScript, + "/scripts/post.sh": postScript, }) hash := computeHash(baseContent) @@ -1275,7 +1305,7 @@ validation_loop: `) server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ - "/harness/scripts/validate.sh": validateScript, + "/scripts/validate.sh": validateScript, }) hash := computeHash(baseContent) @@ -1324,8 +1354,8 @@ forge: `) server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ - "/harness/scripts/gh-pre.sh": forgePre, - "/harness/scripts/gh-post.sh": forgePost, + "/scripts/gh-pre.sh": forgePre, + "/scripts/gh-post.sh": forgePost, }) hash := computeHash(baseContent) @@ -1375,8 +1405,8 @@ post_script: scripts/base-post.sh postScript := []byte("#!/bin/bash\necho base-post") server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ - "/harness/scripts/base-pre.sh": preScript, - "/harness/scripts/base-post.sh": postScript, + "/scripts/base-pre.sh": preScript, + "/scripts/base-post.sh": postScript, }) hash := computeHash(baseContent) @@ -1418,7 +1448,7 @@ pre_script: scripts/pre.sh `) server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ - "/harness/scripts/pre.sh": []byte("#!/bin/bash"), + "/scripts/pre.sh": []byte("#!/bin/bash"), }) hash := computeHash(baseContent) @@ -1432,14 +1462,14 @@ role: test base: `+baseURL+` `) - // Allowlist only covers /harness/triage.yaml, not /harness/scripts/ + // Allowlist only covers /harness/triage.yaml, not /scripts/ _, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ WorkspaceRoot: cacheDir, FetchPolicy: policy, OrgAllowlist: []string{server.URL + "/harness/triage.yaml"}, }) // The allowlist check is prefix-based, so /harness/triage.yaml as prefix - // does NOT cover /harness/scripts/pre.sh + // does NOT cover /scripts/pre.sh require.Error(t, err) assert.Contains(t, err.Error(), "not in allowed_remote_resources") } @@ -1519,10 +1549,10 @@ pre_script: scripts/pre.sh // Pre-populate base harness in cache require.NoError(t, fetch.CachePut(cacheDir, "https://example.com/harness/triage.yaml", baseContent)) // Pre-populate script in cache - require.NoError(t, fetch.CachePut(cacheDir, "https://example.com/harness/scripts/pre.sh", preScript)) + require.NoError(t, fetch.CachePut(cacheDir, "https://example.com/scripts/pre.sh", preScript)) // Add URL index entry scriptHash := fetch.ComputeSHA256(preScript) - require.NoError(t, urlIndexPut(cacheDir, "https://example.com/harness/scripts/pre.sh", scriptHash)) + require.NoError(t, urlIndexPut(cacheDir, "https://example.com/scripts/pre.sh", scriptHash)) baseURL := "https://example.com/harness/triage.yaml#sha256=" + hash @@ -1560,7 +1590,7 @@ pre_script: scripts/pre.sh `) server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ - "/harness/scripts/pre.sh": scriptContent, + "/scripts/pre.sh": scriptContent, }) hash := computeHash(baseContent) @@ -1629,7 +1659,7 @@ pre_script: scripts/pre.sh `) server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ - "/harness/scripts/pre.sh": preScript, + "/scripts/pre.sh": preScript, }) hash := computeHash(baseContent) @@ -1676,7 +1706,7 @@ forge: `) server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ - "/harness/scripts/gh-validate.sh": forgeValidate, + "/scripts/gh-validate.sh": forgeValidate, }) hash := computeHash(baseContent) @@ -1795,9 +1825,9 @@ validation_loop: `) server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ - "/harness/scripts/pre.sh": preScript, - "/harness/scripts/post.sh": postScript, - "/harness/scripts/validate.sh": validateScript, + "/scripts/pre.sh": preScript, + "/scripts/post.sh": postScript, + "/scripts/validate.sh": validateScript, }) hash := computeHash(baseContent) @@ -1947,6 +1977,70 @@ func TestResolveBaseScripts_InvalidBaseURL(t *testing.T) { assert.Contains(t, err.Error(), "cannot determine directory") } +// TestLoadWithBase_URLBase_ScriptsRelativeToScaffoldRoot verifies that URL +// base script resolution matches local resolution: scripts are relative to +// the scaffold root (parent of harness/), not to the YAML file's directory. +// This mirrors the real scaffold layout where harness/ and scripts/ are siblings. +func TestLoadWithBase_URLBase_ScriptsRelativeToScaffoldRoot(t *testing.T) { + preScript := []byte("#!/bin/bash\necho pre") + postScript := []byte("#!/bin/bash\necho post") + + baseContent := []byte(` +agent: agents/triage.md +role: test +model: opus +pre_script: scripts/pre.sh +post_script: scripts/post.sh +`) + + // Mount scripts at /scripts/ (sibling to /harness/), matching real layout. + // The YAML lives at /harness/triage.yaml, so urlDirPrefix gives /harness/. + // Scripts should resolve relative to / (the scaffold root), not /harness/. + server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ + "/scripts/pre.sh": preScript, + "/scripts/post.sh": postScript, + }) + + hash := computeHash(baseContent) + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + + baseURL := server.URL + "/harness/triage.yaml#sha256=" + hash + + path := writeTestHarness(t, dir, "child.yaml", ` +agent: agents/child.md +role: test +base: `+baseURL+` +`) + + h, deps, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: []string{server.URL + "/"}, + }) + require.NoError(t, err) + + assert.Equal(t, "agents/child.md", h.Agent) + + // Scripts resolved to local cache paths + assert.NotEmpty(t, h.PreScript, "pre_script should be resolved") + assert.NotEmpty(t, h.PostScript, "post_script should be resolved") + assert.True(t, filepath.IsAbs(h.PreScript), "pre_script should be absolute cache path") + assert.True(t, filepath.IsAbs(h.PostScript), "post_script should be absolute cache path") + + // Verify cached content matches + preContent, err := os.ReadFile(h.PreScript) + require.NoError(t, err) + assert.Equal(t, preScript, preContent) + + postContent, err := os.ReadFile(h.PostScript) + require.NoError(t, err) + assert.Equal(t, postScript, postContent) + + // Dependencies: 1 for base harness + 2 for scripts + require.Len(t, deps, 3) +} + func TestURLIndexPut_EmptyWorkspaceRoot(t *testing.T) { err := urlIndexPut("", "https://example.com/script.sh", "abc123") assert.NoError(t, err)