feat(harness): fetch scripts from URL-referenced base harnesses

[ggallen]

Summary

• Extends base: composition to fetch pre_script, post_script, validation_loop.script, and agent_input from URL-referenced base harnesses, removing the last blocker for hosting agents in standalone repositories
• Scripts are fetched through the existing SSRF-hardened, integrity-verified pipeline (fetch.FetchURL → fetch.CachePut), cached content-addressed, and paths rewritten to local cache paths before ValidateResourceTypes runs — standalone script URL references remain rejected
• Adds URL-to-hash index (.fullsend-cache/url-index.json) for offline mode support, audit logging for script fetches, forge-level script resolution, and executable permission on cached scripts
• Updates ADR-0038 to document the base: composition exception and its security rationale

Test plan

• 19 new test cases in compose_test.go covering all script types, forge scripts, child overrides, allowlist rejection, 404 errors, offline mode (cache miss + hit), executable permissions, audit logging, and edge cases
• 92% average coverage on new functions (target: 90%)
• Pre-commit hooks pass (gofmt, go vet, ADR linters, secret detection)
• CI pipeline passes

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

harness: fetch scripts from URL-referenced base harnesses
✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Description

• Fetch relative script paths from URL-based base: harnesses into local cache before validation.
• Add URL→SHA256 index, audit logging, and executable-bit handling for cached scripts.
• Expand composition tests and ADR-0038 to document the base: script exception.

Diagram

graph TD
  A["LoadWithBase / loadBaseChain"] --> B["resolveBaseScripts (URL base)"] --> C["fetchBaseScript (per field)"] --> D{{"Remote script URL"}}
  D --> E["Cache write + chmod + rewrite path"] --> G["Fetch audit log (JSONL)"]
  C --> F["url-index.json (URL→SHA)"] --> E

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Move URL→hash mapping into the lockfile format

• ➕ Single source of truth for offline reproducibility (no extra index file).
• ➕ Easier to audit/review changes in a committed artifact vs workspace cache file.
• ➖ Requires lockfile schema change and migration logic.
• ➖ Couples composition-time behavior more tightly to lockfile generation/updates.

2. Fetch scripts lazily after merge (only for non-overridden fields)

• ➕ Avoids fetching scripts that will be overridden by the child harness.
• ➕ Potentially reduces network calls and cache churn.
• ➖ Requires merge-aware resolution and careful ordering to preserve validation invariants.
• ➖ More complex control flow (especially with forge promotion and validation_loop nesting).

3. Require scripts to be SHA-pinned URLs inside the base harness

• ➕ Explicit, per-script integrity pinning rather than transitive trust.
• ➕ Can reduce ambiguity around what content is executed.
• ➖ More authoring friction for base harness maintainers.
• ➖ Still needs allowlist + caching; doesn’t address offline lookup without additional indexing.

Recommendation: The PR’s approach (transitive trust from a SHA256-pinned base harness, fetch scripts from the same origin directory, then rewrite to local cache paths before validation) is a pragmatic way to unblock standalone agent repos while preserving the “no direct script URLs” invariant. Consider folding the URL→hash mapping into the existing lock mechanism in a follow-up to reduce bespoke cache metadata and strengthen offline reproducibility.

Files changed (3) +998 / -5

Enhancement (1) +270 / -2

compose.goResolve base-harness script fields via secure fetch+cache pipeline +270/-2

Resolve base-harness script fields via secure fetch+cache pipeline

• Extends URL-base composition to fetch relative paths for pre/post scripts, validation loop scripts, agent_input, and forge-level scripts from the base URL’s directory. Adds offline support via a workspace URL→SHA index, sets executable permissions on cached scripts, and emits optional audit log entries for script fetches.

internal/harness/compose.go

Tests (1) +724 / -0

compose_test.goAdd coverage for URL-base script fetching, offline mode, and auditing +724/-0

Add coverage for URL-base script fetching, offline mode, and auditing

• Introduces extensive new tests covering all supported script fields, forge promotion/validation loop handling, allowlist enforcement, fetch failures, offline cache hit/miss behavior, executable permissions, and audit logging output.

internal/harness/compose_test.go

Documentation (1) +4 / -3

0038-universal-harness-access.mdDocument 'base:' exception for remote script resolution +4/-3

Document 'base:' exception for remote script resolution

• Updates ADR-0038 to clarify that standalone script URL fields remain invalid, but scripts inherited via a URL-referenced 'base:' harness are fetched, cached, and rewritten to local paths. Expands the security rationale around transitive trust, allowlisting, auditing, and offline behavior.

docs/ADRs/0038-universal-harness-access.md

[github-actions]

Site preview

Preview: https://220a79d7-site.fullsend-ai.workers.dev

Commit: 23a44bf7df5d2b84efa6b8ddcaa1c8570c3fe8b8

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:50 PM UTC · Ended 7:00 PM UTC
Commit: 4e21a60 · View workflow run →

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Tickets: 🎫 ensure ROSA cluster support events will be sent to lightspeed engineering team

✅ Compliance rules (platform): 51 rules

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

1. Abs-path base script bypass 🐞 Bug ⛨ Security

Description

For URL-referenced bases, resolveBaseScripts silently skips absolute paths for
pre_script/post_script/agent_input/validation_loop.script, allowing a remote base to keep an
absolute host path that passes validation and is then executed (scripts) or uploaded into the
sandbox (agent_input). This violates the intended transitive-trust model for URL bases and can
enable host command execution or host filesystem exfiltration.

Code

internal/harness/compose.go[R502-505]

+	for _, f := range scriptFields {
+		if *f.ptr == "" || IsURL(*f.ptr) || filepath.IsAbs(*f.ptr) {
+			continue
+		}

Relevance

⭐⭐⭐ High

Team often tightens security around host-executed scripts; absolute-path bypass likely treated as
high-risk bug.

PR-#716
PR-#2035
PR-#1555

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new resolver explicitly skips absolute paths (leaving them unchanged), and validation only
rejects URLs (not absolute paths). The runner then executes PreScript on the host and uploads
AgentInput from the host filesystem into the sandbox.

internal/harness/compose.go[496-516]
internal/harness/harness.go[653-671]
internal/cli/run.go[474-485]
internal/cli/run.go[682-693]

Agent prompt

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

### Issue description
When the base harness is URL-referenced, absolute paths in executable-ish fields (pre_script/post_script/validation_loop.script) and agent_input are not rewritten (they’re `continue`’d) and are later accepted as “local paths”, enabling a remote base to reference arbitrary host paths.

### Issue Context
- URL bases are integrity-pinned, but absolute host paths are not part of that trusted artifact.
- Runtime executes `PreScript` on the host and uploads `AgentInput` from the host filesystem.

### Fix Focus Areas
- Add strict validation for URL-base script fields: if a URL base declares an absolute path (or `${VAR}`) in pre_script/post_script/agent_input/validation_loop.script (and forge equivalents), return an error.
- Consider also rejecting `..` segments for URL-base script relpaths to ensure scripts stay under the base URL directory.

- internal/harness/compose.go[479-559]

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

---

2. ADR-0038 adds base: exception 📜 Skill insight § Compliance

Description

This PR modifies an already Accepted ADR to introduce a new decision/exception (base:-inherited
scripts fetched from URL bases), which is a substantive change to the original decision and security
model. Accepted ADRs should not be rewritten this way; instead the change should be captured in a
new ADR that supersedes or refines the accepted one.

Code

docs/ADRs/0038-universal-harness-access.md[R135-136]

+- **Executable resources (scripts, binaries) must be local files** (Option C restriction) to preserve auditability and prevent direct code execution from untrusted sources. Standalone URL references in script fields (`pre_script: https://...`) are rejected at validation time
+- **Exception: `base:` composition (ADR-0045).** When a harness inherits from a URL-referenced base via the `base:` field, scripts declared in the base harness are fetched from the same source as the base itself. The trust model is transitive: the base harness content is SHA256-pinned, and scripts referenced within that pinned content are fetched from the same origin (same org, repo, and commit SHA). After fetching, scripts are cached content-addressed and their paths are rewritten to local cache paths before validation, preserving the invariant that all script fields are local paths at validation time

Relevance

⭐⭐ Medium

Policy says substantial accepted-ADR rewrites need new superseding ADR, but ADR-0038 edited
post-acceptance in phases.

PR-#1966
PR-#2049
PR-#2139

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

ADR-0038 is marked Accepted, yet the PR adds a new explicit Decision exception for base:
composition and expands the security implications to justify fetching scripts from URL-referenced
bases. These are substantive changes to the accepted decision rather than an allowed annotation, so
the change should be captured via a superseding/new ADR rather than rewriting the accepted ADR in
place.

Rule 1062057: Restrict modifications to accepted ADRs on main
Rule 1062058: Supersede accepted ADRs with new ADRs instead of modifying history
docs/ADRs/0038-universal-harness-access.md[130-136]
docs/ADRs/0038-universal-harness-access.md[148-151]
docs/ADRs/0038-universal-harness-access.md[176-181]
Skill: writing-adrs

Agent prompt

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

## Issue description
`docs/ADRs/0038-universal-harness-access.md` is in `Accepted` status, but this PR introduces a new exception and rationale (URL-base script fetching) by editing the Decision/Consequences/Security sections. That is a substantive history change to an accepted ADR.

## Issue Context
The compliance rules require accepted ADRs to be modified only with minor annotations (typos, cross-references, small notes). Substantive decision/rationale changes should be recorded in a new ADR that supersedes or refines the old one, with the old ADR updated only to point to the newer ADR.

## Fix Focus Areas
- docs/ADRs/0038-universal-harness-access.md[130-181]
- docs/ADRs/00xx-<new-adr-for-base-script-fetching>.md[1-200]

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

---

3. Agent_input fetched as file 🐞 Bug ≡ Correctness

Description

resolveBaseScripts routes agent_input through fetchBaseScript (single-file fetch + rewrite to cached
file path), but the runtime treats agent_input as a directory and uploads <agent_input>/.
recursively. URL-base harnesses that set agent_input as a directory (per docs/runtime behavior) will
fail to resolve or will break at runtime after being rewritten to a file path.

Code

internal/harness/compose.go[R493-511]

+	scriptFields := []struct {
+		name string
+		ptr  *string
+	}{
+		{"pre_script", &base.PreScript},
+		{"post_script", &base.PostScript},
+		{"agent_input", &base.AgentInput},
+	}
+
+	for _, f := range scriptFields {
+		if *f.ptr == "" || IsURL(*f.ptr) || filepath.IsAbs(*f.ptr) {
+			continue
+		}
+		dep, cachePath, err := fetchBaseScript(ctx, f.name, baseURLDir, *f.ptr, allowlist, opts)
+		if err != nil {
+			return nil, err
+		}
+		*f.ptr = cachePath
+		deps = append(deps, dep)

Relevance

⭐⭐ Medium

No clear prior reviews on agent_input directory vs file; similar compose changes merged recently
without this concern.

PR-#2180
PR-#2270

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

agent_input is documented and used at runtime as a directory upload, but the new URL-base resolver
fetches it via a single file URL fetch and rewrites it to a cached file path.

docs/ADRs/0024-harness-definitions.md[369-375]
internal/cli/run.go[682-693]
internal/harness/compose.go[493-512]
internal/harness/compose.go[561-615]

Agent prompt

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

### Issue description
`agent_input` is treated like a script file during URL-base resolution, but runtime uses it as a directory. This mismatch will cause failures when URL bases attempt to provide agent_input.

### Issue Context
- Docs and runtime behavior indicate `agent_input` is a directory copied into the sandbox.
- Current implementation fetches a single URL into a cached file and rewrites `AgentInput` to that file path.

### Fix Focus Areas
- Decide and enforce contract:
 - If `agent_input` must be a directory: implement directory-tree fetch/caching for URL bases (likely requires forge client / directory listing support) and rewrite `AgentInput` to the cached directory path.
 - If `agent_input` may be a file: update runtime upload logic to handle single files (don’t append `/.`, use UploadFile semantics).
- Avoid chmod-to-executable for agent_input (whether file or directory).

- internal/harness/compose.go[479-640]
- internal/cli/run.go[682-696]
- docs/ADRs/0024-harness-definitions.md[369-375]

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

---

4. urlDirPrefix keeps query 🐞 Bug ≡ Correctness

Description

urlDirPrefix does not clear RawQuery, so base URLs with query parameters produce a directory prefix
containing ?…, and fetchBaseScript then concatenates relPath onto that string, yielding an invalid
script URL. This breaks base-script fetching for any base URL that relies on query parameters.

Code

internal/harness/compose.go[R661-677]

+func urlDirPrefix(rawURL string) string {
+	cleanURL, _, _ := ParseIntegrityHash(rawURL)
+	parsed, err := url.Parse(cleanURL)
+	if err != nil {
+		return ""
+	}
+	dir := path.Dir(parsed.Path)
+	if dir == "." || dir == "" {
+		return ""
+	}
+	if !strings.HasSuffix(dir, "/") {
+		dir += "/"
+	}
+	parsed.Path = dir
+	parsed.RawPath = ""
+	parsed.Fragment = ""
+	return parsed.String()

Relevance

⭐⭐⭐ High

Correctness edge-case fixes in URL/path handling are typically accepted; no counterexamples found.

PR-#1760
PR-#2180

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

urlDirPrefix resets Path/RawPath/Fragment but not RawQuery, and fetchBaseScript uses raw string
concatenation to build the final script URL.

internal/harness/compose.go[661-677]
internal/harness/compose.go[564-566]

Agent prompt

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

### Issue description
`urlDirPrefix` clears fragment but leaves the query string intact; later code concatenates `baseURLDir + relPath`, which becomes malformed if `baseURLDir` contains `?query=...`.

### Issue Context
`fetchBaseScript` constructs script URLs by string concatenation, so `baseURLDir` must be a clean prefix without fragment/query components.

### Fix Focus Areas
- Clear `parsed.RawQuery` (and possibly `parsed.ForceQuery`) in `urlDirPrefix`.
- Consider constructing script URLs via `url.Parse` + `ResolveReference` instead of raw string concatenation.

- internal/harness/compose.go[658-678]
- internal/harness/compose.go[564-566]

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

---

5. URL index non-atomic writes 🐞 Bug ☼ Reliability

Description

urlIndexPut rewrites .fullsend-cache/url-index.json using os.WriteFile without atomic rename or
locking, so concurrent fetches or crashes can corrupt/truncate the file. urlIndexLookup treats JSON
unmarshal errors as cache misses, which can break offline mode even when cached content exists.

Code

internal/harness/compose.go[R704-728]

+func urlIndexPut(workspaceRoot, rawURL, hash string) error {
+	if workspaceRoot == "" {
+		return nil
+	}
+	idxPath := urlIndexPath(workspaceRoot)
+	if err := os.MkdirAll(filepath.Dir(idxPath), 0o700); err != nil {
+		return err
+	}
+
+	var index map[string]string
+	data, err := os.ReadFile(idxPath)
+	if err == nil {
+		_ = json.Unmarshal(data, &index)
+	}
+	if index == nil {
+		index = make(map[string]string)
+	}
+	index[rawURL] = hash
+
+	out, err := json.MarshalIndent(index, "", "  ")
+	if err != nil {
+		return err
+	}
+	return os.WriteFile(idxPath, out, 0o600)
+}

Relevance

⭐⭐⭐ High

Repo prefers atomic temp+rename writes for cache/lock files; non-atomic index write likely rejected
in review.

PR-#1554
PR-#2049

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The index writer uses os.WriteFile directly, while the reader returns a miss on JSON parse error;
thus corruption causes offline lookups to fail silently.

internal/harness/compose.go[685-700]
internal/harness/compose.go[703-728]

Agent prompt

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

### Issue description
`url-index.json` is updated via a read-modify-write pattern with `os.WriteFile`, which is not atomic and has no locking. A partial write or concurrent update can corrupt the JSON; readers then treat it as a miss.

### Issue Context
Offline mode for base scripts depends on urlIndexLookup succeeding.

### Fix Focus Areas
- Use an atomic temp-file-then-rename write (similar to fetch/cache atomicWrite) for urlIndexPut.
- Add file locking (or at least best-effort lock) if concurrent processes are expected.
- Consider surfacing corruption errors distinctly (instead of silent miss) when Offline is enabled.

- internal/harness/compose.go[685-728]

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

---

6. Cached scripts chmod 0755 🐞 Bug ⛨ Security

Description

fetchBaseScript chmods cached content to 0755, widening access from the cache’s default private
permissions and making fetched content world-readable/executable on multi-user systems. This also
currently applies to agent_input because it shares the same fetch path.

Code

internal/harness/compose.go[R616-619]

+	// Make cached script executable.
+	if chErr := os.Chmod(contentPath, 0o755); chErr != nil {
+		return Dependency{}, "", fmt.Errorf("base script %s: setting executable permission: %w", field, chErr)
+	}

Relevance

⭐⭐ Medium

Repo previously added chmod +x workarounds; may prioritize executability over keeping cache
permissions restrictive.

PR-#343
PR-#1954

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The fetch cache writes content with private permissions, but base script fetching modifies the
cached file to be 0755, broadening access beyond what the cache uses by default.

internal/harness/compose.go[616-619]
internal/fetch/cache.go[95-137]
internal/fetch/cache.go[334-365]

Agent prompt

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

### Issue description
Cached resources are written with restrictive permissions, but base script fetches explicitly chmod to `0755`, making content readable/executable by group/others.

### Issue Context
Scripts only need to be executable by the current user to be run via `exec.Command`.

### Fix Focus Areas
- Change chmod mode to `0700` (or `0500`) for scripts.
- Do not chmod agent_input at all (and ensure agent_input doesn’t route through the script chmod path once fixed).

- internal/harness/compose.go[561-640]

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

---

[codecov]

Codecov Report

❌ Patch coverage is 77.33990% with 46 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/harness/compose.go	83.95%	15 Missing and 15 partials ⚠️
internal/cli/lock.go	0.00%	16 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:03 PM UTC · Completed 7:19 PM UTC
Commit: dcbbf8c · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [logic-error] internal/harness/compose.go — resolveBaseScripts does not resolve api_servers[].script from URL-referenced bases. When a URL base declares api_servers entries with relative script paths, those paths are merged into the child via mergeBaseIntoChild (which concatenates api_servers slices) but remain as unresolved relative paths with no valid local referent. ValidateResourceTypes only rejects URLs in api_servers[].script, not dangling relative paths, so these would pass validation but fail at runtime when the runner attempts to execute the script.
  Remediation: Either resolve api_servers[].script in resolveBaseScripts (fetching them like other scripts), or explicitly clear/reject api_servers entries with script fields from URL bases the way agent_input is cleared.

• [missing-integrity-check] internal/harness/compose.go — Scripts fetched via fetchBaseScript are not SHA256-pinned at fetch time. The base harness YAML is integrity-checked via #sha256=, but scripts within it are fetched without a pre-declared expected hash. When the base URL uses a mutable ref (branch name rather than commit SHA), script content could change between fetches even though the base harness hash is pinned. The ADR acknowledges this limitation and recommends commit SHAs for production use, but the code does not enforce or warn about mutable refs. See also: [trust-model-drift] — the exception text in ADR-0038 documents this gap.
  Remediation: Consider supporting per-script integrity hashes (e.g., a sha256 field per script entry in the base harness YAML) or validate that the base URL contains a commit-SHA-like segment for known forge URL patterns.

• [scope-adr-modification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status Accepted on main. The PR adds substantial new policy text to the Decision section introducing the base: composition exception, and modifies the "What changes" and "Security implications" sections. Per AGENTS.md: "Do not substantially rewrite its Context, Decision, or Consequences sections." These additions are substantive new policy (a new exception to the "no remote executables" rule), not minor annotations or cross-references.
  Remediation: Consider documenting the base: composition exception in a new ADR that supersedes the relevant aspect of ADR-0038, with a cross-reference annotation in ADR-0038.

• [adr-coherence-conflict] docs/ADRs/0045-forge-portable-harness-schema.md — ADR-0045 was accepted on main with the explicit design assumption (lines 385-388): "scripts are always scaffolded locally (ADR 0038's 'no remote executables' rule)." This PR replaces that text with the opposite behavior — scripts in URL-referenced bases are now fetched from the base URL's directory. The original design explicitly depended on scripts being local; changing it in-place without a superseding ADR obscures the decision trail.
  Remediation: Add a note to ADR-0045 acknowledging the design change with a reference to the new or superseding ADR, or write a new ADR that supersedes ADR-0045's script resolution model.

Low

• [race-condition] internal/harness/compose.go — urlIndexPut performs a non-atomic read-modify-write (ReadFile → Unmarshal → modify map → MarshalIndent → WriteFile) on url-index.json. Concurrent LoadWithBase calls resolving different scripts could lose index entries. The practical impact is limited — this is an optimization cache for offline mode and a lost entry only causes a re-fetch on the next online run.

• [path-traversal] internal/harness/compose.go — validateBaseScriptPath splits on / to detect .. path traversal but does not handle URL-encoded traversal sequences (e.g., %2e%2e) in the relPath. The downstream matchingAllowedPrefix with normalizeURLPath provides the actual security boundary (URL-decodes, runs path.Clean, checks prefix), so this is a defense-in-depth gap, not a live vulnerability.

• [error-handling-gap] internal/cli/lock.go — In the resolveFromLock validation_loop.script case, when h.ValidationLoop is nil, the cached script path and its chmod are silently skipped. The lock file asserts a dependency exists for cache verification, but the harness silently ignores it. This can occur when a child harness overrides the base's validation_loop to nil after the lock file was created.

• [function-comment-style] internal/harness/compose.go — validateBaseScriptPath lacks a doc comment, unlike other validation functions and the other new functions in this PR (resolveBaseScripts, fetchBaseScript, urlDirPrefix, etc.).

• [naming-alignment] internal/harness/compose.go — fetchBaseScript naming suggests parallel operation with fetchBaseURL but has different integrity guarantees (fetchBaseURL verifies against SHA256; fetchBaseScript does not). Consider a clarifying doc comment noting this distinction.

Previous run

Review

Findings

Medium

• [scope-adr-modification] docs/ADRs/0038-universal-harness-access.md:10 — ADR-0038 has status Accepted on main. The PR adds substantial new policy text to the Decision section introducing the base: composition exception, and modifies the "What changes" and "Security implications" sections. Per AGENTS.md: "Do not substantially rewrite its Context, Decision, or Consequences sections." These additions are substantive new policy (a new exception to the "no remote executables" rule), not minor annotations or cross-references.
  Remediation: Consider documenting the base: composition exception in a new ADR that supersedes the relevant aspect of ADR-0038, with a cross-reference annotation in ADR-0038.

• [adr-coherence-conflict] docs/ADRs/0045-forge-portable-harness-schema.md:41 — ADR-0045 was accepted on main with the explicit design assumption (lines 385-388): "scripts are always scaffolded locally (ADR 0038's 'no remote executables' rule)." This PR replaces that text with the opposite behavior — scripts in URL-referenced bases are now fetched from the base URL's directory. The original design explicitly depended on scripts being local; changing it in-place without a superseding ADR obscures the decision trail.
  Remediation: Add a note to ADR-0045 acknowledging the design change with a reference to the new or superseding ADR, or write a new ADR that supersedes ADR-0045's script resolution model.

• [race-condition] internal/harness/compose.go:402 — urlIndexPut performs a non-atomic read-modify-write (ReadFile → Unmarshal → modify map → MarshalIndent → WriteFile) on url-index.json. Concurrent LoadWithBase calls resolving different scripts could lose index entries. The practical impact is limited — this is an optimization cache for offline mode and a lost entry only causes a re-fetch on the next online run — but the race is real.
  Remediation: Use a file lock or atomic rename (write to temp file, then os.Rename). Alternatively, document the data-loss risk as an accepted trade-off for the current implementation.

Low

• [error-handling-gap] internal/cli/lock.go:637 — In the new resolveFromLock cases for pre_script, post_script, and validation_loop.script, os.Chmod errors are silently discarded with _ = os.Chmod(m.localPath, 0o755). By contrast, fetchBaseScript in compose.go (also in this PR) properly returns chmod errors on both cache-hit and fresh-fetch paths. If chmod fails during lock-based restoration, the script will be non-executable at runtime.

• [error-handling-gap] internal/cli/lock.go:641 — The validation_loop.script case silently drops the cached script path when h.ValidationLoop is nil. While the lock staleness check should catch harness edits in normal operation, the silent drop masks a potential consistency issue during development or partial lock updates.

• [missing-integrity-check] internal/harness/compose.go:256 — Individual scripts fetched via fetchBaseScript are not SHA256-pinned at fetch time. The base harness YAML is integrity-checked via #sha256=, but scripts within it are fetched without a pre-declared expected hash. The ADR acknowledges this: "operators should ensure base URLs contain commit SHAs for production use."

• [path-validation-backslash] internal/harness/compose.go:244 — validateBaseScriptPath splits on / to detect .. path traversal but does not check for backslash separators. Not exploitable on Linux (the target platform), but rejecting \ would improve defense-in-depth.

• [missing-authorization] internal/harness/compose.go — This PR introduces ~305 lines of new script fetching functionality without a linked issue. Linking to an issue that documents the problem, security implications, and design rationale would improve traceability.

• [trust-model-drift] docs/ADRs/0038-universal-harness-access.md:11 — The exception text acknowledges that mutable refs allow scripts to change between fetches despite the base harness hash being pinned. The PR documents this limitation and recommends commit SHAs for production use, which is appropriate, but the code does not enforce or warn about mutable refs.

• [naming-alignment] internal/harness/compose.go — fetchBaseScript naming suggests parallel operation with fetchBaseURL but has different integrity guarantees (fetchBaseURL verifies against SHA256; fetchBaseScript does not). Consider a clarifying doc comment or rename.

• [missing-documentation] Multiple documentation files (docs/plans/universal-harness-access-phase1.md, docs/plans/universal-harness-access.md, docs/guides/user/building-custom-agents.md, docs/guides/user/customizing-agents.md) do not yet reflect the new url-index.json mechanism, the base_script FetchType, or the base: composition exception for scripts.

• [comment-style] internal/harness/compose_test.go:663 — TestLoadWithBase_URLBase_ChildOverridesScript has a misleading initial comment; both scripts are fetched before merge, not just one.

• [function-comment-style] internal/harness/compose.go:231 — validateBaseScriptPath lacks a doc comment, unlike other validation functions in the file (ValidateResourceTypes, ValidateAllowedRemoteResources).

• [error-message-consistency] internal/harness/compose.go:233 — Error messages in validateBaseScriptPath use "base script %s must not contain" while the codebase convention uses "%s must be/must not be" without repeating the field prefix.

Previous run

Review

Findings

High

• [consumer-completeness] internal/cli/lock.go:628 — The lockfile restore path (resolveFromLock) has a switch on m.field that handles "agent", "policy", "base", and skills[N] but has no cases for the new script-typed dependency fields: "pre_script", "post_script", "validation_loop.script", or forge variants. When a lockfile contains script dependencies (produced by resolveBaseScripts), the default branch attempts fmt.Sscanf for skills[%d], fails, and the script's cache path gets appended to h.Skills instead of setting the correct harness field. This corrupts the skills array with spurious entries and leaves the actual script fields unchanged.
  Remediation: Add cases to the switch in resolveFromLock for "pre_script", "post_script", "validation_loop.script", and the forge-prefixed variants. Each case should set the corresponding harness field to m.localPath. Also add os.Chmod(localPath, 0o755) for dependencies where lockDep.Type == "script".

Medium

• [missing-authorization] internal/harness/compose.go — This PR introduces ~305 lines of new script fetching functionality from URL-referenced base harnesses without a linked issue. Per AGENTS.md, non-trivial changes require explicit authorization through a linked issue.
  Remediation: Link this PR to an issue that documents the problem, analyzes security implications, evaluates alternatives, and records the decision.

• [scope-adr-modification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status Accepted on main. The additions introduce new policy text including the base: composition exception. Per AGENTS.md: "Do not substantially rewrite Context, Decision, or Consequences sections." The new exception text adds policy to the Decision section.
  Remediation: Consider creating a new ADR that supersedes the relevant aspect of ADR-0038, or document why the in-place update is appropriate.

• [error-handling-gap] internal/harness/compose.go:234 — On the cache-hit path in fetchBaseScript, the chmod error is silently discarded: _ = os.Chmod(contentPath, 0o755). On the fresh-fetch path (line 273), the same chmod error is propagated as fatal. If chmod fails on a cached script, the function silently returns a non-executable script path, leading to a confusing runtime failure.
  Remediation: Propagate the chmod error on the cache-hit path: if chErr := os.Chmod(contentPath, 0o755); chErr != nil { return Dependency{}, "", fmt.Errorf(...) }.

• [race-condition] internal/harness/compose.go:360 — urlIndexPut performs a non-atomic read-modify-write on url-index.json. Concurrent LoadWithBase calls could lose index entries. Lost entries cause cache misses, and in offline mode this would cause failures.
  Remediation: Use advisory file locking or an atomic write pattern (write to temp file, rename).

• [logic-error] internal/harness/compose.go:505 — resolveBaseScripts excludes agent_input from fetching, yet mergeBaseIntoChild copies the base's AgentInput relative path to the child. When the base is a URL, this relative path resolves against the child's local directory (baseDir = childDir), which won't contain the base's agent_input directory. Results in an invalid path that fails at runtime. See also: [design-coherence] finding about agent_input classification.
  Remediation: Clear base.AgentInput before merge when the base is URL-referenced, or emit a warning/error at compose time when a URL-base declares agent_input and the child does not override it.

• [adr-coherence-conflict] docs/ADRs/0045-forge-portable-harness-schema.md:385 — ADR-0045 was accepted with the explicit design assumption: "scripts are always scaffolded locally (ADR 0038's 'no remote executables' rule)." This PR replaces that text with the opposite behavior without documenting the design change as a superseding decision.
  Remediation: Add a note to ADR-0045 acknowledging the design change with a reference to the new/superseding ADR or the base: composition exception in ADR-0038.

Low

• [missing-integrity-check] internal/harness/compose.go:216 — The base harness YAML is SHA256-pinned, but individual scripts are not. When the base URL uses a mutable ref (branch name rather than commit SHA), script content could change between fetches. The ADR documents this as a known limitation.

• [url-injection-via-query-fragment] internal/harness/compose.go:216 — validateBaseScriptPath does not reject ? or # characters in script paths. Since script paths come from SHA256-pinned base harness content the risk is minimal, but these characters could alter HTTP request semantics via string concatenation in fetchBaseScript. See also: [edge-case] finding at this location.

• [test-accuracy] internal/harness/compose_test.go:641 — TestLoadWithBase_URLBase_ChildOverridesScript: comment says "only post_script should be fetched" but both scripts are fetched (deps has 3 entries). The subsequent comment correctly explains why, but the initial comment is misleading.

• [design-coherence] internal/harness/compose.go:108 — agent_input is excluded from script fetching because it's a directory, but ADR-0038 classifies it alongside scripts as an executable resource field. Clarifying this distinction in ADR documentation would prevent future misalignment. See also: [logic-error] finding about agent_input path resolution.

• [path-validation-backslash] internal/harness/compose.go:204 — validateBaseScriptPath splits on / to detect .. traversal but does not account for backslash separators. Not exploitable on Linux.

• [missing-documentation] Multiple documentation files (docs/plans/universal-harness-access-phase1.md, docs/guides/user/running-agents-locally.md, docs/guides/user/building-custom-agents.md, docs/guides/user/customizing-agents.md, docs/plans/adr-0045-forge-portable-harness-phase1.md, docs/plans/universal-harness-access-phase4.md) do not yet reflect the new url-index.json mechanism, the base_script FetchType, or the base: composition exception for scripts.

Previous run (2)

Review

Findings

Medium

• [missing-integrity-check] internal/harness/compose.go — Fetched scripts have no content integrity verification. The base harness YAML is SHA256-pinned, but individual scripts referenced within it are not. If the base URL uses a branch ref rather than a commit SHA (e.g., https://raw.githubusercontent.com/org/repo/main/harness/triage.yaml), script content at derived URLs could change while the base harness hash remains valid. The ADR claims "transitive trust" but this only holds when the URL path contains an immutable commit SHA.
  Remediation: Consider supporting per-script integrity hashes (e.g., pre_script: scripts/pre.sh#sha256=...) or validate that the base URL contains a commit-SHA-like segment for known forge URL patterns.

• [missing-executable-permission] internal/harness/compose.go — On the cache-hit path (when urlIndexLookup succeeds), the cached script file is returned without calling os.Chmod(contentPath, 0o755). The chmod only runs on the fresh-fetch path. In practice, file permissions persist on disk from the initial fetch, so this only matters if the cache was populated by a different code path (e.g., CachePut uses atomicWrite which sets 0o600). Adding chmod to the cache-hit branch would improve robustness.
  Remediation: Add os.Chmod(contentPath, 0o755) to the cache-hit branch in fetchBaseScript.

• [scope-adr-modification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 is Accepted on main. The additions (~300 words of new policy including a base: composition exception) go beyond the "minor annotations" permitted by AGENTS.md for accepted ADRs. The existing text is preserved and extended rather than rewritten, which is a borderline case, but the new Exception subsection adds substantial new policy to the Decision section.
  Remediation: Consider documenting the base: composition exception in a new ADR that supersedes this aspect of ADR-0038, with a cross-reference annotation in ADR-0038.

• [missing-referenced-adr] docs/ADRs/0038-universal-harness-access.md — The new text references "ADR-0045" for the base: composition feature. ADR-0045 (0045-forge-portable-harness-schema.md) explicitly states at lines 385-387: "resolve against the local .fullsend/ directory, not the base's origin. This works because scripts are always scaffolded locally (ADR 0038's 'no remote executables' rule)." This PR introduces behavior that ADR-0045 explicitly designed around NOT having. ADR-0045 should be updated to reflect the new capability, or a new ADR should document the design rationale.
  Remediation: Update ADR-0045 to reflect the new script-fetching behavior for URL-referenced bases, or file a new ADR that supersedes the relevant section.

Low

• [path-validation] internal/harness/compose.go — validateBaseScriptPath uses strings.Contains(val, "..") which is overly broad (rejects legitimate filenames like foo..bar.sh) and bypassable via URL-encoded variants (%2e%2e). The downstream matchingAllowedPrefix with normalizeURLPath provides the real security boundary (percent-decodes, runs path.Clean, checks prefix). Consider splitting on / and checking each segment for .. as defense-in-depth. See also: [url-input-validation] finding at this location.

• [url-input-validation] internal/harness/compose.go — validateBaseScriptPath doesn't reject null bytes or query string markers in script paths. Downstream normalizeURLPath handles backslashes and url.Parse separates query/fragments, so the allowlist provides the real protection. Minor hardening of input validation would improve defense-in-depth. See also: [path-validation] finding at this location.

• [race-condition] internal/harness/compose.go — urlIndexPut performs a read-modify-write on url-index.json without file locking. Concurrent processes could lose updates. Low severity since lost updates only cause cache misses requiring a re-fetch.

• [test-accuracy] internal/harness/compose_test.go — TestLoadWithBase_URLBase_ChildOverridesScript has a misleading initial comment: "1 base + 1 script (only post_script fetched)" but asserts Len(deps, 3) (1 base + 2 scripts). The subsequent comment correctly explains that both scripts are fetched before merge.

• [error-message-consistency] internal/harness/compose.go — Error messages in validateBaseScriptPath use "are not allowed" phrasing while the codebase convention (e.g., ValidateResourceTypes) uses positive assertions ("must be a local path, not a URL").

---

Labels: PR modifies harness composition logic and ADR documentation

---

Labels: PR modifies the trust model for executable resources by adding remote script fetching, warranting the security label alongside existing component labels.

Previous run

Review

Findings

Medium

• [scope-adr-modification] docs/ADRs/0038-universal-harness-access.md:10 — ADR-0038 has status Accepted on main. The PR adds substantial new policy text to the Decision section introducing the base: composition exception, and modifies the "What changes" and "Security implications" sections. Per AGENTS.md: "Do not substantially rewrite its Context, Decision, or Consequences sections." These additions are substantive new policy (a new exception to the "no remote executables" rule), not minor annotations or cross-references.
  Remediation: Consider documenting the base: composition exception in a new ADR that supersedes the relevant aspect of ADR-0038, with a cross-reference annotation in ADR-0038.

• [adr-coherence-conflict] docs/ADRs/0045-forge-portable-harness-schema.md:41 — ADR-0045 was accepted on main with the explicit design assumption (lines 385-388): "scripts are always scaffolded locally (ADR 0038's 'no remote executables' rule)." This PR replaces that text with the opposite behavior — scripts in URL-referenced bases are now fetched from the base URL's directory. The original design explicitly depended on scripts being local; changing it in-place without a superseding ADR obscures the decision trail.
  Remediation: Add a note to ADR-0045 acknowledging the design change with a reference to the new or superseding ADR, or write a new ADR that supersedes ADR-0045's script resolution model.

• [race-condition] internal/harness/compose.go:402 — urlIndexPut performs a non-atomic read-modify-write (ReadFile → Unmarshal → modify map → MarshalIndent → WriteFile) on url-index.json. Concurrent LoadWithBase calls resolving different scripts could lose index entries. The practical impact is limited — this is an optimization cache for offline mode and a lost entry only causes a re-fetch on the next online run — but the race is real.
  Remediation: Use a file lock or atomic rename (write to temp file, then os.Rename). Alternatively, document the data-loss risk as an accepted trade-off for the current implementation.

Low

• [error-handling-gap] internal/cli/lock.go:637 — In the new resolveFromLock cases for pre_script, post_script, and validation_loop.script, os.Chmod errors are silently discarded with _ = os.Chmod(m.localPath, 0o755). By contrast, fetchBaseScript in compose.go (also in this PR) properly returns chmod errors on both cache-hit and fresh-fetch paths. If chmod fails during lock-based restoration, the script will be non-executable at runtime.

• [error-handling-gap] internal/cli/lock.go:641 — The validation_loop.script case silently drops the cached script path when h.ValidationLoop is nil. While the lock staleness check should catch harness edits in normal operation, the silent drop masks a potential consistency issue during development or partial lock updates.

• [missing-integrity-check] internal/harness/compose.go:256 — Individual scripts fetched via fetchBaseScript are not SHA256-pinned at fetch time. The base harness YAML is integrity-checked via #sha256=, but scripts within it are fetched without a pre-declared expected hash. The ADR acknowledges this: "operators should ensure base URLs contain commit SHAs for production use."

• [path-validation-backslash] internal/harness/compose.go:244 — validateBaseScriptPath splits on / to detect .. path traversal but does not check for backslash separators. Not exploitable on Linux (the target platform), but rejecting \ would improve defense-in-depth.

• [missing-authorization] internal/harness/compose.go — This PR introduces ~305 lines of new script fetching functionality without a linked issue. Linking to an issue that documents the problem, security implications, and design rationale would improve traceability.

• [trust-model-drift] docs/ADRs/0038-universal-harness-access.md:11 — The exception text acknowledges that mutable refs allow scripts to change between fetches despite the base harness hash being pinned. The PR documents this limitation and recommends commit SHAs for production use, which is appropriate, but the code does not enforce or warn about mutable refs.

• [naming-alignment] internal/harness/compose.go — fetchBaseScript naming suggests parallel operation with fetchBaseURL but has different integrity guarantees (fetchBaseURL verifies against SHA256; fetchBaseScript does not). Consider a clarifying doc comment or rename.

• [missing-documentation] Multiple documentation files (docs/plans/universal-harness-access-phase1.md, docs/plans/universal-harness-access.md, docs/guides/user/building-custom-agents.md, docs/guides/user/customizing-agents.md) do not yet reflect the new url-index.json mechanism, the base_script FetchType, or the base: composition exception for scripts.

• [comment-style] internal/harness/compose_test.go:663 — TestLoadWithBase_URLBase_ChildOverridesScript has a misleading initial comment; both scripts are fetched before merge, not just one.

• [function-comment-style] internal/harness/compose.go:231 — validateBaseScriptPath lacks a doc comment, unlike other validation functions in the file (ValidateResourceTypes, ValidateAllowedRemoteResources).

• [error-message-consistency] internal/harness/compose.go:233 — Error messages in validateBaseScriptPath use "base script %s must not contain" while the codebase convention uses "%s must be/must not be" without repeating the field prefix.

Previous run (2)

Review

Findings

High

• [consumer-completeness] internal/cli/lock.go:628 — The lockfile restore path (resolveFromLock) has a switch on m.field that handles "agent", "policy", "base", and skills[N] but has no cases for the new script-typed dependency fields: "pre_script", "post_script", "validation_loop.script", or forge variants. When a lockfile contains script dependencies (produced by resolveBaseScripts), the default branch attempts fmt.Sscanf for skills[%d], fails, and the script's cache path gets appended to h.Skills instead of setting the correct harness field. This corrupts the skills array with spurious entries and leaves the actual script fields unchanged.
  Remediation: Add cases to the switch in resolveFromLock for "pre_script", "post_script", "validation_loop.script", and the forge-prefixed variants. Each case should set the corresponding harness field to m.localPath. Also add os.Chmod(localPath, 0o755) for dependencies where lockDep.Type == "script".

Medium

• [missing-authorization] internal/harness/compose.go — This PR introduces ~305 lines of new script fetching functionality from URL-referenced base harnesses without a linked issue. Per AGENTS.md, non-trivial changes require explicit authorization through a linked issue.
  Remediation: Link this PR to an issue that documents the problem, analyzes security implications, evaluates alternatives, and records the decision.

• [scope-adr-modification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status Accepted on main. The additions introduce new policy text including the base: composition exception. Per AGENTS.md: "Do not substantially rewrite Context, Decision, or Consequences sections." The new exception text adds policy to the Decision section.
  Remediation: Consider creating a new ADR that supersedes the relevant aspect of ADR-0038, or document why the in-place update is appropriate.

• [error-handling-gap] internal/harness/compose.go:234 — On the cache-hit path in fetchBaseScript, the chmod error is silently discarded: _ = os.Chmod(contentPath, 0o755). On the fresh-fetch path (line 273), the same chmod error is propagated as fatal. If chmod fails on a cached script, the function silently returns a non-executable script path, leading to a confusing runtime failure.
  Remediation: Propagate the chmod error on the cache-hit path: if chErr := os.Chmod(contentPath, 0o755); chErr != nil { return Dependency{}, "", fmt.Errorf(...) }.

• [race-condition] internal/harness/compose.go:360 — urlIndexPut performs a non-atomic read-modify-write on url-index.json. Concurrent LoadWithBase calls could lose index entries. Lost entries cause cache misses, and in offline mode this would cause failures.
  Remediation: Use advisory file locking or an atomic write pattern (write to temp file, rename).

• [logic-error] internal/harness/compose.go:505 — resolveBaseScripts excludes agent_input from fetching, yet mergeBaseIntoChild copies the base's AgentInput relative path to the child. When the base is a URL, this relative path resolves against the child's local directory (baseDir = childDir), which won't contain the base's agent_input directory. Results in an invalid path that fails at runtime. See also: [design-coherence] finding about agent_input classification.
  Remediation: Clear base.AgentInput before merge when the base is URL-referenced, or emit a warning/error at compose time when a URL-base declares agent_input and the child does not override it.

• [adr-coherence-conflict] docs/ADRs/0045-forge-portable-harness-schema.md:385 — ADR-0045 was accepted with the explicit design assumption: "scripts are always scaffolded locally (ADR 0038's 'no remote executables' rule)." This PR replaces that text with the opposite behavior without documenting the design change as a superseding decision.
  Remediation: Add a note to ADR-0045 acknowledging the design change with a reference to the new/superseding ADR or the base: composition exception in ADR-0038.

Low

• [missing-integrity-check] internal/harness/compose.go:216 — The base harness YAML is SHA256-pinned, but individual scripts are not. When the base URL uses a mutable ref (branch name rather than commit SHA), script content could change between fetches. The ADR documents this as a known limitation.

• [url-injection-via-query-fragment] internal/harness/compose.go:216 — validateBaseScriptPath does not reject ? or # characters in script paths. Since script paths come from SHA256-pinned base harness content the risk is minimal, but these characters could alter HTTP request semantics via string concatenation in fetchBaseScript. See also: [edge-case] finding at this location.

• [test-accuracy] internal/harness/compose_test.go:641 — TestLoadWithBase_URLBase_ChildOverridesScript: comment says "only post_script should be fetched" but both scripts are fetched (deps has 3 entries). The subsequent comment correctly explains why, but the initial comment is misleading.

• [design-coherence] internal/harness/compose.go:108 — agent_input is excluded from script fetching because it's a directory, but ADR-0038 classifies it alongside scripts as an executable resource field. Clarifying this distinction in ADR documentation would prevent future misalignment. See also: [logic-error] finding about agent_input path resolution.

• [path-validation-backslash] internal/harness/compose.go:204 — validateBaseScriptPath splits on / to detect .. traversal but does not account for backslash separators. Not exploitable on Linux.

• [missing-documentation] Multiple documentation files (docs/plans/universal-harness-access-phase1.md, docs/guides/user/running-agents-locally.md, docs/guides/user/building-custom-agents.md, docs/guides/user/customizing-agents.md, docs/plans/adr-0045-forge-portable-harness-phase1.md, docs/plans/universal-harness-access-phase4.md) do not yet reflect the new url-index.json mechanism, the base_script FetchType, or the base: composition exception for scripts.

Previous run (3)

Review

Findings

High

• [consumer-completeness] internal/cli/lock.go:628 — The lockfile restore path (resolveFromLock) has a switch on m.field that handles "agent", "policy", "base", and skills[N] but has no cases for the new script-typed dependency fields: "pre_script", "post_script", "validation_loop.script", or forge variants. When a lockfile contains script dependencies (produced by resolveBaseScripts), the default branch attempts fmt.Sscanf for skills[%d], fails, and the script's cache path gets appended to h.Skills instead of setting the correct harness field. This corrupts the skills array with spurious entries and leaves the actual script fields unchanged.
  Remediation: Add cases to the switch in resolveFromLock for "pre_script", "post_script", "validation_loop.script", and the forge-prefixed variants. Each case should set the corresponding harness field to m.localPath. Also add os.Chmod(localPath, 0o755) for dependencies where lockDep.Type == "script".

Medium

• [missing-authorization] internal/harness/compose.go — This PR introduces ~305 lines of new script fetching functionality from URL-referenced base harnesses without a linked issue. Per AGENTS.md, non-trivial changes require explicit authorization through a linked issue.
  Remediation: Link this PR to an issue that documents the problem, analyzes security implications, evaluates alternatives, and records the decision.

• [scope-adr-modification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status Accepted on main. The additions introduce new policy text including the base: composition exception. Per AGENTS.md: "Do not substantially rewrite Context, Decision, or Consequences sections." The new exception text adds policy to the Decision section.
  Remediation: Consider creating a new ADR that supersedes the relevant aspect of ADR-0038, or document why the in-place update is appropriate.

• [error-handling-gap] internal/harness/compose.go:234 — On the cache-hit path in fetchBaseScript, the chmod error is silently discarded: _ = os.Chmod(contentPath, 0o755). On the fresh-fetch path (line 273), the same chmod error is propagated as fatal. If chmod fails on a cached script, the function silently returns a non-executable script path, leading to a confusing runtime failure.
  Remediation: Propagate the chmod error on the cache-hit path: if chErr := os.Chmod(contentPath, 0o755); chErr != nil { return Dependency{}, "", fmt.Errorf(...) }.

• [race-condition] internal/harness/compose.go:360 — urlIndexPut performs a non-atomic read-modify-write on url-index.json. Concurrent LoadWithBase calls could lose index entries. Lost entries cause cache misses, and in offline mode this would cause failures.
  Remediation: Use advisory file locking or an atomic write pattern (write to temp file, rename).

• [logic-error] internal/harness/compose.go:505 — resolveBaseScripts excludes agent_input from fetching, yet mergeBaseIntoChild copies the base's AgentInput relative path to the child. When the base is a URL, this relative path resolves against the child's local directory (baseDir = childDir), which won't contain the base's agent_input directory. Results in an invalid path that fails at runtime. See also: [design-coherence] finding about agent_input classification.
  Remediation: Clear base.AgentInput before merge when the base is URL-referenced, or emit a warning/error at compose time when a URL-base declares agent_input and the child does not override it.

• [adr-coherence-conflict] docs/ADRs/0045-forge-portable-harness-schema.md:385 — ADR-0045 was accepted with the explicit design assumption: "scripts are always scaffolded locally (ADR 0038's 'no remote executables' rule)." This PR replaces that text with the opposite behavior without documenting the design change as a superseding decision.
  Remediation: Add a note to ADR-0045 acknowledging the design change with a reference to the new/superseding ADR or the base: composition exception in ADR-0038.

Low

• [missing-integrity-check] internal/harness/compose.go:216 — The base harness YAML is SHA256-pinned, but individual scripts are not. When the base URL uses a mutable ref (branch name rather than commit SHA), script content could change between fetches. The ADR documents this as a known limitation.

• [url-injection-via-query-fragment] internal/harness/compose.go:216 — validateBaseScriptPath does not reject ? or # characters in script paths. Since script paths come from SHA256-pinned base harness content the risk is minimal, but these characters could alter HTTP request semantics via string concatenation in fetchBaseScript. See also: [edge-case] finding at this location.

• [test-accuracy] internal/harness/compose_test.go:641 — TestLoadWithBase_URLBase_ChildOverridesScript: comment says "only post_script should be fetched" but both scripts are fetched (deps has 3 entries). The subsequent comment correctly explains why, but the initial comment is misleading.

• [design-coherence] internal/harness/compose.go:108 — agent_input is excluded from script fetching because it's a directory, but ADR-0038 classifies it alongside scripts as an executable resource field. Clarifying this distinction in ADR documentation would prevent future misalignment. See also: [logic-error] finding about agent_input path resolution.

• [path-validation-backslash] internal/harness/compose.go:204 — validateBaseScriptPath splits on / to detect .. traversal but does not account for backslash separators. Not exploitable on Linux.

• [missing-documentation] Multiple documentation files (docs/plans/universal-harness-access-phase1.md, docs/guides/user/running-agents-locally.md, docs/guides/user/building-custom-agents.md, docs/guides/user/customizing-agents.md, docs/plans/adr-0045-forge-portable-harness-phase1.md, docs/plans/universal-harness-access-phase4.md) do not yet reflect the new url-index.json mechanism, the base_script FetchType, or the base: composition exception for scripts.

Previous run (4)

Review

Findings

Medium

• [missing-integrity-check] internal/harness/compose.go — Fetched scripts have no content integrity verification. The base harness YAML is SHA256-pinned, but individual scripts referenced within it are not. If the base URL uses a branch ref rather than a commit SHA (e.g., https://raw.githubusercontent.com/org/repo/main/harness/triage.yaml), script content at derived URLs could change while the base harness hash remains valid. The ADR claims "transitive trust" but this only holds when the URL path contains an immutable commit SHA.
  Remediation: Consider supporting per-script integrity hashes (e.g., pre_script: scripts/pre.sh#sha256=...) or validate that the base URL contains a commit-SHA-like segment for known forge URL patterns.

• [missing-executable-permission] internal/harness/compose.go — On the cache-hit path (when urlIndexLookup succeeds), the cached script file is returned without calling os.Chmod(contentPath, 0o755). The chmod only runs on the fresh-fetch path. In practice, file permissions persist on disk from the initial fetch, so this only matters if the cache was populated by a different code path (e.g., CachePut uses atomicWrite which sets 0o600). Adding chmod to the cache-hit branch would improve robustness.
  Remediation: Add os.Chmod(contentPath, 0o755) to the cache-hit branch in fetchBaseScript.

• [scope-adr-modification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 is Accepted on main. The additions (~300 words of new policy including a base: composition exception) go beyond the "minor annotations" permitted by AGENTS.md for accepted ADRs. The existing text is preserved and extended rather than rewritten, which is a borderline case, but the new Exception subsection adds substantial new policy to the Decision section.
  Remediation: Consider documenting the base: composition exception in a new ADR that supersedes this aspect of ADR-0038, with a cross-reference annotation in ADR-0038.

• [missing-referenced-adr] docs/ADRs/0038-universal-harness-access.md — The new text references "ADR-0045" for the base: composition feature. ADR-0045 (0045-forge-portable-harness-schema.md) explicitly states at lines 385-387: "resolve against the local .fullsend/ directory, not the base's origin. This works because scripts are always scaffolded locally (ADR 0038's 'no remote executables' rule)." This PR introduces behavior that ADR-0045 explicitly designed around NOT having. ADR-0045 should be updated to reflect the new capability, or a new ADR should document the design rationale.
  Remediation: Update ADR-0045 to reflect the new script-fetching behavior for URL-referenced bases, or file a new ADR that supersedes the relevant section.

Low

• [path-validation] internal/harness/compose.go — validateBaseScriptPath uses strings.Contains(val, "..") which is overly broad (rejects legitimate filenames like foo..bar.sh) and bypassable via URL-encoded variants (%2e%2e). The downstream matchingAllowedPrefix with normalizeURLPath provides the real security boundary (percent-decodes, runs path.Clean, checks prefix). Consider splitting on / and checking each segment for .. as defense-in-depth. See also: [url-input-validation] finding at this location.

• [url-input-validation] internal/harness/compose.go — validateBaseScriptPath doesn't reject null bytes or query string markers in script paths. Downstream normalizeURLPath handles backslashes and url.Parse separates query/fragments, so the allowlist provides the real protection. Minor hardening of input validation would improve defense-in-depth. See also: [path-validation] finding at this location.

• [race-condition] internal/harness/compose.go — urlIndexPut performs a read-modify-write on url-index.json without file locking. Concurrent processes could lose updates. Low severity since lost updates only cause cache misses requiring a re-fetch.

• [test-accuracy] internal/harness/compose_test.go — TestLoadWithBase_URLBase_ChildOverridesScript has a misleading initial comment: "1 base + 1 script (only post_script fetched)" but asserts Len(deps, 3) (1 base + 2 scripts). The subsequent comment correctly explains that both scripts are fetched before merge.

• [error-message-consistency] internal/harness/compose.go — Error messages in validateBaseScriptPath use "are not allowed" phrasing while the codebase convention (e.g., ValidateResourceTypes) uses positive assertions ("must be a local path, not a URL").

---

Labels: PR modifies harness composition logic and ADR documentation

[ralphbean]

I think this needs a few changes before we can merge. See inline comments.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 7:30 PM UTC · Ended 7:30 PM UTC
Commit: 4e21a60 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 7:34 PM UTC · Completed 7:50 PM UTC
Commit: 091c671 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:58 PM UTC · Completed 8:13 PM UTC
Commit: d70669b · View workflow run →

[ralphbean]

Previous findings addressed. One minor note inline.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:16 PM UTC · Completed 9:56 PM UTC
Commit: 23a44bf · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 10:14 PM UTC · Completed 10:24 PM UTC
Commit: 23a44bf · View workflow run →

[fullsend-ai-retro]

Retro: PR #2525 — feat(harness): fetch scripts from URL-referenced base harnesses

Timeline

• 18:46 — PR opened by ggallen (human-authored, 1,172 additions across 5 files)
• 18:47–18:54 — Automated reviews from qodo-code-review[bot] (3 findings including a real security bug: absolute-path bypass) and fullsend-ai-review[bot]
• 18:56 — Author addresses automated findings
• 19:25 — ralphbean requests changes (5 inline comments: script integrity gap, error handling, chmod consistency)
• 19:29 — Author addresses all human findings
• 20:55 — ralphbean approves with one non-blocking note
• 21:12 — Final commit pushed
• 22:09 — PR merged. All CI checks passed (77.33% patch coverage).

Workflow Quality Assessment

Review quality: Strong. The fullsend-ai review bot and human reviewer found complementary issues. The bot caught the api_servers[].script resolution gap and ADR coherence conflict that the human didn't flag. The human caught chmod consistency across files (lock.go vs compose.go) and the non-atomic write concern. Both independently identified the script integrity gap (branch names vs commit SHAs). The bot also caught and helped fix issues in earlier iterations (lockfile restore path, chmod on cache-hit, agent_input path issue) before human review. Net: the bot was a net-positive contributor to review quality.

Rework rate: Efficient. One round of human changes requested, then approved. The author addressed all findings promptly. Total time from open to merge was ~3.5 hours, with most of that being wall-clock wait time.

Token cost: Moderate waste. 6 review dispatches were triggered (2 cancelled, 1 failed, 3 succeeded). 12 pull_request_review shim runs fired but all skipped dispatch. The failed run (ID 27978509380) completed its analysis and found real issues, but the post-review script crashed with a GitHub API 422 error when submitting inline comments targeting invalid diff positions — those findings were lost and had to be re-discovered in subsequent runs.

Existing Issue Coverage

All identified improvement areas are already tracked by open issues:

• Review dispatch waste / debouncing: Debounce review dispatch on rapid synchronize events #1014, Enable cancel-in-progress for review dispatches on the same PR #1357, Deduplicate review dispatches for the same HEAD SHA #1452, Deduplicate review runs on rapid successive pushes #1418, and 30+ other open issues
• pull_request_review event noise: Filter pull_request_review trigger to only fire on changes_requested #1125 (filter to only changes_requested), dispatch: filter bot-triggered pull_request_review events to prevent review self-triggering loop #1271 (filter bot-triggered events)
• Post-review 422 failure: post-review: handle GitHub API 422 on inline comment submission gracefully #1067 (handle 422 on inline comment submission gracefully) — still open and still occurring
• Review findings posted as issue comments instead of formal PR reviews: Use comment updates with collapsible sections for review completion (supersedes #584) #971, Review agent summary comment should reflect inline findings and verdict #2054

No new proposals are warranted. The existing issue backlog comprehensively covers the improvement opportunities identified in this workflow.