From 9692f5a2c712577c8afc2bce6742b946deee6898 Mon Sep 17 00:00:00 2001 From: Ytallo Layon Date: Thu, 16 Jul 2026 16:01:30 -0300 Subject: [PATCH] fix(harness): pre-trigger hold keeps its own argument mutations 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 iii-hq/workers#505 --- .../scenarios/hold-mutation-505/scenario.yaml | 11 +-- harness/src/hooks/runner.rs | 88 ++++++++++++------- 2 files changed, 62 insertions(+), 37 deletions(-) diff --git a/harness/evals/conformance/scenarios/hold-mutation-505/scenario.yaml b/harness/evals/conformance/scenarios/hold-mutation-505/scenario.yaml index 3d6ecb4f..7606290f 100644 --- a/harness/evals/conformance/scenarios/hold-mutation-505/scenario.yaml +++ b/harness/evals/conformance/scenarios/hold-mutation-505/scenario.yaml @@ -8,17 +8,14 @@ # The runner releases the parked call with # `harness::function::resolve { action: "execute" }`. # -# QUARANTINED: the invariants assert the issue's EXPECTED behavior — the -# released call executes with the gate's mutated arguments. Source inspection -# shows the mutations are discarded at parse time (`parse_output` in -# harness/src/hooks/runner.rs maps `decision: "hold"` to a mutation-less -# outcome), and the release path re-reads the transcript's original args -# regardless (#506). Unquarantine when #505 lands. +# Regression gate for #505 (fixed: a holding hook's own `mutations` are +# parsed and folded into the effective arguments checkpointed at hold, and +# the release runs with those). Pairs with #506, which persists and resumes +# the chain over the checkpointed args. schema_version: "1" id: C-E2E-505 description: A pre-trigger hook that holds AND mutates must have its mutation applied to the released call. stack_profile: harness-core -quarantine: true send: session_id: "{{session_id}}" message: "Call the recorder once." diff --git a/harness/src/hooks/runner.rs b/harness/src/hooks/runner.rs index 3a0a9fb4..12be606d 100644 --- a/harness/src/hooks/runner.rs +++ b/harness/src/hooks/runner.rs @@ -29,7 +29,9 @@ struct HookMutations { enum HookOutcome { Continue(HookMutations), Deny(String), - Hold, + // A holding hook may mutate as it holds (approval-gate: park the call AND + // stamp validated context onto it) — its mutations must not be dropped. + Hold(HookMutations), } /// Outcome of the `pre_generate` chain. @@ -91,7 +93,7 @@ impl HookRegistry { match self.invoke(&binding, input).await { HookOutcome::Continue(_) => {} HookOutcome::Deny(reason) => return Err(reason), - HookOutcome::Hold => {} + HookOutcome::Hold(_) => {} } } Ok(()) @@ -128,7 +130,7 @@ impl HookRegistry { merge(&mut annotations, m.annotations); } HookOutcome::Deny(reason) => return PreGenerateOutcome::Deny(reason), - HookOutcome::Hold => {} + HookOutcome::Hold(_) => {} } } PreGenerateOutcome::Continue { @@ -178,12 +180,18 @@ impl HookRegistry { merge(&mut annotations, m.annotations); } HookOutcome::Deny(reason) => return PreTriggerOutcome::Deny(reason), - HookOutcome::Hold => { + HookOutcome::Hold(m) => { + // Apply the holding hook's own rewrite before parking, so + // the release runs with the full chain's effective args. + if let Some(a) = m.arguments { + args = a; + } + merge(&mut annotations, m.annotations); return PreTriggerOutcome::Hold { held_by: binding.function_id.clone(), arguments: args, annotations, - } + }; } } } @@ -238,7 +246,7 @@ impl HookRegistry { // Deny is not valid at post_trigger; fail-open like existing // post-trigger error handling. HookOutcome::Deny(_) => {} - HookOutcome::Hold => { + HookOutcome::Hold(_) => { return PostTriggerOutcome::Hold { held_by: binding.function_id.clone(), annotations, @@ -298,30 +306,34 @@ fn parse_output(value: Value) -> HookOutcome { .unwrap_or("denied by hook") .to_string(), ), - Some("hold") => HookOutcome::Hold, - _ => { - let mut muts = HookMutations::default(); - if let Some(m) = value.get("mutations") { - muts.system_prompt = m - .get("system_prompt") - .and_then(Value::as_str) - .map(str::to_string); - if let Some(arr) = m.get("append_messages").and_then(Value::as_array) { - muts.append_messages = arr.clone(); - } - muts.arguments = m.get("arguments").cloned(); - if let Some(c) = m.get("content") { - muts.content = serde_json::from_value(c.clone()).ok(); - } - muts.details = m.get("details").cloned(); - muts.is_error = m.get("is_error").and_then(Value::as_bool); - } - if let Some(Value::Object(ann)) = value.get("annotations") { - muts.annotations = ann.clone(); - } - HookOutcome::Continue(muts) + Some("hold") => HookOutcome::Hold(parse_mutations(&value)), + _ => HookOutcome::Continue(parse_mutations(&value)), + } +} + +/// Parse a hook's `mutations`/`annotations` — shared by the continue and hold +/// branches so a holding hook's rewrites are honored, not dropped. +fn parse_mutations(value: &Value) -> HookMutations { + let mut muts = HookMutations::default(); + if let Some(m) = value.get("mutations") { + muts.system_prompt = m + .get("system_prompt") + .and_then(Value::as_str) + .map(str::to_string); + if let Some(arr) = m.get("append_messages").and_then(Value::as_array) { + muts.append_messages = arr.clone(); + } + muts.arguments = m.get("arguments").cloned(); + if let Some(c) = m.get("content") { + muts.content = serde_json::from_value(c.clone()).ok(); } + muts.details = m.get("details").cloned(); + muts.is_error = m.get("is_error").and_then(Value::as_bool); + } + if let Some(Value::Object(ann)) = value.get("annotations") { + muts.annotations = ann.clone(); } + muts } fn merge(into: &mut Map, from: Map) { @@ -399,16 +411,32 @@ mod tests { _ => panic!("expected deny"), } match parse_output(json!({ "decision": "hold" })) { - HookOutcome::Hold => {} + HookOutcome::Hold(m) => assert!(m.arguments.is_none()), _ => panic!("expected hold"), } // Legacy hooks may still send pending_timeout_ms; it is ignored. assert!(matches!( parse_output(json!({ "decision": "hold", "pending_timeout_ms": 1000 })), - HookOutcome::Hold + HookOutcome::Hold(_) )); } + #[test] + fn hold_keeps_mutations_and_annotations() { + let out = parse_output(json!({ + "decision": "hold", + "mutations": { "arguments": { "value": "expected+approved" } }, + "annotations": { "approved_by": "gate" } + })); + match out { + HookOutcome::Hold(m) => { + assert_eq!(m.arguments, Some(json!({ "value": "expected+approved" }))); + assert_eq!(m.annotations.get("approved_by"), Some(&json!("gate"))); + } + _ => panic!("expected hold"), + } + } + #[test] fn continue_mutations_parse() { let out = parse_output(json!({