diff --git a/approval-gate/Cargo.lock b/approval-gate/Cargo.lock index effdfce82..57275b4fa 100644 --- a/approval-gate/Cargo.lock +++ b/approval-gate/Cargo.lock @@ -545,12 +545,13 @@ dependencies = [ [[package]] name = "harness" -version = "1.7.0" +version = "1.8.1" dependencies = [ "anyhow", "async-trait", "clap", "globset", + "iii-console-ui", "iii-helpers", "iii-sdk", "jsonschema", @@ -804,6 +805,18 @@ dependencies = [ "icu_properties", ] +[[package]] +name = "iii-console-ui" +version = "0.1.0" +dependencies = [ + "iii-sdk", + "schemars", + "serde", + "serde_json", + "tokio", + "tracing", +] + [[package]] name = "iii-helpers" version = "0.21.8" diff --git a/approval-gate/src/configuration.rs b/approval-gate/src/configuration.rs index b753e837b..d98411ccf 100644 --- a/approval-gate/src/configuration.rs +++ b/approval-gate/src/configuration.rs @@ -118,7 +118,8 @@ pub async fn apply_config(cell: &ConfigCell, cfg: WorkerConfig) { } /// Bind the fixed `harness::hook::pre-trigger` hook at worker startup. -pub fn bind_hook(iii: &IIIClient) { +/// Returns `true` when the engine accepted the registration. +pub fn bind_hook(iii: &IIIClient) -> bool { match iii.register_trigger(RegisterTriggerInput { trigger_type: "harness::hook::pre-trigger".to_string(), function_id: "approval::gate".to_string(), @@ -129,17 +130,23 @@ pub fn bind_hook(iii: &IIIClient) { }), metadata: None, }) { - Ok(_) => tracing::info!( - trigger_type = "harness::hook::pre-trigger", - function_id = "approval::gate", - "trigger binding requested" - ), - Err(e) => tracing::warn!( - trigger_type = "harness::hook::pre-trigger", - function_id = "approval::gate", - error = %e, - "trigger binding failed (sibling absent?)" - ), + Ok(_) => { + tracing::info!( + trigger_type = "harness::hook::pre-trigger", + function_id = "approval::gate", + "trigger binding requested" + ); + true + } + Err(e) => { + tracing::warn!( + trigger_type = "harness::hook::pre-trigger", + function_id = "approval::gate", + error = %e, + "trigger binding failed (sibling absent?)" + ); + false + } } } @@ -147,7 +154,8 @@ pub fn bind_hook(iii: &IIIClient) { /// `approval::filesystem-access-watch` at worker startup — beside `bind_hook`, same /// best-effort discipline (a standalone deployment without the harness /// still boots; a missing binding surfaces as a log, never an `Err` here). -pub fn bind_filesystem_access_watch_hook(iii: &IIIClient) { +/// Returns `true` when the engine accepted the registration. +pub fn bind_filesystem_access_watch_hook(iii: &IIIClient) -> bool { match iii.register_trigger(RegisterTriggerInput { trigger_type: "harness::hook::post-trigger".to_string(), function_id: "approval::filesystem-access-watch".to_string(), @@ -158,41 +166,61 @@ pub fn bind_filesystem_access_watch_hook(iii: &IIIClient) { }), metadata: None, }) { - Ok(_) => tracing::info!( - trigger_type = "harness::hook::post-trigger", - function_id = "approval::filesystem-access-watch", - "trigger binding requested" - ), - Err(e) => tracing::warn!( - trigger_type = "harness::hook::post-trigger", - function_id = "approval::filesystem-access-watch", - error = %e, - "trigger binding failed (sibling absent?)" - ), + Ok(_) => { + tracing::info!( + trigger_type = "harness::hook::post-trigger", + function_id = "approval::filesystem-access-watch", + "trigger binding requested" + ); + true + } + Err(e) => { + tracing::warn!( + trigger_type = "harness::hook::post-trigger", + function_id = "approval::filesystem-access-watch", + error = %e, + "trigger binding failed (sibling absent?)" + ); + false + } } } /// Retry hook bindings until the harness has registered the hook trigger /// types. Approval-gate may start before harness; a one-shot registration in /// that order fails asynchronously and silently leaves the gate detached. +/// +/// Each hook is registered AT MOST ONCE per startup (on the first successful +/// attempt): the engine's instance count can lag a successful registration, +/// and re-binding on a lagging count stacks duplicate hook instances — the +/// harness would then run the gate N times per call, re-holding on release +/// (approval deadlock). A failed attempt (harness not up yet) leaves the +/// flag unset so the next iteration retries; a success is never repeated. +/// +/// Readiness gates only the completion condition, never the attempt: the +/// harness being active says nothing about whether THIS worker bound its +/// hook, so a gate that starts after the harness must still register — +/// otherwise the gate runs detached. pub fn retry_hook_bindings(iii: IIIClient) { tokio::spawn(async move { + let mut pre_bound = false; + let mut post_bound = false; loop { + if !pre_bound { + pre_bound = bind_hook(&iii); + } + if !post_bound { + post_bound = bind_filesystem_access_watch_hook(&iii); + } + let pre_trigger_ready = trigger_instance_count(&iii, "harness::hook::pre-trigger") .await .is_some_and(|count| count > 0); - if !pre_trigger_ready { - bind_hook(&iii); - } - let post_trigger_ready = trigger_instance_count(&iii, "harness::hook::post-trigger") .await .is_some_and(|count| count > 0); - if !post_trigger_ready { - bind_filesystem_access_watch_hook(&iii); - } - if pre_trigger_ready && post_trigger_ready { + if pre_bound && post_bound && pre_trigger_ready && post_trigger_ready { tracing::info!("approval-gate hook bindings confirmed"); break; } diff --git a/approval-gate/src/main.rs b/approval-gate/src/main.rs index b7670398a..8c7980569 100644 --- a/approval-gate/src/main.rs +++ b/approval-gate/src/main.rs @@ -166,8 +166,11 @@ async fn main() -> Result<()> { functions::register_all(&iii, &deps); - configuration::bind_hook(&iii); - configuration::bind_filesystem_access_watch_hook(&iii); + // Hook bindings go through `retry_hook_bindings` alone — it registers + // each hook at most once (the engine's instance count can lag a + // successful registration; a direct bind here plus the loop's first + // iteration would stack duplicate gate instances, which re-hold on + // release and deadlock approval). configuration::retry_hook_bindings(iii.as_ref().clone()); // These two carry no config and are never re-bound — best-effort only. diff --git a/approval-gate/tests/hook_binding_retry.rs b/approval-gate/tests/hook_binding_retry.rs new file mode 100644 index 000000000..addc65ff3 --- /dev/null +++ b/approval-gate/tests/hook_binding_retry.rs @@ -0,0 +1,170 @@ +//! `retry_hook_bindings` registration semantics on a live engine: each hook +//! binds at most once per startup, attempts continue until success, and +//! harness readiness gates only the completion condition — a gate that starts +//! after the harness still binds (the readiness signal says the harness is +//! active, not that THIS worker registered its hook). +//! +//! Self-skips when no `iii` engine binary is on PATH or `III_ENGINE_BIN`. + +use std::time::{Duration, Instant}; + +use approval_gate::configuration::{ + bind_filesystem_access_watch_hook, bind_hook, retry_hook_bindings, +}; +use approval_gate::testkit::{engine_bin, spawn_engine}; +use iii_sdk::errors::Error; +use iii_sdk::protocol::TriggerRequest; +use iii_sdk::trigger::{TriggerConfig, TriggerHandler}; +use iii_sdk::{register_worker, IIIClient, InitOptions, RegisterTriggerType}; +use serde_json::json; + +/// No-op handler standing in for the harness's hook trigger types — a test +/// registers these to simulate "the harness is active". +struct NullHandler; + +#[async_trait::async_trait] +impl TriggerHandler for NullHandler { + async fn register_trigger(&self, _config: TriggerConfig) -> Result<(), Error> { + Ok(()) + } + async fn unregister_trigger(&self, _config: TriggerConfig) -> Result<(), Error> { + Ok(()) + } +} + +fn register_harness_hook_types(iii: &IIIClient) { + for hook_type in ["harness::hook::pre-trigger", "harness::hook::post-trigger"] { + let _ = iii.register_trigger_type(RegisterTriggerType::new( + hook_type, + "test double for the harness hook trigger types", + NullHandler, + )); + } +} + +/// Engine-side instance counts for the two hook types (0 on any error). +async fn hook_instance_counts(iii: &IIIClient) -> (u64, u64) { + let mut counts = (0, 0); + for (index, hook_type) in ["harness::hook::pre-trigger", "harness::hook::post-trigger"] + .iter() + .enumerate() + { + if let Ok(response) = iii + .trigger(TriggerRequest { + function_id: "engine::triggers::info".to_string(), + payload: json!({ "id": hook_type }), + action: None, + timeout_ms: None, + }) + .await + { + if let Some(count) = response.get("instance_count").and_then(serde_json::Value::as_u64) + { + if index == 0 { + counts.0 = count; + } else { + counts.1 = count; + } + } + } + } + counts +} + +/// Poll until both hook types report the expected instance count. +async fn wait_for_counts(iii: &IIIClient, expected: (u64, u64)) -> bool { + let deadline = Instant::now() + Duration::from_secs(15); + loop { + if hook_instance_counts(iii).await == expected { + return true; + } + if Instant::now() > deadline { + return false; + } + tokio::time::sleep(Duration::from_millis(100)).await; + } +} + +/// The gate starts AFTER a previous gate left instances behind: the hook +/// types exist and `engine::triggers::info` already reports count > 0 on the +/// first iteration. Readiness must not suppress this worker's own bind — the +/// old loop broke immediately on the foreign count, leaving the gate +/// detached. And each hook registers exactly once (no re-binding storm once +/// bound). +#[tokio::test(flavor = "multi_thread")] +async fn leftover_instances_do_not_suppress_this_workers_own_registration() { + if engine_bin().is_none() { + eprintln!("skipping: no iii engine"); + return; + } + let Some(engine) = spawn_engine().await else { + eprintln!("skipping: failed to spawn engine"); + return; + }; + let iii = register_worker(&engine.url, InitOptions::default()); + register_harness_hook_types(&iii); + + // Simulate a previous gate's instances that the engine has not yet + // garbage-collected: both types already report count 1. + assert!(bind_hook(&iii), "seed pre-trigger instance"); + assert!( + bind_filesystem_access_watch_hook(&iii), + "seed post-trigger instance" + ); + assert_eq!(hook_instance_counts(&iii).await, (1, 1), "seeded"); + + retry_hook_bindings(iii.clone()); + + // The loop must register its OWN instances on top of the leftovers + // (count 2), not conclude "ready" from the foreign count and skip. + assert!( + wait_for_counts(&iii, (2, 2)).await, + "this worker must bind its own hooks even though the harness reports ready" + ); + // Once bound, the loop must not re-register: counts stay stable across + // several retry intervals instead of stacking duplicates. + tokio::time::sleep(Duration::from_millis(1_300)).await; + assert_eq!( + hook_instance_counts(&iii).await, + (2, 2), + "bindings must not be re-registered after success" + ); +} + +/// The gate starts BEFORE the harness: binds fail while the hook types are +/// absent, and are retried once the harness comes up. The gate must not give +/// up after the first failure. +#[tokio::test(flavor = "multi_thread")] +async fn failed_registration_is_retried_until_the_harness_is_ready() { + if engine_bin().is_none() { + eprintln!("skipping: no iii engine"); + return; + } + let Some(engine) = spawn_engine().await else { + eprintln!("skipping: failed to spawn engine"); + return; + }; + let iii = register_worker(&engine.url, InitOptions::default()); + + // Hook types absent: every bind attempt fails. + retry_hook_bindings(iii.clone()); + tokio::time::sleep(Duration::from_millis(1_300)).await; + assert_eq!( + hook_instance_counts(&iii).await, + (0, 0), + "no binding may register while the harness hook types are absent" + ); + + // The harness comes up; the retry loop must pick the binds up. + register_harness_hook_types(&iii); + assert!( + wait_for_counts(&iii, (1, 1)).await, + "failed registrations must be retried once the harness is ready" + ); + tokio::time::sleep(Duration::from_millis(1_300)).await; + assert_eq!( + hook_instance_counts(&iii).await, + (1, 1), + "bindings must not be re-registered after success" + ); +} diff --git a/harness/src/hooks/mod.rs b/harness/src/hooks/mod.rs index 92019ab45..aadf2f65f 100644 --- a/harness/src/hooks/mod.rs +++ b/harness/src/hooks/mod.rs @@ -129,6 +129,8 @@ pub struct HookTriggerConfig { /// One parsed hook binding. #[derive(Debug, Clone)] pub struct HookBinding { + /// Trigger instance id (for unregister-by-id). + pub id: String, pub function_id: String, /// Static system-prompt contribution declared in trigger metadata. New /// harnesses apply it directly; the bound function remains the fallback @@ -167,6 +169,7 @@ impl HookBinding { .filter(|prompt| !prompt.is_empty()) .map(str::to_string); Ok(HookBinding { + id: config.id.clone(), function_id: config.function_id, inject_prompt, functions: cfg.functions, @@ -189,12 +192,30 @@ pub struct HookSet { impl HookSet { fn add(&self, point: HookPoint, config: TriggerConfig) -> Result<(), String> { let binding = HookBinding::parse(point, config.clone())?; - self.lock().insert(config.id, binding); + let mut inner = self.lock(); + // One binding per function per point: a re-arming registrar (retry + // loops, raced startup) can register the same function repeatedly, + // and the chain must consult it exactly once — `chain_slice`'s + // resume skips the SINGLE holder, so duplicates would re-run the + // hook on release (approval re-hold deadlock). First registration + // wins; later duplicates are dropped. + if let Some(existing) = inner.get(&binding.function_id) { + if existing.id != binding.id { + tracing::warn!( + function_id = %binding.function_id, + existing_id = %existing.id, + dropped_id = %binding.id, + "duplicate hook binding dropped (one per function per point)" + ); + } + return Ok(()); + } + inner.insert(binding.function_id.clone(), binding); Ok(()) } fn remove(&self, id: &str) { - self.lock().remove(id); + self.lock().retain(|_, binding| binding.id != id); } /// Bindings sorted into chain order: ascending priority, ties by @@ -387,6 +408,30 @@ mod tests { } } + #[test] + fn duplicate_function_binding_registers_once() { + // A re-arming registrar (retry loop racing the engine's instance + // count) stacks identical bindings; the chain must consult the hook + // exactly once or release re-runs it (approval re-hold deadlock). + let set = HookSet::default(); + set.add(HookPoint::PreTrigger, cfg("t_1", "approval::gate", json!({}))) + .unwrap(); + set.add(HookPoint::PreTrigger, cfg("t_2", "approval::gate", json!({}))) + .unwrap(); + set.add(HookPoint::PreTrigger, cfg("t_3", "approval::gate", json!({}))) + .unwrap(); + let ordered = set.ordered(); + assert_eq!(ordered.len(), 1); + assert_eq!(ordered[0].function_id, "approval::gate"); + assert_eq!(ordered[0].id, "t_1", "first registration wins"); + // unregistering the kept instance drops the binding; unregistering a + // dropped duplicate is a no-op that must not resurrect anything + set.remove("t_2"); + assert_eq!(set.ordered().len(), 1); + set.remove("t_1"); + assert!(set.is_empty()); + } + #[test] fn bindings_order_by_priority_then_function_id() { let set = HookSet::default(); diff --git a/harness/src/hooks/runner.rs b/harness/src/hooks/runner.rs index cd5846a61..75cd240f6 100644 --- a/harness/src/hooks/runner.rs +++ b/harness/src/hooks/runner.rs @@ -637,6 +637,7 @@ mod tests { fn binding(function_id: &str, priority: i64) -> HookBinding { HookBinding { + id: format!("t_{function_id}"), function_id: function_id.into(), inject_prompt: None, functions: Some(vec!["shell::*".into()]), @@ -755,6 +756,7 @@ mod tests { #[test] fn functions_filter_matches_globs() { let binding = HookBinding { + id: "t_gate".into(), function_id: "gate".into(), inject_prompt: None, functions: Some(vec!["shell::*".into()]), diff --git a/provider-anthropic/Cargo.lock b/provider-anthropic/Cargo.lock index 30b8d6ae2..e850ee3a3 100644 --- a/provider-anthropic/Cargo.lock +++ b/provider-anthropic/Cargo.lock @@ -996,7 +996,7 @@ checksum = "92daf443525c4cce67b150400bc2316076100ce0b3686209eb8cf3c31612e6f0" [[package]] name = "llm-router" -version = "1.4.2" +version = "1.4.7" dependencies = [ "async-trait", "clap", diff --git a/provider-openai-codex/Cargo.lock b/provider-openai-codex/Cargo.lock index 537865d72..c8eed4918 100644 --- a/provider-openai-codex/Cargo.lock +++ b/provider-openai-codex/Cargo.lock @@ -996,7 +996,7 @@ checksum = "92daf443525c4cce67b150400bc2316076100ce0b3686209eb8cf3c31612e6f0" [[package]] name = "llm-router" -version = "1.4.2" +version = "1.4.7" dependencies = [ "async-trait", "clap", diff --git a/provider-openai/Cargo.lock b/provider-openai/Cargo.lock index ebdc990b5..2d015f9ce 100644 --- a/provider-openai/Cargo.lock +++ b/provider-openai/Cargo.lock @@ -996,7 +996,7 @@ checksum = "92daf443525c4cce67b150400bc2316076100ce0b3686209eb8cf3c31612e6f0" [[package]] name = "llm-router" -version = "1.4.2" +version = "1.4.7" dependencies = [ "async-trait", "clap",