From 23cdf4a32afb4298b351832c204a9663a0cf47ba Mon Sep 17 00:00:00 2001 From: Mega Mind <146339422+M3gA-Mind@users.noreply.github.com> Date: Tue, 30 Jun 2026 17:50:43 +0530 Subject: [PATCH] fix(approval): classify benign 'no pending approval' race; keep genuine loss visible (#4183) --- src/core/observability.rs | 82 ++++++++++++++++++++++++++ src/openhuman/approval/gate.rs | 103 +++++++++++++++++++++++++++++++++ src/openhuman/approval/rpc.rs | 42 +++++++++++--- 3 files changed, 219 insertions(+), 8 deletions(-) diff --git a/src/core/observability.rs b/src/core/observability.rs index 28d9f1420..2c023034f 100644 --- a/src/core/observability.rs +++ b/src/core/observability.rs @@ -294,6 +294,24 @@ pub enum ExpectedErrorKind { /// couldn't parse, and the FE *does* page for it, F8). See /// [`crate::openhuman::inference::provider::backend_error_code_skips_sentry`]. BackendErrorCodeOwned, + /// `approval_decide` (`src/openhuman/approval/rpc.rs`) resolved a request_id + /// whose pending row was **already decided, lazily expired, or superseded** + /// — `store::decide` updated 0 rows because `decided_at` was already set, + /// and `store::get_decision` confirms a persisted decision exists. The + /// inline-approvals design spec + /// (`docs/superpowers/specs/2026-05-23-telegram-inline-approvals-design.md`) + /// classifies "no pending approval found" as a **benign** outcome: the + /// frontend `ApprovalRequestCard.decide` and the Telegram callback both call + /// `approval_decide` without server-confirmed dedupe, so double-taps, two + /// operators racing, and expiry-while-live all land here harmlessly. + /// + /// Drops the benign half of Sentry TAURI-RUST-5EH (~1,995 events / 8 users + /// on `openhuman@0.57.53`). Anchored to the benign `"no pending approval + /// found"` wording emitted ONLY when `get_decision` confirms the row was + /// resolved; a genuine **never-registered** id raises the distinct + /// `"no pending approval ever registered"` string (no anchor) so a real + /// lost-registration defect still reaches Sentry. + ApprovalNoPendingRace, /// A remote MCP server answered the connect handshake with HTTP 401 — it /// needs OAuth sign-in, not a code fix. `McpHttpClient::read_response` /// (`src/openhuman/mcp_client/client.rs`) raises the typed @@ -353,6 +371,17 @@ pub fn expected_error_kind(message: &str) -> Option { if lower.contains("codex cli auth") || lower.contains(".codex/auth.json") { return Some(ExpectedErrorKind::CodexCliAuthUnavailable); } + // TAURI-RUST-5EH — `approval_decide` resolved a request_id whose row was + // already decided / lazily expired / superseded (benign race per the + // inline-approvals design spec). `rpc::approval_decide` emits this exact + // `"no pending approval found"` wording ONLY after `store::get_decision` + // confirms a persisted decision exists; the genuine never-registered case + // raises `"no pending approval ever registered"` (matched by neither this + // arm nor any below), so a real lost-registration defect still reaches + // Sentry. See `ExpectedErrorKind::ApprovalNoPendingRace`. + if lower.contains("no pending approval found") { + return Some(ExpectedErrorKind::ApprovalNoPendingRace); + } // TAURI-RUST-CGP — a remote MCP server answered the connect handshake with // HTTP 401 (`McpUnauthorizedError`). `connections::connect` already stores a // `needs_auth` flag so the UI prompts for OAuth sign-in (#3733 / #3719), but @@ -2084,6 +2113,22 @@ fn report_expected_message(kind: ExpectedErrorKind, message: &str, domain: &str, "[observability] {domain}.{operation} skipped backend-owned errorCode={code} error: {message}" ); } + ExpectedErrorKind::ApprovalNoPendingRace => { + // Benign approval race (TAURI-RUST-5EH): the request_id was already + // decided / lazily expired / superseded — `store::get_decision` + // confirmed a persisted decision exists, so this is a double-tap, + // two-operator, or expiry-while-live race classified benign by the + // inline-approvals design spec, not a lost registration. Demote at + // `warn!` so a sustained spike still shows in operator dashboards + // without paging. The genuine never-registered case takes the + // capture path (distinct wording) and stays a Sentry signal. + tracing::warn!( + domain = domain, + operation = operation, + kind = "approval_no_pending_race", + "[observability] {domain}.{operation} skipped benign already-decided/expired approval race: {message}" + ); + } } } @@ -2897,6 +2942,43 @@ mod tests { assert_eq!(format!("{wrapped:#}"), "outer ctx: inner cause"); } + /// Sentry TAURI-RUST-5EH: the benign already-decided/expired approval race + /// (`rpc::approval_decide` after `get_decision` confirms a persisted + /// decision) must classify as `ApprovalNoPendingRace` so the ~1,995-event + /// flood is demoted — while a genuine never-registered id (distinct + /// "ever registered" wording) must stay reportable (`None`). + #[test] + fn classifies_benign_approval_race_but_keeps_genuine_loss() { + assert_eq!( + expected_error_kind( + "rpc.invoke_method failed: no pending approval found for request_id \ + '4f1c…' (already decided or expired)" + ), + Some(ExpectedErrorKind::ApprovalNoPendingRace), + "benign already-decided/expired race must be demoted" + ); + assert_eq!( + expected_error_kind( + "rpc.invoke_method failed: no pending approval ever registered \ + for request_id '4f1c…'" + ), + None, + "a genuine lost registration must remain a Sentry signal" + ); + } + + /// Exercise the full demotion path (classifier -> report arm) for a benign + /// approval race: it must take the expected branch and not panic. + #[test] + fn report_error_or_expected_demotes_benign_approval_race() { + report_error_or_expected( + "no pending approval found for request_id 'abc' (already decided or expired)", + "rpc", + "invoke_method", + &[], + ); + } + #[test] fn classifies_expected_config_errors() { assert_eq!( diff --git a/src/openhuman/approval/gate.rs b/src/openhuman/approval/gate.rs index 5cae27fc3..13ad451e3 100644 --- a/src/openhuman/approval/gate.rs +++ b/src/openhuman/approval/gate.rs @@ -45,6 +45,18 @@ use crate::openhuman::security::POLICY_DENIED_MARKER; use super::store; use super::types::{ApprovalDecision, ExecutionOutcome, GateOutcome, PendingApproval}; +/// Disambiguates why [`ApprovalGate::decide`] returned `Ok(None)`. See +/// [`ApprovalGate::classify_decide_miss`] for the lookup that produces this. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DecideMiss { + /// The pending row was already decided, lazily expired, or superseded — a + /// benign race (TAURI-RUST-5EH). Safe to demote out of Sentry. + AlreadyResolved, + /// No row was ever persisted for this request_id — a genuine lost + /// registration that must stay a Sentry signal. + NeverRegistered, +} + /// How long the gate will park a future before timing out and /// returning `Deny`. 10 minutes matches the default `expires_at` /// written into the persisted row. @@ -718,6 +730,40 @@ impl ApprovalGate { Ok(decided) } + /// Classify a [`Self::decide`] miss — i.e. when `decide` returned + /// `Ok(None)` because its conditional `UPDATE ... WHERE decided_at IS NULL` + /// matched 0 rows. Two very different states collapse into that `None`: + /// + /// - [`DecideMiss::AlreadyResolved`] — the row exists but was **already + /// decided, lazily expired (denied), or superseded**. This is the benign + /// double-tap / two-operator / expiry-while-live race the inline-approvals + /// design spec classifies as benign (TAURI-RUST-5EH). + /// - [`DecideMiss::NeverRegistered`] — no row was ever persisted for this + /// request_id. That is a genuine lost registration (a core restart dropped + /// the parked future before persisting, or a stray id) and must stay a + /// Sentry signal. + /// + /// We disambiguate by consulting [`store::get_decision`], which returns a + /// decision only when `decided_at IS NOT NULL` — exactly the already-resolved + /// case (expiry writes a `Deny` decision, so expired rows report here too). + /// A `decide` miss can't be an undecided-but-present row: that row would have + /// matched the `UPDATE`. If the lookup itself errors we conservatively keep + /// the event visible (`NeverRegistered`) rather than silently demoting. + pub fn classify_decide_miss(&self, request_id: &str) -> DecideMiss { + match store::get_decision(&self.config, request_id) { + Ok(Some(_)) => DecideMiss::AlreadyResolved, + Ok(None) => DecideMiss::NeverRegistered, + Err(err) => { + tracing::warn!( + request_id = %request_id, + error = %err, + "[approval::gate] classify_decide_miss: get_decision failed; treating as never-registered (keep visible)" + ); + DecideMiss::NeverRegistered + } + } + } + /// List all undecided rows, including orphans from prior launches. /// Orphan rows have no live parked future so a `decide` on them /// updates the DB but cannot resume an action — see [`store::list_pending`]. @@ -1083,6 +1129,63 @@ mod tests { assert!(decided.is_none()); } + /// TAURI-RUST-5EH: a `decide` miss must be classified — already-decided and + /// expired rows are benign (`AlreadyResolved`), while an id that was never + /// persisted is a genuine lost registration (`NeverRegistered`) that stays a + /// Sentry signal. + #[tokio::test] + async fn classify_decide_miss_distinguishes_resolved_from_unknown() { + let (gate, _dir) = test_gate(); + + // Never persisted → genuine loss, keep visible. + assert_eq!( + gate.classify_decide_miss("never-existed"), + DecideMiss::NeverRegistered + ); + + // Persist + decide a row, then a second decide misses → already-decided. + let pending = PendingApproval::new( + "req-decided", + "composio", + "send email", + serde_json::json!({}), + Some(chrono::Utc::now() + chrono::Duration::minutes(10)), + ); + store::insert_pending(&gate.config, &pending, &gate.session_id).unwrap(); + assert!(gate + .decide("req-decided", ApprovalDecision::ApproveOnce) + .unwrap() + .is_some()); + // The conditional UPDATE now matches 0 rows (decided_at set). + assert!(gate + .decide("req-decided", ApprovalDecision::Deny) + .unwrap() + .is_none()); + assert_eq!( + gate.classify_decide_miss("req-decided"), + DecideMiss::AlreadyResolved + ); + + // A row past its expiry is lazily denied by `decide`'s expire pass, so + // its decide miss is also benign (the persisted decision exists). + let expired = PendingApproval::new( + "req-expired", + "composio", + "send email", + serde_json::json!({}), + Some(chrono::Utc::now() - chrono::Duration::minutes(1)), + ); + store::insert_pending(&gate.config, &expired, &gate.session_id).unwrap(); + assert!(gate + .decide("req-expired", ApprovalDecision::ApproveOnce) + .unwrap() + .is_none()); + assert_eq!( + gate.classify_decide_miss("req-expired"), + DecideMiss::AlreadyResolved + ); + } + #[tokio::test] async fn pending_for_thread_tracks_request_under_chat_context_and_clears() { let (gate, _dir) = test_gate(); diff --git a/src/openhuman/approval/rpc.rs b/src/openhuman/approval/rpc.rs index ca7bd86de..4298adef1 100644 --- a/src/openhuman/approval/rpc.rs +++ b/src/openhuman/approval/rpc.rs @@ -7,7 +7,7 @@ use anyhow::anyhow; use crate::rpc::RpcOutcome; -use super::gate::{try_boot_state, ApprovalGate, ApprovalGateBootState}; +use super::gate::{try_boot_state, ApprovalGate, ApprovalGateBootState, DecideMiss}; use super::types::{ApprovalAuditEntry, ApprovalDecision, PendingApproval}; /// Read the host-aware approval-gate boot decision so the UI banner can @@ -116,13 +116,39 @@ pub async fn approval_decide( return Err(err); } }; - let row = decided.ok_or_else(|| { - tracing::warn!( - request_id = request_id, - "[rpc:approval_decide] no pending approval found" - ); - anyhow!("no pending approval found for request_id '{request_id}'") - })?; + let row = match decided { + Some(row) => row, + None => { + // `gate.decide` returned `Ok(None)`: the conditional UPDATE matched + // 0 rows. Disambiguate the benign already-decided/expired race from + // a genuine lost registration so only the latter reaches Sentry + // (TAURI-RUST-5EH). Both stay `warn!` — neither is a hard error the + // caller can act on differently — but the benign arm emits the + // "no pending approval found" wording that `expected_error_kind` + // demotes, while the never-registered arm emits a distinct string + // that stays a captured Sentry signal. + return match gate.classify_decide_miss(request_id) { + DecideMiss::AlreadyResolved => { + tracing::warn!( + request_id = request_id, + "[rpc:approval_decide] no pending approval found (already decided/expired — benign race)" + ); + Err(anyhow!( + "no pending approval found for request_id '{request_id}' (already decided or expired)" + )) + } + DecideMiss::NeverRegistered => { + tracing::warn!( + request_id = request_id, + "[rpc:approval_decide] no pending approval ever registered (genuine lost registration)" + ); + Err(anyhow!( + "no pending approval ever registered for request_id '{request_id}'" + )) + } + }; + } + }; let mut logs = vec![format!( "[approval] decided request_id={} tool={} decision={}",