diff --git a/crates/ironclaw_gateway/static/js/core/bootstrap.js b/crates/ironclaw_gateway/static/js/core/bootstrap.js index 30911f67975..6abbf2aa6b1 100644 --- a/crates/ironclaw_gateway/static/js/core/bootstrap.js +++ b/crates/ironclaw_gateway/static/js/core/bootstrap.js @@ -79,6 +79,7 @@ let logEventSource = null; let currentTab = 'chat'; let currentThreadId = null; let currentThreadIsReadOnly = false; +const threadChannelHints = new Map(); let assistantThreadId = null; let hasMore = false; let oldestTimestamp = null; diff --git a/crates/ironclaw_gateway/static/js/core/history.js b/crates/ironclaw_gateway/static/js/core/history.js index 0f3a6ba507b..db762152601 100644 --- a/crates/ironclaw_gateway/static/js/core/history.js +++ b/crates/ironclaw_gateway/static/js/core/history.js @@ -40,6 +40,10 @@ function loadHistory(before) { apiFetch(historyUrl).then((data) => { const container = document.getElementById('chat-messages'); + if (!isPaginating && currentThreadId && data.channel) { + threadChannelHints.set(currentThreadId, data.channel); + } + if (!isPaginating) { // Fresh load: clear and render container.innerHTML = ''; @@ -117,6 +121,16 @@ function loadHistory(before) { } else if (lastTurn && !lastTurn.response && lastTurn.state === 'Processing') { showActivityThinking(ActivityEntry.t('activity.processing', 'Processing...')); } + const hintedChannel = currentThreadId + ? (data.channel || threadChannelHints.get(currentThreadId) || 'gateway') + : 'gateway'; + currentThreadIsReadOnly = isReadOnlyChannel(hintedChannel); + if (currentThreadIsReadOnly) { + disableChatInputReadOnly(); + } else { + enableChatInput(); + } + if (data.pending_gate) { handleGateRequired({ ...data.pending_gate, @@ -467,7 +481,10 @@ function loadThreads() { const currentThread = currentThreadId === assistantThreadId ? data.assistant_thread : threads.find(t => t.id === currentThreadId); - const ch = currentThread ? currentThread.channel : 'gateway'; + const hintedChannel = currentThread + ? currentThread.channel + : threadChannelHints.get(currentThreadId); + const ch = hintedChannel || 'gateway'; currentThreadIsReadOnly = isReadOnlyChannel(ch); if (currentThreadIsReadOnly) { disableChatInputReadOnly(); diff --git a/src/bridge/mod.rs b/src/bridge/mod.rs index 6f52b25546b..1e99bd65a57 100644 --- a/src/bridge/mod.rs +++ b/src/bridge/mod.rs @@ -12,6 +12,7 @@ mod router; pub mod sandbox; pub mod skill_migration; mod store_adapter; +mod user_facing_errors; mod workspace_reader; pub use cost_guard_gate::CostGuardBudgetGate; diff --git a/src/bridge/router.rs b/src/bridge/router.rs index 41442280195..9155c5d2fc0 100644 --- a/src/bridge/router.rs +++ b/src/bridge/router.rs @@ -61,6 +61,26 @@ fn engine_err(context: &str, e: impl std::fmt::Display) -> Error { }) } +/// Build the `BridgeOutcome` for a `ThreadOutcome::Failed`. +/// +/// Raw engine failures can include Python tracebacks, internal file paths, +/// and upstream HTTP bodies (see #2546). This helper keeps the raw error +/// in the server-side logs and returns a short, user-facing summary +/// derived from the error's shape. +/// +/// Extracted into a named function so the sanitization flow (log + map to +/// user-friendly text + wrap in `BridgeOutcome`) can be exercised end-to-end +/// by unit tests without spinning up the full engine. +fn bridge_outcome_for_failed_thread(error: &str, user_id: &str, channel: &str) -> BridgeOutcome { + tracing::warn!( + user_id = %user_id, + channel = %channel, + error = %error, + "engine v2: thread failed; showing user-friendly summary", + ); + BridgeOutcome::Respond(crate::bridge::user_facing_errors::user_facing_thread_failure(error)) +} + const PROJECT_ATTACHMENT_DIR: &str = ".ironclaw/attachments"; #[derive(Debug, Clone)] @@ -3943,7 +3963,11 @@ async fn await_thread_outcome( ThreadOutcome::MaxIterations => Ok(BridgeOutcome::Respond( "Reached maximum iterations without completing.".into(), )), - ThreadOutcome::Failed { error } => Ok(BridgeOutcome::Respond(format!("Error: {error}"))), + ThreadOutcome::Failed { error } => Ok(bridge_outcome_for_failed_thread( + &error, + &message.user_id, + &message.channel, + )), ThreadOutcome::GatePaused { gate_name, action_name, @@ -5907,6 +5931,67 @@ mod tests { static ENGINE_STATE_TEST_LOCK: LazyLock> = LazyLock::new(|| TokioMutex::new(())); static CWD_TEST_LOCK: LazyLock> = LazyLock::new(|| TokioMutex::new(())); + // ────────────────────────────────────────────────────────────────── + // `bridge_outcome_for_failed_thread` — caller-level coverage. + // + // These tests drive the same helper that `handle_with_engine_inner` + // calls when it receives a `ThreadOutcome::Failed { error }`. They + // are the regression fence for issue #2546 (raw Python traceback + // from a 502 reaching the user). The sanitization logic proper + // lives in `bridge::user_facing_errors` and has its own unit tests; + // these assert that the router arm (log + sanitize + wrap) is + // actually wired up — per the "Test Through the Caller" rule. + // ────────────────────────────────────────────────────────────────── + + #[test] + fn failed_thread_outcome_hides_python_traceback_from_user() { + let raw = "Orchestrator error: effect execution error: Orchestrator error after resume: \ + Traceback (most recent call last): \ + File \"orchestrator.py\", line 907, in \ + File \"orchestrator.py\", line 548, in run_loop \ + RuntimeError: LLM call failed: Provider nearai_chat request failed: HTTP 502 Bad Gateway"; + let outcome = bridge_outcome_for_failed_thread(raw, "alice", "web"); + let BridgeOutcome::Respond(text) = outcome else { + panic!("expected Respond, got {outcome:?}"); + }; + assert_eq!( + text, + "The AI model is temporarily unavailable. Please try again in a few moments." + ); + // Defense-in-depth: none of the leaky internals must surface. + assert!(!text.contains("Traceback")); + assert!(!text.contains("orchestrator.py")); + assert!(!text.contains("effect execution error")); + assert!(!text.contains("nearai_chat")); + } + + #[test] + fn failed_thread_outcome_maps_unknown_error_to_generic_message() { + let outcome = + bridge_outcome_for_failed_thread("some unexpected internal failure", "alice", "web"); + let BridgeOutcome::Respond(text) = outcome else { + panic!("expected Respond, got {outcome:?}"); + }; + assert_eq!( + text, + "Something went wrong while processing your message. Please try again." + ); + assert!(!text.contains("some unexpected internal failure")); + } + + #[test] + fn failed_thread_outcome_maps_context_too_large() { + let raw = "Orchestrator error: Llm { reason: \"Context length exceeded: 200000 tokens used, 128000 allowed\" }"; + let outcome = bridge_outcome_for_failed_thread(raw, "alice", "web"); + let BridgeOutcome::Respond(text) = outcome else { + panic!("expected Respond, got {outcome:?}"); + }; + assert!( + text.starts_with("The request was too large"), + "unexpected text: {text}" + ); + } + struct TestStore { conversations: TokioRwLock>, threads: TokioRwLock>, diff --git a/src/bridge/user_facing_errors.rs b/src/bridge/user_facing_errors.rs new file mode 100644 index 00000000000..0f349361e80 --- /dev/null +++ b/src/bridge/user_facing_errors.rs @@ -0,0 +1,448 @@ +//! User-facing error message sanitization. +//! +//! The engine's `ThreadOutcome::Failed { error }` carries a deeply nested +//! error string that reaches the user verbatim via the web/chat UI. In +//! practice the raw string exposes internals: +//! +//! - Rust-level wrapping (`Orchestrator error: effect execution error: ...`) +//! - Python tracebacks from the Monty-hosted orchestrator script +//! (`Traceback ... File "orchestrator.py", line 907, in ...`) +//! - Raw upstream failures (`HTTP 502 Bad Gateway`, JSON payloads) +//! +//! Issue #2546 tracked a case where a 502 from the LLM provider surfaced +//! the full traceback to a user on staging. The fix is two-sided: +//! +//! 1. Keep the raw error in the server-side logs (callers of this module +//! are expected to `tracing::warn!` with the full string before +//! rendering the sanitized text). +//! 2. Return a short, user-friendly message derived from the raw error +//! whenever a known pattern matches. Unknown errors fall back to a +//! generic "something went wrong" message rather than exposing the +//! internal chain. +//! +//! This lives in `bridge::` because the adapter layer (not the engine +//! itself) owns the contract between engine outcomes and channel +//! responses — the engine is intentionally free to surface raw diagnostic +//! text, and the bridge is responsible for the presentation. + +/// Categorization of a failure, used to pick the user-facing message and +/// to let tests assert on intent rather than matching on the full string. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum FailureCategory { + /// The upstream LLM provider returned a transient server-side error + /// (HTTP 502/503/504, explicit "bad gateway"/"service unavailable", + /// provider-level timeouts). Users should retry. + LlmUnavailable, + /// The upstream LLM provider rate-limited the request (HTTP 429). + LlmRateLimited, + /// The request/context was too large for the provider (HTTP 413 or + /// "context length exceeded"). Users can shorten the request. + ContextTooLarge, + /// Authentication with the provider failed (HTTP 401/403 or an + /// `AuthFailed`/"session expired" message). + AuthFailure, + /// The agent stopped because it hit the iteration/step limit for a + /// single turn. Already surfaced separately by `ThreadOutcome::MaxIterations` + /// but can also appear inside a failed outcome in some paths. + IterationLimit, + /// Something else. Render a generic message and log the raw text. + Unknown, +} + +/// Convert a raw `ThreadOutcome::Failed { error }` string into a short, +/// user-friendly message. The returned text is safe to show verbatim in +/// chat — it never includes Python tracebacks, file paths, JSON payloads, +/// or internal wrapping like "effect execution error". +/// +/// Callers should log the raw `error` string themselves before calling +/// this function so that full diagnostic detail is retained server-side. +pub(crate) fn user_facing_thread_failure(error: &str) -> String { + match classify_failure(error) { + FailureCategory::LlmUnavailable => { + "The AI model is temporarily unavailable. Please try again in a few moments.".into() + } + FailureCategory::LlmRateLimited => { + "The AI model is currently rate-limited. Please try again shortly.".into() + } + FailureCategory::ContextTooLarge => { + "The request was too large for the AI model. Please shorten the conversation or attachments and try again." + .into() + } + FailureCategory::AuthFailure => { + "The AI model could not authenticate. Please re-authenticate the provider and try again." + .into() + } + FailureCategory::IterationLimit => { + "The agent reached its step limit before finishing. Please try again.".into() + } + FailureCategory::Unknown => { + "Something went wrong while processing your message. Please try again.".into() + } + } +} + +/// Classify a raw failure string. Public(crate) so tests can assert on the +/// category independently of the user-facing wording. +pub(crate) fn classify_failure(error: &str) -> FailureCategory { + // Case-insensitive substring matching. The raw error is wrapped + // through multiple layers (Rust `Display`, Python traceback, upstream + // HTTP body) so we intentionally do not try to parse it — we scan + // for stable keywords. + let lower = error.to_ascii_lowercase(); + + // Rate limiting: check before the generic 5xx branch because 429s + // sometimes get surfaced alongside "provider request failed". + if lower.contains("http 429") + || lower.contains("rate limited") + || lower.contains("rate-limited") + || lower.contains("ratelimited") + || lower.contains("too many requests") + { + return FailureCategory::LlmRateLimited; + } + + // Context length: HTTP 413 or explicit context-length error strings. + // Note: we deliberately do NOT match on the generic "tokens used" + // phrase — it's overly broad and can appear in informational text. + // The explicit `context length exceeded` / `context_length_exceeded` + // markers cover the real failure modes (including issue #2408). + if lower.contains("http 413") + || lower.contains("payload too large") + || lower.contains("context length exceeded") + || lower.contains("context_length_exceeded") + { + return FailureCategory::ContextTooLarge; + } + + // Authentication failures. Match on specific HTTP status lines and + // explicit auth-failure markers. We deliberately do NOT match on + // bare "unauthorized" because that word also appears in + // tool-level / resource-access errors that are not LLM auth issues. + if lower.contains("http 401") + || lower.contains("http 403") + || lower.contains("401 unauthorized") + || lower.contains("invalid api key") + || lower.contains("invalid_api_key") + || lower.contains("authentication failed") + || lower.contains("session expired") + || lower.contains("session renewal failed") + { + return FailureCategory::AuthFailure; + } + + // Provider unavailability. Matches the exact shape of the issue + // #2546 traceback (`HTTP 502 Bad Gateway`) and also covers 503/504 + // and common upstream-timeout phrasings. We deliberately do NOT + // match on the very generic `"request failed"` or `"provider nearai"` + // phrases — they would misclassify non-5xx provider failures + // (e.g. `Provider openai_codex request failed: HTTP 400`) as + // transient unavailability. + if lower.contains("http 502") + || lower.contains("http 503") + || lower.contains("http 504") + || lower.contains("bad gateway") + || lower.contains("service unavailable") + || lower.contains("gateway timeout") + || lower.contains("upstream connect error") + || lower.contains("upstream") + || lower.contains("provider temporarily unavailable") + || lower.contains("llm call failed") + { + return FailureCategory::LlmUnavailable; + } + + if lower.contains("max iterations") || lower.contains("maximum iterations") { + return FailureCategory::IterationLimit; + } + + FailureCategory::Unknown +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The exact error string from issue #2546. This is the canonical + /// regression fixture — the raw Python traceback must never reach + /// the user verbatim. + const ISSUE_2546_RAW: &str = "Orchestrator error: effect execution error: Orchestrator error after resume: \ + Traceback (most recent call last): \ + File \"orchestrator.py\", line 907, in \ + File \"orchestrator.py\", line 548, in run_loop \ + RuntimeError: LLM call failed: Provider nearai_chat request failed: HTTP 502 Bad Gateway"; + + #[test] + fn issue_2546_502_bad_gateway_is_sanitized() { + let msg = user_facing_thread_failure(ISSUE_2546_RAW); + assert_eq!( + msg, + "The AI model is temporarily unavailable. Please try again in a few moments." + ); + } + + #[test] + fn issue_2546_is_classified_as_llm_unavailable() { + assert_eq!( + classify_failure(ISSUE_2546_RAW), + FailureCategory::LlmUnavailable + ); + } + + #[test] + fn sanitized_message_never_leaks_python_traceback() { + let msg = user_facing_thread_failure(ISSUE_2546_RAW); + assert!(!msg.contains("Traceback"), "msg leaked traceback: {msg}"); + assert!( + !msg.contains("orchestrator.py"), + "msg leaked file path: {msg}" + ); + assert!( + !msg.contains("RuntimeError"), + "msg leaked Python exc: {msg}" + ); + assert!(!msg.contains("run_loop"), "msg leaked internal fn: {msg}"); + assert!( + !msg.contains("effect execution error"), + "msg leaked rust wrap: {msg}" + ); + assert!(!msg.contains("nearai"), "msg leaked provider name: {msg}"); + } + + #[test] + fn http_502_bad_gateway_variants() { + let cases = [ + "HTTP 502 Bad Gateway", + "http 502", + "upstream returned Bad Gateway", + "Provider foo request failed: HTTP 502", + ]; + for case in cases { + assert_eq!( + classify_failure(case), + FailureCategory::LlmUnavailable, + "case: {case}" + ); + } + } + + #[test] + fn http_503_service_unavailable() { + assert_eq!( + classify_failure("HTTP 503 Service Unavailable"), + FailureCategory::LlmUnavailable + ); + } + + #[test] + fn http_504_gateway_timeout() { + assert_eq!( + classify_failure("HTTP 504 Gateway Timeout"), + FailureCategory::LlmUnavailable + ); + } + + #[test] + fn http_429_is_rate_limited() { + assert_eq!( + classify_failure("Provider foo request failed: HTTP 429 Too Many Requests"), + FailureCategory::LlmRateLimited + ); + assert_eq!( + user_facing_thread_failure("HTTP 429 Too Many Requests"), + "The AI model is currently rate-limited. Please try again shortly." + ); + } + + #[test] + fn http_413_is_context_too_large() { + // Related issue #2276 — 413 Payload Too Large from nearai_chat. + assert_eq!( + classify_failure("Provider nearai_chat request failed: HTTP 413 Payload Too Large"), + FailureCategory::ContextTooLarge + ); + } + + #[test] + fn context_length_exceeded_is_context_too_large() { + // Related issue #2408. + assert_eq!( + classify_failure("Context length exceeded: 200000 tokens used, 128000 allowed"), + FailureCategory::ContextTooLarge + ); + } + + #[test] + fn http_401_is_auth_failure() { + assert_eq!( + classify_failure("HTTP 401 Unauthorized"), + FailureCategory::AuthFailure + ); + } + + #[test] + fn authentication_failed_text_is_auth_failure() { + assert_eq!( + classify_failure("Authentication failed for provider 'nearai'."), + FailureCategory::AuthFailure + ); + } + + #[test] + fn session_expired_is_auth_failure() { + assert_eq!( + classify_failure("Session expired for provider nearai"), + FailureCategory::AuthFailure + ); + } + + #[test] + fn unknown_errors_get_generic_message() { + let msg = user_facing_thread_failure("something totally unexpected"); + assert_eq!( + msg, + "Something went wrong while processing your message. Please try again." + ); + assert!(!msg.contains("something totally unexpected")); + } + + #[test] + fn empty_error_string_does_not_panic() { + let msg = user_facing_thread_failure(""); + assert_eq!( + msg, + "Something went wrong while processing your message. Please try again." + ); + } + + #[test] + fn case_insensitive_matching() { + // Real errors come through a chain of `Display` impls, so the + // exact casing is not guaranteed across versions. + assert_eq!( + classify_failure("HTTP 502 BAD GATEWAY"), + FailureCategory::LlmUnavailable + ); + assert_eq!( + classify_failure("http 502 bad gateway"), + FailureCategory::LlmUnavailable + ); + } + + #[test] + fn rate_limit_takes_precedence_over_unavailable() { + // If a response somehow surfaces both keywords (e.g. a 429 + // response body that mentions "bad gateway upstream"), we + // prefer the rate-limit message because it's more actionable. + assert_eq!( + classify_failure("HTTP 429 Too Many Requests (upstream: bad gateway)"), + FailureCategory::LlmRateLimited + ); + } + + #[test] + fn iteration_limit_is_classified() { + assert_eq!( + classify_failure("Reached maximum iterations"), + FailureCategory::IterationLimit + ); + } + + #[test] + fn non_5xx_provider_failure_is_not_llm_unavailable() { + // Regression for PR #2747 review: the old classifier matched + // on the very generic `"request failed"` phrase, which caused + // non-5xx provider failures (like a 400 Bad Request) to be + // misclassified as transient unavailability. Those should now + // fall through to `Unknown` so the user gets the generic + // "something went wrong" message instead of an incorrect + // "try again in a few moments" nudge. + let raw = "Provider openai_codex request failed: HTTP 400 Bad Request"; + assert_ne!( + classify_failure(raw), + FailureCategory::LlmUnavailable, + "non-5xx provider failure must not classify as LlmUnavailable" + ); + assert_eq!(classify_failure(raw), FailureCategory::Unknown); + } + + #[test] + fn bare_unauthorized_is_not_auth_failure() { + // Regression for PR #2747 review: the old classifier matched + // on bare `"unauthorized"`, which caught tool-level resource + // permission errors and mislabeled them as LLM provider auth + // failures. Only explicit LLM-auth markers should match. + let raw = "Tool failed: unauthorized to access /etc/shadow"; + assert_ne!( + classify_failure(raw), + FailureCategory::AuthFailure, + "bare 'unauthorized' must not classify as AuthFailure" + ); + } + + #[test] + fn invalid_api_key_is_auth_failure() { + assert_eq!( + classify_failure("Provider returned: Invalid API key"), + FailureCategory::AuthFailure + ); + assert_eq!( + classify_failure("{\"error\":{\"code\":\"invalid_api_key\"}}"), + FailureCategory::AuthFailure + ); + } + + #[test] + fn http_401_with_unauthorized_word_still_matches() { + // The common wire shape `HTTP 401 Unauthorized` continues to + // classify as AuthFailure via the explicit `"http 401"` marker. + assert_eq!( + classify_failure("401 Unauthorized: invalid token"), + FailureCategory::AuthFailure + ); + } + + #[test] + fn auth_failure_message_is_channel_agnostic() { + // Regression for PR #2747 review: the old copy said "Please + // reconnect the provider", which is web-UI-specific. The + // router is used across channels (web/telegram/CLI), so the + // message must avoid channel-specific verbs. + let msg = user_facing_thread_failure("HTTP 401 Unauthorized"); + assert!( + !msg.contains("reconnect"), + "auth failure copy must not use 'reconnect' (web-only verb): {msg}" + ); + assert!( + msg.contains("re-authenticate"), + "auth failure copy should guide users to re-authenticate: {msg}" + ); + } + + #[test] + fn all_messages_end_with_period() { + // Tiny presentation invariant — every sanitized message is a + // complete sentence. Keeps the UI consistent. + for cat in [ + FailureCategory::LlmUnavailable, + FailureCategory::LlmRateLimited, + FailureCategory::ContextTooLarge, + FailureCategory::AuthFailure, + FailureCategory::IterationLimit, + FailureCategory::Unknown, + ] { + let raw = match cat { + FailureCategory::LlmUnavailable => "HTTP 502 Bad Gateway", + FailureCategory::LlmRateLimited => "HTTP 429", + FailureCategory::ContextTooLarge => "HTTP 413", + FailureCategory::AuthFailure => "HTTP 401", + FailureCategory::IterationLimit => "max iterations reached", + FailureCategory::Unknown => "???", + }; + let msg = user_facing_thread_failure(raw); + assert!( + msg.ends_with('.'), + "category {cat:?} msg does not end with period: {msg}" + ); + } + } +} diff --git a/src/channels/web/features/chat/mod.rs b/src/channels/web/features/chat/mod.rs index 4194a03d487..f2a8934a780 100644 --- a/src/channels/web/features/chat/mod.rs +++ b/src/channels/web/features/chat/mod.rs @@ -523,6 +523,7 @@ pub(crate) async fn chat_history_handler( turns, has_more, oldest_timestamp, + channel: None, pending_gate: history_pending_gate_info(&state, &user.user_id, thread_scope).await, in_progress: None, })); @@ -559,6 +560,7 @@ pub(crate) async fn chat_history_handler( turns, has_more: false, oldest_timestamp: None, + channel: None, pending_gate, in_progress: in_progress_from_thread(thread), })); @@ -588,6 +590,7 @@ pub(crate) async fn chat_history_handler( turns, has_more, oldest_timestamp, + channel: None, pending_gate: history_pending_gate_info(&state, &user.user_id, thread_scope).await, in_progress, })); @@ -608,19 +611,18 @@ pub(crate) async fn chat_history_handler( .enumerate() .filter_map(|(index, entry)| engine_history_entry_to_message(thread_id, index, entry)) .collect(); - if !synthetic.is_empty() { - let oldest_timestamp = synthetic.first().map(|m| m.created_at.to_rfc3339()); - let mut turns = build_turns_from_db_messages(&synthetic); - enforce_generated_image_history_budget(&mut turns); - return Ok(Json(HistoryResponse { - thread_id, - turns, - has_more: false, - oldest_timestamp, - pending_gate: history_pending_gate_info(&state, &user.user_id, thread_scope).await, - in_progress: None, - })); - } + let oldest_timestamp = synthetic.first().map(|m| m.created_at.to_rfc3339()); + let mut turns = build_turns_from_db_messages(&synthetic); + enforce_generated_image_history_budget(&mut turns); + return Ok(Json(HistoryResponse { + thread_id, + turns, + has_more: false, + oldest_timestamp, + channel: Some("engine".to_string()), + pending_gate: history_pending_gate_info(&state, &user.user_id, thread_scope).await, + in_progress: None, + })); } // Empty thread (just created, no messages yet) @@ -639,6 +641,7 @@ pub(crate) async fn chat_history_handler( turns: Vec::new(), has_more: false, oldest_timestamp: None, + channel: None, pending_gate: history_pending_gate_info(&state, &user.user_id, thread_scope).await, in_progress, })) @@ -723,46 +726,12 @@ pub(crate) async fn chat_threads_handler( }); } - // Engine v2 threads for this user in the default project. These - // don't always get a matching v1 conversation row (the assistant - // flow dual-writes into the single assistant conv id, not the - // engine thread id), so without this merge they'd be invisible - // in the sidebar even though the chat history endpoint can now - // render them by id. - if let Ok(engine_threads) = - crate::bridge::list_engine_threads(None, &user.user_id).await - { - let existing_ids: std::collections::HashSet = threads - .iter() - .map(|t| t.id) - .chain(assistant_thread.as_ref().map(|a| a.id)) - .collect(); - for eng in engine_threads { - let Ok(uuid) = uuid::Uuid::parse_str(&eng.id) else { - continue; - }; - if existing_ids.contains(&uuid) { - continue; - } - threads.push(ThreadInfo { - id: uuid, - state: eng.state, - turn_count: eng.step_count, - created_at: eng.created_at, - updated_at: eng.updated_at.clone(), - // Engine threads carry their goal as the only - // human-readable label; reuse it as the sidebar - // title so the user can tell threads apart. - title: Some(eng.goal), - thread_type: Some(eng.thread_type), - channel: Some("engine".to_string()), - }); - } - // Re-sort by updated_at descending so engine threads interleave - // chronologically with v1 conversations. - threads.sort_by(|a, b| b.updated_at.cmp(&a.updated_at)); - } - + // Keep the chat sidebar scoped to persisted chat conversations. + // Engine v2 foreground threads are assistant execution internals + // and can rotate per message, so surfacing them here makes + // ordinary prompts look like standalone `engine` threads. + // Explicit engine-thread history still works via + // `chat_history_handler` when the caller already has a thread id. let active_thread = session.lock().await.active_thread; return Ok(Json(ThreadListResponse { @@ -1900,6 +1869,63 @@ mod tests { assert_eq!(response.threads[0].channel.as_deref(), Some("gateway")); } + #[cfg(feature = "libsql")] + #[tokio::test] + async fn test_chat_threads_handler_hides_engine_threads_from_sidebar() { + let _lock = crate::bridge::test_support::ENGINE_STATE_TEST_LOCK + .lock() + .await; + crate::bridge::test_support::clear_engine_state().await; + + let project_id = + crate::bridge::test_support::install_engine_state_with_threads(Vec::new()).await; + let mut thread = ironclaw_engine::Thread::new( + "assistant hello", + ironclaw_engine::ThreadType::Foreground, + project_id, + "alice", + ironclaw_engine::ThreadConfig::default(), + ); + thread + .messages + .push(ironclaw_engine::ThreadMessage::user("hello")); + let engine_thread_id = thread.id.0; + crate::bridge::test_support::install_engine_state_with_threads(vec![thread]).await; + + let (db, _tmp) = crate::testing::test_db().await; + let session_manager = Arc::new(SessionManager::new()); + let state = test_gateway_state_with_store_and_session_manager(db, session_manager); + + let response = chat_threads_handler( + axum::extract::State(state), + crate::channels::web::auth::AuthenticatedUser(UserIdentity { + user_id: "alice".to_string(), + role: "member".to_string(), + workspace_read_scopes: Vec::new(), + }), + ) + .await + .expect("handler ok"); + + assert!(response.assistant_thread.is_some()); + assert!( + response + .threads + .iter() + .all(|thread| thread.id != engine_thread_id), + "chat sidebar must not surface engine execution threads" + ); + assert!( + response + .threads + .iter() + .all(|thread| thread.channel.as_deref() != Some("engine")), + "chat sidebar must stay scoped to chat conversations" + ); + + crate::bridge::test_support::clear_engine_state().await; + } + #[cfg(feature = "libsql")] #[tokio::test] async fn test_chat_new_thread_handler_persists_to_db_and_session() { @@ -2266,11 +2292,46 @@ mod tests { let turn = &response.turns[0]; assert_eq!(turn.user_input, "hello engine"); assert_eq!(turn.response.as_deref(), Some("hi back")); + assert_eq!(response.channel.as_deref(), Some("engine")); assert!(!response.has_more); crate::bridge::test_support::clear_engine_state().await; } + #[tokio::test] + async fn test_chat_history_returns_engine_channel_hint_without_renderable_messages() { + let _lock = crate::bridge::test_support::ENGINE_STATE_TEST_LOCK + .lock() + .await; + crate::bridge::test_support::clear_engine_state().await; + + let project_id = + crate::bridge::test_support::install_engine_state_with_threads(Vec::new()).await; + let thread = ironclaw_engine::Thread::new( + "empty engine thread", + ironclaw_engine::ThreadType::Foreground, + project_id, + "alice", + ironclaw_engine::ThreadConfig::default(), + ); + let thread_uuid = thread.id.0; + crate::bridge::test_support::install_engine_state_with_threads(vec![thread]).await; + + let mut state = test_gateway_state_with_dependencies(None, None, None, None); + Arc::get_mut(&mut state) + .expect("state should be uniquely owned") + .session_manager = Some(Arc::new(SessionManager::new())); + + let (s, u, q) = history_request(state, "alice", thread_uuid); + let response = chat_history_handler(s, u, q).await.expect("history"); + + assert_eq!(response.thread_id, thread_uuid); + assert!(response.turns.is_empty()); + assert_eq!(response.channel.as_deref(), Some("engine")); + + crate::bridge::test_support::clear_engine_state().await; + } + #[tokio::test] async fn test_chat_history_returns_404_for_cross_user_engine_thread() { let _lock = crate::bridge::test_support::ENGINE_STATE_TEST_LOCK diff --git a/src/channels/web/types.rs b/src/channels/web/types.rs index f4ede474e7f..72532c6d341 100644 --- a/src/channels/web/types.rs +++ b/src/channels/web/types.rs @@ -124,6 +124,10 @@ pub struct HistoryResponse { /// Cursor for the next page (ISO8601 timestamp of the oldest message returned). #[serde(skip_serializing_if = "Option::is_none")] pub oldest_timestamp: Option, + /// Channel hint for history views that are not present in the sidebar. + /// Used by the frontend to keep deep-linked non-gateway threads read-only. + #[serde(skip_serializing_if = "Option::is_none")] + pub channel: Option, /// Unified pending gate state for engine v2. #[serde(skip_serializing_if = "Option::is_none")] pub pending_gate: Option, diff --git a/tests/e2e/scenarios/test_v2_thread_visibility.py b/tests/e2e/scenarios/test_v2_thread_visibility.py index 3be2792450e..80fdd370f49 100644 --- a/tests/e2e/scenarios/test_v2_thread_visibility.py +++ b/tests/e2e/scenarios/test_v2_thread_visibility.py @@ -1,18 +1,17 @@ -"""E2E regression: engine v2 threads are visible in sidebar and history. +"""E2E regression: engine threads stay out of chat sidebar while history works. -Covers the behavior PR #2532 introduced in `chat_threads_handler` and -`chat_history_handler`: +Covers the intended split between the chat sidebar and engine APIs: -- An engine v2 thread created from a `/api/chat/send` call shows up in the - `/api/chat/threads` sidebar with `channel == "engine"`. -- `/api/chat/history?thread_id=` returns the messages - synthesized from engine thread transcript even when the v1 conversation - table has no row for that id (deep-link-by-id path). +- A foreground engine thread spawned by `/api/chat/send` must remain + discoverable via `/api/engine/threads`, but it must *not* surface as an + `engine` entry inside `/api/chat/threads`. +- `/api/chat/history?thread_id=` must still synthesize the + transcript for callers that explicitly deep-link to that engine thread id. -Prior behavior silently dropped these threads from the sidebar and -returned an empty history on deep-link; the fixture drives the HTTP -surface directly so the regression survives independent of frontend -polish. +The staging regression merged engine foreground threads into the normal chat +sidebar, which made ordinary prompts look like separate `ENGINE` +conversations. This fixture keeps that bug from coming back while preserving +explicit engine-thread history access. """ import asyncio @@ -156,26 +155,28 @@ async def _wait_for_assistant_response( ) -async def _engine_only_threads(base_url: str) -> list[dict]: - """Return sidebar entries whose channel is engine (the v2-merge path).""" +async def _chat_sidebar_threads(base_url: str) -> list[dict]: r = await api_get(base_url, "/api/chat/threads", timeout=15) r.raise_for_status() - return [t for t in r.json().get("threads", []) if t.get("channel") == "engine"] + return r.json().get("threads", []) + + +async def _engine_threads(base_url: str) -> list[dict]: + r = await api_get(base_url, "/api/engine/threads", timeout=15) + r.raise_for_status() + return r.json().get("threads", []) class TestV2ThreadVisibility: - async def test_engine_only_thread_appears_in_sidebar_with_engine_channel( + async def test_engine_thread_stays_out_of_chat_sidebar( self, v2_visibility_server ): - """Send without a client-supplied thread_id: the v1 flow dual-writes - into the shared assistant conversation, but the engine spins up a - fresh thread id that has no matching v1 row. The PR's merge should - surface that engine thread in the sidebar with `channel=engine`. + """Assistant sends still spawn engine threads, but those execution + threads must stay out of the normal chat sidebar. """ base = v2_visibility_server - baseline = await _engine_only_threads(base) - baseline_ids = {t["id"] for t in baseline} + baseline_engine_ids = {t["id"] for t in await _engine_threads(base)} send_r = await api_post( base, @@ -185,22 +186,25 @@ async def test_engine_only_thread_appears_in_sidebar_with_engine_channel( ) assert send_r.status_code in (200, 202), send_r.text - new_engine_entry = None + engine_thread = None for _ in range(60): - merged = await _engine_only_threads(base) - new_entries = [t for t in merged if t["id"] not in baseline_ids] - if new_entries: - new_engine_entry = new_entries[0] + engine_threads = await _engine_threads(base) + new_threads = [t for t in engine_threads if t["id"] not in baseline_engine_ids] + if new_threads: + engine_thread = new_threads[0] break await asyncio.sleep(0.5) - assert new_engine_entry is not None, ( - "a new engine-only thread must appear in the sidebar after an " - "assistant send with no thread_id; PR #2532 added this merge path" + assert engine_thread is not None, "engine thread never materialized" + + sidebar_threads = await _chat_sidebar_threads(base) + assert all(t.get("channel") != "engine" for t in sidebar_threads), ( + "chat sidebar must not show engine execution threads as normal " + f"conversations, got {sidebar_threads}" ) - assert new_engine_entry.get("title"), ( - f"engine sidebar entry must carry a goal as title, got " - f"{new_engine_entry}" + assert all(t.get("id") != engine_thread["id"] for t in sidebar_threads), ( + "the newly spawned engine thread must stay discoverable via the " + "/api/engine/threads surface, not /api/chat/threads" ) async def test_history_synthesizes_messages_for_deep_linked_engine_thread( @@ -211,7 +215,7 @@ async def test_history_synthesizes_messages_for_deep_linked_engine_thread( """ base = v2_visibility_server - baseline_ids = {t["id"] for t in await _engine_only_threads(base)} + baseline_engine_ids = {t["id"] for t in await _engine_threads(base)} await api_post( base, @@ -222,17 +226,15 @@ async def test_history_synthesizes_messages_for_deep_linked_engine_thread( engine_thread_id = None for _ in range(60): - merged = await _engine_only_threads(base) - new = [t for t in merged if t["id"] not in baseline_ids] - if new: - engine_thread_id = new[0]["id"] + engine_threads = await _engine_threads(base) + new_threads = [t for t in engine_threads if t["id"] not in baseline_engine_ids] + if new_threads: + engine_thread_id = new_threads[0]["id"] break await asyncio.sleep(0.5) - assert engine_thread_id is not None, "engine-only thread never materialized" + assert engine_thread_id is not None, "engine thread never materialized" - # Deep-link by engine thread id. Before PR #2532 this returned an - # empty turn list because the v1 conversation lookup missed. turns = await _wait_for_assistant_response( base, engine_thread_id, timeout=45 )