fix(#3640): directory-level fetching for pre/post scripts

[fullsend-ai-coder]

Summary

Extend resolveBaseScripts in compose.go to support directory-level fetching for pre/post scripts, analogous to how fetchBaseSkill handles skills with companion files. This prevents the recurring companion-file breakage pattern (#2705, #3182, #3069/PR #3393) where scripts that reference sibling files via BASH_SOURCE-relative paths fail because fetchBaseFile isolates each script in its own content-addressed cache entry.

Related Issue

Fixes #3640

Changes

• Added fetchBaseScriptOrDir function that wraps script fetching with directory-level fetch logic when the URL is parseable by forge.ParseRawContentURL (i.e., raw.githubusercontent.com URLs)
• Added fetchBaseScriptDirTree function (analogous to fetchBaseSkillDir) that uses TreeFetcher (git sparse checkout) to fetch the full script directory, caches it with CachePutDir, and makes all files executable
• Directory caching is keyed by scriptdir:<dirURL> so sibling scripts in the same directory share a single tree fetch — the second script hits the cache populated by the first
• Falls back to single-file fetchBaseFile for non-raw.githubusercontent.com URLs and scripts without a directory component (backward compatible)
• Updated resolveBaseScripts to call fetchBaseScriptOrDir instead of fetchBaseFile for all script fields (pre_script, post_script, validation_loop.script, forge scripts)

Testing

• Added TestFetchBaseScriptOrDir_DirectoryFetch — verifies companion files are co-located
• Added TestFetchBaseScriptOrDir_SiblingCacheHit — verifies cache sharing between sibling scripts
• Added TestFetchBaseScriptOrDir_FallbackToSingleFile — verifies fallback for non-raw URLs
• Added TestFetchBaseScriptOrDir_NoDirComponent — verifies scripts without directory components
• Added TestFetchBaseScriptOrDir_CompanionExecutable — verifies all files are made executable
• All existing harness tests pass (including script, skill, and SourceURL tests)
• make lint — pre-commit could not run due to sandbox network restrictions (git fetch origin --tags returned 403); post-script runs authoritative check

Checklist

• PR title follows Conventional Commits (correct type, ! for breaking changes)
• Commits are signed off (DCO) — human and human-directed agent sessions only
• I wrote this contribution myself and can explain all changes in it

---

Closes #3640

Post-script verification

• Branch is not main/master (agent/3640-dir-fetch-scripts-v2)
• Secret scan passed (gitleaks — b4fd4ae8b290e94e2a8c22918621146e28e92182..HEAD)
• PR body secret scan passed (gitleaks — no-git)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:54 PM UTC · Completed 7:11 PM UTC
Commit: 6b7b425 · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 76.36364% with 26 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/harness/compose.go	76.84%	11 Missing and 11 partials ⚠️
internal/cli/lock.go	73.33%	3 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Looks good to me

Previous run

Review

Findings

High

• [consumer-completeness] internal/cli/lock.go:738 — The lock resolution path (resolveFromLock) does not handle "directory" type for script fields (pre_script, post_script, validation_loop.script). When lockDep.Type == "directory", localPath is set to the tree directory root at line 746. This path is then assigned to h.PreScript (line 844), h.PostScript (line 849), or h.ValidationLoop.Script (line 855), causing the harness to attempt executing a directory instead of a file. The new fetchBaseScriptOrDir correctly stores filepath.Join(treePath, scriptName) in Dependency.LocalPath, but resolveFromLock reconstructs localPath from CacheGetDir which returns only the tree root. Forge scripts are unaffected (no-op mutations at lines 860–867). The existing TestResolveFromLock_DirectoryType only covers skills.
  Remediation: In resolveFromLock, when lockDep.Type == "directory" and the field is a script field, extract the script filename from lockDep.URL using path.Base(), apply CacheNamedSymlink with the parent directory name, and set localPath = filepath.Join(treePath, scriptName).

Low

• [edge-case] internal/harness/compose.go:1063 — fetchBaseScriptDirTree performs a second allowlist check for dirPrefix := scriptDirURL + "/" in addition to the caller's file URL check. This is intentional defense-in-depth (consistent with fetchBaseSkillDir at line 1047–1049) — the directory fetch retrieves all files, so the allowlist must authorize the directory scope. No action required.

• [error-handling] internal/harness/compose.go:978 — Unlike fetchBaseSkill, fetchBaseScriptOrDir does not implement stale-fallback for transient errors. On transient tree-fetch failure it falls through to single-file fetch via fetchBaseFile, which is a reasonable degradation (the script runs; only companion files are lost). Acceptable for a v1 implementation.

• [test-adequacy] internal/harness/compose_test.go — No integration test exercises the lock-file round-trip for directory-fetched scripts. The existing TestResolveFromLock_DirectoryType only covers skills. This gap directly corresponds to the high-severity finding above — a round-trip test would have caught the treePath vs contentPath mismatch.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (2)

Review

Findings

Medium

• [scope-creep] docs/landscape.md — Documentation changes to landscape.md and platform-nativeness.md are not mentioned in issue Harness compose should support directory-level fetching for pre/post scripts to prevent recurring companion-file breakage #3640 and appear to be unrelated updates about gh-aw (GitHub Agentic Workflows). Issue Harness compose should support directory-level fetching for pre/post scripts to prevent recurring companion-file breakage #3640 is specifically about extending harness compose to support directory-level fetching for pre/post scripts. These doc changes update gh-aw status descriptions and competitive analysis but have no connection to the companion-file breakage problem or the compose.go fix. Consider removing these documentation changes and filing them separately, or adding a note to the PR description explaining why they are bundled here.

Low

• [auth-bypass] internal/harness/compose.go:1009 — In the cache-hit path of fetchBaseScriptOrDir, os.Chmod(contentPath, 0o755) executes before the allowlist check at lines 1012–1014. If a previously-cached directory's script URL is later removed from allowed_remote_resources, the file is made executable before the allowlist rejection fires. The function does return an error so the caller never uses the path, but the chmod side-effect persists in the cache. This mirrors a pattern that fetchBaseFile avoids by checking the allowlist before any cache operations (line 889). Move the allowlist check above the chmod call for consistency.

• [pr-title-type] — PR title uses fix(#3640) which is defensible given the context: the change prevents a class of bug that caused three real incidents (bug(harness): v0.22.0 URL-base skill resolution drops companion files, breaks review agent for all users #2705, fix(prioritize): inline CSMA library into post-script #3182, Post-script companion scripts not accessible when resolved from URL-based cache #3069). While new code is added, the purpose is to fix a systemic deficiency that caused recurring breakage. The companion-file breakage was visible to users (silent failures, missing helper scripts). The new directory-fetching capability is the mechanism of the fix, not a standalone feature.

---

Labels: PR modifies internal/harness/compose.go — harness compose subsystem

Previous run (3)

Review

Findings

Medium

• [error-handling] internal/harness/compose.go:1012 — fetchBaseScriptOrDir propagates errors from fetchBaseScriptDirTree immediately without falling back to single-file fetch via fetchBaseFile or stale cached content. The skill analog (fetchBaseSkill) handles transient errors by checking isTransientFetchError and returning stale cached content with a warning. A transient tree-fetch failure will cause the entire compose to fail even if the individual script was previously cached. See also: [edge-case] finding for the offline mode variant.

• [auth-bypass] internal/harness/compose.go:993 — The cache-hit path in fetchBaseScriptOrDir computes allowedBy via matchingAllowedPrefix but does not reject the request when allowedBy is empty. If a previously-cached directory's URL is later removed from allowed_remote_resources, the cache-hit path still serves the content. fetchBaseFile enforces the allowlist at the top of the function (before any cache lookup), and fetchBaseSkill does the same. Add: if allowedBy == "" { return ..., fmt.Errorf("base %s: URL %q is not in allowed_remote_resources", field, fileURL) }.

Low

• [edge-case] internal/harness/compose.go:1005 — In offline mode, when the directory cache misses on the scriptdir: key, the fallback to fetchBaseFile may fail for scripts only ever directory-fetched, because CacheGet (single-file cache) cannot retrieve a tree hash stored by CachePutDir (directory cache).

• [test-inadequate] internal/harness/compose_test.go — Missing tests for error/resilience paths: tree fetcher transient error behavior, offline mode with directory-cached scripts, and offline cache-miss fallback to fetchBaseFile.

• [privilege-escalation] internal/harness/compose.go:1096 — fetchBaseScriptDirTree sets 0o755 on ALL files in the fetched directory. The analog fetchBaseSkillDir does not chmod any files, and fetchBaseFile only makes the target script executable. Consider limiting chmod to the target script file.

• [error-handling-idiom] internal/harness/compose.go — Error messages in fetchBaseScriptDirTree omit the directory path (e.g., "fetching script directory: %w"), unlike fetchBaseSkillDir which includes it ("fetching skill directory %s: %w").

• [pattern-inconsistency] internal/harness/compose.go — URL index key uses "scriptdir:"+scriptDirURL (directory URL), while the skill analog uses "skill:"+skillFileURL (file URL). The different granularity is arguably intentional for multi-script directories but diverges from the established convention.

• [code-organization] internal/harness/compose.go — The fetchBaseScriptDirTree call result is unnecessarily destructured and re-returned instead of a direct return fetchBaseScriptDirTree(...), which is the pattern used at the bottom of the same function.

• [error-handling-idiom] internal/harness/compose.go — chmod errors are silently discarded (_ = os.Chmod(...)) in both the cache-hit path and the bulk-chmod loop, whereas fetchBaseFile checks and returns chmod errors.

---

Labels: PR modifies internal/harness/compose.go — harness compose subsystem

[github-actions]

Site preview

Preview: https://2d650df2-site.fullsend-ai.workers.dev

Commit: c4c1f832877f4bb2382250ad02822f982fbc7006

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:11 PM UTC · Completed 8:25 PM UTC
Commit: c4c1f83 · View workflow run →

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:38 PM UTC · Completed 8:57 PM UTC
Commit: 2b928f3 · View workflow run →

[ggallen]

/fs-review

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:15 AM UTC · Completed 1:30 AM UTC
Commit: b963c6e · View workflow run →

Superseded by updated review

[rh-hemartin]

LGTM but I think there is overlap with script composition over at fullsend-ai/agents, we need a followup issue to decide which are we going with.

[ggallen]

Follow-up items (for retro bot)

This PR fixes the immediate breakage by adding directory-level fetching for pre/post scripts that have companion files (e.g., Python scripts invoked via subprocess). There are longer-term approaches that should replace or complement this:

1. Shell script composition / bundling — Refactor scripts that use source to inline their dependencies at build time (like bundle-sh.sh in fullsend-ai/agents). This eliminates the need for directory fetching for shell scripts that source libraries. Tracked partly by the existing work to refactor install-precommit-tools.sh into a sourceable library.

2. Python self-containment via uv (PEP 723) — Python companion scripts can declare their dependencies inline with PEP 723 script metadata and be run with uv run, making them fully self-contained single files. This eliminates the need for directory fetching for Python companions.

3. Undo directory fetching once the above are done — Once shell scripts are bundled and Python scripts use uv, the directory-level fetching added here becomes unnecessary and can be removed to keep the fetching logic simple.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 1:23 PM UTC · Completed 1:46 PM UTC
Commit: b963c6e · View workflow run →

[fullsend-ai-retro]

Retro: PR #5688 — directory-level fetching for pre/post scripts

Overall assessment: This workflow went well. The review agent delivered high-quality findings (100% true positive rate across 3 review passes), and the code agent produced a working 787-line implementation with comprehensive tests on its second attempt. All identified gaps are already tracked in existing open issues. No new proposals are warranted.

Timeline

1. Issue Harness compose should support directory-level fetching for pre/post scripts to prevent recurring companion-file breakage #3640 filed Jul 8 by retro agent — recurring companion-file breakage pattern (4 incidents: bug(harness): v0.22.0 URL-base skill resolution drops companion files, breaks review agent for all users #2705, fix(prioritize): inline CSMA library into post-script #3182, Post-script companion scripts not accessible when resolved from URL-based cache #3069, resurfaced Jul 28).
2. PR feat(#3640): directory-level fetching for pre/post scripts #5443 (Jul 22) — first code agent attempt, immediately closed by rh-hemartin (believed issue was fixed via script composition in fullsend-ai/agents).
3. Critical escalation (Jul 28) — ggallen reported the issue resurfaced after PR refactor(scaffold)!: delete agent files from scaffold embed #5588 deleted scaffold fallback copies. Re-triaged as priority/critical.
4. Code agent run (Jul 28 18:38) → PR fix(#3640): directory-level fetching for pre/post scripts #5688 opened at 18:52.
5. Review 1 (19:10) — 4 true-positive findings (2 medium: error-handling fallback, auth-bypass on cache hit; 2 low: offline cache miss, bulk chmod). All on compose.go. Missed lock.go consumer gap.
6. ggallen fixes (19:38) — addressed all 4 findings, force-pushed.
7. Review 2 (20:57) — found HIGH severity lock.go consumer-completeness bug: resolveFromLock didn't handle "directory" type for script fields.
8. Fix agent auto-triggered (20:58) — failed at eligibility check: misidentified bot-authored PR as human-authored.
9. ggallen fixes lock.go (~01:00 Jul 29) — added isScriptLockField() helper and lock round-trip test.
10. Review 3 (01:30) — APPROVED.
11. rh-hemartin (10:31) — APPROVED with strategic note: "overlap with script composition in fullsend-ai/agents, need followup issue."
12. Merged (13:21 Jul 29).

Review quality

Strengths: Zero false positives. All 5 actionable findings were real bugs that ggallen confirmed and fixed. The multi-pass review was essential — the HIGH-severity lock.go consumer gap was only caught on the second pass after the initial compose.go issues were resolved.

Gap: The first review missed the lock.go consumer-completeness issue despite lock.go being in the diff. This required an additional fix-review cycle.

Autonomy readiness

The review agent performed exceptionally on tactical correctness: every finding was real, well-calibrated in severity, and included actionable suggested fixes. Human review added strategic value that the agent structurally cannot provide — rh-hemartin's cross-repo architectural observation about overlap with fullsend-ai/agents script composition. Current autonomy level is appropriate.

Evidence for existing issues (no new proposals needed)

• fullsend#5536 (fix agent eligibility misidentification, priority/high, ready-to-code): PR fix(#3640): directory-level fetching for pre/post scripts #5688 is another instance. The fix agent was auto-triggered by the review agent's CHANGES_REQUESTED but blocked at eligibility because gh pr view --json author returns app/fullsend-ai-coder (no [bot] suffix). This prevented automated fix of the 4 initial review findings, forcing ggallen to fix manually.
• agents#455 (code agent should enumerate struct consumers): The code agent added directory-type fetching to compose.go but didn't update lock.go's resolveFromLock to handle the new type — the same consumer-completeness pattern.
• fullsend#1582 (review agent first-pass completeness): The lock.go consumer gap was present from the initial code agent output but only caught on review pass 2. Consistent with the pattern of incremental finding discovery across cycles.
• agents#511 (review coverage stability across re-reviews): The HIGH-severity finding appeared only after compose.go fixes were pushed, despite being present in the original diff — another data point for unstable coverage across passes.
• fullsend#2873 / fullsend#853 (code agent consumer tracing): The code agent's failure to update lock.go when adding a new dependency type to compose.go matches the general pattern of incomplete consumer tracing.