fix(#847): preserve child pre_script during base forge composition

[fullsend-ai-coder]

Summary

• Fix base-composition bug where a child harness's explicit pre_script was silently overwritten by the base's forge.<platform>.pre_script during ResolveForge
• After mergeForgeBlocks, clear forge-level pre_script/post_script entries inherited from the base when the child explicitly declared the corresponding top-level field
• Preserve child-authored forge-level scripts (only base-inherited values are cleared)
• Add triage-specific runAgent tests verifying pre-script abort on exit 1 and proceed on exit 0

Related Issue

Closes #847

Changes

• internal/harness/compose.go: Added clearInheritedForgeScripts, snapshotChildForgeScripts, and forgeScriptSnapshot to detect and clear base-inherited forge-level scripts that would shadow the child's explicit top-level pre_script/post_script
• internal/harness/compose_test.go: Four new tests covering merge behavior and end-to-end LoadWithBase scenarios
• internal/cli/prescript_run_test.go: Two new runAgent tests for triage agent pre-script execution; refactored newSkipHarnessDir to support arbitrary agent names via newSkipHarnessDirForAgent
• docs/ADRs/0045-forge-portable-harness-schema.md: Documented the inherited forge-level scripts exception in the merge rules

Root cause

mergeBaseIntoChild correctly preserves the child's top-level pre_script during scalar merge. However, it also merges the base's forge section via mergeForgeBlocks. When the child has no forge section, it inherits the base's forge configs entirely — including forge.github.pre_script. Later, ResolveForge unconditionally overwrites the top-level pre_script with the non-empty forge-level value, silently dropping the child's explicit declaration.

Testing

• TestMergeBaseIntoChild_ChildPreScriptSurvivesBaseForge — child top-level pre_script survives when base has forge.github.pre_script
• TestMergeBaseIntoChild_ChildForgePreScriptPreserved — child-authored forge-level pre_script is preserved (not cleared)
• TestLoadWithBase_ChildPreScriptSurvivesBaseForgeResolution — end-to-end: child pre_script survives full LoadWithBase + ResolveForge pipeline
• TestLoadWithBase_BaseForgePreScriptInheritedWhenChildOmits — existing behavior unchanged: base forge pre_script inherited when child omits
• TestRunAgent_TriagePreScriptExitsNonZero — triage agent aborts on pre-script exit 1
• TestRunAgent_TriagePreScriptExitsZero_ProceedsToSandbox — triage agent proceeds past pre-script on exit 0
• All existing harness and CLI tests pass

Checklist

• PR title follows Conventional Commits (correct type, ! for breaking changes)
• Tests added/updated for new or modified logic

Note: pre-commit hooks could not run (network unavailable in sandbox).
Manual verification of pre-commit is required.

---

Closes #847

Post-script verification

• Branch is not main/master (agent/847-triage-pre-script-forge)
• Secret scan passed (gitleaks — c887fc47c59d8b87b5282ca44044d2933d30df41..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 9:44 AM UTC · Completed 10:00 AM UTC
Commit: cab62da · View workflow run →

[fullsend-ai-review]

Looks good to me

Previous run

Looks good to me

Previous run (2)

Review

Findings

Medium

• [stale-doc] docs/guides/user/bring-your-own-agent.md:299 — The "Field merge rules (for base and forge)" table describes scalar fields (including pre_script) as "Child wins if non-empty" and does not mention the new exception introduced by this PR: inherited forge-level pre_script/post_script from a base harness are cleared when they would shadow the child's own top-level scripts. The ADR was updated but the user-facing guide was not.
  Remediation: Add a footnote or row to the merge rules table noting the exception for inherited forge-level scripts.

Low

• [shared-pointer-mutation] internal/harness/compose.go — In clearInheritedForgeScripts, when a platform key was not present in the child's original forge map, mergeForgeBlocks assigns child[key] = baseFC (a direct pointer copy). Subsequent fc.PreScript = "" mutates the base's ForgeConfig struct through the shared pointer. Currently safe because all callers discard the base after mergeBaseIntoChild returns, but this is a latent correctness hazard if a future caller reuses the base struct.
• [declaration-ordering] internal/harness/compose.go — The forgeScriptSnapshot type is declared after clearInheritedForgeScripts which uses it. The codebase convention (e.g., Dependency, ComposeOpts) is to declare types before the functions that reference them.
• [naming-consistency] internal/harness/compose.go — Parameter name childForgeScripts in clearInheritedForgeScripts differs from the local variable childForgeSnap at the call site in mergeBaseIntoChild.
• [comment-style] internal/harness/compose.go — The doc comment on clearInheritedForgeScripts is ~17 lines, substantially longer than comparable package-internal helpers in the same file (2–5 lines). The motivation section duplicates information already in the ADR update and the inline comment in mergeBaseIntoChild.

---

Labels: PR fixes a bug in harness base composition (pre_script/post_script override), matching the type/bug label

[guyoron1]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:31 AM UTC · Completed 10:45 AM UTC
Commit: cab62da · View workflow run →

[guyoron1]

/fs-fix

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 10:51 AM UTC · Completed 11:00 AM UTC
Commit: cab62da · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed all 5 review findings (1 medium, 4 low). Updated user guide merge table with footnote for forge script exception. Fixed shared-pointer mutation by cloning ForgeConfig before clearing inherited fields. Reordered type declaration, aligned parameter naming, and trimmed doc comment to match codebase conventions.

Fixed (5):

1. stale-doc: merge rules table missing forge script exception (docs/guides/user/bring-your-own-agent.md): Added footnote to the scalars row in the merge rules table documenting that inherited forge-level pre_script/post_script are cleared when the child explicitly declares the corresponding top-level field
2. shared-pointer-mutation: clearInheritedForgeScripts mutates base ForgeConfig (internal/harness/compose.go): Clone ForgeConfig before clearing inherited script fields to avoid mutating the base struct through the shared pointer assigned by mergeForgeBlocks
3. declaration-ordering: forgeScriptSnapshot type declared after function that uses it (internal/harness/compose.go): Moved forgeScriptSnapshot type declaration before clearInheritedForgeScripts to follow codebase convention (types before functions that reference them)
4. naming-consistency: parameter name differs from call-site variable (internal/harness/compose.go): Renamed clearInheritedForgeScripts parameter from childForgeScripts to childForgeSnap to match the call-site variable name
5. comment-style: doc comment on clearInheritedForgeScripts too long (internal/harness/compose.go): Trimmed the doc comment from ~17 lines to 4 lines, removing the motivation section that duplicated the ADR and inline comment

Tests: passed

Next steps:

• /fs-review — request a re-review of the changes
• /fs-fix <your instruction> — run another fix pass with specific guidance
• Push commits directly — review re-runs automatically on push
  _{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:01 AM UTC · Completed 11:10 AM UTC
Commit: 5c5875d · View workflow run →

[github-actions]

This pull request has been automatically marked as stale because it has not had any activity in the last month. It will be closed in 2 weeks if no further activity occurs. Remove the stale label to reset the inactivity timer.