From 904e3786772ddd3d4afb3e717c21fd6a4faed3da Mon Sep 17 00:00:00 2001 From: "firat.sertgoz" Date: Mon, 20 Apr 2026 21:38:41 +0300 Subject: [PATCH] fix(gateway): keep engine threads out of chat sidebar (#2751) * fix(gateway): keep engine threads out of chat sidebar * fix: address review findings (iteration 1) * fix: address review findings (iteration 2) --- .../static/js/core/bootstrap.js | 1 + .../static/js/core/history.js | 19 +- src/channels/web/features/chat/mod.rs | 167 ++++++++++++------ src/channels/web/types.rs | 4 + .../scenarios/test_v2_thread_visibility.py | 84 ++++----- 5 files changed, 180 insertions(+), 95 deletions(-) diff --git a/crates/ironclaw_gateway/static/js/core/bootstrap.js b/crates/ironclaw_gateway/static/js/core/bootstrap.js index 30911f6797..6abbf2aa6b 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 0f3a6ba507..db76215260 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/channels/web/features/chat/mod.rs b/src/channels/web/features/chat/mod.rs index 4194a03d48..f2a8934a78 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 f4ede474e7..72532c6d34 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 3be2792450..80fdd370f4 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 @@ class TestV2ThreadVisibility: ) 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 @@ class TestV2ThreadVisibility: """ 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 @@ class TestV2ThreadVisibility: 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 )