fix(harness): pre-trigger hold keeps its argument mutations#519
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skill-check — worker0 verified, 44 skipped (no docs/).
Four for four. Nicely done. |
d96fe06 to
5aa6b80
Compare
2ba78ef to
e0ac05b
Compare
A pre-trigger hook returning decision:"hold" WITH mutations had them discarded at parse (parse_output mapped "hold" to a data-less outcome), so the approval-gate pattern (hold AND stamp validated context) was impossible. HookOutcome::Hold now carries HookMutations via a shared parse_mutations helper, and run_pre_trigger folds the holding hook's argument rewrite into the effective arguments before parking. Those args ride the call checkpoint added for #506 (fix/506-release-runs-original-args, which this is based on), so the resolve(execute) release runs with the gate's mutation. Unquarantines C-E2E-505, now a regression gate. Fixes #505
e0ac05b to
9692f5a
Compare
1 participant
Fixes #505.
A
harness::hook::pre-triggerhook returningdecision: "hold"together withmutations.argumentshad the mutations silently discarded, so a released call executed pre-hold arguments — the approval-gate pattern (hold AND stamp validated context) was impossible.Root cause
Two compounding drops:
parse_output(harness/src/hooks/runner.rs) mappeddecision:"hold"to the data-lessHookOutcome::Hold— anything undermutationswas dropped at parse time.PreTriggerOutcome::Holdcarried no arguments, and theresolve {action:"execute"}path (harness/src/deferred.rs) recovered arguments from the transcript — the model's original call.Fix
HookOutcome::Hold(HookMutations)— hold parses mutations via a sharedparse_mutationshelper.run_pre_triggerfolds the holding hook's rewrite into the accumulated chain arguments and returns them inPreTriggerOutcome::Hold.CallCheckpoint.argumentspopulated when the call parks (old records stay deserializable); the post-trigger hold park checkpoints effective args too.filesystem_scope::injectis re-stamped at release so a hook rewrite can never widen scope.Verification
C-E2E-505(from (MOT-4107) feat(harness): integration E2E runner (first slice) + issue repro scenarios #518): contract_failure → pass — the target executes{value:"expected+approved"}exactly once; the gate held once seeing the original args.C-E2E-506also passes with this change;C-E2E-001/C-E2E-002regressions pass.hold_keeps_mutations_and_annotations); fmt/clippy clean.Note for review
Overlaps with the #506 fix branch (
fix/506-release-runs-original-args): both persist held arguments on the call checkpoint but choose different release semantics (this branch: execute checkpointed args; #506's: resume the remaining chain after the holder, per harness.md). Suggest merging #506's mechanism and grafting this branch's hold-parses-mutations onto it, reconciling the checkpoint field (argumentsvsheld_arguments).