Skip to content

fix(#799): preserve child pre_script during base forge composition - #823

Open
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/799-triage-pre-script
Open

fix(#799): preserve child pre_script during base forge composition#823
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/799-triage-pre-script

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown

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

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.

Test plan

  • 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

🤖 Generated with Claude Code


Closes #799

Post-script verification

  • Branch is not main/master (agent/799-triage-pre-script)
  • Secret scan passed (gitleaks — ba77dbdbbd17e9bb18c47bf6efa023c98b697158..HEAD)
  • PR body secret scan passed (gitleaks — no-git)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

When a child harness declares a top-level pre_script and inherits
from a base that has forge.<platform>.pre_script,
mergeBaseIntoChild correctly preserves the child's top-level
value but also merges the base's forge section. ResolveForge then
unconditionally overwrites the top-level pre_script with the
forge-level value — silently dropping the child's explicit script.

Fix: after merging forge blocks, clear forge-level pre_script and
post_script entries that were inherited from the base (not authored
by the child). This prevents ResolveForge from overwriting the
child's explicit declarations while preserving child-authored
forge-level scripts.

Add triage-specific runAgent tests verifying pre-script execution
for the triage agent (exit 1 aborts, exit 0 proceeds to sandbox).

Note: pre-commit could not run (network error in sandbox). The
post-script runs an authoritative pre-commit check on the runner.

Closes #799
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:16 AM UTC · Completed 3:31 AM UTC
Commit: ab9850b · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [naming-consistency] internal/harness/compose.go — The parameter childForgeScripts in clearInheritedForgeScripts and the caller variable childForgeSnap in mergeBaseIntoChild use different abbreviated forms for the same map[string]forgeScriptSnapshot value. Aligning the names would reduce cognitive overhead.
Previous run

Review

Findings

Low

  • [architectural-coherence] internal/harness/compose.go:591 — ADR-0045 states forge blocks use "same merge rules" but the fix introduces forge-specific clearing logic that creates an exception for pre_script/post_script during base composition. The inline code comments are thorough, but the ADR itself does not document this exception. Consider adding a brief addendum to ADR-0045 noting that forge-level scripts inherited from a base are cleared when the child declares top-level equivalents.

Labels: PR fixes a bug in harness base composition (forge script inheritance), part of the dispatch/composition system

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge type/bug Confirmed defect in existing behavior component/dispatch Workflow dispatch and triggers labels Aug 2, 2026
@guyoron1

guyoron1 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

/fs-fix

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 2, 2026

Copy link
Copy Markdown
Author

🤖 Finished Fix · ✅ Success · Started 6:48 AM UTC · Completed 6:53 AM UTC
Commit: ab9850b · View workflow run →

Add a brief note to the base→child merge rules documenting that
forge-level pre_script/post_script inherited from a base are cleared
when the child explicitly declares the corresponding top-level field.
This exception was introduced by the #799 fix to prevent ResolveForge
from silently overriding child-authored scripts.

Addresses review feedback on #823
@fullsend-ai-coder

Copy link
Copy Markdown
Author

🔧 Fix agent — iteration 1 (human-triggered)

Addressed 1 review finding: added a brief addendum to ADR-0045 documenting the forge-script clearing exception introduced by the #799 fix. The annotation notes that inherited forge-level pre_script/post_script values are cleared when the child declares top-level equivalents, preventing ResolveForge from silently overriding child-authored scripts.

Fixed (1):

  1. ADR-0045 does not document forge-script clearing exception (docs/ADRs/0045-forge-portable-harness-schema.md): Added a brief annotation to the base-child merge rules in ADR-0045 documenting that forge-level pre_script/post_script inherited from a base are cleared when the child explicitly declares the corresponding top-level field. This is permitted under ADR rules as a minor annotation (cross-reference to Triage runner does not execute pre_script from harness configuration #799).

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

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:58 AM UTC · Completed 7:11 AM UTC
Commit: 7239ee3 · View workflow run →

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

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.

@github-actions github-actions Bot added the stale label Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/dispatch Workflow dispatch and triggers ready-for-merge All reviewers approved — ready to merge stale type/bug Confirmed defect in existing behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Triage runner does not execute pre_script from harness configuration

1 participant