diff --git a/src/core/observability.rs b/src/core/observability.rs index 74c7866b4..7214d00b6 100644 --- a/src/core/observability.rs +++ b/src/core/observability.rs @@ -3746,6 +3746,47 @@ mod tests { } } + #[test] + fn tool_execute_backend_401_invalid_token_does_not_hard_report() { + // TAURI-RUST-84E: the integrations client demotes its backend 401 via + // `report_error_or_expected`, but ALSO `anyhow::bail!`s it. The error + // bubbles to the agent tool-execute loop + // (`agent::harness::engine::tools::run_one_tool`'s `Ok(Err(e))` arm), + // which previously called the unconditional `report_error` — a second, + // hard Sentry event (domain=tool / operation=execute) for an + // already-classified user-end invalid-token condition. The arm now + // routes through `report_error_or_expected`; this test pins the exact + // wire shape so a classifier regression that lets the 401 escape (and + // resume double-reporting) fails CI. + // + // Mirror the tool-execute call path: the integrations client bails with + // `anyhow::anyhow!("Backend returned {status} for POST {url}: {detail}")`, + // the agent arm renders it with `{e:#}`, then `report_error_or_expected` + // consults `expected_error_kind`. Assert that classifier returns the + // expected user-state bucket (so the report is demoted, not hard). + let e = anyhow::anyhow!( + "Backend returned 401 Unauthorized for POST \ + https://api.tinyhumans.ai/agent-integrations/parallel/search: Invalid token" + ); + assert_eq!( + expected_error_kind(&format!("{e:#}")), + Some(ExpectedErrorKind::BackendUserError), + "the 84E tool-execute backend-401 wire shape must classify as expected \ + user-state so report_error_or_expected demotes it instead of firing a \ + hard tool/execute Sentry event" + ); + + // A genuine tool failure (no classifier arm) must keep surfacing as a + // hard error — confirm routing ALL tool errors through + // `report_error_or_expected` does not silently swallow real failures. + assert_eq!( + expected_error_kind("tool 'web_search' panicked: index out of bounds"), + None, + "a genuine tool failure must NOT be classified as expected — it still \ + reaches Sentry as a hard error" + ); + } + #[test] fn does_not_classify_transient_or_server_backend_errors_as_user_error() { // 408 / 429 are transient — they belong to the diff --git a/src/openhuman/agent/harness/engine/tools.rs b/src/openhuman/agent/harness/engine/tools.rs index 07297ec3b..9ed91893f 100644 --- a/src/openhuman/agent/harness/engine/tools.rs +++ b/src/openhuman/agent/harness/engine/tools.rs @@ -305,8 +305,22 @@ pub(crate) async fn run_one_tool( } } Ok(Err(e)) => { - crate::core::observability::report_error( - &e, + // Route through `report_error_or_expected` (not the unconditional + // `report_error`) so an error a downstream layer already classified + // as expected user-state isn't re-reported as a hard Sentry event + // here. The integrations client (`integrations/client.rs`) already + // demotes its backend 4xx/auth-state failures via + // `report_error_or_expected`, but it ALSO bubbles the error up; it + // lands in this `Ok(Err(_))` arm and was being re-reported as a + // hard `tool`/`execute` event — the double-report behind Sentry + // TAURI-RUST-84E (`Backend returned 401 Unauthorized for POST + // .../agent-integrations/parallel/search: Invalid token`, a + // user-end invalid/expired session token with no openhuman-side + // lever). Genuine tool failures don't match any classifier arm and + // still surface as hard errors — only already-classified + // user-state errors are demoted to a warn/info breadcrumb. + crate::core::observability::report_error_or_expected( + format!("{e:#}").as_str(), "tool", "execute", &[ diff --git a/src/openhuman/integrations/client.rs b/src/openhuman/integrations/client.rs index 4e24aa924..e04ed4b5c 100644 --- a/src/openhuman/integrations/client.rs +++ b/src/openhuman/integrations/client.rs @@ -41,6 +41,95 @@ fn managed_budget_applies_to_path(path: &str) -> bool { path != "/agent-integrations/pricing" && path.starts_with("/agent-integrations/") } +/// Handle a `401 Unauthorized` from the OpenHuman backend's +/// `/agent-integrations/*` routes. +/// +/// **Why this 401 is unambiguously a session-JWT rejection.** Every request +/// from [`IntegrationClient`] attaches the *app-session JWT* as its +/// `Authorization: Bearer` — [`build_client`] resolves the token via +/// [`crate::api::jwt::get_session_token`], the same token billing / team / +/// webhooks / memory all use. The backend's auth middleware +/// (`backend-openhuman`) is what answers `401 {"error":"Invalid token"}` when +/// that JWT is expired / revoked / rotated server-side — see the identical +/// envelope pinned in `inference/provider/config_rejection.rs` and the socket +/// reconnect loop's `"Invalid token"` handling in +/// `openhuman::socket::ws_loop`. A *third-party* integration's auth failure +/// never reaches this arm: +/// +/// - **Composio backend mode** (the default that routes through this client): +/// provider-side failures come back as `2xx` envelope `success:false` +/// (`"Toolkit X is not enabled"`, `"Missing required fields: …"`) or a +/// descriptive non-401 4xx/5xx — handled by +/// [`crate::core::observability::is_provider_user_state_message`] / +/// [`crate::core::observability::is_backend_user_error_message`], NOT a bare +/// 401. The backend's auth wall is the only thing that returns 401 here. +/// - **Composio direct mode**: bypasses this client entirely (it talks to +/// `backend.composio.dev` with `x-api-key` via `ComposioTool`), so a +/// direct-mode key 401 carries the distinct `"[composio-direct] … HTTP 401: +/// Invalid API key"` shape and never lands here. +/// +/// So narrowing to `status == 401` (and *only* 401 — 403 stays generic, it can +/// be an authz/scope rejection on a backend-mediated resource rather than a +/// dead session) targets exactly the session-JWT rejection with zero risk of +/// logging the user out for an unrelated integration problem. +/// +/// What this does, mirroring `api/rest.rs::flatten_authed_error` (typed-401 → +/// `SESSION_EXPIRED:` sentinel) and +/// `inference/provider/ops/http_error.rs::publish_backend_session_expired` +/// (direct publish for paths whose error is consumed inline / swallowed): +/// +/// 1. Build a `SESSION_EXPIRED:`-prefixed message so it (a) classifies as +/// [`crate::core::observability::ExpectedErrorKind::SessionExpired`] and +/// stays demoted from Sentry, and (b) is recognised by +/// `core::jsonrpc::is_session_expired_error` *if* it ever propagates up to +/// the RPC boundary. +/// 2. **Publish `DomainEvent::SessionExpired` directly.** The autonomous agent +/// loop (`agent/harness/engine/tools.rs::run_one_tool`) swallows tool errors +/// into a `role:tool` result string fed back to the model — the `Err` never +/// reaches `jsonrpc::invoke_method`, so relying on propagation alone would +/// leave re-login un-triggered (this is the root-cause gap behind +/// TAURI-RUST-84E: the prior fix demoted the noise but never drove +/// recovery). Publishing here makes the credentials subscriber clear the +/// session and the UI prompt re-sign-in regardless of which call site +/// surfaced the 401. +fn handle_session_jwt_unauthorized(method: &str, path: &str, url: &str, detail: &str) -> String { + let message = format!( + "SESSION_EXPIRED: backend rejected session token on {method} {path} \ + (401 for {url}: {detail}) — sign in again to resume" + ); + + tracing::warn!( + path = %path, + method = %method, + "[integrations] backend rejected session JWT (401) — publishing SessionExpired to drive re-login" + ); + + // Demote from Sentry (SESSION_EXPIRED classifies as expected) — keeps the + // noise suppression the prior fix established. + crate::core::observability::report_error_or_expected( + message.as_str(), + "integrations", + "session_expired", + &[ + ("path", path), + ("status", "401"), + ("failure", "session_jwt"), + ], + ); + + // Drive recovery: publish SessionExpired so the credentials subscriber + // clears the stale token and the UI prompts re-sign-in. The reason string + // is already free of secrets (it names the path + sanitized backend + // `error` detail), but re-scrub for defense-in-depth before it reaches the + // subscriber's logs. + crate::core::event_bus::publish_global(crate::core::event_bus::DomainEvent::SessionExpired { + source: format!("integrations.{method}:{path}"), + reason: crate::openhuman::inference::provider::ops::sanitize_api_error(&message), + }); + + message +} + /// Strip any inference-style path that snuck into a backend URL before /// it becomes the [`IntegrationClient::backend_url`] field. Idempotent — /// returns the input unchanged when already clean. @@ -197,6 +286,19 @@ impl IntegrationClient { let body_text = resp.text().await.unwrap_or_default(); let detail = extract_error_detail(&body_text, MAX_ERROR_BODY_LEN); let status_str = status.as_u16().to_string(); + // A 401 is the backend's auth middleware rejecting our app-session + // JWT (not a third-party provider 401 — see + // `handle_session_jwt_unauthorized` for the full + // session-JWT-vs-provider argument). Route it into the + // session-expiry recovery flow: demote from Sentry AND publish + // `SessionExpired` so re-login fires even though the autonomous + // agent loop swallows the propagated error (TAURI-RUST-84E). This + // must come BEFORE the generic non-2xx branch so the 401 doesn't + // fall through to a plain `BackendUserError` that only demotes. + if status == reqwest::StatusCode::UNAUTHORIZED { + let message = handle_session_jwt_unauthorized("POST", path, &url, &detail); + anyhow::bail!("{message}"); + } // Route through `report_error_or_expected` so 4xx user-input / // auth-state failures (e.g. OPENHUMAN-TAURI-BC: SharePoint // authorize 400 because the user didn't fill in the required @@ -280,6 +382,14 @@ impl IntegrationClient { let body_text = resp.text().await.unwrap_or_default(); let detail = extract_error_detail(&body_text, MAX_ERROR_BODY_LEN); let status_str = status.as_u16().to_string(); + // Mirrors the post() session-JWT 401 arm — a 401 here is the + // backend rejecting our session JWT, so drive re-login (publish + // SessionExpired) and demote from Sentry, before the generic + // non-2xx branch. See `handle_session_jwt_unauthorized`. + if status == reqwest::StatusCode::UNAUTHORIZED { + let message = handle_session_jwt_unauthorized("GET", path, &url, &detail); + anyhow::bail!("{message}"); + } // Mirrors the post() site — see OPENHUMAN-TAURI-BC. 4xx // user-input / auth-state shapes demote to a warn breadcrumb // via the observability classifier; 5xx and non-transient 4xx diff --git a/src/openhuman/integrations/client_tests.rs b/src/openhuman/integrations/client_tests.rs index be618c68b..6a96522ab 100644 --- a/src/openhuman/integrations/client_tests.rs +++ b/src/openhuman/integrations/client_tests.rs @@ -391,6 +391,169 @@ async fn jira_generic_400_classifies_as_backend_user_error() { ); } +// ── TAURI-RUST-84E: session-JWT 401 → session-expiry recovery ───── + +/// Root-cause regression guard for TAURI-RUST-84E. A `401 Unauthorized` from +/// the OpenHuman backend's `/agent-integrations/*` routes is the backend +/// rejecting our app-session JWT. The propagated error string MUST: +/// 1. classify as `SessionExpired` (so it stays demoted from Sentry — the +/// noise suppression the prior fix established), AND +/// 2. be recognised by `is_session_expired_message` (the same predicate the +/// JSON-RPC dispatcher uses to publish `DomainEvent::SessionExpired` and +/// drive re-login). +/// +/// This couples the exact wire string `IntegrationClient::post`/`get` builds +/// for a session-JWT 401 to the session-expiry classifier — if either side +/// drifts, web_search (and every other backend-proxied tool) would silently +/// stop driving re-login on session expiry, leaving the user stuck behind an +/// opaque "parallel search failed: Invalid token" with no sign-in nudge. +#[tokio::test] +async fn post_401_session_jwt_classifies_as_session_expired() { + use crate::core::observability::{ + expected_error_kind, is_session_expired_message, ExpectedErrorKind, + }; + + // Canonical TAURI-RUST-84E shape: backend auth middleware rejects the + // session JWT with `401 {"error":"Invalid token"}`. + let app = Router::new().route( + "/agent-integrations/parallel/search", + post(|| async { + ( + StatusCode::UNAUTHORIZED, + Json(json!({ "success": false, "error": "Invalid token" })), + ) + .into_response() + }), + ); + let base = start_mock_backend(app).await; + let client = client_for(base); + let err = client + .post::( + "/agent-integrations/parallel/search", + &json!({ "objective": "x" }), + ) + .await + .expect_err("401 must surface as Err"); + let msg = format!("{err:#}"); + + // The propagated message carries the SESSION_EXPIRED sentinel so the model + // (and any caller that wraps it, e.g. `parallel search failed: {e:#}`) + // sees an actionable "sign in again" instruction. + assert!( + msg.contains("SESSION_EXPIRED"), + "session-JWT 401 must carry the SESSION_EXPIRED sentinel; got: {msg}" + ); + + // 1. Demotes from Sentry: classifies as SessionExpired (expected kind), + // not a hard error. + assert_eq!( + expected_error_kind(&msg), + Some(ExpectedErrorKind::SessionExpired), + "session-JWT 401 must classify as SessionExpired (stays demoted from Sentry); got: {msg}" + ); + + // 2. Recognised by the session-expiry predicate that the JSON-RPC + // dispatcher keys `DomainEvent::SessionExpired` publication on — so the + // re-login flow fires (the client also publishes it directly to cover + // the swallowing agent loop, but this keeps the propagation path + // correct too). + assert!( + is_session_expired_message(&msg), + "session-JWT 401 must be recognised by is_session_expired_message so re-login \ + fires; got: {msg}" + ); +} + +/// GET counterpart — the connections/toolkits/pricing reads must drive the +/// same session-expiry recovery on a session-JWT 401. +#[tokio::test] +async fn get_401_session_jwt_classifies_as_session_expired() { + use crate::core::observability::{ + expected_error_kind, is_session_expired_message, ExpectedErrorKind, + }; + + let app = Router::new().route( + "/agent-integrations/composio/connections", + get(|| async { + ( + StatusCode::UNAUTHORIZED, + Json(json!({ "success": false, "error": "Invalid token" })), + ) + .into_response() + }), + ); + let base = start_mock_backend(app).await; + let client = client_for(base); + let err = client + .get::("/agent-integrations/composio/connections") + .await + .expect_err("401 must surface as Err"); + let msg = format!("{err:#}"); + + assert_eq!( + expected_error_kind(&msg), + Some(ExpectedErrorKind::SessionExpired), + "GET session-JWT 401 must classify as SessionExpired; got: {msg}" + ); + assert!( + is_session_expired_message(&msg), + "GET session-JWT 401 must be recognised by is_session_expired_message; got: {msg}" + ); +} + +/// Negative guard (task #3 — the key correctness risk): a NON-401 4xx must NOT +/// be turned into session-expiry. A 403 (authz/scope rejection on a +/// backend-mediated resource) or a 400 (user-input failure) must keep the +/// generic `BackendUserError` / `ProviderUserState` classification so we never +/// log the user out for an unrelated integration problem. If the 401 arm ever +/// widened to swallow other statuses, this catches it. +#[tokio::test] +async fn non_401_4xx_does_not_classify_as_session_expired() { + use crate::core::observability::{expected_error_kind, is_session_expired_message}; + + // 403 — e.g. an authz/scope rejection on a backend-mediated provider + // resource. This is NOT a dead session; it must stay a generic backend + // user-error and must NOT publish SessionExpired. + let app = Router::new().route( + "/agent-integrations/composio/connections", + get(|| async { + ( + StatusCode::FORBIDDEN, + Json(json!({ "success": false, "error": "forbidden" })), + ) + .into_response() + }), + ); + let base = start_mock_backend(app).await; + let client = client_for(base); + let err = client + .get::("/agent-integrations/composio/connections") + .await + .expect_err("403 must surface as Err"); + let msg = format!("{err:#}"); + + assert!( + !msg.contains("SESSION_EXPIRED"), + "403 must NOT be turned into a session-expiry sentinel; got: {msg}" + ); + assert!( + !is_session_expired_message(&msg), + "403 must NOT be recognised as session expiry (would log the user out for an \ + unrelated integration authz problem); got: {msg}" + ); + // It still demotes from Sentry as a generic backend user-error (the prior + // behaviour), just not as session-expiry. + assert!( + expected_error_kind(&msg).is_some(), + "403 should still classify as an expected backend user-error; got: {msg}" + ); + assert_ne!( + expected_error_kind(&msg), + Some(crate::core::observability::ExpectedErrorKind::SessionExpired), + "403 must NOT classify as SessionExpired; got: {msg}" + ); +} + // ── Unit: `sanitize_backend_url` (issue #2075) ──────────────────── #[test]