mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-30 23:14:37 +00:00
fix(integrations): drive re-login on session-JWT 401 + demote tool-execute noise (TAURI-RUST-84E) (#3749)
Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
This commit is contained in:
co-authored by
Steven Enamakel
parent
4a65691cea
commit
8fca02a22f
@@ -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
|
||||
|
||||
@@ -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",
|
||||
&[
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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::<serde_json::Value>(
|
||||
"/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::<serde_json::Value>("/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::<serde_json::Value>("/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]
|
||||
|
||||
Reference in New Issue
Block a user