From 30e45d17ccb869ad1e6e70bf24cabf3995c9ea18 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sat, 13 Jun 2026 11:39:01 -0700 Subject: [PATCH 1/4] fix(slack): exclude auth gates from bare-approve ambiguity check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a conversation had both a live `gate:auth-*` record and a live `gate:approval-*` record in the same `DeliveredGateRouteStore` fingerprint bucket (e.g. a previously-started-but-unfinished OAuth flow), a bare "approve" triggered `AmbiguousGate` ("Multiple requests are pending…") instead of resolving the single approval gate. Root cause: `load_delivered_routes_for_envelope` had no gate-kind filter, so auth-gate records counted toward the `live.len()` ambiguity check alongside approval-gate records. Fix: add an optional `gate_kind_filter: Option bool>` parameter to `load_delivered_routes_for_envelope` (and threaded through `select_delivered_gate_route`). The approval path passes `Some(is_approval_gate_ref)` so only `gate:approval-*` records count; the auth path passes `Some(is_auth_gate_ref)` so only `gate:auth-*` / `gate:hook-auth-*` records count. The filter is applied after the expiry/actor checks and before the ambiguity gate, using the already-public typed predicates from `approval_interaction` and `auth_interaction` — no new string literals introduced. Existing test constants `GATE` and `GATE_B` (previously `gate:approve-slack{,-b}`) are updated to use real `gate:approval-` prefixed UUIDs so the approval-path filter exercises the correct code path in all related delivered-route tests. Regression test `bare_approve_with_one_approval_and_one_stale_auth_gate_resolves_approval` drives the full inbound→workflow resolution path (Slack e2e harness, `ForeignScopeApprovalService`, two records in the same DM fingerprint bucket) and asserts exactly one approval resolve request is forwarded. This test would fail on the pre-fix path (live.len()==2 → Ambiguous). Co-Authored-By: Claude Fable 5 --- .../ironclaw_product_workflow/src/workflow.rs | 37 +++++- .../src/slack_serve/e2e_tests.rs | 109 +++++++++++++++++- 2 files changed, 137 insertions(+), 9 deletions(-) diff --git a/crates/ironclaw_product_workflow/src/workflow.rs b/crates/ironclaw_product_workflow/src/workflow.rs index 7ead82076dd..3345b6c5fe9 100644 --- a/crates/ironclaw_product_workflow/src/workflow.rs +++ b/crates/ironclaw_product_workflow/src/workflow.rs @@ -27,11 +27,12 @@ use crate::action::{ActionDispatchKind, ActionFingerprintKey, SourceBindingKey}; use crate::approval_interaction::{ ApprovalInteractionDecision, ApprovalInteractionRejectionKind, ApprovalInteractionService, ListPendingApprovalsRequest, RejectingApprovalInteractionService, - ResolveApprovalInteractionRequest, ResolveApprovalInteractionResponse, + ResolveApprovalInteractionRequest, ResolveApprovalInteractionResponse, is_approval_gate_ref, }; use crate::auth_interaction::{ AuthInteractionDecision, AuthInteractionRejectionKind, AuthInteractionService, RejectingAuthInteractionService, ResolveAuthInteractionRequest, ResolveAuthInteractionResponse, + is_auth_gate_ref, }; use crate::binding::{ ConversationBindingService, ProductConversationRouteKind, ResolveBindingRequest, @@ -465,6 +466,12 @@ async fn load_delivered_routes_for_envelope( binding: &ResolvedBinding, delivered_gate_routes: &dyn ironclaw_outbound::DeliveredGateRouteStore, expected_gate_ref: Option<&str>, + // When `Some`, only routes whose `gate_ref` satisfies the predicate are + // considered live. This separates approval routes (`is_approval_gate_ref`) + // from auth routes (`is_auth_gate_ref`) so that a lingering auth gate + // recorded in the same conversation bucket cannot make a bare "approve" + // look ambiguous, and vice-versa. + gate_kind_filter: Option bool>, ) -> DeliveredRouteOutcome { let conversation_ref = match delivered_route_conversation_ref(envelope) { Ok(conversation_ref) => conversation_ref, @@ -495,8 +502,8 @@ async fn load_delivered_routes_for_envelope( return DeliveredRouteOutcome::Miss; } }; - // Filter: non-expired, tenant+actor match, and (if explicit ref supplied) - // gate_ref match. + // Filter: non-expired, tenant+actor match, gate-kind match (when a kind + // filter is supplied), and (if an explicit ref is supplied) gate_ref match. let live: Vec = all_routes .into_iter() .filter(|r| { @@ -519,6 +526,20 @@ async fn load_delivered_routes_for_envelope( if r.tenant_id != binding.tenant_id || r.user_id != binding.actor_user_id { return false; } + // Gate-kind filter: when the caller knows it is resolving an + // approval (or auth) interaction, drop routes that belong to the + // other kind. This prevents a lingering auth gate recorded in the + // same conversation fingerprint bucket from inflating the live-route + // count and triggering a spurious AmbiguousGate error on a bare + // "approve" (and vice-versa for a bare "auth deny"). + if let Some(kind_filter) = gate_kind_filter { + let Ok(gate_ref) = GateRef::new(r.gate_ref.clone()) else { + return false; + }; + if !kind_filter(&gate_ref) { + return false; + } + } if let Some(expected) = expected_gate_ref && r.gate_ref != expected { @@ -566,6 +587,7 @@ async fn select_delivered_gate_route( binding_service: &dyn ConversationBindingService, delivered_gate_routes: &dyn ironclaw_outbound::DeliveredGateRouteStore, expected_gate_ref: Option<&str>, + gate_kind_filter: Option bool>, pre_resolved_binding: Option<&ResolvedBinding>, ambiguity_error: impl Fn() -> ProductWorkflowError, ) -> Option> { @@ -588,6 +610,7 @@ async fn select_delivered_gate_route( binding, delivered_gate_routes, expected_gate_ref, + gate_kind_filter, ) .await { @@ -618,6 +641,10 @@ async fn resolve_via_delivered_approval_route( binding_service, delivered_gate_routes, expected_gate_ref, + // Bare approve must only match approval gates; a lingering auth gate + // recorded in the same conversation bucket must not count toward the + // ambiguity check or be forwarded to the approval service. + Some(is_approval_gate_ref), pre_resolved_binding, || ProductWorkflowError::ApprovalInteractionRejected { kind: ApprovalInteractionRejectionKind::AmbiguousGate, @@ -691,6 +718,10 @@ async fn resolve_via_delivered_auth_route( binding_service, delivered_gate_routes, expected_gate_ref, + // Bare "auth deny" must only match auth gates; a lingering approval + // gate recorded in the same conversation bucket must not count toward + // the ambiguity check or be forwarded to the auth service. + Some(is_auth_gate_ref), pre_resolved_binding, || ProductWorkflowError::AuthInteractionRejected { kind: AuthInteractionRejectionKind::AmbiguousAuth, diff --git a/crates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rs b/crates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rs index d4200d6ddf2..c0f8b70b8ac 100644 --- a/crates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rs +++ b/crates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rs @@ -80,8 +80,8 @@ const CHANNEL: &str = "D123"; const SLACK_SIGNATURE_HEADER: &str = "X-Slack-Signature"; const SLACK_TIMESTAMP_HEADER: &str = "X-Slack-Request-Timestamp"; const SECRET: &str = "topsecret"; -const GATE: &str = "gate:approve-slack"; -const GATE_B: &str = "gate:approve-slack-b"; +const GATE: &str = "gate:approval-00000000-0000-0000-0000-000000000001"; +const GATE_B: &str = "gate:approval-00000000-0000-0000-0000-000000000002"; const AUTH_GATE: &str = "gate:auth-slack"; struct Harness { @@ -788,6 +788,101 @@ async fn bare_approve_with_two_live_routes_fails_closed_ambiguous() { ); } +/// Bare `approve` in the DM with ONE approval gate AND one stale/uncompleted +/// auth gate both delivered to the same DM resolves the approval gate — +/// NOT AmbiguousGate. +/// +/// Scenario: a run first triggered an auth gate (e.g. OAuth not yet completed, +/// still live in the store) and later a second run triggered an approval gate, +/// both delivered to the same DM. The user sends a bare "approve". +/// `list_pending` returns [] (ForeignScopeApprovalService). The workflow falls +/// back to the conversation-fingerprint index and finds TWO records. Before +/// this fix, both records counted toward `live.len()` → `Ambiguous` → error. +/// After this fix, the approval-path gate-kind filter drops the auth record, +/// leaving exactly one approval record → `Single` → resolved successfully. +/// +/// This test would fail on the pre-fix code path: the auth-gate record would +/// inflate `live.len()` to 2 and trigger `AmbiguousGate`. +#[tokio::test] +async fn bare_approve_with_one_approval_and_one_stale_auth_gate_resolves_approval() { + let (harness, inner_approvals) = build_harness_for_delivered_route_tests().await; + + // Submit a turn so the DM conversation binding is created. + let block_response = harness.post_event(DM_BLOCK).await; + assert_eq!(block_response.status(), StatusCode::OK); + harness.drain().await; + let blocked_run_id = harness + .coordinator + .blocked_run_id() + .expect("run must be blocked after DM_BLOCK"); // safety: E2E test assertion. + + let fingerprint = dm_conversation_fingerprint(); + + // Seed the approval-gate route record (the "real" pending gate the user + // wants to resolve). + harness + .route_store + .record_delivered_gate_route(ironclaw_outbound::DeliveredGateRouteRecord { + tenant_id: TenantId::new(TENANT).expect("tenant"), // safety: static test tenant id is valid. + user_id: UserId::new(USER).expect("user"), // safety: static test user id is valid. + gate_ref: GATE.to_string(), // gate:approval-... prefix — is_approval_gate_ref → true + run_id: blocked_run_id, + scope: foreign_run_scope(), + recorded_at: chrono::Utc::now(), + delivered_conversation_fingerprints: vec![fingerprint.clone()], + }) + .await + .expect("approval route record write"); // safety: in-memory store should not fail. + + // Seed a stale/uncompleted auth-gate route record in the SAME conversation. + // This simulates a lingering `gate:auth-*` record that was never completed + // (e.g. the user dismissed the OAuth flow without finishing it). Because + // the 48h TTL has not elapsed it is still "live" and would previously + // contaminate the approval bare-resolve lookup. + harness + .route_store + .record_delivered_gate_route(ironclaw_outbound::DeliveredGateRouteRecord { + tenant_id: TenantId::new(TENANT).expect("tenant"), // safety: static test tenant id is valid. + user_id: UserId::new(USER).expect("user"), // safety: static test user id is valid. + gate_ref: AUTH_GATE.to_string(), // gate:auth-... prefix — is_auth_gate_ref → true + run_id: ironclaw_turns::TurnRunId::new(), + scope: foreign_run_scope(), + recorded_at: chrono::Utc::now(), + delivered_conversation_fingerprints: vec![fingerprint], + }) + .await + .expect("auth route record write"); // safety: in-memory store should not fail. + + // Post a bare "approve". Two records exist in the conversation bucket but + // only the approval-gate record passes the gate-kind filter, so the workflow + // should resolve Single → forward exactly one approval resolve request. + let approve_response = harness.post_event(DM_APPROVE).await; + assert_eq!(approve_response.status(), StatusCode::OK); + harness.drain().await; + + let requests = inner_approvals.requests(); + assert_eq!( + requests.len(), + 1, + "exactly one approval resolve must be forwarded — auth gate must be filtered out; got {} request(s)", + requests.len() + ); + assert_eq!( + requests[0].run_id_hint, + Some(blocked_run_id), + "run_id_hint must come from the approval route record" + ); + assert_eq!( + requests[0].gate_ref.as_str(), + GATE, + "resolved gate_ref must be the approval gate" + ); + assert_eq!( + requests[0].decision, + ApprovalInteractionDecision::ApproveOnce + ); +} + /// Bare `approve` in the DM with no delivered-route record reports a "couldn't /// match" hint and does NOT forward any resolve to the approval service. /// @@ -1830,7 +1925,9 @@ fn dm_message(event_id: &'static str, text: &'static str) -> &'static str { ("Ev-approval", "needs approval") => DM_APPROVAL, ("Ev-block", "needs approval") => DM_BLOCK, ("Ev-approve", "approve") => DM_APPROVE, - ("Ev-approve-explicit", "approve gate:approve-slack") => DM_APPROVE_EXPLICIT_GATE, + ("Ev-approve-explicit", "approve gate:approval-00000000-0000-0000-0000-000000000001") => { + DM_APPROVE_EXPLICIT_GATE + } ("Ev-forged", "hello") => DM_FORGED, ("Ev-identity", "hello") => DM_IDENTITY, ("Ev-auth", "needs auth") => DM_AUTH, @@ -1969,8 +2066,8 @@ const DM_THREAD_AUTH_CANCEL: &str = r#"{ "event":{"type":"message","channel_type":"im","user":"U123","channel":"D123","text":"`auth deny gate:auth-slack`","ts":"1710000001.123457","thread_ts":"1710000001.123456"} }"#; -/// Explicit gate-ref approve in the DM: `approve gate:approve-slack`. -/// The gate ref token after "approve " is `gate:approve-slack` (= GATE). +/// Explicit gate-ref approve in the DM: `approve gate:approval-00000000-0000-0000-0000-000000000001`. +/// The gate ref token after "approve " is GATE (a valid `gate:approval-` prefixed ref). /// Used by the delivered-gate-route test that verifies explicit gate ref resolves /// directly (binding found → no cross-scope rewrite). const DM_APPROVE_EXPLICIT_GATE: &str = r#"{ @@ -1978,5 +2075,5 @@ const DM_APPROVE_EXPLICIT_GATE: &str = r#"{ "team_id":"T-A", "api_app_id":"A-slack", "event_id":"Ev-approve-explicit", - "event":{"type":"message","channel_type":"im","user":"U123","channel":"D123","text":"approve gate:approve-slack","ts":"1710000000.000005"} + "event":{"type":"message","channel_type":"im","user":"U123","channel":"D123","text":"approve gate:approval-00000000-0000-0000-0000-000000000001","ts":"1710000000.000005"} }"#; From e85083f25eff361b4888b3ea205056395946ebc6 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sat, 13 Jun 2026 12:52:24 -0700 Subject: [PATCH 2/4] fix(slack): filter delivered gate routes by raw gate string (auth vs approval) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FIX 1 — change gate_kind_filter predicate from fn(&GateRef)->bool to fn(&str)->bool The old signature forced each candidate route's stored gate string to be re-wrapped into a GateRef before the predicate ran. This had two bugs: • Per-route allocation: every route required a GateRef::new() call just to run a prefix check. • Silent drop: any route whose stored string failed GateRef::new() validation was dropped before the predicate ran, meaning the filter never executed on those routes. The new fn(&str)->bool predicate receives the raw stored gate string directly, eliminating both issues. Updated: - is_approval_gate_ref (approval_interaction/gate_ref.rs) - is_auth_gate_ref (auth_interaction/gate_ref.rs) - load_delivered_routes_for_envelope and select_delivered_gate_route signatures in workflow.rs - All callers in reborn_services.rs, slack_delivery.rs, turn_events.rs updated to pass gate_ref.as_str() instead of &gate_ref. FIX 2 — add symmetric bare-auth-deny e2e test covering the regression bare_auth_deny_with_stale_approval_route_selects_auth_route_not_approval: A bare auth-deny arrives in a conversation that has a stale APPROVAL gate route in the same conversation fingerprint bucket. Asserts the auth-kind filter drops the approval route (Miss), and the stale approval route's run_id is never forwarded to the auth service as a run_id_hint. Mirrors scoped_approval_two_live_routes_same_conversation_rejects_ambiguous in the opposite direction. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/approval_interaction/gate_ref.rs | 10 +- .../src/auth_interaction/gate_ref.rs | 25 ++--- .../src/reborn_services.rs | 4 +- .../ironclaw_product_workflow/src/workflow.rs | 20 ++-- .../tests/product_workflow_contract.rs | 95 +++++++++++++++++++ .../src/projection/turn_events.rs | 2 +- .../src/slack_delivery.rs | 4 +- 7 files changed, 124 insertions(+), 36 deletions(-) diff --git a/crates/ironclaw_product_workflow/src/approval_interaction/gate_ref.rs b/crates/ironclaw_product_workflow/src/approval_interaction/gate_ref.rs index 29a94c5b903..7bd1b6284e1 100644 --- a/crates/ironclaw_product_workflow/src/approval_interaction/gate_ref.rs +++ b/crates/ironclaw_product_workflow/src/approval_interaction/gate_ref.rs @@ -9,8 +9,8 @@ use crate::error::ProductWorkflowError; const APPROVAL_GATE_PREFIX: &str = "gate:approval-"; -pub fn is_approval_gate_ref(gate_ref: &GateRef) -> bool { - gate_ref.as_str().starts_with(APPROVAL_GATE_PREFIX) +pub fn is_approval_gate_ref(gate_ref_str: &str) -> bool { + gate_ref_str.starts_with(APPROVAL_GATE_PREFIX) } pub fn approval_gate_ref(request_id: ApprovalRequestId) -> Result { @@ -62,8 +62,8 @@ mod tests { let generic = GateRef::new("gate:approve-slack").expect("generic gate"); let adjacent = GateRef::new("gate:approvalish-test").expect("adjacent gate"); - assert!(is_approval_gate_ref(&typed)); - assert!(!is_approval_gate_ref(&generic)); - assert!(!is_approval_gate_ref(&adjacent)); + assert!(is_approval_gate_ref(typed.as_str())); + assert!(!is_approval_gate_ref(generic.as_str())); + assert!(!is_approval_gate_ref(adjacent.as_str())); } } diff --git a/crates/ironclaw_product_workflow/src/auth_interaction/gate_ref.rs b/crates/ironclaw_product_workflow/src/auth_interaction/gate_ref.rs index db05f9ecbb3..54f5cf6222e 100644 --- a/crates/ironclaw_product_workflow/src/auth_interaction/gate_ref.rs +++ b/crates/ironclaw_product_workflow/src/auth_interaction/gate_ref.rs @@ -1,14 +1,11 @@ -use ironclaw_turns::GateRef; - const AUTH_GATE_REF: &str = "gate:auth"; const AUTH_GATE_PREFIX: &str = "gate:auth-"; const HOOK_AUTH_GATE_PREFIX: &str = "gate:hook-auth-"; -pub fn is_auth_gate_ref(gate_ref: &GateRef) -> bool { - let value = gate_ref.as_str(); - value == AUTH_GATE_REF - || value.starts_with(AUTH_GATE_PREFIX) - || value.starts_with(HOOK_AUTH_GATE_PREFIX) +pub fn is_auth_gate_ref(gate_ref_str: &str) -> bool { + gate_ref_str == AUTH_GATE_REF + || gate_ref_str.starts_with(AUTH_GATE_PREFIX) + || gate_ref_str.starts_with(HOOK_AUTH_GATE_PREFIX) } #[cfg(test)] @@ -17,14 +14,10 @@ mod tests { #[test] fn is_auth_gate_ref_matches_only_auth_gate_shapes() { - assert!(is_auth_gate_ref(&GateRef::new("gate:auth").unwrap())); - assert!(is_auth_gate_ref(&GateRef::new("gate:auth-oauth").unwrap())); - assert!(is_auth_gate_ref( - &GateRef::new("gate:hook-auth-oauth").unwrap() - )); - assert!(!is_auth_gate_ref( - &GateRef::new("gate:approval-123").unwrap() - )); - assert!(!is_auth_gate_ref(&GateRef::new("gate:other-auth").unwrap())); + assert!(is_auth_gate_ref("gate:auth")); + assert!(is_auth_gate_ref("gate:auth-oauth")); + assert!(is_auth_gate_ref("gate:hook-auth-oauth")); + assert!(!is_auth_gate_ref("gate:approval-123")); + assert!(!is_auth_gate_ref("gate:other-auth")); } } diff --git a/crates/ironclaw_product_workflow/src/reborn_services.rs b/crates/ironclaw_product_workflow/src/reborn_services.rs index a4127a7766f..c37cfdca26d 100644 --- a/crates/ironclaw_product_workflow/src/reborn_services.rs +++ b/crates/ironclaw_product_workflow/src/reborn_services.rs @@ -579,8 +579,8 @@ impl GateResolutionRoute { fn from_gate_shape(gate_ref: &GateRef, resolution: &WebUiGateResolution) -> Self { match ( - is_approval_gate_ref(gate_ref), - is_auth_gate_ref(gate_ref), + is_approval_gate_ref(gate_ref.as_str()), + is_auth_gate_ref(gate_ref.as_str()), matches!(resolution, WebUiGateResolution::CredentialProvided { .. }), ) { (true, _, _) => Self::Approval, diff --git a/crates/ironclaw_product_workflow/src/workflow.rs b/crates/ironclaw_product_workflow/src/workflow.rs index 3345b6c5fe9..3b864f2de8c 100644 --- a/crates/ironclaw_product_workflow/src/workflow.rs +++ b/crates/ironclaw_product_workflow/src/workflow.rs @@ -470,8 +470,11 @@ async fn load_delivered_routes_for_envelope( // considered live. This separates approval routes (`is_approval_gate_ref`) // from auth routes (`is_auth_gate_ref`) so that a lingering auth gate // recorded in the same conversation bucket cannot make a bare "approve" - // look ambiguous, and vice-versa. - gate_kind_filter: Option bool>, + // look ambiguous, and vice-versa. The predicate receives the raw stored + // gate string directly — no `GateRef::new` wrap — so routes whose stored + // string fails validation are not silently dropped before the predicate + // runs. + gate_kind_filter: Option bool>, ) -> DeliveredRouteOutcome { let conversation_ref = match delivered_route_conversation_ref(envelope) { Ok(conversation_ref) => conversation_ref, @@ -532,13 +535,10 @@ async fn load_delivered_routes_for_envelope( // same conversation fingerprint bucket from inflating the live-route // count and triggering a spurious AmbiguousGate error on a bare // "approve" (and vice-versa for a bare "auth deny"). - if let Some(kind_filter) = gate_kind_filter { - let Ok(gate_ref) = GateRef::new(r.gate_ref.clone()) else { - return false; - }; - if !kind_filter(&gate_ref) { - return false; - } + if let Some(kind_filter) = gate_kind_filter + && !kind_filter(&r.gate_ref) + { + return false; } if let Some(expected) = expected_gate_ref && r.gate_ref != expected @@ -587,7 +587,7 @@ async fn select_delivered_gate_route( binding_service: &dyn ConversationBindingService, delivered_gate_routes: &dyn ironclaw_outbound::DeliveredGateRouteStore, expected_gate_ref: Option<&str>, - gate_kind_filter: Option bool>, + gate_kind_filter: Option bool>, pre_resolved_binding: Option<&ResolvedBinding>, ambiguity_error: impl Fn() -> ProductWorkflowError, ) -> Option> { diff --git a/crates/ironclaw_product_workflow/tests/product_workflow_contract.rs b/crates/ironclaw_product_workflow/tests/product_workflow_contract.rs index f9dcd7d0d46..e0b09a3dc1c 100644 --- a/crates/ironclaw_product_workflow/tests/product_workflow_contract.rs +++ b/crates/ironclaw_product_workflow/tests/product_workflow_contract.rs @@ -2050,6 +2050,101 @@ async fn auth_two_live_routes_same_conversation_rejects_ambiguous() { ); } +/// A bare auth-deny arrives in a conversation that has a stale APPROVAL gate +/// route stored under the same conversation fingerprint. The auth-kind filter +/// (`is_auth_gate_ref`) must drop the approval route so it neither: +/// (a) inflates the live-route count and produces a spurious `AmbiguousAuth` error, nor +/// (b) forwards the approval route's `run_id` as a `run_id_hint` to the auth service. +/// +/// This is the symmetric counterpart of `scoped_approval_two_live_routes_same_conversation_rejects_ambiguous` +/// (which verifies the opposite direction: a lingering auth route does not pollute a bare "approve"). +/// +/// Regression test for the `fn(&GateRef) -> bool` filter shape: the old code +/// called `GateRef::new(r.gate_ref.clone())` before the predicate, which would +/// silently drop any route whose stored string failed `GateRef` validation — +/// meaning the filter never even ran on invalid-but-stale routes. The new +/// `fn(&str) -> bool` shape tests the raw stored string directly, eliminating +/// both the per-route allocation and the silent-drop. +#[tokio::test] +async fn bare_auth_deny_with_stale_approval_route_selects_auth_route_not_approval() { + let route_store: Arc = + Arc::new(ironclaw_outbound::InMemoryDeliveredGateRouteStore::default()); + // Store a live APPROVAL-prefixed route in the same conversation bucket. + let stale_approval_gate = + approval_gate_ref(ApprovalRequestId::new()).expect("approval gate ref"); + let (stale_run_id, _stale_scope) = record_conversation_route_for_gate_ref( + route_store.as_ref(), + stale_approval_gate.as_str(), + Utc::now(), + ) + .await; + + // The auth deny uses a different, auth-prefixed gate_ref — the one that + // was actually delivered with the auth prompt. + let auth_gate_ref = GateRef::new("gate:auth-deny-with-stale-approval").expect("auth gate ref"); + let auth_service = Arc::new(MissingAuthThenRecordingAuthService::default()); + let workflow = DefaultProductWorkflow::new( + Arc::new(FakeInboundTurnService::new()), + Arc::new(FakeIdempotencyLedger::new()), + Arc::new(FakeConversationBindingService::new()), + ) + .with_auth_interaction_service(auth_service.clone()) + .with_delivered_gate_routes(route_store); + + // The auth deny must not succeed (the auth route was never stored), but + // the key assertion is that the stale approval route is not forwarded to + // the auth service as a run_id_hint. + let err = workflow + .accept_inbound(auth_thread_reply_envelope( + "bare-auth-deny-stale-approval", + auth_gate_ref.as_str(), + )) + .await + .expect_err("auth gate not stored — must fall through to MissingAuth rejection"); + + // MissingAuth (404) — the auth route was never stored, so after the kind + // filter drops the stale approval route the lookup is a Miss and the + // workflow falls back to the normal auth-service path with no run_id_hint. + assert!( + matches!( + err, + ProductAdapterError::WorkflowRejected { + kind: ProductWorkflowRejectionKind::ScopeNotFound, + status_code: 404, + .. + } + ), + "expected ScopeNotFound/404 (MissingAuth fallthrough), got: {err:?}" + ); + + // The auth service receives exactly one call: + // • The initial direct-binding attempt, which returns `MissingAuth`. + // After that, the delivered-route fallback runs: the kind filter drops + // the stale approval route (wrong prefix), leaving a Miss. A Miss means + // the fallback returns `None`, so the workflow re-surfaces the original + // `MissingAuth` error — no second service call is made. + let resolutions = auth_service.resolutions(); + assert_eq!( + resolutions.len(), + 1, + "auth service must receive exactly one call (direct attempt; fallback Miss suppresses second), got: {resolutions:?}" + ); + // The single call must not carry the stale approval route's run_id. + assert_ne!( + resolutions[0].run_id_hint, + Some(stale_run_id), + "stale approval route's run_id must never reach the auth service" + ); + assert_eq!( + resolutions[0].run_id_hint, None, + "no auth delivered route matched — run_id_hint must be None" + ); + assert_eq!( + resolutions[0].gate_ref, auth_gate_ref, + "auth service must receive the auth gate_ref, not the stale approval gate_ref" + ); +} + /// Explicit auth with a gate_ref that matches no stored delivered route must /// fall through to the interaction service with the original gate_ref. #[tokio::test] diff --git a/crates/ironclaw_reborn_composition/src/projection/turn_events.rs b/crates/ironclaw_reborn_composition/src/projection/turn_events.rs index 6b4b677b34a..7cae1b508da 100644 --- a/crates/ironclaw_reborn_composition/src/projection/turn_events.rs +++ b/crates/ironclaw_reborn_composition/src/projection/turn_events.rs @@ -448,7 +448,7 @@ async fn approval_gate_prompt( event, gate_ref_string, "Approval required", - is_approval_gate_ref(gate_ref), + is_approval_gate_ref(gate_ref.as_str()), context, ) } diff --git a/crates/ironclaw_reborn_composition/src/slack_delivery.rs b/crates/ironclaw_reborn_composition/src/slack_delivery.rs index 617829674bc..e1d237fbda5 100644 --- a/crates/ironclaw_reborn_composition/src/slack_delivery.rs +++ b/crates/ironclaw_reborn_composition/src/slack_delivery.rs @@ -924,7 +924,7 @@ fn slack_approval_gate_prompt_view(run_id: TurnRunId, gate_ref: &GateRef) -> Gat "A step in the workflow requires your approval to resume.\nReply `approve` or `deny` in this thread, or use `approve {}` from anywhere.", gate_ref.as_str() ), - allow_always: is_approval_gate_ref(gate_ref), + allow_always: is_approval_gate_ref(gate_ref.as_str()), approval_context: None, } } @@ -1887,7 +1887,7 @@ async fn triggered_notification_for_state( gate_ref: gate_ref_str.clone(), headline: "Approval needed".to_string(), body: format!("Reply `approve {gate_ref_str}` to continue."), - allow_always: is_approval_gate_ref(gate_ref), + allow_always: is_approval_gate_ref(gate_ref.as_str()), approval_context: None, }), gate_ref_for_routing: Some(gate_ref_str), From 4aa9214bd2490655f5a1baafaaadb767e5e256eb Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 14 Jun 2026 00:49:16 -0700 Subject: [PATCH 3/4] fix(slack): skip gate-kind filter for exact-named gate refs The delivered-route gate-kind filter ran before the expected_gate_ref exact-match check, so an explicitly-named generic/legacy gate (whose stored string is not a typed approval/auth prefix) was dropped before the exact match could forward it, falling through to BindingRequired/MissingAuth. For an exact-ref lookup the kind filter can only total-drop, never disambiguate; it is only meaningful for bare lookups. Apply gate_kind_filter only when expected_gate_ref is None. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ironclaw_product_workflow/src/workflow.rs | 56 +++++----- .../tests/product_workflow_contract.rs | 102 ++++++++++++++++++ 2 files changed, 133 insertions(+), 25 deletions(-) diff --git a/crates/ironclaw_product_workflow/src/workflow.rs b/crates/ironclaw_product_workflow/src/workflow.rs index 3b864f2de8c..f8b8a663129 100644 --- a/crates/ironclaw_product_workflow/src/workflow.rs +++ b/crates/ironclaw_product_workflow/src/workflow.rs @@ -466,14 +466,18 @@ async fn load_delivered_routes_for_envelope( binding: &ResolvedBinding, delivered_gate_routes: &dyn ironclaw_outbound::DeliveredGateRouteStore, expected_gate_ref: Option<&str>, - // When `Some`, only routes whose `gate_ref` satisfies the predicate are - // considered live. This separates approval routes (`is_approval_gate_ref`) - // from auth routes (`is_auth_gate_ref`) so that a lingering auth gate - // recorded in the same conversation bucket cannot make a bare "approve" - // look ambiguous, and vice-versa. The predicate receives the raw stored - // gate string directly — no `GateRef::new` wrap — so routes whose stored - // string fails validation are not silently dropped before the predicate - // runs. + // When `Some` AND `expected_gate_ref` is `None` (bare lookup), only routes + // whose `gate_ref` satisfies the predicate are considered live. This + // separates approval routes (`is_approval_gate_ref`) from auth routes + // (`is_auth_gate_ref`) so that a lingering auth gate recorded in the same + // conversation bucket cannot make a bare "approve" look ambiguous, and + // vice-versa. For exact-ref lookups (`expected_gate_ref == Some(_)`) this + // predicate is NOT applied: all routes surviving the exact-match share the + // identical gate_ref string, so the kind filter can only total-drop a + // validly named generic/legacy gate — it can never disambiguate. The + // predicate receives the raw stored gate string directly — no `GateRef::new` + // wrap — so routes whose stored string fails validation are not silently + // dropped before the predicate runs. gate_kind_filter: Option bool>, ) -> DeliveredRouteOutcome { let conversation_ref = match delivered_route_conversation_ref(envelope) { @@ -505,8 +509,9 @@ async fn load_delivered_routes_for_envelope( return DeliveredRouteOutcome::Miss; } }; - // Filter: non-expired, tenant+actor match, gate-kind match (when a kind - // filter is supplied), and (if an explicit ref is supplied) gate_ref match. + // Filter: non-expired, tenant+actor match, then either exact-ref match + // (when the caller names a specific gate) or gate-kind filter (for bare + // lookups only — see gate_kind_filter parameter comment above). let live: Vec = all_routes .into_iter() .filter(|r| { @@ -529,21 +534,22 @@ async fn load_delivered_routes_for_envelope( if r.tenant_id != binding.tenant_id || r.user_id != binding.actor_user_id { return false; } - // Gate-kind filter: when the caller knows it is resolving an - // approval (or auth) interaction, drop routes that belong to the - // other kind. This prevents a lingering auth gate recorded in the - // same conversation fingerprint bucket from inflating the live-route - // count and triggering a spurious AmbiguousGate error on a bare - // "approve" (and vice-versa for a bare "auth deny"). - if let Some(kind_filter) = gate_kind_filter - && !kind_filter(&r.gate_ref) - { - return false; - } - if let Some(expected) = expected_gate_ref - && r.gate_ref != expected - { - return false; + if let Some(expected) = expected_gate_ref { + // Exact ref named: the named ref is authoritative and the downstream + // interaction service decides resolvability. Do NOT apply the gate-kind + // filter here — for an exact-ref lookup every surviving route shares the + // same gate_ref string, so the kind filter can only total-drop a validly + // named generic/legacy gate, never disambiguate. + if r.gate_ref != expected { + return false; + } + } else if let Some(kind_filter) = gate_kind_filter { + // Bare lookup: use the kind filter so a lingering gate of the other kind + // (e.g. an auth gate when resolving a bare "approve") cannot inflate the + // live-route count and trigger a spurious ambiguity. + if !kind_filter(&r.gate_ref) { + return false; + } } true }) diff --git a/crates/ironclaw_product_workflow/tests/product_workflow_contract.rs b/crates/ironclaw_product_workflow/tests/product_workflow_contract.rs index e0b09a3dc1c..552b2ba4f5a 100644 --- a/crates/ironclaw_product_workflow/tests/product_workflow_contract.rs +++ b/crates/ironclaw_product_workflow/tests/product_workflow_contract.rs @@ -2145,6 +2145,108 @@ async fn bare_auth_deny_with_stale_approval_route_selects_auth_route_not_approva ); } +/// An exact-named generic/legacy gate ref (one whose stored string does NOT +/// start with `"gate:auth"` / `"gate:hook-auth-"`) must be FORWARDED to the +/// auth interaction service on the delivered-route fallback path, not silently +/// dropped by the gate-kind filter. +/// +/// Regression test for the ordering bug: the old code applied +/// `gate_kind_filter` before the `expected_gate_ref` exact-match check, so an +/// explicitly-named generic gate (e.g. `"gate:approve-slack"` for the auth +/// side, any string without the auth prefix) was dropped by `is_auth_gate_ref` +/// returning `false` before the exact-match could select it. For an +/// exact-ref lookup the kind filter can never disambiguate (all surviving +/// routes share the same gate_ref string); it can only total-drop a validly +/// named route — which is exactly the wrong outcome. +/// +/// After the fix: `gate_kind_filter` is skipped entirely when +/// `expected_gate_ref` is `Some(_)`. The exact match is authoritative, and +/// the route is selected and forwarded to the interaction service. +/// +/// We exercise the auth-resolution path here because it has a convenient +/// `MissingAuth` fallback that triggers `resolve_via_delivered_auth_route` +/// even when the initial binding lookup succeeds: the first auth-service call +/// returns `MissingAuth`, then the workflow enters the delivered-route fallback +/// with `expected_gate_ref = Some(payload.auth_request_ref)`. The approval +/// side's equivalent fallback only triggers on `BindingRequired`, which +/// requires a more complex binding-service setup. +#[tokio::test] +async fn exact_named_generic_approval_gate_is_forwarded_not_dropped_by_kind_filter() { + // Use a gate_ref that does NOT match any auth prefix ("gate:auth", + // "gate:auth-", "gate:hook-auth-"), so is_auth_gate_ref returns false — + // the old code would have dropped this route; the fixed code must not. + let generic_gate_ref = "gate:approve-slack"; + let route_store: Arc = + Arc::new(ironclaw_outbound::InMemoryDeliveredGateRouteStore::default()); + let (run_id, route_scope) = + record_conversation_route_for_gate_ref(route_store.as_ref(), generic_gate_ref, Utc::now()) + .await; + // MissingAuthThenRecordingAuthService: on the first call (run_id_hint=None) + // returns MissingAuth, which triggers the delivered-route fallback. On + // the second call (run_id_hint=Some) it returns Canceled (Deny path), which + // maps to Accepted. + let auth_service = Arc::new(MissingAuthThenRecordingAuthService::default()); + let workflow = DefaultProductWorkflow::new( + Arc::new(FakeInboundTurnService::new()), + Arc::new(FakeIdempotencyLedger::new()), + Arc::new(FakeConversationBindingService::new()), + ) + .with_auth_interaction_service(auth_service.clone()) + .with_delivered_gate_routes(route_store); + + // Drive the AuthResolution path: FakeConversationBindingService returns a + // binding, so the direct auth-service call fires first with run_id_hint=None + // → MissingAuth → triggers resolve_via_delivered_auth_route with + // expected_gate_ref = Some("gate:approve-slack"). The kind filter must NOT + // drop the stored route (even though is_auth_gate_ref returns false for it). + let ack = workflow + .accept_inbound(auth_thread_reply_envelope( + "generic-gate-forwarded-not-dropped", + generic_gate_ref, + )) + .await + .expect("generic gate named explicitly must be forwarded via delivered-route, not dropped by kind filter"); + + // The route must be selected and the auth interaction service called a + // second time with the run_id_hint from the stored delivered route. + assert!( + matches!(ack, ProductInboundAck::Accepted { submitted_run_id, .. } if submitted_run_id == run_id), + "expected Accepted with run_id from the generic gate route, got: {ack:?}" + ); + let resolutions = auth_service.resolutions(); + assert_eq!( + resolutions.len(), + 2, + "auth service must receive two calls: initial MissingAuth + delivered-route forwarding, got: {resolutions:?}" + ); + // First call: direct path, no run_id_hint — this returns MissingAuth. + assert_eq!( + resolutions[0].run_id_hint, None, + "first call must be the direct path with no run_id_hint" + ); + assert_eq!( + resolutions[0].gate_ref.as_str(), + generic_gate_ref, + "first call must carry the generic gate_ref" + ); + // Second call: delivered-route path — must carry the run_id_hint and + // gate_ref from the stored route. + assert_eq!( + resolutions[1].run_id_hint, + Some(run_id), + "second call must carry the run_id_hint from the stored delivered route" + ); + assert_eq!( + resolutions[1].gate_ref.as_str(), + generic_gate_ref, + "second call must carry the generic gate_ref string unchanged" + ); + assert_eq!( + resolutions[1].scope.thread_id, route_scope.thread_id, + "second call must carry the scope from the stored delivered route" + ); +} + /// Explicit auth with a gate_ref that matches no stored delivered route must /// fall through to the interaction service with the original gate_ref. #[tokio::test] From afd32a4a004e90a8f369a9aceb90574901e97d1a Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 14 Jun 2026 01:06:09 -0700 Subject: [PATCH 4/4] fix(slack): make delivered-route kind filter mandatory + cover invalid stored gate refs - gate_kind_filter is now a required fn(&str)->bool (both callers always supplied one); removes the unreachable None branch. - Add caller-level tests for stored gate strings that pass the kind prefix predicate but fail GateRef::new, exercising the InvalidGateRef branch that the raw-string filter newly makes reachable. - Correct a test comment that overclaimed invalid-stored-route coverage. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ironclaw_product_workflow/src/workflow.rs | 35 ++- .../tests/product_workflow_contract.rs | 216 +++++++++++++++++- 2 files changed, 226 insertions(+), 25 deletions(-) diff --git a/crates/ironclaw_product_workflow/src/workflow.rs b/crates/ironclaw_product_workflow/src/workflow.rs index f8b8a663129..d055f595821 100644 --- a/crates/ironclaw_product_workflow/src/workflow.rs +++ b/crates/ironclaw_product_workflow/src/workflow.rs @@ -466,19 +466,18 @@ async fn load_delivered_routes_for_envelope( binding: &ResolvedBinding, delivered_gate_routes: &dyn ironclaw_outbound::DeliveredGateRouteStore, expected_gate_ref: Option<&str>, - // When `Some` AND `expected_gate_ref` is `None` (bare lookup), only routes - // whose `gate_ref` satisfies the predicate are considered live. This + // Applied only on bare lookups (`expected_gate_ref == None`): only routes + // whose `gate_ref` satisfies this predicate are considered live. This // separates approval routes (`is_approval_gate_ref`) from auth routes - // (`is_auth_gate_ref`) so that a lingering auth gate recorded in the same - // conversation bucket cannot make a bare "approve" look ambiguous, and - // vice-versa. For exact-ref lookups (`expected_gate_ref == Some(_)`) this - // predicate is NOT applied: all routes surviving the exact-match share the - // identical gate_ref string, so the kind filter can only total-drop a - // validly named generic/legacy gate — it can never disambiguate. The - // predicate receives the raw stored gate string directly — no `GateRef::new` - // wrap — so routes whose stored string fails validation are not silently - // dropped before the predicate runs. - gate_kind_filter: Option bool>, + // (`is_auth_gate_ref`) so that a lingering gate of the other kind recorded + // in the same conversation bucket cannot make a bare lookup ambiguous. + // For exact-ref lookups (`expected_gate_ref == Some(_)`) this predicate is + // NOT applied: the exact match is authoritative, and the kind filter can + // only total-drop a validly named generic/legacy gate — it can never + // disambiguate. The predicate receives the raw stored gate string directly + // — no `GateRef::new` wrap — so routes whose stored string fails validation + // are not silently dropped before the predicate runs. + gate_kind_filter: fn(&str) -> bool, ) -> DeliveredRouteOutcome { let conversation_ref = match delivered_route_conversation_ref(envelope) { Ok(conversation_ref) => conversation_ref, @@ -543,13 +542,11 @@ async fn load_delivered_routes_for_envelope( if r.gate_ref != expected { return false; } - } else if let Some(kind_filter) = gate_kind_filter { + } else if !gate_kind_filter(&r.gate_ref) { // Bare lookup: use the kind filter so a lingering gate of the other kind // (e.g. an auth gate when resolving a bare "approve") cannot inflate the // live-route count and trigger a spurious ambiguity. - if !kind_filter(&r.gate_ref) { - return false; - } + return false; } true }) @@ -593,7 +590,7 @@ async fn select_delivered_gate_route( binding_service: &dyn ConversationBindingService, delivered_gate_routes: &dyn ironclaw_outbound::DeliveredGateRouteStore, expected_gate_ref: Option<&str>, - gate_kind_filter: Option bool>, + gate_kind_filter: fn(&str) -> bool, pre_resolved_binding: Option<&ResolvedBinding>, ambiguity_error: impl Fn() -> ProductWorkflowError, ) -> Option> { @@ -650,7 +647,7 @@ async fn resolve_via_delivered_approval_route( // Bare approve must only match approval gates; a lingering auth gate // recorded in the same conversation bucket must not count toward the // ambiguity check or be forwarded to the approval service. - Some(is_approval_gate_ref), + is_approval_gate_ref, pre_resolved_binding, || ProductWorkflowError::ApprovalInteractionRejected { kind: ApprovalInteractionRejectionKind::AmbiguousGate, @@ -727,7 +724,7 @@ async fn resolve_via_delivered_auth_route( // Bare "auth deny" must only match auth gates; a lingering approval // gate recorded in the same conversation bucket must not count toward // the ambiguity check or be forwarded to the auth service. - Some(is_auth_gate_ref), + is_auth_gate_ref, pre_resolved_binding, || ProductWorkflowError::AuthInteractionRejected { kind: AuthInteractionRejectionKind::AmbiguousAuth, diff --git a/crates/ironclaw_product_workflow/tests/product_workflow_contract.rs b/crates/ironclaw_product_workflow/tests/product_workflow_contract.rs index 552b2ba4f5a..8add1eab80e 100644 --- a/crates/ironclaw_product_workflow/tests/product_workflow_contract.rs +++ b/crates/ironclaw_product_workflow/tests/product_workflow_contract.rs @@ -566,6 +566,49 @@ impl ironclaw_outbound::DeliveredGateRouteStore for FailingRouteStore { } } +/// A binding service that returns `BindingRequired` for the first +/// `fail_count` calls and then returns the default `FakeConversationBindingService` +/// binding. Used to drive the auth BindingRequired delivered-route fallback +/// while allowing `delivered_route_base_binding` (a subsequent call) to succeed. +struct BindingRequiredThenSucceedingService { + fail_count: usize, + call_count: AtomicUsize, + inner: FakeConversationBindingService, +} + +impl BindingRequiredThenSucceedingService { + fn new(fail_count: usize) -> Self { + Self { + fail_count, + call_count: AtomicUsize::new(0), + inner: FakeConversationBindingService::new(), + } + } +} + +#[async_trait] +impl ConversationBindingService for BindingRequiredThenSucceedingService { + async fn resolve_binding( + &self, + request: ResolveBindingRequest, + ) -> Result { + self.lookup_binding(request).await + } + + async fn lookup_binding( + &self, + request: ResolveBindingRequest, + ) -> Result { + let n = self.call_count.fetch_add(1, Ordering::SeqCst); + if n < self.fail_count { + return Err(ProductWorkflowError::BindingRequired { + reason: format!("injected failure #{n}"), + }); + } + self.inner.lookup_binding(request).await + } +} + #[test] fn action_fingerprint_retains_typed_identifiers() { let adapter_id = ProductAdapterId::new("test_adapter").expect("valid"); @@ -2059,12 +2102,10 @@ async fn auth_two_live_routes_same_conversation_rejects_ambiguous() { /// This is the symmetric counterpart of `scoped_approval_two_live_routes_same_conversation_rejects_ambiguous` /// (which verifies the opposite direction: a lingering auth route does not pollute a bare "approve"). /// -/// Regression test for the `fn(&GateRef) -> bool` filter shape: the old code -/// called `GateRef::new(r.gate_ref.clone())` before the predicate, which would -/// silently drop any route whose stored string failed `GateRef` validation — -/// meaning the filter never even ran on invalid-but-stale routes. The new -/// `fn(&str) -> bool` shape tests the raw stored string directly, eliminating -/// both the per-route allocation and the silent-drop. +/// Both stored routes carry valid gate ref strings. The assertion is that the +/// auth-kind filter (`is_auth_gate_ref`) drops the stale approval route by +/// prefix — not by GateRef validation — leaving only the live auth route to be +/// forwarded to the auth interaction service. #[tokio::test] async fn bare_auth_deny_with_stale_approval_route_selects_auth_route_not_approval() { let route_store: Arc = @@ -2291,6 +2332,169 @@ async fn explicit_auth_gate_ref_mismatch_leaves_original_rejection() { ); } +/// A stored delivered-route whose raw gate_ref string passes the approval-kind +/// prefix predicate (`is_approval_gate_ref`: `starts_with("gate:approval-")`) +/// but is too long to pass `GateRef::new` (> 256 bytes) must be SELECTED by +/// the kind filter — not silently dropped — and then surface an +/// `InvalidGateRef` rejection rather than a silent Miss or BindingRequired. +/// +/// This verifies the `InvalidGateRef` branch in +/// `resolve_via_delivered_approval_route` that was previously unreachable +/// because the old `fn(&GateRef) -> bool` filter pre-validated the stored +/// string with `GateRef::new`, silently dropping any route that failed +/// construction before the predicate could run. The new `fn(&str) -> bool` +/// predicate receives the raw stored string directly, so an +/// oversized-but-prefixed string is selected and surfaces the error. +/// +/// The invalid string used here is `"gate:approval-" + "a" * 243` = 257 bytes: +/// - passes `is_approval_gate_ref` (starts with `"gate:approval-"`) +/// - passes `validate_token_string` used by adapter payloads (max 512 bytes) +/// - fails `GateRef::new` (`validate_ref` cap is 256 bytes) +#[tokio::test] +async fn bare_approve_with_invalid_stored_approval_route_rejects_invalid_gate_ref() { + // "gate:approval-" = 14 bytes; 14 + 243 = 257 bytes → fails GateRef::new. + let invalid_gate_ref_str = format!("gate:approval-{}", "a".repeat(243)); + assert_eq!(invalid_gate_ref_str.len(), 257); + // Confirm predicate accepts but GateRef::new rejects. + assert!( + ironclaw_product_workflow::is_approval_gate_ref(&invalid_gate_ref_str), + "test string must pass is_approval_gate_ref" + ); + assert!( + ironclaw_turns::GateRef::new(invalid_gate_ref_str.as_str()).is_err(), + "test string must fail GateRef::new" + ); + + let route_store: Arc = + Arc::new(ironclaw_outbound::InMemoryDeliveredGateRouteStore::default()); + record_conversation_route_for_gate_ref(route_store.as_ref(), &invalid_gate_ref_str, Utc::now()) + .await; + + // with_pending(Vec::new()) → list_pending returns [] → MissingGate + // fallback fires → resolve_via_delivered_approval_route(None, …) → + // kind filter runs → route is selected → GateRef::new fails → InvalidGateRef. + let approval_service = Arc::new(RecordingApprovalInteractionService::with_pending(Vec::new())); + let workflow = DefaultProductWorkflow::new( + Arc::new(FakeInboundTurnService::new()), + Arc::new(FakeIdempotencyLedger::new()), + Arc::new(FakeConversationBindingService::new()), + ) + .with_approval_interaction_service(approval_service.clone()) + .with_delivered_gate_routes(route_store); + + let err = workflow + .accept_inbound(scoped_approval_thread_reply_envelope( + "bare-approve-invalid-stored-gate-ref", + )) + .await + .expect_err("invalid stored gate_ref must surface InvalidGateRef, not a silent Miss"); + + assert!( + matches!( + err, + ProductAdapterError::WorkflowRejected { + kind: ProductWorkflowRejectionKind::InvalidRequest, + status_code: 400, + retryable: false, + .. + } + ), + "expected InvalidGateRef → InvalidRequest/400, got: {err:?}" + ); + // The approval service must NOT be called — the error comes from + // GateRef reconstruction in the delivered-route path, before the + // interaction service is reached. + assert!( + approval_service.resolutions().is_empty(), + "approval service must not be called when GateRef reconstruction fails" + ); +} + +/// A stored delivered-route whose raw gate_ref string passes the auth-kind +/// prefix predicate (`is_auth_gate_ref`: `starts_with("gate:auth-")`) but is +/// too long to pass `GateRef::new` (> 256 bytes) must be SELECTED by the +/// exact-ref match in the BindingRequired delivered-route fallback — and then +/// surface an `InvalidGateRef` rejection rather than a silent Miss. +/// +/// This verifies the `InvalidGateRef` branch in +/// `resolve_via_delivered_auth_route`. The BindingRequired fallback path is +/// used because it fires BEFORE `dispatch_auth_resolution` calls +/// `GateRef::new` on the payload string (line ~1135), allowing the oversized +/// invalid gate_ref to reach the delivered-route selection code. The +/// BindingRequired path calls `resolve_via_delivered_auth_route` with +/// `expected_gate_ref = Some(payload.auth_request_ref)`, so the oversized +/// stored string is selected via exact-ref match; the kind filter is not used. +/// +/// The invalid string used here is `"gate:auth-" + "a" * 247` = 257 bytes: +/// - passes `is_auth_gate_ref` (starts with `"gate:auth-"`) +/// - passes `validate_token_string` used by adapter payloads (max 512 bytes) +/// - fails `GateRef::new` (`validate_ref` cap is 256 bytes) +#[tokio::test] +async fn bare_auth_deny_with_invalid_stored_auth_route_rejects_invalid_gate_ref() { + // "gate:auth-" = 10 bytes; 10 + 247 = 257 bytes → fails GateRef::new. + let invalid_gate_ref_str = format!("gate:auth-{}", "a".repeat(247)); + assert_eq!(invalid_gate_ref_str.len(), 257); + // Confirm predicate accepts but GateRef::new rejects. + assert!( + ironclaw_product_workflow::is_auth_gate_ref(&invalid_gate_ref_str), + "test string must pass is_auth_gate_ref" + ); + assert!( + ironclaw_turns::GateRef::new(invalid_gate_ref_str.as_str()).is_err(), + "test string must fail GateRef::new" + ); + + let route_store: Arc = + Arc::new(ironclaw_outbound::InMemoryDeliveredGateRouteStore::default()); + record_conversation_route_for_gate_ref(route_store.as_ref(), &invalid_gate_ref_str, Utc::now()) + .await; + + // BindingRequiredThenSucceedingService(fail_count=2): the first two + // lookup_binding calls (topic-specific + base fallback) both return + // BindingRequired, so lookup_interaction_binding returns BindingRequired. + // The third call (delivered_route_base_binding inside the fallback) succeeds, + // so the delivered-route lookup can resolve actor identity. + // + // BindingRequired fallback → resolve_via_delivered_auth_route with + // expected_gate_ref=Some(invalid_gate_ref_str) → exact-ref match selects + // the stored route → GateRef::new on the stored gate_ref fails → InvalidGateRef. + let auth_service = Arc::new(MissingAuthThenRecordingAuthService::default()); + let workflow = DefaultProductWorkflow::new( + Arc::new(FakeInboundTurnService::new()), + Arc::new(FakeIdempotencyLedger::new()), + Arc::new(BindingRequiredThenSucceedingService::new(2)), + ) + .with_auth_interaction_service(auth_service.clone()) + .with_delivered_gate_routes(route_store); + + let err = workflow + .accept_inbound(auth_thread_reply_envelope( + "bare-auth-invalid-stored-gate-ref", + &invalid_gate_ref_str, + )) + .await + .expect_err("invalid stored gate_ref must surface InvalidGateRef, not BindingRequired"); + + assert!( + matches!( + err, + ProductAdapterError::WorkflowRejected { + kind: ProductWorkflowRejectionKind::InvalidRequest, + status_code: 400, + retryable: false, + .. + } + ), + "expected InvalidGateRef → InvalidRequest/400, got: {err:?}" + ); + // The BindingRequired fallback fires BEFORE the auth interaction service + // is consulted — service must not be called at all. + assert!( + auth_service.resolutions().is_empty(), + "auth service must not be called when GateRef reconstruction fails in the BindingRequired fallback" + ); +} + #[tokio::test] async fn approval_resolution_without_interaction_service_returns_retryable_unavailable() { let inbound = Arc::new(FakeInboundTurnService::new());