fix(harness): resolve(execute) resumes the pre-trigger chain with mutated arguments#520
Merged
ytallo merged 4 commits intoJul 20, 2026
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 |
Contributor
skill-check — worker0 verified, 46 skipped (no docs/).
Four for four. Nicely done. |
ytallo
force-pushed
the
feat/harness-conformance-e2e
branch
from
July 16, 2026 17:48
d96fe06 to
5aa6b80
Compare
ytallo
force-pushed
the
fix/506-release-runs-original-args
branch
from
July 16, 2026 17:50
103273a to
c2600d6
Compare
ytallo
force-pushed
the
feat/harness-conformance-e2e
branch
from
July 17, 2026 13:29
97ab899 to
a6ebf0d
Compare
A call held by a pre_trigger hook and released with
harness::function::resolve {action: "execute"} ran the target with the
model's ORIGINAL arguments recovered from the transcript — mutations from
hooks that ran before the holder were dropped, and the remaining chain
never resumed, breaking the harness.md § function::resolve promise that
the pre_trigger chain resumes after the holding hook.
- PreTriggerOutcome::Hold now carries the arguments as mutated up to the
hold; the loop persists them on the call's checkpoint (held_arguments),
so they survive the park durably. Post-trigger holds checkpoint the
fully pre-mutated args for the same reason.
- The execute release path prefers the checkpointed arguments (transcript
recovery remains the fallback for records written before the field
existed), resumes the pre_trigger chain AFTER the holder — hooks up to
and including it already ran, so re-running would double-apply
mutations and re-hold — and handles a resumed Deny/Hold like the loop.
The filesystem scope stamp is still re-applied after the resumed chain.
- A holder no longer bound at release runs no further hooks: the chain
shape changed under the hold and the safe resume point is unknowable.
Fixes #506
The fix on this branch makes the scenario pass; --scenario all (and the per-PR CI job) now executes it. Verified locally: C-E2E-001, C-E2E-002, C-E2E-506 all pass.
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
ytallo
force-pushed
the
fix/506-release-runs-original-args
branch
from
July 20, 2026 13:36
57d8ba0 to
476ed93
Compare
ytallo
added a commit
that referenced
this pull request
Jul 21, 2026
Composes the two features that collided on the resolve(execute) release path: the pre-trigger chain re-run from #520 computes the (possibly mutated) arguments first, and main's fire-and-forget subagent spawn branch now consumes those arguments. The parked-spawn PendingInfo tail in subagent.rs gives way to main's spawned_result, and the spawn Done checkpoint carries held_arguments: None.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #506.
Releasing a hook-held call with
harness::function::resolve {action: "execute"}executed the ORIGINAL model arguments: hook-chain mutations — from hooks before the holding hook and after it — were not applied.Root cause
The execute arm in
harness/src/deferred.rsrecovered arguments viafind_call_argumentsfrom the transcript (the model's originalfunction_call), and pre-hold mutations existed only in-memory insiderun_pre_trigger—PreTriggerOutcome::Holdreturned{held_by, annotations}without the accumulated args, and nothing persisted them.Fix — design (a): resume through the remaining pipeline
harness.md§function::resolveexplicitly promises "thepre_triggerchain resumes after the holding hook", so that's what release now does:CallCheckpoint.held_arguments, serde-defaulted for old records).chain_slicehelper), then re-appliesfilesystem_scope::inject(session grants ∪req.fs_scope) so a hook rewrite can never widen scope.Denyappends ahook_deniederror result; a resumedHoldre-parks under the new holder with the mutations so far; if the holder was unbound since the hold, no hooks run (safe resume point unknowable — degrades to executing the checkpointed args).Verification
C-E2E-506(from (MOT-4107) feat(harness): integration E2E runner (first slice) + issue repro scenarios #518): contract_failure → pass — target received{value:"expected+scope"}exactly once; the mutator saw original args and the holder saw mutated args, each ran exactly once.C-E2E-001/C-E2E-002regressions pass.C-E2E-505still fails on this branch by design — the hold-parse drop is harness: pre-trigger hook returningholddiscards that hook's argument mutations #505's half (fix/505-hold-discards-mutations); once merged, this branch's checkpoint persistence carries a holding hook's own mutations automatically.chain_slicetests); fmt/clippy clean.Note for review
Docs say execute "re-enqueues the turn"; the (pre-existing) implementation runs the release inline in
deferred.rs. Observable semantics now match the docs; the mechanism difference is worth a doc touch-up later.