fix(harness): resolve URL base scripts relative to scaffold root by ralphbean · Pull Request #2613 · fullsend-ai/fullsend

ralphbean · 2026-06-24T14:34:32Z

Summary

resolveBaseScripts resolved script relative paths against the YAML file's URL directory (urlDirPrefix), but script paths are relative to the scaffold root (parent of harness/), matching local resolution via ResolveRelativeTo(absFullsendDir)
Added urlParentDirPrefix that goes up one additional directory level and use it in resolveBaseScripts
Fixed existing tests that encoded the bug by mounting scripts under /harness/scripts/ instead of /scripts/

Test plan

Added TestLoadWithBase_URLBase_ScriptsRelativeToScaffoldRoot — mirrors real scaffold layout where harness/ and scripts/ are siblings
Added TestURLParentDirPrefix unit tests
Fixed 9 existing tests to match real scaffold layout
All harness tests pass
go vet clean

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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

qodo-code-review · 2026-06-24T14:35:23Z

PR Summary by Qodo

Fix harness URL base script resolution to scaffold root
🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Description

• Resolve remote base scripts relative to scaffold root, not harness YAML directory.
• Add URL parent-directory helper to mirror local scaffold path behavior.
• Update and add tests to match real harness/ + scripts/ sibling layout.

Diagram

graph TD
  A["LoadWithBase"] --> B["resolveBaseScripts"] --> C["urlParentDirPrefix"] --> D{{"Base harness URL"}}
  D --> E{{"Remote scripts"}} --> F[("Local cache")]
  D --> F

  subgraph Legend
    direction LR
    _fn["Function"] ~~~ _ext{{"Remote"}} ~~~ _db[("Cache")]
  end

High-Level Assessment

The following are alternative approaches to this PR:

1. Make script URLs absolute in harness YAMLs

➕ Removes any ambiguity about relative resolution semantics
➕ Avoids directory-walking logic in URL handling
➖ More verbose harness YAMLs and more burden on authors
➖ Harder to move scaffolds across repos/paths without editing URLs

2. Derive scaffold root by stripping a literal "harness/" path segment

➕ More explicit about intent when repo layout is strictly enforced
➕ Avoids relying on two path.Dir calls
➖ More brittle if harness YAML path changes (e.g., nested harness/ dirs)
➖ Requires careful handling of edge cases and URL-encoded paths

Recommendation: Keep the PR’s approach: a dedicated urlParentDirPrefix helper is a minimal, layout-agnostic fix that matches existing local resolution behavior, and the updated tests lock the behavior to the real scaffold structure. The only follow-up worth considering is documenting the relative-path contract (scripts relative to scaffold root) near the harness YAML schema/README, if such docs exist.

Files changed (2) +141 / -19

Bug fix (1) +29 / -1

compose.goResolve remote base scripts relative to scaffold-root URL prefix +29/-1

Resolve remote base scripts relative to scaffold-root URL prefix

• Fixes resolveBaseScripts to resolve script paths relative to the scaffold root (parent of harness/) rather than the base YAML directory. Introduces urlParentDirPrefix to compute that prefix robustly (handling integrity-hash fragments and URL parsing).

internal/harness/compose.go

Tests (1) +112 / -18

compose_test.goAlign script-fetching tests with real scaffold layout and add coverage +112/-18

Align script-fetching tests with real scaffold layout and add coverage

• Adds unit coverage for urlParentDirPrefix and a regression test ensuring URL-based script resolution matches local scaffold behavior. Updates existing tests to serve and cache scripts under /scripts/ (sibling of /harness/) instead of under /harness/scripts/ to avoid encoding the prior bug.

internal/harness/compose_test.go

github-actions · 2026-06-24T14:35:59Z

Site preview

Preview: https://235819b8-site.fullsend-ai.workers.dev

Commit: 8ef07180e0561a2c87b753c9c923774c596e4a9b

qodo-code-review · 2026-06-24T14:39:49Z

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 harness: URL base script resolution resolves relative to YAML dir instead of scaffold root

✅ Compliance rules (platform): 51 rules

✅ Skills: writing-user-docs, writing-adrs

1. Query breaks script URL join 🐞 Bug ≡ Correctness

Description

urlParentDirPrefix preserves the base URL query string, and fetchBaseScript constructs scriptURL via
string concatenation (baseURLDir + relPath). If the base URL contains a query (e.g. signed URLs),
derived script URLs become malformed (query ends up mid-string), causing failed fetches/allowlist
checks or fetching the wrong resource.

Code

internal/harness/compose.go[R749-752]

+	parsed.Path = dir
+	parsed.RawPath = ""
+	parsed.Fragment = ""
+	return parsed.String()

Relevance

⭐⭐ Medium
No prior accepted/rejected review about clearing RawQuery in urlDirPrefix; URL joining logic added
in recent PR #2525.
PR-#2525
PR-#1623
PR-#1555

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The helper returns a URL string without clearing the query component, and the script URL is built by
concatenating that returned string with a relative path; this is unsafe when a query is present
because concatenation occurs after the '?', producing an invalid URL.
internal/harness/compose.go[736-753]
internal/harness/compose.go[608-614]
internal/harness/url.go[9-28]
internal/harness/compose.go[262-276]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`urlParentDirPrefix` returns `parsed.String()` after updating `Path`/`RawPath`/`Fragment`, but it does **not** clear `RawQuery`. Since `fetchBaseScript` later builds the script URL via `baseURLDir + relPath`, any query on the base URL will yield an invalid/mis-targeted script URL.

### Issue Context
Base URLs are accepted as general HTTPS URLs (queries allowed), so this can occur with signed/presigned URLs.

### Fix Focus Areas
- internal/harness/compose.go[736-753]
- internal/harness/compose.go[608-614]

### Suggested fix
1. In **both** `urlParentDirPrefix` and `urlDirPrefix`, explicitly clear the query before returning:
  - `parsed.RawQuery = ""`
2. (Optional but more robust) Avoid plain string concatenation in `fetchBaseScript`:
  - Parse `baseURLDir` with `url.Parse`
  - Join paths using `path.Join` (preserving trailing slash semantics)
  - Re-serialize with `u.String()`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

codecov · 2026-06-24T14:41:56Z

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/harness/compose.go	86.66%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

waynesun09

LGTM — clean, well-scoped bugfix. The core change correctly aligns URL-based script resolution with local resolution. Tests are comprehensive and CI is green. Two non-blocking suggestions posted inline.

[MEDIUM] urlDirPrefix is now dead production code

After this change, urlDirPrefix (line 712) has zero production callers — only referenced in TestURLDirPrefix and a test comment. Dead unexported code adds maintenance burden.

Non-blocking suggestion: remove in a follow-up cleanup, or add a comment explaining why it is retained.

Assisted-by: Claude (review), Gemini (review), Codex (review)

waynesun09 · 2026-06-24T14:50:28Z

+	if err != nil {
+		return ""
+	}
+	dir := path.Dir(path.Dir(parsed.Path))


[MEDIUM] Hardcoded one-level parent assumption

path.Dir(path.Dir(parsed.Path)) assumes the harness YAML is always exactly one directory below the scaffold root. Today this holds (scaffold layout enforces it, and resolveHarnessPath in lock.go also hardcodes single-depth), so this is safe.

Non-blocking suggestion: consider a doc comment noting this constraint, or renaming to urlScaffoldRootPrefix to make the intent explicit.

github-actions Bot deployed to site-preview June 24, 2026 14:35 View deployment

waynesun09 approved these changes Jun 24, 2026

View reviewed changes

ascerra approved these changes Jun 24, 2026

View reviewed changes

ralphbean added this pull request to the merge queue Jun 24, 2026

Merged via the queue into main with commit d59366d Jun 24, 2026
16 checks passed

ralphbean deleted the fix/url-base-script-resolution branch June 24, 2026 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(harness): resolve URL base scripts relative to scaffold root#2613

fix(harness): resolve URL base scripts relative to scaffold root#2613
ralphbean merged 1 commit into
mainfrom
fix/url-base-script-resolution

ralphbean commented Jun 24, 2026

Uh oh!

qodo-code-review Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

qodo-code-review Bot commented Jun 24, 2026

Uh oh!

codecov Bot commented Jun 24, 2026

Uh oh!

waynesun09 left a comment

Uh oh!

waynesun09 Jun 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ralphbean commented Jun 24, 2026

Summary

Test plan

Uh oh!

qodo-code-review Bot commented Jun 24, 2026

PR Summary by Qodo

Uh oh!

github-actions Bot commented Jun 24, 2026

Site preview

Uh oh!

qodo-code-review Bot commented Jun 24, 2026

Code Review by Qodo

Uh oh!

codecov Bot commented Jun 24, 2026

Codecov Report

Uh oh!

waynesun09 left a comment

Choose a reason for hiding this comment

Uh oh!

waynesun09 Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants