fix(approval): classify benign 'no pending approval' race; keep genuine loss visible (#4183)

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<ExpectedErrorKind> {
     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!(

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();

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={}",