From b0b53500b6873091af9cf1715851f9fbab0b62d6 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Mon, 31 Aug 2026 17:54:27 +0700 Subject: [PATCH 1/7] test(approval): stop the gate tests racing their own TTL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_gate() hard-codes a 2s park window that every test in this suite shares, including the ones that never wait for an expiry. Most park a call, poll for the row, then decide it — so the window only has to outlast the poll, and it twice did not. #2367 raised it 500ms → 2s after the row expired before decide() could fire; 2s then lost the same race under cargo-llvm-cov, where each sleep(10ms) in the 50×10ms poll budget stretches on a contended runner. Once the row is past expires_at, the expire_stale_with_now pass inside store::decide denies it before that call's own UPDATE ... WHERE decided_at IS NULL can match, decide returns Ok(None), the waiter is never woken, and the park resolves as a TTL Deny. Raising the number a third time would only move the threshold, so the coupling is gone instead. test_gate() now uses the production DEFAULT_APPROVAL_TTL, and the five tests that actually exercise expiry ask for the short window through expiry_gate(). part_02 already worked around the old coupling by hand — copilot_streaming_park_persists_the_clamped_expiry builds its own gate because test_gate()'s 2s "would make this assertion vacuous". expiry_gate() also holds TEST_ENV_LOCK and clears OPENHUMAN_APPROVAL_TTL_SECS while it does. effective_ttl() reads that variable at park time in debug builds, so without the lock an expiry test can park under the effective_ttl_* tests' value of 42 — and a row meant to die in two seconds outlives the test waiting for it. Clearing it also covers a developer who exported the variable in their own shell, which no lock can protect against. This addresses the CodeRabbit review point on the previous revision. decide_parked() replaces a bare decide().unwrap() at two call sites. The unwrap there unwraps the Result, not the Option, so a lazily-expired row passes silently and the test fails a few lines later on "the outcome was not Allow" — naming the wrong event. The assert names the real one. The effective_ttl_* tests move to a boot TTL of 7s, deliberately a value nothing else here uses: they read the TTL back out of the gate, so sharing a number with the default would let them pass against the wrong source. --- src/openhuman/security/approval/gate_tests.rs | 83 +++++++++++++++++-- .../approval/gate_tests_part_01_tests.rs | 4 +- .../approval/gate_tests_part_02_tests.rs | 28 +++---- .../approval/gate_tests_part_03_tests.rs | 10 ++- 4 files changed, 98 insertions(+), 27 deletions(-) diff --git a/src/openhuman/security/approval/gate_tests.rs b/src/openhuman/security/approval/gate_tests.rs index 9f21b952af5..dd4c64e6d62 100644 --- a/src/openhuman/security/approval/gate_tests.rs +++ b/src/openhuman/security/approval/gate_tests.rs @@ -1,8 +1,39 @@ use super::*; use tempfile::TempDir; +/// TTL for the tests that assert the *timeout* path — the only ones that +/// need a park to expire while the test is still running. +const EXPIRY_TEST_TTL: Duration = Duration::from_secs(2); + +/// Boot-time TTL for the `effective_ttl` fallback tests, which need a +/// window they can name rather than one that has to expire. +/// +/// Deliberately a value nothing else in this module uses: those tests read +/// the TTL back out of the gate, so a number shared with the default (or +/// with [`EXPIRY_TEST_TTL`]) would let them pass against the wrong source. +const BOOT_TTL_UNDER_TEST: Duration = Duration::from_secs(7); + +/// A gate whose park window is the production [`DEFAULT_APPROVAL_TTL`]. +/// +/// Most tests here park a call, poll for the row, then decide it; none of +/// them wait for expiry, so the window only has to outlast the poll. It +/// twice did not. #2367 raised it 500ms → 2s after the row expired before +/// `decide` could fire, and 2s then lost the same race under +/// `cargo-llvm-cov`: the poll loops budget 50×10ms, but each +/// `sleep(10ms)` stretches on a contended runner, and once the row is past +/// `expires_at` the `expire_stale_with_now` pass inside [`store::decide`] +/// denies it before that call's own `UPDATE ... WHERE decided_at IS NULL` +/// can match — so `decide` returns `Ok(None)`, the waiter is never woken, +/// and the park resolves as a TTL `Deny`. +/// +/// Raising the number a third time would only move the threshold, so the +/// coupling is gone instead: tests that do not exercise expiry cannot +/// reach it, and the five that do ask for [`EXPIRY_TEST_TTL`] explicitly +/// via [`expiry_gate`]. That also removes an undocumented contract +/// between this helper and those distant call sites — two of them still +/// carried a `// TTL = 500ms` comment three raises later. fn test_gate() -> (ApprovalGate, TempDir) { - test_gate_with_ttl(Duration::from_secs(2)) + test_gate_with_ttl(DEFAULT_APPROVAL_TTL) } /// Build an approval gate with an explicit park deadline for tests that must @@ -18,16 +49,54 @@ fn test_gate_with_ttl(ttl: Duration) -> (ApprovalGate, TempDir) { // `debug_assert!` regression guard in `ApprovalGate::new` // doesn't trip in tests. let session = format!("session-{}", uuid::Uuid::new_v4()); - // 500ms TTL was racing the 50×10ms poll loop on slow CI - // runners — the row would expire (and get denied by - // list_pending's lazy-expire) before `decide` could fire, - // surfacing as "pending row never appeared". 2s gives the - // most polling tests enough headroom while keeping - // `timeout_returns_deny` fast (PR #2367 CI flake). let gate = ApprovalGate::new(config, session, ttl); (gate, dir) } +/// A gate for the tests that let a park expire, plus the env lock they +/// have to hold while it does. +/// +/// [`ApprovalGate::effective_ttl`] reads `OPENHUMAN_APPROVAL_TTL_SECS` at +/// park time, not at construction, and this whole suite is a debug build, +/// so the override is live. The `effective_ttl_*` tests set that variable +/// for the length of their assertion. Rust runs tests as threads in one +/// process, so without sharing their lock an expiry test can park under +/// their `42`, and a row that was supposed to die in two seconds outlives +/// the test that is waiting for it instead. +/// +/// The guard is returned rather than dropped here: holding it only while +/// the gate is built would leave the window open for the park itself. +/// `test_gate_with_ttl` must not take the lock, because the +/// `effective_ttl_*` tests call it while already holding it. +fn expiry_gate() -> (ApprovalGate, TempDir, std::sync::MutexGuard<'static, ()>) { + let env = crate::openhuman::config::TEST_ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + // The lock keeps a sibling test from setting the override, but not a + // developer who exported it in their shell. effective_ttl would then + // replace EXPIRY_TEST_TTL at park time and the wait would be measuring + // their value, so clear it while the lock is held. + unsafe { std::env::remove_var("OPENHUMAN_APPROVAL_TTL_SECS") }; + let (gate, dir) = test_gate_with_ttl(EXPIRY_TEST_TTL); + (gate, dir, env) +} + +/// Decide a row that the test has just seen parked, failing on the expiry +/// race rather than through it. +/// +/// [`ApprovalGate::decide`] returns `Ok(None)` when the row was already +/// resolved — which, per [`DecideMiss::AlreadyResolved`], includes *lazily +/// expired*. A bare `.unwrap()` there unwraps the `Result`, not the +/// `Option`, so that case passes silently and the test instead fails a few +/// lines later on "the outcome was not `Allow`", naming the wrong event. +fn decide_parked(gate: &ApprovalGate, request_id: &str, decision: ApprovalDecision) { + assert!( + gate.decide(request_id, decision).unwrap().is_some(), + "the parked row {request_id} was already resolved before the decision \ + landed — it expired mid-test rather than being decided" + ); +} + /// A chat context — the gate only parks within a live chat turn now, so /// tests that exercise parking must run intercept inside this scope. fn chat_ctx() -> ApprovalChatContext { diff --git a/src/openhuman/security/approval/gate_tests_part_01_tests.rs b/src/openhuman/security/approval/gate_tests_part_01_tests.rs index 27dec459463..8ecefaf77b5 100644 --- a/src/openhuman/security/approval/gate_tests_part_01_tests.rs +++ b/src/openhuman/security/approval/gate_tests_part_01_tests.rs @@ -511,7 +511,7 @@ async fn auto_approve_all_allows_a_remote_triage_dispatch_without_an_audit_row() #[tokio::test] async fn timeout_returns_deny() { - let (gate, _dir) = test_gate(); // TTL = 500ms + let (gate, _dir, _env) = expiry_gate(); let gate = Arc::new(gate); let outcome = turn_origin::with_origin( web_origin(), @@ -538,7 +538,7 @@ async fn timeout_returns_deny() { /// unapproved, mirroring `timeout_returns_deny` above. #[tokio::test] async fn cancel_flow_run_parks_for_approval_when_a_gate_is_present() { - let (gate, _dir) = test_gate(); // TTL = 500ms + let (gate, _dir, _env) = expiry_gate(); let gate = Arc::new(gate); let outcome = turn_origin::with_origin( web_origin(), diff --git a/src/openhuman/security/approval/gate_tests_part_02_tests.rs b/src/openhuman/security/approval/gate_tests_part_02_tests.rs index 77c8d824960..d45adb2cbed 100644 --- a/src/openhuman/security/approval/gate_tests_part_02_tests.rs +++ b/src/openhuman/security/approval/gate_tests_part_02_tests.rs @@ -5,7 +5,7 @@ async fn pending_for_thread_tracks_request_under_chat_context_and_clears() { // This test polls a parked task while the full Rust suite runs thousands // of other tests concurrently. Keep its coordination deadline independent // of the short TTL used by the timeout behavior tests. - let (gate, _dir) = test_gate_with_ttl(Duration::from_secs(10)); + let (gate, _dir) = test_gate(); let gate = Arc::new(gate); // Run intercept inside a scoped chat context + matching WebChat @@ -40,8 +40,7 @@ async fn pending_for_thread_tracks_request_under_chat_context_and_clears() { }; // Decide via the mapped request_id (as the chat ingress router will). - gate.decide(&request_id, ApprovalDecision::ApproveOnce) - .unwrap(); + decide_parked(&gate, &request_id, ApprovalDecision::ApproveOnce); assert!(matches!(handle.await.unwrap(), GateOutcome::Allow)); // Mapping is cleared once intercept returns. @@ -61,7 +60,7 @@ async fn pending_for_thread_tracks_request_under_chat_context_and_clears() { async fn webchat_origin_routes_park_when_approval_chat_context_absent() { // See the companion chat-context routing test above: this is a // coordination deadline, not the timeout behavior under test. - let (gate, _dir) = test_gate_with_ttl(Duration::from_secs(10)); + let (gate, _dir) = test_gate(); let gate = Arc::new(gate); // WebChat origin scoped, but NO `APPROVAL_CHAT_CONTEXT` — exactly the @@ -100,8 +99,7 @@ async fn webchat_origin_routes_park_when_approval_chat_context_absent() { // A decision on that mapped request resolves the park (the card can // surface and be approved), instead of silently TTL-denying. - gate.decide(&request_id, ApprovalDecision::ApproveOnce) - .unwrap(); + decide_parked(&gate, &request_id, ApprovalDecision::ApproveOnce); assert!(matches!(handle.await.unwrap(), GateOutcome::Allow)); assert!(gate.pending_for_thread("thread-async").is_none()); } @@ -187,7 +185,7 @@ async fn waiter_future_dropped_mid_park_evicts_waiter_clears_routing_and_denies_ // card-click still resolves it in the DB. #[tokio::test] async fn intercept_audited_bounded_abandons_park_and_leaves_row_pending() { - let (gate, _dir) = test_gate(); // boot-time TTL = 2s + let (gate, _dir) = test_gate(); let gate = Arc::new(gate); let g = gate.clone(); @@ -200,7 +198,7 @@ async fn intercept_audited_bounded_abandons_park_and_leaves_row_pending() { client_id: "client-1".into(), request_id: Some("req-bound".into()), }; - // 100ms caller bound — far below the 2s gate TTL — so the bound is what + // 100ms caller bound — far below the gate TTL — so the bound is what // elapses, not the gate's own timeout. let handle = tokio::spawn(async move { turn_origin::with_origin( @@ -269,7 +267,7 @@ fn effective_ttl_uses_env_override_when_valid() { let _env = crate::openhuman::config::TEST_ENV_LOCK .lock() .unwrap_or_else(|e| e.into_inner()); - let (gate, _dir) = test_gate(); // boot-time TTL = 2s + let (gate, _dir) = test_gate_with_ttl(BOOT_TTL_UNDER_TEST); unsafe { std::env::set_var("OPENHUMAN_APPROVAL_TTL_SECS", "42") }; assert_eq!( gate.effective_ttl(), @@ -284,11 +282,11 @@ fn effective_ttl_falls_back_to_boot_ttl_for_garbage_value() { let _env = crate::openhuman::config::TEST_ENV_LOCK .lock() .unwrap_or_else(|e| e.into_inner()); - let (gate, _dir) = test_gate(); // boot-time TTL = 2s + let (gate, _dir) = test_gate_with_ttl(BOOT_TTL_UNDER_TEST); unsafe { std::env::set_var("OPENHUMAN_APPROVAL_TTL_SECS", "not-a-number") }; assert_eq!( gate.effective_ttl(), - Duration::from_secs(2), + BOOT_TTL_UNDER_TEST, "garbage OPENHUMAN_APPROVAL_TTL_SECS must fall back to boot-time TTL" ); unsafe { std::env::remove_var("OPENHUMAN_APPROVAL_TTL_SECS") }; @@ -299,11 +297,11 @@ fn effective_ttl_falls_back_to_boot_ttl_when_unset() { let _env = crate::openhuman::config::TEST_ENV_LOCK .lock() .unwrap_or_else(|e| e.into_inner()); - let (gate, _dir) = test_gate(); // boot-time TTL = 2s + let (gate, _dir) = test_gate_with_ttl(BOOT_TTL_UNDER_TEST); unsafe { std::env::remove_var("OPENHUMAN_APPROVAL_TTL_SECS") }; assert_eq!( gate.effective_ttl(), - Duration::from_secs(2), + BOOT_TTL_UNDER_TEST, "unset OPENHUMAN_APPROVAL_TTL_SECS must fall back to boot-time TTL" ); } @@ -359,7 +357,7 @@ mod resolve_park_ttl_tests { /// above, but that alone doesn't prove `intercept_audited_inner` actually /// persists the clamped TTL when the copilot-streaming context is scoped. /// Builds a gate with the full `DEFAULT_APPROVAL_TTL` boot TTL (unlike -/// `test_gate()`'s 2s, which is already shorter than either clamp and +/// `test_gate()` before this suite stopped hard-coding a 2s window, which /// would make this assertion vacuous), scopes /// `APPROVAL_COPILOT_STREAM_CONTEXT` alongside the chat context + WebChat /// origin the way `flows::ops::flows_build` does in production, and @@ -596,7 +594,7 @@ async fn intercept_with_workflow_require_approval_persists_and_ttl_denies() { // trust root — same conservative park-and-audit shape as // `GoalContinuation` / `ExternalChannel`, since there is no flow // review surface to route the prompt to yet (B3). - let (gate, _dir) = test_gate(); // 2s TTL + let (gate, _dir, _env) = expiry_gate(); let gate = Arc::new(gate); let origin = AgentTurnOrigin::TrustedAutomation { job_id: "flow-2".into(), diff --git a/src/openhuman/security/approval/gate_tests_part_03_tests.rs b/src/openhuman/security/approval/gate_tests_part_03_tests.rs index bf015dcc05f..ef320d10c63 100644 --- a/src/openhuman/security/approval/gate_tests_part_03_tests.rs +++ b/src/openhuman/security/approval/gate_tests_part_03_tests.rs @@ -111,7 +111,7 @@ async fn intercept_with_external_channel_origin_persists_and_ttl_denies() { // Non-web channel inbound (Telegram / Discord / Slack / etc.): // persist an audit row but TTL-deny — there is no channel-routed // approval surface yet, and the input is remote-attacker text. - let (gate, _dir) = test_gate(); // 2s TTL + let (gate, _dir, _env) = expiry_gate(); let gate = Arc::new(gate); let origin = AgentTurnOrigin::ExternalChannel { channel: "telegram".into(), @@ -355,7 +355,10 @@ async fn flow_tool_trust_auto_allows_before_parking() { // short-circuit to `Allow` even for a `require_approval: true` flow — // that is the whole point of "approve always for this workflow": no // pending row is created and the call never parks. - let (gate, _dir) = test_gate(); + // + // The second half of this test *does* wait a park out, so it needs the + // short window even though it never inspects the "timed out" reason. + let (gate, _dir, _env) = expiry_gate(); store::insert_flow_trust(&gate.config, "flow-trusted", "composio").unwrap(); let outcome = turn_origin::with_origin( @@ -377,7 +380,8 @@ async fn flow_tool_trust_auto_allows_before_parking() { ); // A different tool on the same trusted flow is unaffected — it still - // parks (TTL-denies on the 2s test gate). + // parks, and nothing decides it, so it TTL-denies after + // `EXPIRY_TEST_TTL`. let untrusted_outcome = turn_origin::with_origin( flow_origin("flow-trusted", true), APPROVAL_FLOW_RUN_CONTEXT.scope( From 93f631eef876fca1912ccbaf507832b422b7b2d9 Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sat, 12 Sep 2026 04:21:57 +0300 Subject: [PATCH 2/7] test(approval): restore env var in expiry_gate helper The `expiry_gate` test helper now saves and restores the `OPENHUMAN_APPROVAL_TTL_SECS` environment variable, preventing test pollution when a developer has the variable set in their shell. A new `ExpiryEnvGuard` struct with a `Drop` implementation handles the restoration automatically when the guard goes out of scope. Auto-committed-on: dragonfly Co-authored-by: Medulla --- src/openhuman/security/approval/gate_tests.rs | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/src/openhuman/security/approval/gate_tests.rs b/src/openhuman/security/approval/gate_tests.rs index dd4c64e6d62..a25f0aa6e68 100644 --- a/src/openhuman/security/approval/gate_tests.rs +++ b/src/openhuman/security/approval/gate_tests.rs @@ -68,7 +68,23 @@ fn test_gate_with_ttl(ttl: Duration) -> (ApprovalGate, TempDir) { /// the gate is built would leave the window open for the park itself. /// `test_gate_with_ttl` must not take the lock, because the /// `effective_ttl_*` tests call it while already holding it. -fn expiry_gate() -> (ApprovalGate, TempDir, std::sync::MutexGuard<'static, ()>) { +struct ExpiryEnvGuard { + previous_ttl: Option, + _env_lock: std::sync::MutexGuard<'static, ()>, +} + +impl Drop for ExpiryEnvGuard { + fn drop(&mut self) { + match self.previous_ttl.take() { + Some(value) => unsafe { + std::env::set_var("OPENHUMAN_APPROVAL_TTL_SECS", value) + }, + None => unsafe { std::env::remove_var("OPENHUMAN_APPROVAL_TTL_SECS") }, + } + } +} + +fn expiry_gate() -> (ApprovalGate, TempDir, ExpiryEnvGuard) { let env = crate::openhuman::config::TEST_ENV_LOCK .lock() .unwrap_or_else(|e| e.into_inner()); @@ -76,9 +92,17 @@ fn expiry_gate() -> (ApprovalGate, TempDir, std::sync::MutexGuard<'static, ()>) // developer who exported it in their shell. effective_ttl would then // replace EXPIRY_TEST_TTL at park time and the wait would be measuring // their value, so clear it while the lock is held. + let previous_ttl = std::env::var("OPENHUMAN_APPROVAL_TTL_SECS").ok(); unsafe { std::env::remove_var("OPENHUMAN_APPROVAL_TTL_SECS") }; let (gate, dir) = test_gate_with_ttl(EXPIRY_TEST_TTL); - (gate, dir, env) + ( + gate, + dir, + ExpiryEnvGuard { + previous_ttl, + _env_lock: env, + }, + ) } /// Decide a row that the test has just seen parked, failing on the expiry From e9979f4c3fa68c2b15ef3229285442ca97a71ccd Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sat, 12 Sep 2026 04:24:02 +0300 Subject: [PATCH 3/7] chore(approval): collapse multi-line env setter in test guard Collapsed the `set_var` call in the `ExpiryEnvGuard` drop implementation from three lines to one, reducing visual noise without changing behaviour. Auto-committed-on: dragonfly Co-authored-by: Medulla --- src/openhuman/security/approval/gate_tests.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/openhuman/security/approval/gate_tests.rs b/src/openhuman/security/approval/gate_tests.rs index a25f0aa6e68..ecbc118f2d4 100644 --- a/src/openhuman/security/approval/gate_tests.rs +++ b/src/openhuman/security/approval/gate_tests.rs @@ -76,9 +76,7 @@ struct ExpiryEnvGuard { impl Drop for ExpiryEnvGuard { fn drop(&mut self) { match self.previous_ttl.take() { - Some(value) => unsafe { - std::env::set_var("OPENHUMAN_APPROVAL_TTL_SECS", value) - }, + Some(value) => unsafe { std::env::set_var("OPENHUMAN_APPROVAL_TTL_SECS", value) }, None => unsafe { std::env::remove_var("OPENHUMAN_APPROVAL_TTL_SECS") }, } } From fb93c98f620d2bc3735c79595c1c65bd91c5043b Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sat, 12 Sep 2026 04:51:08 +0300 Subject: [PATCH 4/7] test(approval): wait for audit row before dropping env in TTL tests Several approval gate tests that verify TTL-based denial were dropping the test environment before the gate had a chance to persist the audit row, causing flaky failures. The tests now spawn the intercept call on a separate task, poll for the pending audit row to appear, and only then drop the environment to ensure the row is written before the expiry timer starts. Auto-committed-on: dragonfly Co-authored-by: Medulla --- .../approval/gate_tests_part_01_tests.rs | 64 +++++++++++++------ .../approval/gate_tests_part_02_tests.rs | 3 +- .../approval/gate_tests_part_03_tests.rs | 35 ++++++---- 3 files changed, 68 insertions(+), 34 deletions(-) diff --git a/src/openhuman/security/approval/gate_tests_part_01_tests.rs b/src/openhuman/security/approval/gate_tests_part_01_tests.rs index 8ecefaf77b5..ea760c1747a 100644 --- a/src/openhuman/security/approval/gate_tests_part_01_tests.rs +++ b/src/openhuman/security/approval/gate_tests_part_01_tests.rs @@ -511,16 +511,27 @@ async fn auto_approve_all_allows_a_remote_triage_dispatch_without_an_audit_row() #[tokio::test] async fn timeout_returns_deny() { - let (gate, _dir, _env) = expiry_gate(); + let (gate, _dir, env) = expiry_gate(); let gate = Arc::new(gate); - let outcome = turn_origin::with_origin( - web_origin(), - APPROVAL_CHAT_CONTEXT.scope( - chat_ctx(), - gate.intercept("composio", "timed out", serde_json::json!({})), - ), - ) - .await; + let g = gate.clone(); + let handle = tokio::spawn(async move { + turn_origin::with_origin( + web_origin(), + APPROVAL_CHAT_CONTEXT.scope( + chat_ctx(), + g.intercept("composio", "timed out", serde_json::json!({})), + ), + ) + .await + }); + let mut tries = 0; + while gate.list_pending().unwrap().is_empty() { + tries += 1; + assert!(tries < 50, "audit row never appeared for timeout test"); + tokio::time::sleep(Duration::from_millis(10)).await; + } + drop(env); + let outcome = handle.await.unwrap(); match outcome { GateOutcome::Deny { reason } => assert!(reason.contains("timed out")), other => panic!("expected deny, got {other:?}"), @@ -538,20 +549,31 @@ async fn timeout_returns_deny() { /// unapproved, mirroring `timeout_returns_deny` above. #[tokio::test] async fn cancel_flow_run_parks_for_approval_when_a_gate_is_present() { - let (gate, _dir, _env) = expiry_gate(); + let (gate, _dir, env) = expiry_gate(); let gate = Arc::new(gate); - let outcome = turn_origin::with_origin( - web_origin(), - APPROVAL_CHAT_CONTEXT.scope( - chat_ctx(), - gate.intercept( - "cancel_flow_run", - "cancel run r-1 of flow f-1", - serde_json::json!({ "flow_id": "f-1", "run_id": "r-1" }), + let g = gate.clone(); + let handle = tokio::spawn(async move { + turn_origin::with_origin( + web_origin(), + APPROVAL_CHAT_CONTEXT.scope( + chat_ctx(), + g.intercept( + "cancel_flow_run", + "cancel run r-1 of flow f-1", + serde_json::json!({ "flow_id": "f-1", "run_id": "r-1" }), + ), ), - ), - ) - .await; + ) + .await + }); + let mut tries = 0; + while gate.list_pending().unwrap().is_empty() { + tries += 1; + assert!(tries < 50, "audit row never appeared for cancel_flow_run"); + tokio::time::sleep(Duration::from_millis(10)).await; + } + drop(env); + let outcome = handle.await.unwrap(); // No decision ever arrives — the call must NOT auto-execute. It // parks until the gate's TTL elapses, then denies (never `Allow`). match outcome { diff --git a/src/openhuman/security/approval/gate_tests_part_02_tests.rs b/src/openhuman/security/approval/gate_tests_part_02_tests.rs index 405dc051065..a985df5e64e 100644 --- a/src/openhuman/security/approval/gate_tests_part_02_tests.rs +++ b/src/openhuman/security/approval/gate_tests_part_02_tests.rs @@ -594,7 +594,7 @@ async fn intercept_with_workflow_require_approval_persists_and_ttl_denies() { // trust root — same conservative park-and-audit shape as // `GoalContinuation` / `ExternalChannel`, since there is no flow // review surface to route the prompt to yet (B3). - let (gate, _dir, _env) = expiry_gate(); + let (gate, _dir, env) = expiry_gate(); let gate = Arc::new(gate); let origin = AgentTurnOrigin::TrustedAutomation { job_id: "flow-2".into(), @@ -625,6 +625,7 @@ async fn intercept_with_workflow_require_approval_persists_and_ttl_denies() { tokio::time::sleep(Duration::from_millis(10)).await; } + drop(env); let outcome = handle.await.unwrap(); match outcome { GateOutcome::Deny { reason } => assert!(reason.contains("timed out")), diff --git a/src/openhuman/security/approval/gate_tests_part_03_tests.rs b/src/openhuman/security/approval/gate_tests_part_03_tests.rs index 99aef3ec2c3..0589e94e056 100644 --- a/src/openhuman/security/approval/gate_tests_part_03_tests.rs +++ b/src/openhuman/security/approval/gate_tests_part_03_tests.rs @@ -136,7 +136,7 @@ async fn intercept_with_external_channel_origin_persists_and_ttl_denies() { // Non-web channel inbound (Telegram / Discord / Slack / etc.): // persist an audit row but TTL-deny — there is no channel-routed // approval surface yet, and the input is remote-attacker text. - let (gate, _dir, _env) = expiry_gate(); + let (gate, _dir, env) = expiry_gate(); let gate = Arc::new(gate); let origin = AgentTurnOrigin::ExternalChannel { channel: "telegram".into(), @@ -407,17 +407,28 @@ async fn flow_tool_trust_auto_allows_before_parking() { // A different tool on the same trusted flow is unaffected — it still // parks, and nothing decides it, so it TTL-denies after // `EXPIRY_TEST_TTL`. - let untrusted_outcome = turn_origin::with_origin( - flow_origin("flow-trusted", true), - APPROVAL_FLOW_RUN_CONTEXT.scope( - FlowRunContext { - flow_id: "flow-trusted".to_string(), - run_id: "run-1".to_string(), - }, - gate.intercept("pushover", "send push", serde_json::json!({})), - ), - ) - .await; + let g = gate.clone(); + let handle = tokio::spawn(async move { + turn_origin::with_origin( + flow_origin("flow-trusted", true), + APPROVAL_FLOW_RUN_CONTEXT.scope( + FlowRunContext { + flow_id: "flow-trusted".to_string(), + run_id: "run-1".to_string(), + }, + g.intercept("pushover", "send push", serde_json::json!({})), + ), + ) + .await + }); + let mut tries = 0; + while gate.list_pending().unwrap().is_empty() { + tries += 1; + assert!(tries < 50, "audit row never appeared for untrusted flow tool"); + tokio::time::sleep(Duration::from_millis(10)).await; + } + drop(env); + let untrusted_outcome = handle.await.unwrap(); assert!( matches!(untrusted_outcome, GateOutcome::Deny { .. }), "trust must be scoped to the exact tool granted, not the whole flow" From 10464b7a735e5363fe0994ffc0f0cef56a4fc4dd Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sat, 12 Sep 2026 04:52:17 +0300 Subject: [PATCH 5/7] fix(tests): release env lock before waiting for TTL expiry in approval gate tests The `intercept_with_external_channel_origin_persists_and_ttl_denies` test was holding the process-wide environment lock while waiting for the parked future to TTL-deny, which prevented concurrent expiry tests from running. The lock is now dropped before the wait, allowing those tests to proceed in parallel. The `flow_tool_trust_auto_allows_before_parking` test similarly now captures the env guard so it can be released after the audit row appears, avoiding unnecessary serialization. The `ExpiryEnvGuard` doc comment is updated to clarify that callers should drop the guard after observing the pending row rather than holding it through the entire expiry. Auto-committed-on: dragonfly Co-authored-by: Medulla --- src/openhuman/security/approval/gate_tests.rs | 8 +++++--- .../security/approval/gate_tests_part_03_tests.rs | 12 +++++++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/openhuman/security/approval/gate_tests.rs b/src/openhuman/security/approval/gate_tests.rs index ecbc118f2d4..98f7b0a0e0b 100644 --- a/src/openhuman/security/approval/gate_tests.rs +++ b/src/openhuman/security/approval/gate_tests.rs @@ -54,7 +54,7 @@ fn test_gate_with_ttl(ttl: Duration) -> (ApprovalGate, TempDir) { } /// A gate for the tests that let a park expire, plus the env lock they -/// have to hold while it does. +/// hold until the pending row captures its effective TTL. /// /// [`ApprovalGate::effective_ttl`] reads `OPENHUMAN_APPROVAL_TTL_SECS` at /// park time, not at construction, and this whole suite is a debug build, @@ -64,8 +64,10 @@ fn test_gate_with_ttl(ttl: Duration) -> (ApprovalGate, TempDir) { /// their `42`, and a row that was supposed to die in two seconds outlives /// the test that is waiting for it instead. /// -/// The guard is returned rather than dropped here: holding it only while -/// the gate is built would leave the window open for the park itself. +/// Callers drop the returned guard after observing the pending row. Holding +/// it only while the gate is built would leave the window open for the park's +/// initial `effective_ttl` read, while holding it through expiry serializes +/// these tests unnecessarily. /// `test_gate_with_ttl` must not take the lock, because the /// `effective_ttl_*` tests call it while already holding it. struct ExpiryEnvGuard { diff --git a/src/openhuman/security/approval/gate_tests_part_03_tests.rs b/src/openhuman/security/approval/gate_tests_part_03_tests.rs index 0589e94e056..34f5b8d85c1 100644 --- a/src/openhuman/security/approval/gate_tests_part_03_tests.rs +++ b/src/openhuman/security/approval/gate_tests_part_03_tests.rs @@ -165,8 +165,11 @@ async fn intercept_with_external_channel_origin_persists_and_ttl_denies() { tokio::time::sleep(Duration::from_millis(10)).await; } + // The TTL was captured when the row was inserted; release the process-wide + // environment lock before waiting so expiry tests can run concurrently. + drop(env); // Without a routable channel approval surface, the parked future - // TTL-denies (2s — matches the test_gate fixture). + // TTL-denies after `EXPIRY_TEST_TTL`. let outcome = handle.await.unwrap(); match outcome { GateOutcome::Deny { reason } => assert!(reason.contains("timed out")), @@ -383,7 +386,7 @@ async fn flow_tool_trust_auto_allows_before_parking() { // // The second half of this test *does* wait a park out, so it needs the // short window even though it never inspects the "timed out" reason. - let (gate, _dir, _env) = expiry_gate(); + let (gate, _dir, env) = expiry_gate(); store::insert_flow_trust(&gate.config, "flow-trusted", "composio").unwrap(); let outcome = turn_origin::with_origin( @@ -424,7 +427,10 @@ async fn flow_tool_trust_auto_allows_before_parking() { let mut tries = 0; while gate.list_pending().unwrap().is_empty() { tries += 1; - assert!(tries < 50, "audit row never appeared for untrusted flow tool"); + assert!( + tries < 50, + "audit row never appeared for untrusted flow tool" + ); tokio::time::sleep(Duration::from_millis(10)).await; } drop(env); From 968efd0b704b92a245423b04a0ddc529a93df530 Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sat, 12 Sep 2026 06:43:07 +0300 Subject: [PATCH 6/7] test(approval): replace list_pending polling with waiter-key observation The tests for short-TTL approval gates were polling `list_pending` to wait for a row to appear, but that method lazily expires rows and could miss the entry when the TTL is very short. A new helper `parked_request_id` reads the waiter map directly, which is populated before the row is visible to `list_pending`, making the wait loop reliable regardless of TTL duration. Auto-committed-on: dragonfly Co-authored-by: Medulla --- src/openhuman/security/approval/gate_tests.rs | 7 +++++++ .../security/approval/gate_tests_part_02_tests.rs | 7 ++----- .../security/approval/gate_tests_part_03_tests.rs | 8 ++++---- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/openhuman/security/approval/gate_tests.rs b/src/openhuman/security/approval/gate_tests.rs index 98f7b0a0e0b..418b60af7df 100644 --- a/src/openhuman/security/approval/gate_tests.rs +++ b/src/openhuman/security/approval/gate_tests.rs @@ -53,6 +53,13 @@ fn test_gate_with_ttl(ttl: Duration) -> (ApprovalGate, TempDir) { (gate, dir) } +/// Return a request id after the approval row has been persisted and its +/// waiter has been registered. Unlike `list_pending`, this observation does +/// not lazily expire the row while a short-TTL test is waiting for insertion. +fn parked_request_id(gate: &ApprovalGate) -> Option { + gate.waiters.lock().keys().next().cloned() +} + /// A gate for the tests that let a park expire, plus the env lock they /// hold until the pending row captures its effective TTL. /// diff --git a/src/openhuman/security/approval/gate_tests_part_02_tests.rs b/src/openhuman/security/approval/gate_tests_part_02_tests.rs index a985df5e64e..77fb04a46bd 100644 --- a/src/openhuman/security/approval/gate_tests_part_02_tests.rs +++ b/src/openhuman/security/approval/gate_tests_part_02_tests.rs @@ -613,14 +613,11 @@ async fn intercept_with_workflow_require_approval_persists_and_ttl_denies() { }); let mut tries = 0; - loop { - if !gate.list_pending().unwrap().is_empty() { - break; - } + while parked_request_id(&gate).is_none() { tries += 1; assert!( tries < 50, - "audit row never appeared for require_approval workflow origin" + "approval waiter never appeared for require_approval workflow origin" ); tokio::time::sleep(Duration::from_millis(10)).await; } diff --git a/src/openhuman/security/approval/gate_tests_part_03_tests.rs b/src/openhuman/security/approval/gate_tests_part_03_tests.rs index 34f5b8d85c1..899dc685667 100644 --- a/src/openhuman/security/approval/gate_tests_part_03_tests.rs +++ b/src/openhuman/security/approval/gate_tests_part_03_tests.rs @@ -470,15 +470,15 @@ async fn decide_approve_always_for_flow_then_insert_flow_trust_composes_to_auto_ .await }); - let pending = loop { - if let Some(p) = gate.list_pending().unwrap().into_iter().next() { - break p; + let request_id = loop { + if let Some(request_id) = parked_request_id(&gate) { + break request_id; } tokio::time::sleep(Duration::from_millis(10)).await; }; let decided = gate - .decide(&pending.request_id, ApprovalDecision::ApproveAlwaysForFlow) + .decide(&request_id, ApprovalDecision::ApproveAlwaysForFlow) .unwrap() .expect("decided row"); From 36e88b619e5167fe4a6c8b729952fe8dc4445c8d Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sat, 12 Sep 2026 07:06:58 +0300 Subject: [PATCH 7/7] fix(approval): wrap gate in Arc and fix pending check in test The test `flow_tool_trust_auto_allows_before_parking` was failing because the gate was not wrapped in an `Arc`, preventing shared ownership across async contexts. Additionally, the loop checking for pending requests was using `list_pending` which returns all pending items, but the test needed to wait specifically for a parked request to appear. Changing the condition to use `parked_request_id` correctly waits for the expected state. Auto-committed-on: dragonfly Co-authored-by: Medulla --- src/openhuman/security/approval/gate_tests_part_03_tests.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/openhuman/security/approval/gate_tests_part_03_tests.rs b/src/openhuman/security/approval/gate_tests_part_03_tests.rs index 899dc685667..f087fbaa404 100644 --- a/src/openhuman/security/approval/gate_tests_part_03_tests.rs +++ b/src/openhuman/security/approval/gate_tests_part_03_tests.rs @@ -387,6 +387,7 @@ async fn flow_tool_trust_auto_allows_before_parking() { // The second half of this test *does* wait a park out, so it needs the // short window even though it never inspects the "timed out" reason. let (gate, _dir, env) = expiry_gate(); + let gate = Arc::new(gate); store::insert_flow_trust(&gate.config, "flow-trusted", "composio").unwrap(); let outcome = turn_origin::with_origin( @@ -425,7 +426,7 @@ async fn flow_tool_trust_auto_allows_before_parking() { .await }); let mut tries = 0; - while gate.list_pending().unwrap().is_empty() { + while parked_request_id(&gate).is_none() { tries += 1; assert!( tries < 50,