diff --git a/project-plans/issue222/plan.md b/project-plans/issue222/plan.md new file mode 100644 index 0000000..ea223d8 --- /dev/null +++ b/project-plans/issue222/plan.md @@ -0,0 +1,235 @@ +# Plan: Dedicated Stop control + mid-turn steering via Send + +Plan ID: PLAN-20260903-ISSUE222 +Generated: 2026-09-03 +Issue: #222 +Requirements: REQ-222-001 .. REQ-222-008 + +## Problem + +During an active turn the chat offers exactly one control. `render_send_stop_button` +(`src/ui_gpui/views/chat_view/render.rs:740`) renders a single button that reads +"Stop" while streaming and "Send" otherwise, and the Send branch bails out with +`"Send button ignored while stream is active"`. `handle_enter` +(`src/ui_gpui/views/chat_view/mod.rs:822`) returns early on +`StreamingState::Streaming`. The only way to redirect the agent is to kill the +turn and re-send, which discards the work the turn already produced. + +## Delivery boundary: what is buildable without a dependency change + +The issue names the injection point as "after tool results, before the next model +call". That point exists in the dependency, not here: +`serdes-ai-agent/src/stream.rs` pushes `tool_req` onto `messages` and `continue`s +the loop inside a `tokio::spawn`ed task. `AgentStream`'s public surface is +`new` / `new_with_cancel` / `cancel` / `Stream` — there is no input channel, and +`grep -rn "steer" serdes-ai-agent/src/` at the pinned rev `e675674` returns +nothing. + +Three shapes were considered: + +1. **Upstream queue in `AgentStream`.** Drain a steering handle where `tool_req` + is appended, in both `new` and `new_with_cancel`. Delivers at the tool + boundary as the issue describes. Requires a commit on `acoliver/serdesAI` and + a rev bump across all seven pinned `serdes-ai-*` entries in `Cargo.toml` — + a dependency change, which is held for explicit approval and is **not** part + of this plan. +2. **Drive `AgentRun::step()` locally.** Rejected: `step()` calls + `model.request()`, not `request_stream()`, so adopting it would trade token + streaming for steering. That is a regression against current behavior. +3. **Turn chaining, local only.** `AgentStreamEvent::RunComplete` carries the + full `messages: Vec` for the turn. When a turn ends with a + non-empty steering queue, start a follow-up `AgentStream` seeded with those + messages as `message_history` and the steering text as the prompt, inside the + same `run_stream_task`. No upstream change. + +This plan implements **shape 3**. It satisfies the issue's stated user value +("without losing the work it has already produced") and every Decision recorded +on 2026-08-31: no implicit aborts, text-only turns deliver at end of turn, +mid-thinking sends wait for a real boundary. It does **not** deliver between two +tool calls of a single turn; a steer typed during a long tool sequence lands when +that sequence finishes. Closing that gap is shape 1 and is deferred pending +approval of the dependency change. + +## Requirements + +### REQ-222-001: Stop is a dedicated control + +**Behavior** +- GIVEN a conversation whose stream is active +- WHEN the composer renders +- THEN a Stop control (`chat-stop-button`) and a Send control + (`chat-send-button`) are both present, as separate elements +- AND when the conversation is idle, only `chat-send-button` is present. + +Stop keeps its current behavior: emit `UserEvent::StopStreaming`, reset local +streaming state, refocus the composer. + +### REQ-222-002: Send stays active during streaming + +**Behavior** +- GIVEN an active stream and non-empty trimmed composer text +- WHEN the user clicks Send or presses Enter +- THEN `UserEvent::SteerStreaming { conversation_id, text }` is emitted +- AND `UserEvent::SendMessage` is **not** emitted +- AND the composer clears. + +Whitespace-only input remains a no-op while streaming and while idle. Idle Send +and idle Enter keep emitting `SendMessage` unchanged. + +### REQ-222-003: Queued steering messages are visible + +**Behavior** +- GIVEN a steering message has been submitted and not yet delivered +- WHEN the transcript renders +- THEN the text appears as a queued entry, distinct from a sent user message +- AND when the message is delivered it stops rendering as queued. + +### REQ-222-004: Active-turn precondition + +**Behavior** +- GIVEN a conversation with no stream in `StreamLifecycle::Running` +- WHEN `ChatService::steer(conversation_id, text)` is called +- THEN it returns `Err(ServiceError::Validation(..))` naming no active turn +- AND the queued entry is withdrawn from the transcript with a visible error. + +Steering reaches only the conversation that owns the running stream. A steer for +conversation A never lands on conversation B's turn. + +### REQ-222-005: Delivery at a turn boundary + +**Behavior** +- GIVEN a queued steering message and a running turn +- WHEN the turn reaches its boundary +- THEN the queued text is delivered to the model as a user message seeded with + the completed turn's message history +- AND the queue is drained in FIFO order +- AND delivery happens without a new user-initiated send. + +### REQ-222-006: No implicit abort + +**Behavior** +- GIVEN a queued steering message +- WHEN it is submitted or delivered +- THEN the in-flight generation is never cancelled on the user's behalf +- AND `chat_service.cancel` is reached only from `UserEvent::StopStreaming`. + +### REQ-222-007: Ordering survives reload + +**Behavior** +- GIVEN a turn that produced tool results, then received a steering message, + then produced assistant output +- WHEN the conversation is reloaded +- THEN the steering message is persisted as a user message ordered after the + preceding assistant output and before the assistant output it produced. + +### REQ-222-008: Approvals are not raced + +**Behavior** +- GIVEN a pending tool approval for a conversation +- WHEN a steering message arrives for that conversation +- THEN the approval is neither resolved nor cancelled by the steer +- AND the steering message is delivered only after the turn passes the + approval and reaches its boundary. + +## Bounded queue + +Queue depth is capped per conversation at `MAX_QUEUED_STEERING_MESSAGES = 5`. +A steer submitted against a full queue is rejected with +`ServiceError::Validation`, surfaced the same way as REQ-222-004. Unlimited +depth is rejected: an unbounded queue lets a user stack arbitrary instructions +that all flush at one boundary, which is the unpredictable-flush failure mode +reported against Claude Code (anthropics/claude-code#49373). + +The steering text is delivered as a plain user message with no wrapper or +framing text. + +## Out of scope + +- Whether Stop preserves partial output (issue #218). +- The Responses websocket transport (#217). +- Mid-turn delivery between two tool calls (shape 1 above; needs the dependency + change). + +## Phases + +### P01 — Events and service contract (test-first) + +Files: `src/events/types.rs`, `src/services/chat.rs`, `src/services/chat_impl.rs` + +- Add `UserEvent::SteerStreaming { conversation_id: Uuid, text: String }`. +- Add `ChatEvent::SteeringQueued { conversation_id, steer_id, text }` and + `ChatEvent::SteeringDelivered { conversation_id, steer_id }`. +- Add `ChatService::steer(&self, conversation_id: Uuid, text: String) -> + ServiceResult` returning the steer id. +- `ChatServiceImpl` gains a per-conversation FIFO queue keyed the same way as + `active_streams`, guarded by the existing `StreamLifecycle::Running` check. + +Tests (behavioral, `src/services/chat_impl/tests/`): +- steer with no active stream → `Err`, queue unchanged (REQ-222-004) +- steer with a running stream → `Ok`, entry queued, `SteeringQueued` emitted +- steer for conversation A while only B is running → `Err` (REQ-222-004) +- sixth steer against a full queue → `Err`, queue still holds five +- steer never calls `cancel` (REQ-222-006) + +### P02 — Delivery at the turn boundary (test-first) + +Files: `src/llm/client_agent.rs`, `src/services/chat_impl/streaming.rs` + +- `do_run_agent_stream` returns the turn's final `Vec` from + `RunComplete` instead of discarding it. +- `run_stream_task` loops: after `stream_agent_response` returns, drain the + conversation's steering queue; for each drained message persist it as a user + message, emit `SteeringDelivered`, and run another `AgentStream` seeded with + the accumulated history. The loop exits when the queue is empty, the + cancellation token fires, or the turn errored. +- The existing single-turn finalization runs once, on the last turn. + +Tests: +- a queued steer produces a second model request carrying the steer text + (REQ-222-005) +- FIFO order across two queued steers +- an empty queue leaves behavior byte-identical to today (regression) +- a cancelled turn drains nothing and starts no follow-up (REQ-222-006) +- a steer queued while an approval is pending is delivered after the decision + (REQ-222-008) +- the steering user message persists between the preceding assistant output and + the following one (REQ-222-007) + +### P03 — Presenter wiring (test-first) + +Files: `src/presentation/chat_presenter.rs`, `src/presentation/chat_presenter_event.rs` + +- `handle_steer_streaming` calls `chat_service.steer`; on `Err` it sends + `ViewCommand::SteeringRejected { conversation_id, error }` and a `ShowError`, + on `Ok` nothing further (the service emits `SteeringQueued`). `SteeringRejected` + carries no `steer_id`: `steer` returns `ServiceResult`, so a refusal + yields an error and no id, and a refused steer was never queued under one. +- Route `ChatEvent::SteeringQueued` / `SteeringDelivered` to + `ViewCommand::SteeringQueued` / `SteeringDelivered`. + +Tests (`src/presentation/chat_presenter_tests.rs`): each branch asserts the +view commands emitted, using the existing mock `ChatService`. + +### P04 — UI controls and queued rendering (test-first) + +Files: `src/ui_gpui/views/chat_view/render.rs`, `mod.rs`, `state.rs`, +`transcript.rs` + +- Split `render_send_stop_button` into `render_stop_button` and + `render_send_button`; the composer row renders Stop only while streaming and + Send always, so both are present mid-stream (REQ-222-001). +- The Send handler branches on `StreamingState::Streaming` to emit + `SteerStreaming` instead of returning early (REQ-222-002). +- `handle_enter` drops its streaming early-return and takes the same branch. +- `ChatViewState` gains `queued_steering: Vec`; + `SteeringDelivered` and `SteeringRejected` remove by id (REQ-222-003). +- The transcript renders queued entries after the live stream block, styled + distinctly from a sent user message. + +Tests (`mod_tests.rs`, `render_bars_tests.rs`): button presence per state, +emitted event per state and per input, queued-entry lifecycle add/deliver/reject. + +### P05 — Verification + +`cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, +`cargo test --lib --tests`, structural checks, lizard complexity. diff --git a/src/events/types.rs b/src/events/types.rs index e962f67..7a84d0c 100644 --- a/src/events/types.rs +++ b/src/events/types.rs @@ -62,6 +62,15 @@ pub enum UserEvent { /// @requirement REQ-173-002.3 StopStreaming { conversation_id: Uuid }, + /// User submitted a steering message while a turn is active. + /// + /// Carries the composer text verbatim; the service trims it and decides + /// whether the conversation has a turn that can accept it. + /// + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-002 + SteerStreaming { conversation_id: Uuid, text: String }, + /// User clicked new conversation NewConversation, @@ -452,6 +461,42 @@ pub enum ChatEvent { conversation_id: Uuid, message_id: Uuid, }, + + /// A steering message was accepted for a conversation's active turn and is + /// waiting for the next turn boundary. + /// + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-003 + SteeringQueued { + conversation_id: Uuid, + steer_id: Uuid, + text: String, + }, + + /// A queued steering message left the queue and was handed to the model. + /// + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-005 + SteeringDelivered { + conversation_id: Uuid, + steer_id: Uuid, + }, + + /// A queued steering message will never be handed to the model: the turn + /// it was waiting on ended first. + /// + /// This is the terminal state of a queued entry that is not delivered. + /// A steer is accepted, announced with `SteeringQueued`, and then either + /// reaches the model (`SteeringDelivered`) or does not (this). Without + /// the second terminal state the view has no way to learn that the entry + /// it is rendering as waiting is never going to resolve. + /// + /// @plan PLAN-20260903-ISSUE222.P06 + /// @requirement REQ-222-003 + SteeringDiscarded { + conversation_id: Uuid, + steer_id: Uuid, + }, } /// MCP server lifecycle events diff --git a/src/presentation/chat_presenter.rs b/src/presentation/chat_presenter.rs index e82ffbb..2fadeef 100644 --- a/src/presentation/chat_presenter.rs +++ b/src/presentation/chat_presenter.rs @@ -570,6 +570,52 @@ impl ChatPresenter { // StreamCancelled event will be emitted by the service } + /// Handle `SteerStreaming` user event + /// + /// Offers the composer text to the service and reports only a refusal. An + /// accepted steer is announced by `ChatEvent::SteeringQueued`, which + /// reaches the view through `handle_chat_event`, so queueing it here too + /// would show the same entry twice. + /// + /// Steering is additive: this never cancels the turn it steers, which + /// leaves `StopStreaming` as the only path to `ChatService::cancel`. + /// + /// A refusal carries the submitted text back. The composer cleared when + /// the user hit Send, so this handler holds the only copy left of what + /// they typed, and a refusal that dropped it would destroy the message + /// it is reporting on. + /// + /// @plan PLAN-20260903-ISSUE222.P03 + /// @plan PLAN-20260903-ISSUE222.P08 + /// @requirement REQ-222-002 + /// @requirement REQ-222-004 + /// @requirement REQ-222-006 + pub(super) async fn handle_steer_streaming( + chat_service: &Arc, + view_tx: &mut mpsc::Sender, + conversation_id: Uuid, + text: String, + ) { + if let Err(e) = chat_service.steer(conversation_id, text.clone()).await { + let error_msg = e.to_string(); + tracing::warn!(%conversation_id, "Steering message rejected: {}", error_msg); + let _ = view_tx + .send(ViewCommand::SteeringRejected { + conversation_id, + error: error_msg.clone(), + text, + }) + .await; + let _ = view_tx + .send(ViewCommand::ShowError { + title: "Steering Error".to_string(), + message: error_msg, + severity: ErrorSeverity::Warning, + }) + .await; + } + } + /// Handle `SelectChatProfile` user event. /// /// When the user picks a different profile in the chat title-bar dropdown, diff --git a/src/presentation/chat_presenter_cancel_tests.rs b/src/presentation/chat_presenter_cancel_tests.rs index 914c0b4..2c0b3f9 100644 --- a/src/presentation/chat_presenter_cancel_tests.rs +++ b/src/presentation/chat_presenter_cancel_tests.rs @@ -10,21 +10,52 @@ use std::sync::{Arc, Mutex}; use tokio::sync::mpsc; use uuid::Uuid; -/// Mock `ChatService` that records cancel calls for verification -struct RecordingChatService { +/// Mock `ChatService` that records cancel and steer calls for verification +pub(super) struct RecordingChatService { cancelled_conversations: Arc>>, + /// `(conversation_id, text)` for every `steer` call, in order. + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-006 + steered: Arc>>, + /// When set, `steer` records the call and then refuses it with this text, + /// standing in for a conversation with no running turn or a full queue. + /// @plan PLAN-20260903-ISSUE222.P03 + /// @requirement REQ-222-004 + steer_error: Option, } impl RecordingChatService { - fn new() -> Self { + pub(super) fn new() -> Self { Self { cancelled_conversations: Arc::new(Mutex::new(Vec::new())), + steered: Arc::new(Mutex::new(Vec::new())), + steer_error: None, } } - fn cancelled_ids(&self) -> Vec { + /// A double whose `steer` records the call and then refuses it with + /// `error`, the way the service refuses a steer with no turn to join. + /// + /// @plan PLAN-20260903-ISSUE222.P03 + /// @requirement REQ-222-004 + pub(super) fn rejecting(error: &str) -> Self { + Self { + steer_error: Some(error.to_string()), + ..Self::new() + } + } + + pub(super) fn cancelled_ids(&self) -> Vec { self.cancelled_conversations.lock().unwrap().clone() } + + /// Every `steer` call this double received, in order. + /// + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-006 + pub(super) fn steer_calls(&self) -> Vec<(Uuid, String)> { + self.steered.lock().unwrap().clone() + } } #[async_trait::async_trait] @@ -56,6 +87,20 @@ impl ChatService for RecordingChatService { false } + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-006 + async fn steer( + &self, + conversation_id: Uuid, + text: String, + ) -> Result { + self.steered.lock().unwrap().push((conversation_id, text)); + if let Some(error) = &self.steer_error { + return Err(crate::services::ServiceError::Validation(error.clone())); + } + Ok(Uuid::new_v4()) + } + async fn resolve_tool_approval( &self, _request_id: String, @@ -110,6 +155,15 @@ async fn handle_stop_streaming_forwards_conversation_id() { cancelled[0], conversation_id_a, "Expected cancel to be called with conversation_id A" ); + + // Stop and steer are separate controls: stopping a turn must not queue a + // steering message. + // @plan PLAN-20260903-ISSUE222.P01 + // @requirement REQ-222-006 + assert!( + chat_service.steer_calls().is_empty(), + "StopStreaming must not reach ChatService::steer" + ); } /// Test that `StopStreaming` for conversation A does NOT cancel conversation B. @@ -160,7 +214,7 @@ async fn handle_stop_streaming_does_not_cancel_other_conversations() { } /// Mock `AppSettingsService` for testing -struct MockAppSettingsService; +pub(super) struct MockAppSettingsService; #[async_trait::async_trait] impl AppSettingsService for MockAppSettingsService { diff --git a/src/presentation/chat_presenter_event.rs b/src/presentation/chat_presenter_event.rs index 1718b77..86a88e2 100644 --- a/src/presentation/chat_presenter_event.rs +++ b/src/presentation/chat_presenter_event.rs @@ -29,6 +29,15 @@ impl ChatPresenter { UserEvent::StopStreaming { conversation_id } => { Self::handle_stop_streaming(deps.chat_service, view_tx, conversation_id).await; } + // @plan PLAN-20260903-ISSUE222.P03 + // @requirement REQ-222-002 + UserEvent::SteerStreaming { + conversation_id, + text, + } => { + Self::handle_steer_streaming(deps.chat_service, view_tx, conversation_id, text) + .await; + } UserEvent::NewConversation => { Self::handle_new_conversation_for_event(deps, state, view_tx).await; } diff --git a/src/presentation/chat_presenter_handlers.rs b/src/presentation/chat_presenter_handlers.rs index 81f5334..218072d 100644 --- a/src/presentation/chat_presenter_handlers.rs +++ b/src/presentation/chat_presenter_handlers.rs @@ -85,9 +85,84 @@ impl ChatPresenter { } => { Self::handle_message_saved(view_tx, conversation_id).await; } + ChatEvent::SteeringQueued { + conversation_id, + steer_id, + text, + } => { + Self::handle_steering_queued(view_tx, conversation_id, steer_id, text).await; + } + ChatEvent::SteeringDelivered { + conversation_id, + steer_id, + } => { + Self::handle_steering_delivered(view_tx, conversation_id, steer_id).await; + } + ChatEvent::SteeringDiscarded { + conversation_id, + steer_id, + } => { + Self::handle_steering_discarded(view_tx, conversation_id, steer_id).await; + } } } + /// A steering message is waiting for the running turn's boundary. + /// + /// @plan PLAN-20260903-ISSUE222.P03 + /// @requirement REQ-222-003 + async fn handle_steering_queued( + view_tx: &mut mpsc::Sender, + conversation_id: Uuid, + steer_id: Uuid, + text: String, + ) { + let _ = view_tx + .send(ViewCommand::SteeringQueued { + conversation_id, + steer_id, + text, + }) + .await; + } + + /// A queued steering message reached the model, so it is no longer waiting. + /// + /// @plan PLAN-20260903-ISSUE222.P03 + /// @requirement REQ-222-003 + async fn handle_steering_delivered( + view_tx: &mut mpsc::Sender, + conversation_id: Uuid, + steer_id: Uuid, + ) { + let _ = view_tx + .send(ViewCommand::SteeringDelivered { + conversation_id, + steer_id, + }) + .await; + } + + /// A queued steering message will never reach the model, because the turn + /// it was waiting on ended first. Same id, same withdrawal as delivery: + /// the view has no other way to learn that the entry it is rendering as + /// waiting is never going to resolve. + /// + /// @plan PLAN-20260903-ISSUE222.P06 + /// @requirement REQ-222-003 + async fn handle_steering_discarded( + view_tx: &mut mpsc::Sender, + conversation_id: Uuid, + steer_id: Uuid, + ) { + let _ = view_tx + .send(ViewCommand::SteeringDiscarded { + conversation_id, + steer_id, + }) + .await; + } + async fn handle_stream_started( view_tx: &mut mpsc::Sender, conversation_id: Uuid, diff --git a/src/presentation/chat_presenter_steering_tests.rs b/src/presentation/chat_presenter_steering_tests.rs new file mode 100644 index 0000000..cf2a6b9 --- /dev/null +++ b/src/presentation/chat_presenter_steering_tests.rs @@ -0,0 +1,351 @@ +// ============================================================================ +// Steering presenter wiring (issue #222). +// +// The presenter sits on both halves of the steering path: the user event that +// offers a steer to the service, and the service events that say what became +// of it. These cover each half against the doubles the cancel tests already +// established, so a steer that is accepted, refused, queued or delivered is +// asserted through the same router the application runs. +// +// @plan PLAN-20260903-ISSUE222.P03 +// ============================================================================ + +use super::cancel_tests::{MockAppSettingsService, RecordingChatService}; +use super::*; +use crate::events::types::UserEvent; + +/// Every `ViewCommand` already queued for the view, in order. +fn drain_view_commands(view_rx: &mut mpsc::Receiver) -> Vec { + let mut commands = Vec::new(); + while let Ok(command) = view_rx.try_recv() { + commands.push(command); + } + commands +} + +/// Route `event` through the real user-event dispatcher, so these tests fail +/// if the `SteerStreaming` arm is missing as well as if the handler is wrong. +async fn dispatch_user_event( + chat_service: &Arc, + view_tx: &mpsc::Sender, + event: UserEvent, +) { + let conversation_service = Arc::new(MockConversationService) as Arc; + let profile_service = Arc::new(MockProfileService) as Arc; + let app_settings_service = Arc::new(MockAppSettingsService) as Arc; + let current_export_format = Arc::new(std::sync::Mutex::new( + crate::models::ConversationExportFormat::Md, + )); + let pending_draft_conversation_id = Arc::new(std::sync::Mutex::new(None)); + + let deps = ChatPresenterDeps { + conversation_service: &conversation_service, + chat_service, + profile_service: &profile_service, + }; + let state = ChatPresenterState { + app_settings_service: &app_settings_service, + current_export_format: ¤t_export_format, + pending_draft_conversation_id: &pending_draft_conversation_id, + }; + ChatPresenter::handle_user_event(&deps, &state, &mut view_tx.clone(), event).await; +} + +/// An accepted steer reaches the service carrying the conversation and text it +/// was typed for, and tells the view nothing: the queued entry is announced by +/// `ChatEvent::SteeringQueued`, so reporting it here would show it twice. +/// +/// @plan PLAN-20260903-ISSUE222.P03 +/// @requirement REQ-222-002 +#[tokio::test] +async fn steer_streaming_reaches_the_service_and_reports_no_rejection() { + let chat_service = Arc::new(RecordingChatService::new()); + let (view_tx, mut view_rx) = mpsc::channel::(100); + let conversation_id = Uuid::new_v4(); + + dispatch_user_event( + &(chat_service.clone() as Arc), + &view_tx, + UserEvent::SteerStreaming { + conversation_id, + text: "read the failing test first".to_string(), + }, + ) + .await; + + assert_eq!( + chat_service.steer_calls(), + vec![(conversation_id, "read the failing test first".to_string())], + "SteerStreaming must reach ChatService::steer with its own conversation and text" + ); + + let commands = drain_view_commands(&mut view_rx); + assert!( + !commands + .iter() + .any(|command| matches!(command, ViewCommand::SteeringRejected { .. })), + "an accepted steer must not be reported as rejected, got {commands:?}" + ); + assert!( + !commands + .iter() + .any(|command| matches!(command, ViewCommand::SteeringQueued { .. })), + "the queued entry is announced by ChatEvent::SteeringQueued, not by the send path, \ + got {commands:?}" + ); +} + +/// A refused steer withdraws nothing and shows the service's own words, so a +/// user who steers a finished turn learns why it did not take. +/// +/// @plan PLAN-20260903-ISSUE222.P03 +/// @requirement REQ-222-004 +#[tokio::test] +async fn steer_streaming_rejection_surfaces_the_service_error() { + let chat_service = Arc::new(RecordingChatService::rejecting("No active turn to steer")); + let (view_tx, mut view_rx) = mpsc::channel::(100); + let conversation_id = Uuid::new_v4(); + + dispatch_user_event( + &(chat_service.clone() as Arc), + &view_tx, + UserEvent::SteerStreaming { + conversation_id, + text: "too late".to_string(), + }, + ) + .await; + + let commands = drain_view_commands(&mut view_rx); + + let Some(rejected_at) = commands + .iter() + .position(|command| matches!(command, ViewCommand::SteeringRejected { .. })) + else { + panic!("a refused steer must emit SteeringRejected, got {commands:?}"); + }; + let ViewCommand::SteeringRejected { + conversation_id: rejected_conversation_id, + error, + text: _, + } = &commands[rejected_at] + else { + unreachable!("position matched SteeringRejected"); + }; + assert_eq!( + *rejected_conversation_id, conversation_id, + "the rejection must name the conversation the steer was typed for" + ); + assert!( + error.contains("No active turn to steer"), + "the rejection must carry the service's reason, got {error:?}" + ); + + let Some(error_at) = commands + .iter() + .position(|command| matches!(command, ViewCommand::ShowError { .. })) + else { + panic!("a refused steer must also surface a ShowError, got {commands:?}"); + }; + let ViewCommand::ShowError { + title, + message, + severity, + } = &commands[error_at] + else { + unreachable!("position matched ShowError"); + }; + assert_eq!(title, "Steering Error"); + assert_eq!( + message, error, + "the shown error must be the same text the rejection carried" + ); + assert_eq!( + *severity, + ErrorSeverity::Warning, + "a refused steer costs the user nothing already sent, so it is a warning" + ); + assert!( + rejected_at < error_at, + "the queued entry is withdrawn before the error explains why, got {commands:?}" + ); +} + +/// A refusal hands the submitted text back to the view. +/// +/// The composer clears on submit rather than on acceptance, so by the time +/// the service refuses, this handler holds the only copy of what the user +/// typed. Reporting the refusal without it would destroy the message the +/// refusal is about. +/// +/// @plan PLAN-20260903-ISSUE222.P08 +/// @requirement REQ-222-002 +#[tokio::test] +async fn steer_streaming_rejection_carries_the_submitted_text() { + let chat_service = Arc::new(RecordingChatService::rejecting( + "Steering queue is full (5 messages already waiting)", + )); + let (view_tx, mut view_rx) = mpsc::channel::(100); + let conversation_id = Uuid::new_v4(); + + dispatch_user_event( + &(chat_service.clone() as Arc), + &view_tx, + UserEvent::SteerStreaming { + conversation_id, + text: "and check the migration too".to_string(), + }, + ) + .await; + + assert_eq!( + chat_service.steer_calls(), + vec![(conversation_id, "and check the migration too".to_string())], + "the text on the rejection must be the text the service was offered" + ); + + let commands = drain_view_commands(&mut view_rx); + let rejected_text = commands.iter().find_map(|command| match command { + ViewCommand::SteeringRejected { text, .. } => Some(text.clone()), + _ => None, + }); + assert_eq!( + rejected_text, + Some("and check the migration too".to_string()), + "a refused steer must come back with its text, or the composer cannot restore it, \ + got {commands:?}" + ); +} + +/// Steering is additive in both directions: neither an accepted nor a refused +/// steer may reach `cancel`, which stays reachable only from an explicit stop. +/// +/// @plan PLAN-20260903-ISSUE222.P03 +/// @requirement REQ-222-006 +#[tokio::test] +async fn steer_streaming_never_cancels_the_turn() { + for chat_service in [ + Arc::new(RecordingChatService::new()), + Arc::new(RecordingChatService::rejecting("Steering queue is full")), + ] { + let (view_tx, _view_rx) = mpsc::channel::(100); + let conversation_id = Uuid::new_v4(); + + dispatch_user_event( + &(chat_service.clone() as Arc), + &view_tx, + UserEvent::SteerStreaming { + conversation_id, + text: "keep going, but check the config".to_string(), + }, + ) + .await; + + assert_eq!( + chat_service.steer_calls().len(), + 1, + "the steer must have been offered to the service" + ); + assert!( + chat_service.cancelled_ids().is_empty(), + "SteerStreaming must never cancel a turn, cancelled {:?}", + chat_service.cancelled_ids() + ); + } +} + +/// The service's report that a steer is waiting becomes the view's queued +/// entry, with the id the view needs to withdraw it later. +/// +/// @plan PLAN-20260903-ISSUE222.P03 +/// @requirement REQ-222-003 +#[tokio::test] +async fn steering_queued_event_becomes_a_queued_view_command() { + let (view_tx, mut view_rx) = mpsc::channel::(100); + let conversation_id = Uuid::new_v4(); + let steer_id = Uuid::new_v4(); + + ChatPresenter::handle_chat_event( + &mut view_tx.clone(), + ChatEvent::SteeringQueued { + conversation_id, + steer_id, + text: "use the cached index".to_string(), + }, + ) + .await; + + let commands = drain_view_commands(&mut view_rx); + assert_eq!( + commands, + vec![ViewCommand::SteeringQueued { + conversation_id, + steer_id, + text: "use the cached index".to_string(), + }], + "SteeringQueued must forward every field to the view unchanged" + ); +} + +/// Delivery is reported with the same id the queued entry carried, which is +/// what lets the view stop rendering that entry as waiting. +/// +/// @plan PLAN-20260903-ISSUE222.P03 +/// @requirement REQ-222-003 +#[tokio::test] +async fn steering_delivered_event_becomes_a_delivered_view_command() { + let (view_tx, mut view_rx) = mpsc::channel::(100); + let conversation_id = Uuid::new_v4(); + let steer_id = Uuid::new_v4(); + + ChatPresenter::handle_chat_event( + &mut view_tx.clone(), + ChatEvent::SteeringDelivered { + conversation_id, + steer_id, + }, + ) + .await; + + let commands = drain_view_commands(&mut view_rx); + assert_eq!( + commands, + vec![ViewCommand::SteeringDelivered { + conversation_id, + steer_id, + }], + "SteeringDelivered must carry the queued entry's own id" + ); +} + +/// A steer whose turn ended before it could be delivered is reported with the +/// same id the queued entry carried, which is what lets the view stop +/// rendering an entry nobody will ever deliver. +/// +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-003 +#[tokio::test] +async fn steering_discarded_event_becomes_a_discarded_view_command() { + let (view_tx, mut view_rx) = mpsc::channel::(100); + let conversation_id = Uuid::new_v4(); + let steer_id = Uuid::new_v4(); + + ChatPresenter::handle_chat_event( + &mut view_tx.clone(), + ChatEvent::SteeringDiscarded { + conversation_id, + steer_id, + }, + ) + .await; + + let commands = drain_view_commands(&mut view_rx); + assert_eq!( + commands, + vec![ViewCommand::SteeringDiscarded { + conversation_id, + steer_id, + }], + "SteeringDiscarded must forward every field to the view unchanged" + ); +} diff --git a/src/presentation/chat_presenter_tests.rs b/src/presentation/chat_presenter_tests.rs index 101f2f5..8b2cfba 100644 --- a/src/presentation/chat_presenter_tests.rs +++ b/src/presentation/chat_presenter_tests.rs @@ -147,6 +147,19 @@ impl ChatService for MockChatService { false } + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-004 + async fn steer( + &self, + _conversation_id: Uuid, + _text: String, + ) -> Result { + // Mock has no active turn to steer. + Err(crate::services::ServiceError::Validation( + "No active turn to steer".to_string(), + )) + } + async fn resolve_tool_approval( &self, _request_id: String, @@ -890,3 +903,9 @@ fn test_conversation_search_result_fields() { #[path = "chat_presenter_cancel_tests.rs"] mod cancel_tests; + +/// Presenter wiring for mid-turn steering. +/// +/// @plan PLAN-20260903-ISSUE222.P03 +#[path = "chat_presenter_steering_tests.rs"] +mod steering_tests; diff --git a/src/presentation/view_command.rs b/src/presentation/view_command.rs index 79f900c..7f418c8 100644 --- a/src/presentation/view_command.rs +++ b/src/presentation/view_command.rs @@ -112,6 +112,59 @@ pub enum ViewCommand { /// Message was saved MessageSaved { conversation_id: Uuid }, + /// A steering message was accepted and is waiting for the running turn to + /// reach its boundary. The view shows `text` as queued until the matching + /// `SteeringDelivered` or `SteeringRejected` arrives for `steer_id`. + /// + /// @plan PLAN-20260903-ISSUE222.P03 + /// @requirement REQ-222-003 + SteeringQueued { + conversation_id: Uuid, + steer_id: Uuid, + text: String, + }, + + /// A queued steering message was handed to the model, so the view stops + /// showing `steer_id` as waiting. + /// + /// @plan PLAN-20260903-ISSUE222.P03 + /// @requirement REQ-222-003 + SteeringDelivered { + conversation_id: Uuid, + steer_id: Uuid, + }, + + /// A queued steering message will never be handed to the model, because + /// the turn it was waiting on ended first. The view stops showing + /// `steer_id` as waiting, the same withdrawal `SteeringDelivered` + /// performs: an entry nobody will ever deliver must not wait forever. + /// + /// @plan PLAN-20260903-ISSUE222.P06 + /// @requirement REQ-222-003 + SteeringDiscarded { + conversation_id: Uuid, + steer_id: Uuid, + }, + + /// A steering message was refused: no turn was running, the queue was + /// full, or the text was blank. The service queued nothing, so no + /// `SteeringQueued` follows for it, and `error` says why it did not take. + /// + /// `text` is what the user submitted. The composer clears on submit + /// rather than waiting for an answer, so a refusal is the only thing + /// that can give those words back, and nothing else on this path still + /// holds them. + /// + /// @plan PLAN-20260903-ISSUE222.P03 + /// @plan PLAN-20260903-ISSUE222.P08 + /// @requirement REQ-222-002 + /// @requirement REQ-222-004 + SteeringRejected { + conversation_id: Uuid, + error: String, + text: String, + }, + /// Toggle thinking visibility ToggleThinkingVisibility, diff --git a/src/services/chat.rs b/src/services/chat.rs index 86a0977..70d7370 100644 --- a/src/services/chat.rs +++ b/src/services/chat.rs @@ -39,6 +39,21 @@ pub trait ChatService: Send + Sync { /// Is this conversation streaming? @plan PLAN-20260416-ISSUE173.P03 @requirement REQ-173-001.1 fn is_streaming_for(&self, conversation_id: Uuid) -> bool; + /// Queue a steering message for a conversation's active turn. + /// + /// Returns the id of the queued steering message. The in-flight + /// generation is never cancelled on the caller's behalf. + /// + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-004 + /// @requirement REQ-222-006 + /// + /// # Errors + /// Returns `ServiceError::Validation` when the conversation has no turn in + /// `StreamLifecycle::Running`, or when the conversation's steering queue is + /// full. + async fn steer(&self, conversation_id: Uuid, text: String) -> ServiceResult; + /// Resolve a pending tool approval request from user interaction. async fn resolve_tool_approval( &self, @@ -50,79 +65,4 @@ pub trait ChatService: Send + Sync { // Note: ChatServiceImpl is implemented in chat_impl.rs #[cfg(test)] -mod tests { - use super::*; - use futures::StreamExt; - - #[tokio::test] - async fn test_stream_event_token() { - let event = ChatStreamEvent::Token("test token".to_string()); - assert!(matches!(event, ChatStreamEvent::Token(ref s) if s == "test token")); - } - - #[tokio::test] - async fn test_stream_event_complete() { - assert!(matches!( - ChatStreamEvent::Complete { - input_tokens: None, - output_tokens: None, - }, - ChatStreamEvent::Complete { .. } - )); - } - - #[tokio::test] - async fn test_stream_event_error() { - let event = ChatStreamEvent::Error(ServiceError::NotFound("test error".to_string())); - assert!( - matches!(event, ChatStreamEvent::Error(ref e) if e.to_string().contains("test error")) - ); - } - - #[tokio::test] - async fn test_service_error_not_found() { - let error = ServiceError::NotFound("Conversation not found".to_string()); - assert!(error.to_string().contains("Not found")); - } - - #[tokio::test] - async fn test_service_error_validation() { - let error = ServiceError::Validation("Invalid input".to_string()); - assert!(error.to_string().contains("Validation error")); - } - - #[tokio::test] - async fn test_service_result_type() { - let ok: ServiceResult + Send + Unpin>> = - Ok(Box::new(futures::stream::empty())); - assert!(ok.is_ok()); - let err: ServiceResult + Send + Unpin>> = - Err(ServiceError::NotFound("test".to_string())); - assert!(err.is_err()); - } - - #[tokio::test] - async fn test_stream_type() { - let mut stream: Box + Send + Unpin> = - Box::new(futures::stream::iter(vec![ - ChatStreamEvent::Token("test".to_string()), - ChatStreamEvent::Complete { - input_tokens: None, - output_tokens: None, - }, - ])); - assert!(matches!( - stream.next().await, - Some(ChatStreamEvent::Token(_)) - )); - } - - #[tokio::test] - async fn test_stream_event_traits() { - let event1 = ChatStreamEvent::Token("test".to_string()); - let event2 = event1.clone(); - assert!(matches!(event1, ChatStreamEvent::Token(_))); - assert!(matches!(event2, ChatStreamEvent::Token(_))); - assert!(format!("{event1:?}").contains("Token")); - } -} +mod tests; diff --git a/src/services/chat/tests.rs b/src/services/chat/tests.rs new file mode 100644 index 0000000..66d7a39 --- /dev/null +++ b/src/services/chat/tests.rs @@ -0,0 +1,78 @@ +//! Trait-level tests for the chat service contract. +//! +//! Split out of `chat.rs` so the trait declaration is the whole of that +//! file, which keeps the structural length check reading the trait rather +//! than the trait plus its tests. + +use super::*; +use futures::StreamExt; + +#[tokio::test] +async fn test_stream_event_token() { + let event = ChatStreamEvent::Token("test token".to_string()); + assert!(matches!(event, ChatStreamEvent::Token(ref s) if s == "test token")); +} + +#[tokio::test] +async fn test_stream_event_complete() { + assert!(matches!( + ChatStreamEvent::Complete { + input_tokens: None, + output_tokens: None, + }, + ChatStreamEvent::Complete { .. } + )); +} + +#[tokio::test] +async fn test_stream_event_error() { + let event = ChatStreamEvent::Error(ServiceError::NotFound("test error".to_string())); + assert!(matches!(event, ChatStreamEvent::Error(ref e) if e.to_string().contains("test error"))); +} + +#[tokio::test] +async fn test_service_error_not_found() { + let error = ServiceError::NotFound("Conversation not found".to_string()); + assert!(error.to_string().contains("Not found")); +} + +#[tokio::test] +async fn test_service_error_validation() { + let error = ServiceError::Validation("Invalid input".to_string()); + assert!(error.to_string().contains("Validation error")); +} + +#[tokio::test] +async fn test_service_result_type() { + let ok: ServiceResult + Send + Unpin>> = + Ok(Box::new(futures::stream::empty())); + assert!(ok.is_ok()); + let err: ServiceResult + Send + Unpin>> = + Err(ServiceError::NotFound("test".to_string())); + assert!(err.is_err()); +} + +#[tokio::test] +async fn test_stream_type() { + let mut stream: Box + Send + Unpin> = + Box::new(futures::stream::iter(vec![ + ChatStreamEvent::Token("test".to_string()), + ChatStreamEvent::Complete { + input_tokens: None, + output_tokens: None, + }, + ])); + assert!(matches!( + stream.next().await, + Some(ChatStreamEvent::Token(_)) + )); +} + +#[tokio::test] +async fn test_stream_event_traits() { + let event1 = ChatStreamEvent::Token("test".to_string()); + let event2 = event1.clone(); + assert!(matches!(event1, ChatStreamEvent::Token(_))); + assert!(matches!(event2, ChatStreamEvent::Token(_))); + assert!(format!("{event1:?}").contains("Token")); +} diff --git a/src/services/chat_impl.rs b/src/services/chat_impl.rs index 346cb17..37e7f68 100644 --- a/src/services/chat_impl.rs +++ b/src/services/chat_impl.rs @@ -27,16 +27,21 @@ use tokio_util::sync::CancellationToken; use uuid::Uuid; mod prompt; +mod steering; mod streaming; mod titling; use prompt::{build_system_prompt, filter_emoji_setting}; +use steering::{drain_steering_queue, emit_steering_discarded, QueuedSteering, SteeringQueues}; use streaming::{ build_stream_error_diagnostics, clear_streaming_state, emit_stream_error, run_stream_task, StreamDiagnosticContext, StreamTranscript, STREAM_ERROR_MESSAGE, }; use titling::{generate_and_apply_title, TitleGenerationRequest}; +#[cfg(test)] +use steering::MAX_QUEUED_STEERING_MESSAGES; + // Re-export for tests #[cfg(test)] use streaming::persist_context_state; @@ -79,6 +84,10 @@ pub struct ChatServiceImpl { /// @plan PLAN-20260416-ISSUE173.P03 /// @requirement REQ-173-001.1 active_streams: Arc>>, + /// Steering messages queued per conversation while its turn runs. + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-004 + steering_queues: SteeringQueues, /// Channel for sending view commands (used for tool approval UI) view_tx: tokio::sync::mpsc::Sender, /// Approval gate for coordinating user approval of tool execution @@ -110,6 +119,9 @@ impl ChatServiceImpl { // @plan PLAN-20260416-ISSUE173.P03 // @requirement REQ-173-001.1 active_streams: Arc::new(StdMutex::new(HashMap::new())), + // @plan PLAN-20260903-ISSUE222.P01 + // @requirement REQ-222-004 + steering_queues: Arc::new(StdMutex::new(HashMap::new())), view_tx, approval_gate, policy, @@ -318,6 +330,27 @@ impl ChatServiceImpl { } } + // A cancelled turn never reaches a delivery boundary, so anything it + // had queued is dropped rather than left to leak into the next turn. + // The view is rendering each of those entries as waiting, so each one + // is announced: a stopped turn is the terminal state of everything it + // was holding. + // The `active_streams` guard above is already released here, and + // `drain_steering` releases `steering_queues` before this returns. + // @plan PLAN-20260903-ISSUE222.P01 + // @plan PLAN-20260903-ISSUE222.P06 + // @requirement REQ-222-003 + // @requirement REQ-222-004 + let discarded = self.drain_steering(conversation_id); + if !discarded.is_empty() { + tracing::debug!( + conversation_id = %conversation_id, + discarded = discarded.len(), + "Discarded queued steering messages for a cancelled turn" + ); + } + emit_steering_discarded(conversation_id, &discarded); + // @plan PLAN-20260416-ISSUE173.P07 // @requirement REQ-173-003.3 let resolved = self @@ -575,6 +608,7 @@ impl ChatServiceImpl { tx: tokio::sync::mpsc::UnboundedSender, ) { let active_streams = self.active_streams.clone(); + let steering_queues = self.steering_queues.clone(); let conversation_service = self.conversation_service.clone(); let view_tx = self.view_tx.clone(); let approval_gate = self.approval_gate.clone(); @@ -586,6 +620,7 @@ impl ChatServiceImpl { mcp_tools, tx, active_streams, + steering_queues, cancel, conversation_service, conversation_id, @@ -722,6 +757,7 @@ async fn create_stream_agent( stream_id: Uuid, tx: &tokio::sync::mpsc::UnboundedSender, active_streams: &Arc>>, + steering_queues: &SteeringQueues, _cancel: &tokio_util::sync::CancellationToken, diagnostics_context: &StreamDiagnosticContext, ) -> Option> { @@ -746,7 +782,7 @@ async fn create_stream_agent( Some(Box::new(diagnostics)), tx, ); - clear_streaming_state(active_streams, conversation_id, stream_id); + clear_streaming_state(active_streams, steering_queues, conversation_id, stream_id); None } } @@ -832,6 +868,14 @@ impl ChatService for ChatServiceImpl { .is_some_and(|a| matches!(a.state, StreamLifecycle::Running)) } + /// Queue a steering message for this conversation's running turn. + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-004 + /// @requirement REQ-222-006 + async fn steer(&self, conversation_id: Uuid, text: String) -> ServiceResult { + self.queue_steering(conversation_id, &text) + } + async fn resolve_tool_approval( &self, request_id: String, @@ -911,6 +955,18 @@ impl ChatServiceImpl { map.get(&conversation_id).map(|a| a.stream_id) } + /// Test-only handles to the two registries a spawned stream task is + /// given, so tests can drive the steering delivery loop against the same + /// state the real service reads through `is_streaming_for` and `steer`. + /// + /// @plan PLAN-20260903-ISSUE222.P02 + /// @requirement REQ-222-005 + pub(in crate::services::chat_impl) fn stream_registries_for_test( + &self, + ) -> (Arc>>, SteeringQueues) { + (self.active_streams.clone(), self.steering_queues.clone()) + } + /// Test-only helper to clear all mock streams. /// @plan PLAN-20260416-ISSUE173.P03 /// @requirement REQ-173-001.3 diff --git a/src/services/chat_impl/steering.rs b/src/services/chat_impl/steering.rs new file mode 100644 index 0000000..b5bb83c --- /dev/null +++ b/src/services/chat_impl/steering.rs @@ -0,0 +1,314 @@ +//! The per-conversation steering queue (issue #222). +//! +//! A message typed while a turn is running waits here until that turn reaches +//! a boundary. Accepting a steer never disturbs the generation it steers: +//! nothing in this module touches a cancellation token or an approval. +//! +//! @plan PLAN-20260903-ISSUE222.P01 +//! @requirement REQ-222-004 +//! @requirement REQ-222-006 + +use super::{ChatService, ChatServiceImpl, ServiceError, ServiceResult}; +use crate::events::types::ChatEvent; +use crate::events::{emit, AppEvent}; +use std::collections::{HashMap, VecDeque}; +use std::sync::{Arc, Mutex as StdMutex}; +use uuid::Uuid; + +/// Upper bound on steering messages a single conversation may hold. +/// +/// An unbounded queue lets a user stack arbitrary instructions that all flush +/// at one boundary, which is the unpredictable-flush failure mode this feature +/// exists to avoid. +/// +/// @plan PLAN-20260903-ISSUE222.P01 +/// @requirement REQ-222-004 +pub(super) const MAX_QUEUED_STEERING_MESSAGES: usize = 5; + +/// A steering message waiting for its conversation's next turn boundary. +/// +/// @plan PLAN-20260903-ISSUE222.P01 +/// @requirement REQ-222-004 +pub(super) struct QueuedSteering { + pub(super) id: Uuid, + pub(super) text: String, +} + +/// Per-conversation FIFO steering queues, keyed the same way as +/// `ChatServiceImpl::active_streams`. +/// +/// Lock discipline: `active_streams` and `steering_queues` are never held at +/// the same time. Every path that needs both takes one, releases it, then +/// takes the other, so there is no acquisition order to invert and no +/// deadlock to construct. Nothing is emitted while either is held. +/// +/// What that discipline costs is atomicity, and it is worth naming: a path +/// that reads one registry and then writes the other never sees a single +/// consistent state, so a turn can end in the gap. The path where that +/// matters is accepting a steer, and +/// [`ChatServiceImpl::confirm_or_withdraw_steering`] is what it does about +/// it — it reads the stream state again once the entry is queued, so +/// whichever of the two ran first, the second one observes it. +/// +/// @plan PLAN-20260903-ISSUE222.P01 +/// @plan PLAN-20260903-ISSUE222.P07 +/// @requirement REQ-222-004 +pub(super) type SteeringQueues = Arc>>>; + +/// The refusal a steer gets when its conversation has no turn to steer. +/// +/// Shared by the two places that reach that conclusion — the check before +/// the insert and the re-check after it — so a caller cannot tell which of +/// them refused and the two cannot drift apart. +/// +/// @plan PLAN-20260903-ISSUE222.P07 +/// @requirement REQ-222-004 +fn no_active_turn() -> ServiceError { + ServiceError::Validation("No active turn to steer".to_string()) +} + +/// Take every steering message queued for a conversation, in FIFO order, +/// leaving its queue empty. +/// +/// A free function because the spawned stream task holds only the shared +/// `SteeringQueues` handle, never the service, and it is the task that drains +/// the queue at a turn boundary. +/// +/// @plan PLAN-20260903-ISSUE222.P01 +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-005 +pub(super) fn drain_steering_queue( + queues: &SteeringQueues, + conversation_id: Uuid, +) -> Vec { + queues + .lock() + .expect("steering_queues poisoned") + .remove(&conversation_id) + .map(Vec::from) + .unwrap_or_default() +} + +/// Announce that queued steering messages are never going to be delivered. +/// +/// A queued entry is rendered as waiting until something reports what became +/// of it, and delivery is only one of the two answers. Every path that ends a +/// turn while entries are still queued owes the other one, or the user is +/// left watching an instruction that will never be acted on. +/// +/// Callers pass entries they have already taken out of the queue, so this +/// holds no lock and can be called with none held. +/// +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-003 +pub(super) fn emit_steering_discarded(conversation_id: Uuid, discarded: &[QueuedSteering]) { + for entry in discarded { + emit_steering_discarded_id(conversation_id, entry.id); + } +} + +/// Announce that one steer, named by the id its `SteeringQueued` carried, is +/// never going to be delivered. +/// +/// Takes an id rather than an entry because the entry may already be gone: +/// the caller that has to say so is not always the one that took it off the +/// queue. See [`ChatServiceImpl::confirm_or_withdraw_steering`]. +/// +/// @plan PLAN-20260903-ISSUE222.P08 +/// @requirement REQ-222-003 +pub(super) fn emit_steering_discarded_id(conversation_id: Uuid, steer_id: Uuid) { + let _ = emit(AppEvent::Chat(ChatEvent::SteeringDiscarded { + conversation_id, + steer_id, + })); +} + +impl ChatServiceImpl { + /// Queue a steering message for a conversation whose turn is running. + /// + /// Steering is purely additive: this never cancels the in-flight + /// generation, never touches the conversation's cancellation token, and + /// never resolves a pending approval. `cancel` stays reachable only from + /// an explicit user stop (REQ-222-006). + /// + /// Lock discipline: the `active_streams` guard taken by + /// `is_streaming_for` is released before `steering_queues` is locked, so + /// the two locks are never held at once. See [`SteeringQueues`]. That + /// also means the check and the insert are not one atomic step, which is + /// why the entry is confirmed against the stream state again once it is + /// queued. + /// + /// # Errors + /// + /// Returns `ServiceError::Validation` when the text is blank, when the + /// conversation has no turn in `StreamLifecycle::Running` either before + /// or after the entry is queued, or when the conversation already holds + /// `MAX_QUEUED_STEERING_MESSAGES` entries. + /// + /// @plan PLAN-20260903-ISSUE222.P01 + /// @plan PLAN-20260903-ISSUE222.P07 + /// @requirement REQ-222-003 + /// @requirement REQ-222-004 + /// @requirement REQ-222-006 + pub(super) fn queue_steering(&self, conversation_id: Uuid, text: &str) -> ServiceResult { + let trimmed = text.trim(); + if trimmed.is_empty() { + return Err(ServiceError::Validation( + "Steering message is empty".to_string(), + )); + } + + if !ChatService::is_streaming_for(self, conversation_id) { + return Err(no_active_turn()); + } + + let entry = QueuedSteering { + id: Uuid::new_v4(), + text: trimmed.to_string(), + }; + let steer_id = entry.id; + let queued_text = entry.text.clone(); + + if !self.push_steering(conversation_id, entry) { + return Err(ServiceError::Validation(format!( + "Steering queue is full ({MAX_QUEUED_STEERING_MESSAGES} messages already waiting)" + ))); + } + + // Announced before it is confirmed, so the view renders the entry + // before it can be told to withdraw it. The other order would leave a + // withdrawal landing on an entry that does not exist yet, and nothing + // would come back for it afterwards. + // + // A teardown draining this queue can still announce its discard + // ahead of this, because the insert above released the lock. What + // follows this emit is therefore a terminal event for `steer_id` + // whether or not that entry is still on the queue: at least one + // discard always lands after this, never none. + // @plan PLAN-20260903-ISSUE222.P08 + // @requirement REQ-222-003 + let _ = emit(AppEvent::Chat(ChatEvent::SteeringQueued { + conversation_id, + steer_id, + text: queued_text, + })); + + self.confirm_or_withdraw_steering(conversation_id, steer_id) + } + + /// Confirm a steer that is now on the queue, or take it back. + /// + /// The precondition check and the insert are two separate lock + /// acquisitions, so a turn can end between them. The entry then lands on + /// a queue that turn's teardown has already drained, and nothing will + /// ever come back for it: the view renders an instruction that never + /// reaches a terminal state. Reading the stream state once more, with + /// the entry already queued, is what makes that case observable — + /// whichever of the two ran first, the second one sees it. + /// + /// The withdrawal is announced because the caller has already emitted + /// `SteeringQueued` for this entry, and `SteeringDiscarded` is the only + /// other way the view stops rendering it. It is announced whether or not + /// this call is the one that took the entry off the queue: a teardown + /// that drained it first may have announced its own discard *before* the + /// `SteeringQueued` was emitted, in which case the view processed a + /// withdrawal for an entry it had not rendered yet and then rendered it. + /// Staying quiet here on the strength of the teardown's announcement + /// would strand that entry on screen for the rest of the session. + /// + /// The guarantee is therefore at least one terminal event after every + /// `SteeringQueued`, not at most one. A repeated discard costs nothing: + /// the view removes by id, so the second one finds no entry and takes + /// nothing, which `a_discard_of_an_unknown_id_withdraws_nothing` pins. + /// + /// Lock discipline: `is_streaming_for` releases `active_streams` before + /// `remove_steering` locks `steering_queues`, and both are released + /// before anything is emitted. See [`SteeringQueues`]. + /// + /// # Errors + /// + /// Returns the same `ServiceError::Validation` the precondition check + /// returns, so a caller cannot tell the two orderings apart. + /// + /// @plan PLAN-20260903-ISSUE222.P07 + /// @plan PLAN-20260903-ISSUE222.P08 + /// @requirement REQ-222-003 + /// @requirement REQ-222-004 + pub(super) fn confirm_or_withdraw_steering( + &self, + conversation_id: Uuid, + steer_id: Uuid, + ) -> ServiceResult { + if ChatService::is_streaming_for(self, conversation_id) { + return Ok(steer_id); + } + + let _ = self.remove_steering(conversation_id, steer_id); + emit_steering_discarded_id(conversation_id, steer_id); + Err(no_active_turn()) + } + + /// Append `entry` to a conversation's steering queue. + /// + /// Returns `false` without queuing when the conversation already holds + /// `MAX_QUEUED_STEERING_MESSAGES` entries. + /// + /// Reachable from the steering tests so they can put an entry on a queue + /// without announcing it, which is the state `queue_steering` leaves + /// behind between the insert and the emit. + /// + /// @plan PLAN-20260903-ISSUE222.P01 + /// @plan PLAN-20260903-ISSUE222.P08 + /// @requirement REQ-222-004 + pub(super) fn push_steering(&self, conversation_id: Uuid, entry: QueuedSteering) -> bool { + let mut queues = self + .steering_queues + .lock() + .expect("steering_queues poisoned"); + let depth = queues.get(&conversation_id).map_or(0, VecDeque::len); + if depth >= MAX_QUEUED_STEERING_MESSAGES { + return false; + } + queues.entry(conversation_id).or_default().push_back(entry); + true + } + + /// Take one steering message off a conversation's queue by id, leaving + /// everything else queued in order. + /// + /// `drain_steering` is the wrong tool for withdrawing a single entry: the + /// rest of the queue may belong to a turn that is about to deliver it, + /// and taking it would cancel instructions the user is still owed. + /// + /// Returns `None` when the entry is no longer queued, which is what a + /// teardown racing the withdrawal leaves behind — `clear_streaming_state` + /// drains the whole queue, so it may already have taken and announced + /// this entry. The caller announces the discard either way; see + /// [`ChatServiceImpl::confirm_or_withdraw_steering`] for why a `None` + /// here is not permission to stay quiet. + /// + /// @plan PLAN-20260903-ISSUE222.P07 + /// @plan PLAN-20260903-ISSUE222.P08 + /// @requirement REQ-222-003 + fn remove_steering(&self, conversation_id: Uuid, steer_id: Uuid) -> Option { + let mut queues = self + .steering_queues + .lock() + .expect("steering_queues poisoned"); + let removed = queues.get_mut(&conversation_id).and_then(|queue| { + let position = queue.iter().position(|entry| entry.id == steer_id)?; + queue.remove(position) + }); + drop(queues); + removed + } + + /// Take every steering message queued for a conversation, in FIFO order, + /// leaving its queue empty. + /// + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-005 + pub(super) fn drain_steering(&self, conversation_id: Uuid) -> Vec { + drain_steering_queue(&self.steering_queues, conversation_id) + } +} diff --git a/src/services/chat_impl/streaming.rs b/src/services/chat_impl/streaming.rs index f7b0b29..bb4f30f 100644 --- a/src/services/chat_impl/streaming.rs +++ b/src/services/chat_impl/streaming.rs @@ -1,9 +1,9 @@ //! Streaming helper functions for `ChatServiceImpl`. use super::{ - ActiveStream, AgentClientExt, ApprovalGate, AsyncMutex, ChatEvent, ChatStreamEvent, - CompressionResult, LlmMessage, PreparedMessageContext, ServiceError, StdMutex, - ToolApprovalPolicy, ViewCommand, + drain_steering_queue, emit_steering_discarded, ActiveStream, AgentClientExt, ApprovalGate, + AsyncMutex, ChatEvent, ChatStreamEvent, CompressionResult, LlmMessage, PreparedMessageContext, + ServiceError, StdMutex, SteeringQueues, ToolApprovalPolicy, ViewCommand, }; use crate::events::{emit, AppEvent}; use crate::llm::error::{debug_error_message, LlmError}; @@ -22,6 +22,10 @@ use uuid::Uuid; use super::{build_stream_context, create_stream_agent}; +pub(super) mod steering_delivery; + +use steering_delivery::{run_steered_turns_and_finalize, SteeringDeliveryContext}; + pub(super) const STREAM_ERROR_MESSAGE: &str = "An error interrupted the chat stream."; #[derive(Default)] @@ -165,28 +169,43 @@ pub(super) fn handle_stream_run_failure( /// @plan PLAN-20260416-ISSUE173.P03 /// @plan PLAN-20260416-ISSUE173.P14-CR4 /// @requirement REQ-173-001.3 -#[allow(clippy::too_many_arguments)] pub(super) async fn finalize_stream_task( - conversation_service: &Arc, - conversation_id: Uuid, - stream_id: Uuid, + ctx: &StreamFinalizeContext<'_>, compression_result: CompressionResult, transcript: StreamTranscript, - active_streams: &Arc>>, model_label: &str, ) { persist_assistant_response( - conversation_service, - conversation_id, + ctx.conversation_service, + ctx.conversation_id, &transcript, model_label, false, ) .await; + finalize_completed_turn(ctx, compression_result, &transcript).await; +} + +/// Everything finalizing a cleanly finished turn does apart from writing its +/// assistant output: record context state, announce the completion, and +/// release the conversation's stream slot. +/// +/// Split out for the steering chain, which persists each intermediate turn's +/// output itself so the steering message that follows is ordered after it. A +/// chain that stops right after one of those writes still has to finalize, +/// but writing that output again would record it twice. +/// +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-007 +pub(super) async fn finalize_completed_turn( + ctx: &StreamFinalizeContext<'_>, + compression_result: CompressionResult, + transcript: &StreamTranscript, +) { persist_context_state( - conversation_service, - conversation_id, + ctx.conversation_service, + ctx.conversation_id, compression_result, transcript.input_tokens, transcript.output_tokens, @@ -194,45 +213,42 @@ pub(super) async fn finalize_stream_task( .await; let _ = emit(AppEvent::Chat(ChatEvent::StreamCompleted { - conversation_id, + conversation_id: ctx.conversation_id, message_id: Uuid::new_v4(), total_tokens: transcript .input_tokens .and_then(|input| transcript.output_tokens.map(|output| input + output)), })); - clear_streaming_state(active_streams, conversation_id, stream_id); + ctx.clear_streaming_state(); } /// Finalize a stream that failed partway: persist whatever partial text and/or /// thinking was produced, marked as interrupted, without touching context /// state or emitting a completion (issue #193). pub(super) async fn finalize_interrupted_stream( - conversation_service: &Arc, - conversation_id: Uuid, - stream_id: Uuid, + ctx: &StreamFinalizeContext<'_>, transcript: &StreamTranscript, - active_streams: &Arc>>, model_label: &str, ) { // Raw provider errors can embed credentials; only the sanitized form is // safe to log. let sanitized_error = sanitize_text(transcript.error.as_deref().unwrap_or("unknown")); tracing::warn!( - conversation_id = %conversation_id, + conversation_id = %ctx.conversation_id, error = %sanitized_error, response_chars = transcript.response_text.len(), thinking_chars = transcript.thinking_text.len(), "Finalizing interrupted chat stream with partial output" ); persist_assistant_response( - conversation_service, - conversation_id, + ctx.conversation_service, + ctx.conversation_id, transcript, model_label, true, ) .await; - clear_streaming_state(active_streams, conversation_id, stream_id); + ctx.clear_streaming_state(); } /// Finalize a stream according to its recorded outcome (issue #193): @@ -251,52 +267,59 @@ pub(super) async fn finalize_by_outcome( model_label: &str, ) { if transcript.completed { - finalize_stream_task( - ctx.conversation_service, - ctx.conversation_id, - ctx.stream_id, - compression_result, - transcript, - ctx.active_streams, - model_label, - ) - .await; + finalize_stream_task(ctx, compression_result, transcript, model_label).await; } else if transcript.error.is_some() { - finalize_interrupted_stream( - ctx.conversation_service, - ctx.conversation_id, - ctx.stream_id, - &transcript, - ctx.active_streams, - model_label, - ) - .await; + finalize_interrupted_stream(ctx, &transcript, model_label).await; } else { // Neither completed nor errored (e.g. cancelled): nothing to persist. - clear_streaming_state(ctx.active_streams, ctx.conversation_id, ctx.stream_id); + ctx.clear_streaming_state(); } } /// The stream-scoped state every outcome of `finalize_by_outcome` operates /// on: the conversation service, the (conversation, stream) coordinates, and -/// the shared active-stream registry. +/// the shared per-conversation registries. pub(super) struct StreamFinalizeContext<'a> { pub(super) conversation_service: &'a Arc, pub(super) conversation_id: Uuid, pub(super) stream_id: Uuid, pub(super) active_streams: &'a Arc>>, + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-004 + pub(super) steering_queues: &'a SteeringQueues, +} + +impl StreamFinalizeContext<'_> { + /// Release this stream's slot and its conversation's steering queue. + fn clear_streaming_state(&self) { + clear_streaming_state( + self.active_streams, + self.steering_queues, + self.conversation_id, + self.stream_id, + ); + } } /// Run a stream task for a conversation. +/// +/// One user send runs one turn, plus any further turns queued steering +/// messages chain onto it (issue #222). The agent is built once and reused; +/// each turn gets a freshly built tool context. Finalization happens once, +/// on the last turn. +/// /// @plan PLAN-20260416-ISSUE173.P03 /// @plan PLAN-20260416-ISSUE173.P14-CR4 +/// @plan PLAN-20260903-ISSUE222.P02 /// @requirement REQ-173-001.1, REQ-173-001.3 +/// @requirement REQ-222-005 #[allow(clippy::too_many_arguments)] pub(super) async fn run_stream_task( prepared: PreparedMessageContext, mcp_tools: Vec, tx: tokio::sync::mpsc::UnboundedSender, active_streams: Arc>>, + steering_queues: SteeringQueues, cancel: CancellationToken, conversation_service: Arc, conversation_id: Uuid, @@ -325,6 +348,7 @@ pub(super) async fn run_stream_task( stream_id, &tx, &active_streams, + &steering_queues, &cancel, &diagnostics_context, ) @@ -333,33 +357,53 @@ pub(super) async fn run_stream_task( return; }; - let context = build_stream_context( - conversation_id, - view_tx.clone(), - approval_gate.clone(), - policy.clone(), - skills_service, - filter_emoji, - ); + let delivery_ctx = SteeringDeliveryContext { + finalize: StreamFinalizeContext { + conversation_service: &conversation_service, + conversation_id, + stream_id, + active_streams: &active_streams, + steering_queues: &steering_queues, + }, + cancel: &cancel, + model_label: &profile.name, + }; - let transcript = stream_agent_response( - &client, - &agent, - &messages, - context, - &diagnostics_context, - conversation_id, - &tx, + // Bound outside the turn runner so the future it returns has one type for + // every turn, independent of the call that produced it. + let client = &client; + let agent = &agent; + let diagnostics_context = &diagnostics_context; + let tx = &tx; + + run_steered_turns_and_finalize( + &delivery_ctx, + compression_result, + messages, + move |turn_messages: Vec| { + let context = build_stream_context( + conversation_id, + view_tx.clone(), + approval_gate.clone(), + policy.clone(), + skills_service.clone(), + filter_emoji, + ); + async move { + stream_agent_response( + client, + agent, + &turn_messages, + context, + diagnostics_context, + conversation_id, + tx, + ) + .await + } + }, ) .await; - - let finalize_ctx = StreamFinalizeContext { - conversation_service: &conversation_service, - conversation_id, - stream_id, - active_streams: &active_streams, - }; - finalize_by_outcome(&finalize_ctx, compression_result, transcript, &profile.name).await; } pub(super) fn handle_llm_stream_event( @@ -596,6 +640,18 @@ fn summarize_tool_output(content: &str) -> String { } } +/// Whether a finished turn produced assistant-visible output. +/// +/// A transcript with neither text nor thinking is not persisted (#187), so +/// the steering delivery loop leaves it out of the history it chains onward +/// and that history keeps matching what a reload rebuilds. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-007 +pub(super) const fn has_assistant_output(transcript: &StreamTranscript) -> bool { + !transcript.response_text.is_empty() || !transcript.thinking_text.is_empty() +} + /// Persist the assistant output for a finished turn. /// /// `interrupted` marks output persisted after a stream failure (issue #193); @@ -608,7 +664,7 @@ pub(super) async fn persist_assistant_response( model_label: &str, interrupted: bool, ) { - if transcript.response_text.is_empty() && transcript.thinking_text.is_empty() { + if !has_assistant_output(transcript) { if !transcript.tool_calls.is_empty() || !transcript.tool_results.is_empty() { tracing::warn!( conversation_id = %conversation_id, @@ -673,19 +729,47 @@ pub(super) async fn persist_assistant_response( /// same conversation id. Without this epoch check the old task would evict /// the new reservation as soon as it finished its own cleanup. /// +/// When the entry is removed the conversation's steering queue goes with it: +/// that turn is definitively over, so anything still queued for it would only +/// leak into the next turn. +/// +/// A steer accepted between the delivery loop's last drain and this removal +/// passes `is_streaming_for` — the entry still reads `Running` — and is +/// announced to the view as queued. This is where that entry's turn ends, so +/// this is where it is announced as discarded. Taking the queue rather than +/// dropping it is what makes that possible. +/// +/// Lock discipline: the `active_streams` guard is released before +/// `steering_queues` is locked, so the two are never held at once, and both +/// are released before anything is emitted. See [`SteeringQueues`]. +/// /// @plan PLAN-20260416-ISSUE173.P03 /// @plan PLAN-20260416-ISSUE173.P14-CR4 +/// @plan PLAN-20260903-ISSUE222.P01 +/// @plan PLAN-20260903-ISSUE222.P06 /// @requirement REQ-173-001.3 +/// @requirement REQ-222-003 +/// @requirement REQ-222-004 pub(super) fn clear_streaming_state( active_streams: &Arc>>, + steering_queues: &SteeringQueues, conversation_id: Uuid, stream_id: Uuid, ) { - let mut map = active_streams.lock().expect("active_streams poisoned"); - if let Some(entry) = map.get(&conversation_id) { - if entry.stream_id == stream_id { - map.remove(&conversation_id); + let cleared = { + let mut map = active_streams.lock().expect("active_streams poisoned"); + match map.get(&conversation_id) { + Some(entry) if entry.stream_id == stream_id => { + map.remove(&conversation_id); + true + } + _ => false, } + }; + + if cleared { + let discarded = drain_steering_queue(steering_queues, conversation_id); + emit_steering_discarded(conversation_id, &discarded); } } diff --git a/src/services/chat_impl/streaming/steering_delivery.rs b/src/services/chat_impl/streaming/steering_delivery.rs new file mode 100644 index 0000000..92d2672 --- /dev/null +++ b/src/services/chat_impl/streaming/steering_delivery.rs @@ -0,0 +1,258 @@ +//! Delivery of queued steering messages at the turn boundary (issue #222). +//! +//! A user send runs one `AgentStream` turn. When that turn finishes cleanly +//! and the conversation has steering messages waiting, this module runs +//! another turn seeded with the conversation so far plus the steering text — +//! no new user-initiated send, and nothing cancelled. The chain ends when the +//! queue is empty, the turn did not finish cleanly, the turn cap is hit, or a +//! steering message cannot be recorded; finalization then runs once, on the +//! last turn. +//! +//! Every ending that leaves messages queued announces them as discarded. A +//! queued entry is on screen until something says what became of it, and +//! delivery is only one of the two answers. +//! +//! @plan PLAN-20260903-ISSUE222.P02 +//! @plan PLAN-20260903-ISSUE222.P06 +//! @requirement REQ-222-003 +//! @requirement REQ-222-005 +//! @requirement REQ-222-006 +//! @requirement REQ-222-007 + +use super::{ + finalize_by_outcome, finalize_completed_turn, has_assistant_output, persist_assistant_response, + StreamFinalizeContext, StreamTranscript, +}; +use crate::compression::pipeline::CompressionResult; +use crate::events::types::ChatEvent; +use crate::events::{emit, AppEvent}; +use crate::llm::Message as LlmMessage; +use crate::models::Message; +use crate::services::chat_impl::{drain_steering_queue, emit_steering_discarded, QueuedSteering}; +use std::future::Future; +use tokio_util::sync::CancellationToken; + +/// Upper bound on the turns one user send may chain through steering. +/// +/// The queue itself is capped, but a user can refill it during every +/// follow-up turn, so the cap alone never terminates the chain. This does. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-005 +pub(in crate::services::chat_impl) const MAX_STEERING_TURNS: usize = 10; + +/// Everything a chained turn needs beyond the turn runner itself. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-005 +pub(in crate::services::chat_impl) struct SteeringDeliveryContext<'a> { + /// Coordinates and registries the single finalization operates on. + pub(in crate::services::chat_impl) finalize: StreamFinalizeContext<'a>, + /// The send's cancellation token. A cancelled turn chains nothing. + pub(in crate::services::chat_impl) cancel: &'a CancellationToken, + /// Profile name recorded on every assistant message this send persists. + pub(in crate::services::chat_impl) model_label: &'a str, +} + +/// Run a user send's turns — the first, plus any that steering chains onto +/// it — and finalize exactly once, on the last. +/// +/// `run_turn` is handed the history for a turn and yields that turn's +/// transcript. Intermediate turns persist their own assistant output here so +/// it is ordered before the steering message that follows it; the last turn's +/// output is normally persisted by `finalize_by_outcome`, so nothing is +/// written twice. The one chain that ends between those two writes reports +/// itself with [`ChainOutcome::OutputPersisted`] and finalizes without the +/// second, which is what keeps that guarantee true. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-005 +/// @requirement REQ-222-007 +pub(in crate::services::chat_impl) async fn run_steered_turns_and_finalize( + ctx: &SteeringDeliveryContext<'_>, + compression_result: CompressionResult, + messages: Vec, + run_turn: R, +) where + R: FnMut(Vec) -> Fut, + Fut: Future, +{ + let (transcript, outcome) = run_steered_turns(ctx, messages, run_turn).await; + match outcome { + ChainOutcome::Unfinalized => { + finalize_by_outcome( + &ctx.finalize, + compression_result, + transcript, + ctx.model_label, + ) + .await; + } + ChainOutcome::OutputPersisted => { + finalize_completed_turn(&ctx.finalize, compression_result, &transcript).await; + } + } +} + +/// What the chain has left for finalization to do with the last turn. +/// +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-007 +enum ChainOutcome { + /// Nothing about the last turn has been recorded, so finalization decides + /// what to persist from the transcript itself. This is every ordinary + /// ending: the queue ran dry, the turn did not finish cleanly, or the + /// chain hit its turn cap. + Unfinalized, + /// The last turn's assistant output is already recorded, because the + /// chain stopped after writing it and before the follow-up turn it was + /// written for could start. The rest of finalization still applies; + /// writing that output again would record it twice. + OutputPersisted, +} + +/// Run turns until the conversation stops steering, returning the last +/// turn's transcript and what finalization still owes it. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-005 +/// @requirement REQ-222-006 +async fn run_steered_turns( + ctx: &SteeringDeliveryContext<'_>, + mut messages: Vec, + mut run_turn: R, +) -> (StreamTranscript, ChainOutcome) +where + R: FnMut(Vec) -> Fut, + Fut: Future, +{ + let conversation_id = ctx.finalize.conversation_id; + let mut turns = 0_usize; + + loop { + // The runner needs an owned history because its future outlives this + // borrow of `messages`, which the next iteration extends. + let transcript = run_turn(messages.clone()).await; + turns += 1; + + if !reaches_delivery_boundary(&transcript, ctx.cancel) { + return (transcript, ChainOutcome::Unfinalized); + } + + let queued = drain_steering_queue(ctx.finalize.steering_queues, conversation_id); + if queued.is_empty() { + return (transcript, ChainOutcome::Unfinalized); + } + + if turns >= MAX_STEERING_TURNS { + tracing::warn!( + conversation_id = %conversation_id, + turns, + dropped = queued.len(), + "Steering chain reached its turn cap; dropping the messages still queued" + ); + emit_steering_discarded(conversation_id, &queued); + return (transcript, ChainOutcome::Unfinalized); + } + + persist_assistant_response( + ctx.finalize.conversation_service, + conversation_id, + &transcript, + ctx.model_label, + false, + ) + .await; + if has_assistant_output(&transcript) { + messages.push(assistant_message(&transcript)); + } + + if !deliver_steering(ctx, &queued, &mut messages).await { + return (transcript, ChainOutcome::OutputPersisted); + } + } +} + +/// Whether a finished turn reached a boundary a steering message may be +/// delivered at. +/// +/// A turn that failed, that never completed, or whose send has been stopped +/// is over: it drains nothing and chains nothing (REQ-222-006). This is also +/// the re-check that runs before every follow-up turn starts. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-006 +fn reaches_delivery_boundary(transcript: &StreamTranscript, cancel: &CancellationToken) -> bool { + transcript.completed && transcript.error.is_none() && !cancel.is_cancelled() +} + +/// Persist, announce, and seed each drained steering message in FIFO order. +/// +/// Returns `false` when a steering message could not be persisted, which +/// stops the chain. Recording the steer is what keeps the history a chained +/// turn is seeded with equal to the history a reload rebuilds; a turn run +/// over text the store rejected would break that for the rest of the +/// conversation. So the failed message is not seeded, is not announced as +/// delivered, and neither is anything behind it: they are announced as +/// discarded instead, because no later turn is going to pick them up. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-003 +/// @requirement REQ-222-005 +/// @requirement REQ-222-007 +async fn deliver_steering( + ctx: &SteeringDeliveryContext<'_>, + queued: &[QueuedSteering], + messages: &mut Vec, +) -> bool { + let conversation_id = ctx.finalize.conversation_id; + for (index, entry) in queued.iter().enumerate() { + if let Err(error) = ctx + .finalize + .conversation_service + .add_message(conversation_id, Message::user(entry.text.clone())) + .await + { + // ServiceError's Display never carries user content; the steering + // text itself is intentionally not logged. + tracing::warn!( + conversation_id = %conversation_id, + steer_id = %entry.id, + error = %error, + "Failed to persist a delivered steering message" + ); + emit_steering_discarded(conversation_id, &queued[index..]); + return false; + } + + let _ = emit(AppEvent::Chat(ChatEvent::SteeringDelivered { + conversation_id, + steer_id: entry.id, + })); + + messages.push(LlmMessage::user(entry.text.clone())); + } + true +} + +/// The finished turn's assistant output, shaped the way +/// `ChatServiceImpl::build_llm_messages` shapes a persisted assistant +/// message, so the chained turn sees the history a reload would rebuild. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-005 +fn assistant_message(transcript: &StreamTranscript) -> LlmMessage { + let mut message = LlmMessage::assistant(transcript.response_text.clone()); + if !transcript.thinking_text.is_empty() { + message = message.with_thinking(transcript.thinking_text.clone()); + } + if !transcript.tool_calls.is_empty() { + message = message.with_tool_uses(transcript.tool_calls.clone()); + } + if !transcript.tool_results.is_empty() { + message = message.with_tool_results(transcript.tool_results.clone()); + } + message +} diff --git a/src/services/chat_impl/tests.rs b/src/services/chat_impl/tests.rs index 2906965..3085eef 100644 --- a/src/services/chat_impl/tests.rs +++ b/src/services/chat_impl/tests.rs @@ -12,12 +12,38 @@ mod approval_persistence; mod chat_test_support; mod compression_persistence; mod concurrent_streams; +mod steering; +mod steering_delivery; mod stream_failure; mod three_stream_concurrency; mod title_generation; use chat_test_support::*; +/// Serializes the steering tests that observe the global event bus. +/// +/// The bus is a 16-slot broadcast ring shared by every test in the binary, +/// and each `#[tokio::test]` brings its own runtime running in parallel. If +/// two bus-observing steering tests emit concurrently, a collector that goes +/// even briefly unpolled can be lagged past all of its own events, end +/// marker included, and then wait forever for what was already overwritten. +/// With at most one steering test emitting at a time, one test's events +/// total well under the ring's capacity, so every collector still finds all +/// of them in order. +/// +/// @plan PLAN-20260903-ISSUE222.P08 +/// @requirement REQ-222-008 +static STEERING_BUS_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); + +/// Hold the returned guard for the whole body of a steering test that emits +/// on the bus, acquired before the test subscribes to the bus. +/// +/// @plan PLAN-20260903-ISSUE222.P08 +/// @requirement REQ-222-008 +async fn lock_steering_bus() -> tokio::sync::MutexGuard<'static, ()> { + STEERING_BUS_LOCK.lock().await +} + struct FailingSkillsService; #[async_trait] diff --git a/src/services/chat_impl/tests/concurrent_streams.rs b/src/services/chat_impl/tests/concurrent_streams.rs index c37da00..fb5d760 100644 --- a/src/services/chat_impl/tests/concurrent_streams.rs +++ b/src/services/chat_impl/tests/concurrent_streams.rs @@ -33,6 +33,53 @@ fn make_test_chat_service() -> ChatServiceImpl { ChatServiceImpl::new_for_tests(conversation_service, profile_service) } +/// Drop everything already buffered for this receiver. +/// +/// A `Lagged` reply means other tests' events fell out of the shared ring, not +/// that this receiver is drained, so it keeps going. +fn discard_buffered_events(rx: &mut tokio::sync::broadcast::Receiver) { + while matches!( + rx.try_recv(), + Ok(_) | Err(tokio::sync::broadcast::error::TryRecvError::Lagged(_)) + ) {} +} + +/// Count `StreamCancelled` events for conversations `a` and `b` over `window`. +/// +/// The global event bus is shared by concurrently running tests, so every +/// event is matched against a conversation UUID unique to this test and a +/// lagging receiver keeps draining rather than giving up. +async fn count_stream_cancelled( + rx: &mut tokio::sync::broadcast::Receiver, + a: Uuid, + b: Uuid, + window: Duration, +) -> (usize, usize) { + let mut counts = (0usize, 0usize); + let deadline = tokio::time::Instant::now() + window; + loop { + let remaining = deadline.saturating_duration_since(tokio::time::Instant::now()); + if remaining.is_zero() { + break; + } + match tokio::time::timeout(remaining, rx.recv()).await { + Ok(Ok(AppEvent::Chat(ChatEvent::StreamCancelled { + conversation_id, .. + }))) => { + if conversation_id == a { + counts.0 += 1; + } else if conversation_id == b { + counts.1 += 1; + } + } + Ok(Ok(_) | Err(tokio::sync::broadcast::error::RecvError::Lagged(_))) => {} + Ok(Err(tokio::sync::broadcast::error::RecvError::Closed)) => break, + Err(_elapsed) => break, + } + } + counts +} + /// @plan PLAN-20260416-ISSUE173.P02 /// @plan PLAN-20260416-ISSUE173.P14-CR3 /// @requirement REQ-173-001.1 @@ -156,29 +203,21 @@ async fn cancel_emits_event_only_for_target() { .begin_stream_for_test(conversation_b) .expect("begin_stream(B) should succeed"); + // The global bus is a small ring shared with every concurrently running + // test. Clear what they have already published so this test's own + // cancellation cannot be evicted from the ring before it is read. + discard_buffered_events(&mut event_rx); + // Cancel only conversation A using the trait method ChatService::cancel(&service, conversation_a); - // Give a moment for cancellation to process and events to be emitted - sleep(Duration::from_millis(50)).await; - - // Drain events and look for StreamCancelled - let mut a_cancellations = 0; - let mut b_cancellations = 0; - - // Process events that have been received (non-blocking) - while let Ok(event) = event_rx.try_recv() { - if let AppEvent::Chat(ChatEvent::StreamCancelled { - conversation_id, .. - }) = &event - { - if conversation_id == &conversation_a { - a_cancellations += 1; - } else if conversation_id == &conversation_b { - b_cancellations += 1; - } - } - } + let (a_cancellations, b_cancellations) = count_stream_cancelled( + &mut event_rx, + conversation_a, + conversation_b, + Duration::from_millis(250), + ) + .await; // Should have exactly one StreamCancelled for A, zero for B assert_eq!( diff --git a/src/services/chat_impl/tests/steering.rs b/src/services/chat_impl/tests/steering.rs new file mode 100644 index 0000000..f7ee387 --- /dev/null +++ b/src/services/chat_impl/tests/steering.rs @@ -0,0 +1,586 @@ +//! Steering-queue behavior for `ChatServiceImpl`. +//! +//! Covers issue #222: a steering message submitted while a turn is running is +//! queued against that conversation, is rejected when there is no running +//! turn, is capped, and never disturbs the in-flight generation. +//! +//! @plan PLAN-20260903-ISSUE222.P01 +//! @requirement REQ-222-004 +//! @requirement REQ-222-006 + +use super::lock_steering_bus; +use crate::events::types::ChatEvent; +use crate::events::{subscribe, AppEvent}; +use crate::services::chat_impl::{ChatServiceImpl, MAX_QUEUED_STEERING_MESSAGES}; +use crate::services::{ChatService, ConversationService, ProfileService, ServiceError}; +use std::sync::Arc; +use tokio::sync::broadcast::Receiver; +use tokio::sync::Notify; +use tokio::time::Duration; +use uuid::Uuid; + +// Import test support utilities from parent tests module (support.rs) +use super::chat_test_support::{MockConversationService, MockProfileService}; + +mod withdrawal; + +fn make_test_chat_service() -> ChatServiceImpl { + let conversation_service: Arc = + Arc::new(MockConversationService::new(Uuid::new_v4())); + let profile_service: Arc = Arc::new(MockProfileService::new()); + ChatServiceImpl::new_for_tests(conversation_service, profile_service) +} + +/// The queued entries for a conversation, as `(steer_id, text)` pairs. +/// +/// Draining is the only way to observe the queue, so every caller that +/// inspects it also empties it. +fn drained_pairs(service: &ChatServiceImpl, conversation_id: Uuid) -> Vec<(Uuid, String)> { + service + .drain_steering(conversation_id) + .into_iter() + .map(|entry| (entry.id, entry.text)) + .collect() +} + +/// Collect chat events addressed to `conversation_id` within a short window. +/// +/// The global event bus is shared by concurrently running tests, so every +/// assertion filters by a conversation UUID unique to this test. +async fn collect_chat_events( + rx: &mut Receiver, + conversation_id: Uuid, + window: Duration, +) -> Vec { + let mut matched = Vec::new(); + let deadline = tokio::time::Instant::now() + window; + loop { + let remaining = deadline.saturating_duration_since(tokio::time::Instant::now()); + if remaining.is_zero() { + break; + } + match tokio::time::timeout(remaining, rx.recv()).await { + Ok(Ok(AppEvent::Chat(event))) => { + if event_conversation_id(&event) == Some(conversation_id) { + matched.push(event); + } + } + // Other subsystems' events and lagged receivers (which just missed + // other tests' events) are skipped; keep draining. + Ok(Ok(_) | Err(tokio::sync::broadcast::error::RecvError::Lagged(_))) => {} + Ok(Err(tokio::sync::broadcast::error::RecvError::Closed)) => break, + Err(_elapsed) => break, + } + } + matched +} + +/// Run `body` while a task drains the bus into the chat events for +/// `conversation_ids`, stopping at the `StreamCancelled` that reports +/// `cancelled` as stopped. +/// +/// `cancel_active_stream` discards the conversation's queued steering before +/// it announces the cancellation, and the bus preserves the order events were +/// emitted in, so that announcement is the point at which every consequence +/// of the cancel has been seen, with nothing to wait out. The timeout only +/// bounds a hang. +/// +/// The collector is spawned before `body` runs, because the bus is a shared +/// 16-slot ring: a receiver that only starts draining once the action is over +/// can be lagged past the events under test by whatever else is running in +/// parallel, and the loop's tolerance of `Lagged` would then quietly return a +/// short list. It signals `ready` the moment it holds a subscription and +/// `body` does not start until that arrives — a broadcast receiver buffers +/// from the instant it exists, so a subscription in hand is exactly the +/// guarantee these assertions need, and it is a fact the collector reports +/// rather than an interval this test hopes is long enough. +/// +/// @plan PLAN-20260903-ISSUE222.P06 +/// @plan PLAN-20260903-ISSUE222.P08 +/// @requirement REQ-222-003 +async fn events_until_cancelled( + conversation_ids: Vec, + cancelled: Uuid, + body: F, +) -> (T, Vec) +where + F: std::future::Future, +{ + const GIVE_UP_AFTER: Duration = Duration::from_secs(10); + + let ready = Arc::new(Notify::new()); + let ready_for_collector = ready.clone(); + + let collector = tokio::spawn(async move { + let mut rx = subscribe(); + // Subscribed. `Notify` keeps the permit even with nobody waiting yet, + // so this cannot be signalled into the void. + ready_for_collector.notify_one(); + + let mut matched = Vec::new(); + let deadline = tokio::time::Instant::now() + GIVE_UP_AFTER; + loop { + let remaining = deadline.saturating_duration_since(tokio::time::Instant::now()); + match tokio::time::timeout(remaining, rx.recv()).await { + Ok(Ok(AppEvent::Chat(event))) => { + if event_conversation_id(&event) + .is_none_or(|id| !conversation_ids.contains(&id)) + { + continue; + } + let done = matches!(&event, ChatEvent::StreamCancelled { conversation_id, .. } + if *conversation_id == cancelled); + matched.push(event); + if done { + return matched; + } + } + // Other subsystems' events and lagged receivers (which just + // missed other tests' events) are skipped; keep draining. + Ok(Ok(_) | Err(tokio::sync::broadcast::error::RecvError::Lagged(_))) => {} + Ok(Err(tokio::sync::broadcast::error::RecvError::Closed)) => { + panic!("the event bus closed before {cancelled} was reported cancelled") + } + Err(elapsed) => { + panic!( + "waiting {GIVE_UP_AFTER:?} for {cancelled} to be reported cancelled: \ + {elapsed}, saw {matched:?}" + ) + } + } + } + }); + + ready.notified().await; + + let outcome = body.await; + let events = collector.await.expect("event collector must not panic"); + (outcome, events) +} + +/// Conversation id carried by the chat events these tests care about. +fn event_conversation_id(event: &ChatEvent) -> Option { + match event { + ChatEvent::SteeringQueued { + conversation_id, .. + } + | ChatEvent::SteeringDelivered { + conversation_id, .. + } + | ChatEvent::SteeringDiscarded { + conversation_id, .. + } + | ChatEvent::StreamCancelled { + conversation_id, .. + } => Some(*conversation_id), + _ => None, + } +} + +/// The `steer_id`s carried by the `SteeringDiscarded` events collected, in +/// the order they were announced. +/// +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-003 +fn discarded_ids(events: &[ChatEvent]) -> Vec { + events + .iter() + .filter_map(|event| match event { + ChatEvent::SteeringDiscarded { steer_id, .. } => Some(*steer_id), + _ => None, + }) + .collect() +} + +/// The `(steer_id, text)` payloads of the `SteeringQueued` events collected. +fn queued_payloads(events: &[ChatEvent]) -> Vec<(Uuid, String)> { + events + .iter() + .filter_map(|event| match event { + ChatEvent::SteeringQueued { steer_id, text, .. } => Some((*steer_id, text.clone())), + _ => None, + }) + .collect() +} + +/// A conversation with no turn in flight has nothing to steer, so the steer is +/// rejected and nothing is queued. +/// +/// @plan PLAN-20260903-ISSUE222.P01 +/// @requirement REQ-222-004 +#[tokio::test] +async fn steer_without_active_stream_is_rejected_and_queues_nothing() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_id = Uuid::new_v4(); + let mut event_rx = subscribe(); + + let error = ChatService::steer(&service, conversation_id, "turn left".to_string()) + .await + .expect_err("steering an idle conversation must be rejected"); + + assert!( + matches!(&error, ServiceError::Validation(message) if message.contains("No active turn to steer")), + "expected a no-active-turn validation error, got: {error}" + ); + assert!( + drained_pairs(&service, conversation_id).is_empty(), + "a rejected steer must not queue anything" + ); + + let events = + collect_chat_events(&mut event_rx, conversation_id, Duration::from_millis(100)).await; + assert!( + queued_payloads(&events).is_empty(), + "a rejected steer must not emit SteeringQueued, got {events:?}" + ); +} + +/// A steer against a running turn is accepted, lands on that conversation's +/// queue, and announces itself with the id it returned. +/// +/// @plan PLAN-20260903-ISSUE222.P01 +/// @requirement REQ-222-004 +#[tokio::test] +async fn steer_with_running_stream_queues_the_entry_and_emits_queued_event() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_id = Uuid::new_v4(); + let mut event_rx = subscribe(); + + service + .begin_stream_for_test(conversation_id) + .expect("begin_stream should succeed"); + + let steer_id = ChatService::steer( + &service, + conversation_id, + " focus on the parser ".to_string(), + ) + .await + .expect("steering a running turn must be accepted"); + + assert_eq!( + drained_pairs(&service, conversation_id), + vec![(steer_id, "focus on the parser".to_string())], + "the queue must hold exactly the accepted steer, trimmed" + ); + + let events = + collect_chat_events(&mut event_rx, conversation_id, Duration::from_millis(100)).await; + assert_eq!( + queued_payloads(&events), + vec![(steer_id, "focus on the parser".to_string())], + "the accepted steer must emit exactly one SteeringQueued, got {events:?}" + ); + + service.clear_all_streams_for_test(); +} + +/// Steering reaches only the conversation that owns the running stream: a +/// steer for A is rejected while only B is running, and B's queue stays empty. +/// +/// @plan PLAN-20260903-ISSUE222.P01 +/// @requirement REQ-222-004 +#[tokio::test] +async fn steer_for_one_conversation_does_not_land_on_another() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_a = Uuid::new_v4(); + let conversation_b = Uuid::new_v4(); + + service + .begin_stream_for_test(conversation_b) + .expect("begin_stream(B) should succeed"); + + let error = ChatService::steer(&service, conversation_a, "reroute".to_string()) + .await + .expect_err("steering A while only B is running must be rejected"); + + assert!( + matches!(&error, ServiceError::Validation(message) if message.contains("No active turn to steer")), + "expected a no-active-turn validation error for A, got: {error}" + ); + assert!( + drained_pairs(&service, conversation_a).is_empty(), + "A must have nothing queued" + ); + assert!( + drained_pairs(&service, conversation_b).is_empty(), + "a steer addressed to A must never land on B's queue" + ); + + service.clear_all_streams_for_test(); +} + +/// The queue is capped: the steer past the cap is rejected with an error that +/// names the cap, and the already-queued entries survive in submission order. +/// +/// @plan PLAN-20260903-ISSUE222.P01 +/// @requirement REQ-222-004 +#[tokio::test] +async fn steer_past_the_cap_is_rejected_and_preserves_the_full_queue() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_id = Uuid::new_v4(); + + service + .begin_stream_for_test(conversation_id) + .expect("begin_stream should succeed"); + + let mut accepted = Vec::new(); + for index in 0..MAX_QUEUED_STEERING_MESSAGES { + let text = format!("steer {index}"); + let steer_id = ChatService::steer(&service, conversation_id, text.clone()) + .await + .expect("steers up to the cap must be accepted"); + accepted.push((steer_id, text)); + } + + let error = ChatService::steer(&service, conversation_id, "one too many".to_string()) + .await + .expect_err("a steer past the cap must be rejected"); + + let message = error.to_string(); + assert!( + message.contains(&MAX_QUEUED_STEERING_MESSAGES.to_string()), + "the rejection must name the cap of {MAX_QUEUED_STEERING_MESSAGES}, got: {message}" + ); + assert_eq!( + drained_pairs(&service, conversation_id), + accepted, + "the rejected steer must leave the full queue untouched and in order" + ); + + service.clear_all_streams_for_test(); +} + +/// Steering text that carries no instruction is rejected rather than queued. +/// +/// @plan PLAN-20260903-ISSUE222.P01 +/// @requirement REQ-222-004 +#[tokio::test] +async fn whitespace_only_steering_text_is_rejected() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_id = Uuid::new_v4(); + + service + .begin_stream_for_test(conversation_id) + .expect("begin_stream should succeed"); + + let error = ChatService::steer(&service, conversation_id, " \n\t ".to_string()) + .await + .expect_err("whitespace-only steering text must be rejected"); + + assert!( + matches!(error, ServiceError::Validation(_)), + "expected a validation error, got: {error}" + ); + assert!( + drained_pairs(&service, conversation_id).is_empty(), + "blank steering text must not queue anything" + ); + + service.clear_all_streams_for_test(); +} + +/// Draining hands back every queued steer in submission order and leaves the +/// conversation with an empty queue. +/// +/// @plan PLAN-20260903-ISSUE222.P01 +/// @requirement REQ-222-005 +#[tokio::test] +async fn drain_steering_returns_fifo_order_and_empties_the_queue() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_id = Uuid::new_v4(); + + service + .begin_stream_for_test(conversation_id) + .expect("begin_stream should succeed"); + + let mut submitted = Vec::new(); + for text in ["first", "second", "third"] { + let steer_id = ChatService::steer(&service, conversation_id, text.to_string()) + .await + .expect("steering a running turn must be accepted"); + submitted.push((steer_id, text.to_string())); + } + + assert_eq!( + drained_pairs(&service, conversation_id), + submitted, + "drain must return queued steers in FIFO order" + ); + assert!( + drained_pairs(&service, conversation_id).is_empty(), + "drain must leave the queue empty" + ); + + service.clear_all_streams_for_test(); +} + +/// Steering is additive: it never aborts the generation it is steering. +/// +/// @plan PLAN-20260903-ISSUE222.P01 +/// @requirement REQ-222-006 +#[tokio::test] +async fn steering_does_not_cancel_the_active_turn() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_id = Uuid::new_v4(); + let mut event_rx = subscribe(); + + service + .begin_stream_for_test(conversation_id) + .expect("begin_stream should succeed"); + + ChatService::steer( + &service, + conversation_id, + "keep going, but add tests".to_string(), + ) + .await + .expect("steering a running turn must be accepted"); + + assert!( + ChatService::is_streaming_for(&service, conversation_id), + "the turn must still be running after a steer" + ); + + let events = + collect_chat_events(&mut event_rx, conversation_id, Duration::from_millis(100)).await; + assert!( + events + .iter() + .all(|event| !matches!(event, ChatEvent::StreamCancelled { .. })), + "steering must not cancel the turn, got {events:?}" + ); + + service.clear_all_streams_for_test(); +} + +/// A cancelled turn will never reach a delivery boundary, so its queued steers +/// are discarded — and only its own. +/// +/// @plan PLAN-20260903-ISSUE222.P01 +/// @requirement REQ-222-004 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cancelling_a_stream_discards_only_that_conversations_queue() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_a = Uuid::new_v4(); + let conversation_b = Uuid::new_v4(); + + service + .begin_stream_for_test(conversation_a) + .expect("begin_stream(A) should succeed"); + service + .begin_stream_for_test(conversation_b) + .expect("begin_stream(B) should succeed"); + + // The cancel discards A's queue before it announces itself, so the + // collector returning on that announcement is the guarantee that the + // discard already happened. + let ((a_steer_id, b_steer_id), events) = events_until_cancelled( + vec![conversation_a, conversation_b], + conversation_a, + async { + let a_steer_id = ChatService::steer(&service, conversation_a, "steer A".to_string()) + .await + .expect("steering A must be accepted"); + let b_steer_id = ChatService::steer(&service, conversation_b, "steer B".to_string()) + .await + .expect("steering B must be accepted"); + + ChatService::cancel(&service, conversation_a); + (a_steer_id, b_steer_id) + }, + ) + .await; + + // The collector stops at the announcement, so A's discard is in this + // list only if it was emitted first. That order is what makes the reads + // below safe: after a cancel that announced itself first, the queue they + // inspect could still be mid-teardown. + assert!( + discarded_ids(&events).contains(&a_steer_id), + "the cancel must discard A's queue before it announces itself, got {events:?}" + ); + + assert!( + drained_pairs(&service, conversation_a).is_empty(), + "cancelling A must discard A's queued steering" + ); + assert_eq!( + drained_pairs(&service, conversation_b), + vec![(b_steer_id, "steer B".to_string())], + "cancelling A must leave B's queue untouched" + ); + + service.clear_all_streams_for_test(); +} + +/// Stopping a turn announces every steer it was holding as discarded. +/// +/// The view is rendering each queued entry as waiting, and a cancel is the +/// moment it stops being true. Without the announcement the bubble the user +/// typed stays on screen for the rest of the session. +/// +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-003 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cancelling_a_stream_announces_every_discarded_steer() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_a = Uuid::new_v4(); + let conversation_b = Uuid::new_v4(); + + service + .begin_stream_for_test(conversation_a) + .expect("begin_stream(A) should succeed"); + service + .begin_stream_for_test(conversation_b) + .expect("begin_stream(B) should succeed"); + + let ((first, second, b_steer_id), events) = events_until_cancelled( + vec![conversation_a, conversation_b], + conversation_a, + async { + let first = ChatService::steer(&service, conversation_a, "first steer".to_string()) + .await + .expect("steering A must be accepted"); + let second = ChatService::steer(&service, conversation_a, "second steer".to_string()) + .await + .expect("steering A must be accepted"); + let b_steer_id = ChatService::steer(&service, conversation_b, "steer B".to_string()) + .await + .expect("steering B must be accepted"); + + ChatService::cancel(&service, conversation_a); + (first, second, b_steer_id) + }, + ) + .await; + + assert_eq!( + discarded_ids(&events), + vec![first, second], + "every steer the cancelled turn held must be announced as discarded, in queue order, \ + got {events:?}" + ); + assert!( + events + .iter() + .all(|event| !matches!(event, ChatEvent::SteeringDelivered { .. })), + "a cancelled turn hands nothing to the model, got {events:?}" + ); + assert_eq!( + drained_pairs(&service, conversation_b), + vec![(b_steer_id, "steer B".to_string())], + "cancelling A must neither discard nor announce B's queued steering" + ); + + service.clear_all_streams_for_test(); +} diff --git a/src/services/chat_impl/tests/steering/withdrawal.rs b/src/services/chat_impl/tests/steering/withdrawal.rs new file mode 100644 index 0000000..9b535b9 --- /dev/null +++ b/src/services/chat_impl/tests/steering/withdrawal.rs @@ -0,0 +1,440 @@ +//! Withdrawing a steer whose turn ended while it was being accepted. +//! +//! `queue_steering` decides a conversation has a running turn and then puts +//! the steer on that conversation's queue. Those are two lock acquisitions, +//! one after the other, and nothing holds the turn still in between. A +//! teardown that lands there drains the queue before the entry is on it, so +//! the entry arrives on a queue nothing will ever come back for — already +//! announced to the view as waiting, and never resolved. That is the stuck +//! bubble `SteeringDiscarded` exists to prevent, reached through a narrower +//! window than the paths that already emit it. +//! +//! Reading the stream state once more, after the entry is queued, is what +//! closes it. These tests drive that re-check against the state the window +//! leaves behind: an announced entry on the queue, and no turn left to +//! deliver it. +//! +//! The window has a second half. The insert releases the queue lock before +//! the `SteeringQueued` is emitted, so a teardown can land there too, drain +//! the entry and announce the discard ahead of the announcement that the +//! entry exists. The re-check then has nothing left to remove, and says so +//! anyway: what the view needs is a terminal event after every +//! `SteeringQueued`, not a unique one. +//! +//! @plan PLAN-20260903-ISSUE222.P07 +//! @plan PLAN-20260903-ISSUE222.P08 +//! @requirement REQ-222-003 +//! @requirement REQ-222-004 + +use super::*; +use crate::events::emit; +use crate::services::chat_impl::steering::QueuedSteering; +use crate::services::chat_impl::streaming::clear_streaming_state; +use tokio::sync::Notify; +use tokio_util::sync::CancellationToken; + +/// What the view was told about one steer. +/// +/// @plan PLAN-20260903-ISSUE222.P07 +/// @requirement REQ-222-003 +#[derive(Debug, PartialEq, Eq)] +enum Announcement { + Queued, + Discarded, +} + +/// Every announcement carrying `steer_id`, in the order it was emitted. +/// +/// Order is the assertion, not a detail of it: the view has to be told the +/// entry exists before it is told to take it away, or the withdrawal lands +/// on an entry that has not been rendered yet and that entry then stays on +/// screen for the rest of the session. +/// +/// @plan PLAN-20260903-ISSUE222.P07 +/// @requirement REQ-222-003 +fn announcements(events: &[ChatEvent], steer_id: Uuid) -> Vec { + events + .iter() + .filter_map(|event| match event { + ChatEvent::SteeringQueued { steer_id: id, .. } if *id == steer_id => { + Some(Announcement::Queued) + } + ChatEvent::SteeringDiscarded { steer_id: id, .. } if *id == steer_id => { + Some(Announcement::Discarded) + } + _ => None, + }) + .collect() +} + +/// The event that tells the collector it has seen everything the body did. +/// +/// The bus delivers in emission order, so a collector holding this has +/// already drained every event emitted before it. That is a fact about the +/// bus rather than an interval these tests hope is long enough under load. +/// Nothing here cancels a stream, so this id cannot be confused with a real +/// one. +fn end_marker(conversation_id: Uuid) -> AppEvent { + AppEvent::Chat(ChatEvent::StreamCancelled { + conversation_id, + message_id: Uuid::new_v4(), + partial_content: String::new(), + }) +} + +/// Run `body` while a task drains the event bus into this conversation's +/// chat events, and return both. +/// +/// The bus is a shared 16-slot ring. A receiver that only drains once the +/// work is finished can be lagged past the events under test by whatever +/// else is running in parallel, which turns an exact-count assertion into a +/// statement about how busy the process was. Draining concurrently keeps +/// these counts about this conversation. +/// +/// The collector signals `ready` the moment it holds a subscription and the +/// body does not start until that arrives: a broadcast receiver buffers from +/// the instant it exists, so a subscription in hand is exactly the guarantee +/// these counts need. +async fn events_during(conversation_id: Uuid, body: F) -> (T, Vec) +where + F: std::future::Future, +{ + /// Only bounds a hang; the collector stops on the marker, not on time. + const GIVE_UP_AFTER: Duration = Duration::from_secs(10); + + let ready = Arc::new(Notify::new()); + let ready_for_collector = ready.clone(); + let subscribed = CancellationToken::new(); + let subscribed_for_collector = subscribed.clone(); + + let collector = tokio::spawn(async move { + let mut rx = subscribe(); + // Subscribed. `Notify` keeps the permit even with nobody waiting + // yet, so this cannot be signalled into the void. + ready_for_collector.notify_one(); + subscribed_for_collector.cancel(); + + let mut matched = Vec::new(); + let deadline = tokio::time::Instant::now() + GIVE_UP_AFTER; + loop { + let remaining = deadline.saturating_duration_since(tokio::time::Instant::now()); + match tokio::time::timeout(remaining, rx.recv()).await { + Ok(Ok(AppEvent::Chat(event))) => { + if event_conversation_id(&event) != Some(conversation_id) { + continue; + } + if matches!(event, ChatEvent::StreamCancelled { .. }) { + return matched; + } + matched.push(event); + } + // Other subsystems' events, and lags caused by other tests' + // traffic, are skipped; keep draining. + Ok(Ok(_) | Err(tokio::sync::broadcast::error::RecvError::Lagged(_))) => {} + Ok(Err(tokio::sync::broadcast::error::RecvError::Closed)) => { + panic!("the event bus closed before {conversation_id} reached its end marker") + } + Err(elapsed) => panic!( + "waiting {GIVE_UP_AFTER:?} for {conversation_id} to reach its end marker: \ + {elapsed}, saw {matched:?}" + ), + } + } + }); + + ready.notified().await; + subscribed.cancelled().await; + + let outcome = body.await; + let _ = emit(end_marker(conversation_id)); + + let events = collector.await.expect("event collector must not panic"); + (outcome, events) +} + +/// Queue a steer through the real service path. +/// +/// Going through `ChatService::steer` rather than the queue directly is what +/// makes the entry and its `SteeringQueued` the production ones: the state +/// these tests re-check is the state an accepted steer actually leaves. +async fn accept_steer(service: &ChatServiceImpl, conversation_id: Uuid, text: &str) -> Uuid { + ChatService::steer(service, conversation_id, text.to_string()) + .await + .expect("the turn is running here, so the steer must be accepted") +} + +/// A steer whose turn is gone by the time its insert is re-checked is taken +/// back off the queue, announced as discarded, and refused. +/// +/// The refusal has to be the one the precondition check gives, or a caller +/// can tell which side of the window it landed on and starts caring. +/// +/// @plan PLAN-20260903-ISSUE222.P07 +/// @requirement REQ-222-003 +/// @requirement REQ-222-004 +#[tokio::test] +async fn steer_whose_turn_ended_during_the_insert_is_withdrawn_and_announced() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_id = Uuid::new_v4(); + + service + .begin_stream_for_test(conversation_id) + .expect("begin_stream should succeed"); + + let ((steer_id, error), events) = events_during(conversation_id, async { + let steer_id = accept_steer(&service, conversation_id, "take the other branch").await; + + // The turn ends, taking the stream slot with it and leaving this + // entry behind: the drain that would have collected it already ran. + service.clear_all_streams_for_test(); + + let error = service + .confirm_or_withdraw_steering(conversation_id, steer_id) + .expect_err("a steer with no turn left to deliver it must be refused"); + (steer_id, error) + }) + .await; + + assert!( + matches!(&error, ServiceError::Validation(message) if message.contains("No active turn to steer")), + "the refusal must be the one the precondition check gives, got: {error}" + ); + assert!( + drained_pairs(&service, conversation_id).is_empty(), + "a withdrawn steer must not stay on the queue" + ); + assert_eq!( + announcements(&events, steer_id), + vec![Announcement::Queued, Announcement::Discarded], + "the view must be told the entry exists exactly once and withdrawn exactly once, \ + in that order, got {events:?}" + ); +} + +/// Withdrawing one steer takes only that steer. +/// +/// The queue behind it may belong to a turn that is about to drain it, so +/// taking the whole queue would cancel instructions the user is still owed. +/// +/// @plan PLAN-20260903-ISSUE222.P07 +/// @requirement REQ-222-003 +#[tokio::test] +async fn withdrawing_one_steer_leaves_the_rest_of_the_queue_alone() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_id = Uuid::new_v4(); + + service + .begin_stream_for_test(conversation_id) + .expect("begin_stream should succeed"); + + let ((first, withdrawn, third), events) = events_during(conversation_id, async { + let first = accept_steer(&service, conversation_id, "first steer").await; + let withdrawn = accept_steer(&service, conversation_id, "second steer").await; + let third = accept_steer(&service, conversation_id, "third steer").await; + + service.clear_all_streams_for_test(); + service + .confirm_or_withdraw_steering(conversation_id, withdrawn) + .expect_err("a steer with no turn left to deliver it must be refused"); + (first, withdrawn, third) + }) + .await; + + assert_eq!( + drained_pairs(&service, conversation_id), + vec![ + (first, "first steer".to_string()), + (third, "third steer".to_string()), + ], + "only the withdrawn steer may be taken, and the rest must keep their order" + ); + assert_eq!( + discarded_ids(&events), + vec![withdrawn], + "only the withdrawn steer may be announced as discarded, got {events:?}" + ); +} + +/// An entry a teardown already drained is still announced as discarded. +/// +/// Finding the entry gone says nothing about whether the view has been told +/// what became of it: the teardown that took it may have announced its +/// discard before this steer's `SteeringQueued` was even emitted, in which +/// case the view withdrew an entry it had not rendered yet and then rendered +/// it. So the announcement is unconditional. Announcing twice costs nothing, +/// because the view removes by id and the second one finds nothing to take. +/// +/// @plan PLAN-20260903-ISSUE222.P07 +/// @plan PLAN-20260903-ISSUE222.P08 +/// @requirement REQ-222-003 +#[tokio::test] +async fn withdrawing_an_entry_a_teardown_already_drained_still_announces_a_discard() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_id = Uuid::new_v4(); + + service + .begin_stream_for_test(conversation_id) + .expect("begin_stream should succeed"); + + let (steer_id, events) = events_during(conversation_id, async { + let steer_id = accept_steer(&service, conversation_id, "already collected").await; + + // The teardown reached this queue first and took the entry with it. + service.clear_all_streams_for_test(); + assert_eq!( + drained_pairs(&service, conversation_id), + vec![(steer_id, "already collected".to_string())], + "the drain standing in for the teardown must be what took the entry" + ); + + service + .confirm_or_withdraw_steering(conversation_id, steer_id) + .expect_err("a steer with no turn left to deliver it must still be refused"); + steer_id + }) + .await; + + assert_eq!( + announcements(&events, steer_id), + vec![Announcement::Queued, Announcement::Discarded], + "an announced entry must reach a terminal event even when this call did not take it, \ + got {events:?}" + ); +} + +/// A steer whose entry a teardown drains before its `SteeringQueued` is even +/// emitted still reaches a terminal event. +/// +/// `queue_steering` inserts, releases the queue lock, and only then +/// announces the entry. A teardown that lands in that gap drains the queue +/// and announces the discard first, so the view processes a withdrawal for +/// an entry it has not rendered yet and then renders it. The re-check that +/// follows finds nothing left to remove, and if that silenced it the entry +/// would wait on screen for the rest of the session. +/// +/// The window is driven by call order — the production insert, the +/// production teardown, the announcement `queue_steering` emits next, then +/// the re-check — so this is a fact about the sequence rather than a race +/// two threads have to be caught in. +/// +/// @plan PLAN-20260903-ISSUE222.P08 +/// @requirement REQ-222-003 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_steer_drained_before_it_was_announced_still_reaches_a_terminal_event() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_id = Uuid::new_v4(); + + service + .begin_stream_for_test(conversation_id) + .expect("begin_stream should succeed"); + let stream_id = service + .stream_id_for_test(conversation_id) + .expect("the running turn must have a stream id"); + let (active_streams, steering_queues) = service.stream_registries_for_test(); + let steer_id = Uuid::new_v4(); + + let ((), events) = events_during(conversation_id, async { + // queue_steering has decided the turn is running and put the entry on + // the queue. Nothing has been announced yet. + assert!( + service.push_steering( + conversation_id, + QueuedSteering { + id: steer_id, + text: "take the other branch".to_string(), + }, + ), + "the queue has room, so the insert must take" + ); + + // The turn's teardown lands in the gap, takes the entry and reports + // it gone before anyone has been told it exists. + clear_streaming_state( + &active_streams, + &steering_queues, + conversation_id, + stream_id, + ); + + // queue_steering resumes and announces the entry it queued, knowing + // nothing of the teardown. + let _ = emit(AppEvent::Chat(ChatEvent::SteeringQueued { + conversation_id, + steer_id, + text: "take the other branch".to_string(), + })); + + service + .confirm_or_withdraw_steering(conversation_id, steer_id) + .expect_err("a steer with no turn left to deliver it must be refused"); + }) + .await; + + let announced = announcements(&events, steer_id); + assert_eq!( + announced, + vec![ + Announcement::Discarded, + Announcement::Queued, + Announcement::Discarded, + ], + "the teardown's discard arrives before the entry is announced, so the re-check owes \ + the view another one, got {events:?}" + ); + assert_eq!( + announced.last(), + Some(&Announcement::Discarded), + "the last thing said about a steer nobody will deliver must be that it is gone, \ + got {events:?}" + ); +} + +/// A steer whose turn is still running is confirmed, not withdrawn. +/// +/// The re-check runs on every accepted steer, so the ordinary path is the +/// one it has to leave alone: the entry stays queued for its boundary, the +/// caller gets its id, and the view hears nothing beyond the announcement +/// that it is waiting. +/// +/// @plan PLAN-20260903-ISSUE222.P07 +/// @requirement REQ-222-003 +/// @requirement REQ-222-004 +#[tokio::test] +async fn steer_against_a_running_turn_is_confirmed_and_never_discarded() { + let _steering_bus_guard = lock_steering_bus().await; + let service = make_test_chat_service(); + let conversation_id = Uuid::new_v4(); + + service + .begin_stream_for_test(conversation_id) + .expect("begin_stream should succeed"); + + let (steer_id, events) = events_during(conversation_id, async { + ChatService::steer(&service, conversation_id, "keep going".to_string()) + .await + .expect("steering a running turn must be accepted") + }) + .await; + + assert!( + ChatService::is_streaming_for(&service, conversation_id), + "the re-check must not disturb the turn it is checking" + ); + assert_eq!( + drained_pairs(&service, conversation_id), + vec![(steer_id, "keep going".to_string())], + "an accepted steer must stay queued for its delivery boundary" + ); + assert_eq!( + announcements(&events, steer_id), + vec![Announcement::Queued], + "a steer the turn can still deliver must be announced queued and nothing else, \ + got {events:?}" + ); + + service.clear_all_streams_for_test(); +} diff --git a/src/services/chat_impl/tests/steering_delivery.rs b/src/services/chat_impl/tests/steering_delivery.rs new file mode 100644 index 0000000..32bcce5 --- /dev/null +++ b/src/services/chat_impl/tests/steering_delivery.rs @@ -0,0 +1,917 @@ +//! Delivery of queued steering messages at the turn boundary. +//! +//! Covers issue #222: when a turn finishes with steering messages waiting, +//! the send chains another turn seeded with the conversation so far plus the +//! steering text. A cancelled or failed turn chains nothing, finalization +//! still happens exactly once, and the chain is bounded. +//! +//! The turn itself is the only double here: each test hands the delivery loop +//! a runner that records the history it was seeded with and replies with a +//! scripted transcript, which is the same level the approval-gate tests in +//! `three_stream_concurrency.rs` operate at. +//! +//! @plan PLAN-20260903-ISSUE222.P02 +//! @requirement REQ-222-005 +//! @requirement REQ-222-006 +//! @requirement REQ-222-007 +//! @requirement REQ-222-008 + +use super::*; +use crate::events::subscribe; +use crate::events::types::ChatEvent; +use crate::llm::Role as LlmRole; +use crate::services::chat_impl::streaming::steering_delivery::{ + run_steered_turns_and_finalize, SteeringDeliveryContext, MAX_STEERING_TURNS, +}; +use std::pin::Pin; +use std::sync::atomic::{AtomicBool, Ordering}; +use tokio::sync::Notify; +use tokio::time::Duration; + +mod discard; + +/// The future a scripted turn runner hands back. +/// +/// Boxed because one runner is a constant reply while another awaits a tool +/// approval decision, and the delivery loop takes a single future type. +type TurnFuture = Pin + Send>>; + +/// The seed history handed to each turn, in order. +type TurnLog = Arc>>>; + +/// A turn that produced `text` and ended cleanly. +fn completed_turn(text: &str) -> streaming::StreamTranscript { + streaming::StreamTranscript { + response_text: text.to_string(), + completed: true, + ..streaming::StreamTranscript::default() + } +} + +/// A `CompressionResult` that leaves the conversation's context untouched +/// apart from what finalization itself writes. +fn no_compression() -> CompressionResult { + CompressionResult { + messages: vec![], + phase: crate::models::CompressionPhase::None, + masked_tool_seqs: None, + summary_range: None, + preserved_facts: None, + estimated_tokens: 0, + } +} + +/// `(role, content)` for every message in a turn's seed history. +/// +/// `LlmMessage` has no `PartialEq`, and role plus content is what an +/// ordering assertion is actually about. +fn shape(messages: &[LlmMessage]) -> Vec<(LlmRole, String)> { + messages + .iter() + .map(|message| (message.role, message.content.clone())) + .collect() +} + +/// `(role, content)` for every message persisted to the conversation. +async fn persisted_shape( + conversations: &Arc, +) -> Vec<(MessageRole, String)> { + conversations + .messages + .read() + .await + .iter() + .map(|message| (message.role.clone(), message.content.clone())) + .collect() +} + +/// Conversation id carried by the chat events these tests assert on. +fn event_conversation_id(event: &ChatEvent) -> Option { + match event { + ChatEvent::SteeringQueued { + conversation_id, .. + } + | ChatEvent::SteeringDelivered { + conversation_id, .. + } + | ChatEvent::SteeringDiscarded { + conversation_id, .. + } + | ChatEvent::StreamCompleted { + conversation_id, .. + } + | ChatEvent::StreamCancelled { + conversation_id, .. + } + | ChatEvent::StreamError { + conversation_id, .. + } => Some(*conversation_id), + _ => None, + } +} + +/// The `steer_id`s carried by the `SteeringDelivered` events, in order. +fn delivered_ids(events: &[ChatEvent]) -> Vec { + events + .iter() + .filter_map(|event| match event { + ChatEvent::SteeringDelivered { steer_id, .. } => Some(*steer_id), + _ => None, + }) + .collect() +} + +/// The `steer_id`s carried by the `SteeringQueued` events, in order. +fn queued_ids(events: &[ChatEvent]) -> Vec { + events + .iter() + .filter_map(|event| match event { + ChatEvent::SteeringQueued { steer_id, .. } => Some(*steer_id), + _ => None, + }) + .collect() +} + +/// The `steer_id`s carried by the `SteeringDiscarded` events, in order. +/// +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-003 +fn discarded_ids(events: &[ChatEvent]) -> Vec { + events + .iter() + .filter_map(|event| match event { + ChatEvent::SteeringDiscarded { steer_id, .. } => Some(*steer_id), + _ => None, + }) + .collect() +} + +/// How many `StreamCompleted` events were seen. +fn completion_count(events: &[ChatEvent]) -> usize { + events + .iter() + .filter(|event| matches!(event, ChatEvent::StreamCompleted { .. })) + .count() +} + +/// Run `body` while a task drains the event bus into this conversation's +/// chat events. +/// +/// The bus is a shared 16-slot ring, so a receiver that only drains after the +/// fact can be lagged past the events under test by other tests running in +/// parallel. Draining concurrently keeps the exact-count assertions honest. +/// +/// The collector signals `ready` the moment it holds a subscription, and +/// `body` does not start until that signal arrives. A broadcast receiver +/// buffers from the instant it is created, so a subscription in hand is +/// exactly the guarantee the exact-count assertions need. It is also a fact +/// the collector reports, rather than an interval this test hopes is long +/// enough under load. +async fn events_during(conversation_id: Uuid, body: F) -> (T, Vec) +where + F: std::future::Future, +{ + let stop = CancellationToken::new(); + let stop_for_collector = stop.clone(); + let ready = Arc::new(Notify::new()); + let ready_for_collector = ready.clone(); + + let collector = tokio::spawn(async move { + let mut rx = subscribe(); + // Subscribed. `Notify` holds the permit even if nobody is waiting + // yet, so this cannot be signalled into the void. + ready_for_collector.notify_one(); + + let mut matched = Vec::new(); + loop { + tokio::select! { + received = rx.recv() => match received { + Ok(AppEvent::Chat(event)) => { + if event_conversation_id(&event) == Some(conversation_id) { + matched.push(event); + } + } + // Other subsystems' events, and lags caused by other + // tests' traffic, are skipped; keep draining. + Ok(_) | Err(tokio::sync::broadcast::error::RecvError::Lagged(_)) => {} + Err(tokio::sync::broadcast::error::RecvError::Closed) => break, + }, + () = stop_for_collector.cancelled() => break, + } + } + matched + }); + + ready.notified().await; + + let outcome = body.await; + + tokio::time::sleep(Duration::from_millis(50)).await; + stop.cancel(); + let events = collector.await.expect("event collector must not panic"); + (outcome, events) +} + +/// A conversation with a running turn, wired to the registries the delivery +/// loop operates on. +struct DeliveryFixture { + service: Arc, + conversations: Arc, + conversation_service: Arc, + conversation_id: Uuid, + stream_id: Uuid, + active_streams: Arc>>, + steering_queues: SteeringQueues, + cancel: CancellationToken, +} + +impl DeliveryFixture { + /// Model label recorded on every assistant message these tests persist. + const MODEL_LABEL: &'static str = "Steered Model"; + + fn new() -> Self { + let conversations = Arc::new(MockConversationService::new(Uuid::new_v4())); + let conversation_service = + conversations.clone() as Arc; + Self::with_conversation_service(conversations, conversation_service) + } + + /// A fixture whose turns persist through `conversation_service`, with + /// `conversations` the mock underneath it that records what landed. + /// + /// The two are the same object for `new`. The discard tests interpose a + /// double so a write can fail, or can steer the turn that is tearing + /// down around it. + /// + /// @plan PLAN-20260903-ISSUE222.P06 + /// @requirement REQ-222-003 + fn with_conversation_service( + conversations: Arc, + conversation_service: Arc, + ) -> Self { + let profile_service = + Arc::new(MockProfileService::new()) as Arc; + let service = Arc::new(ChatServiceImpl::new_for_tests( + conversation_service.clone(), + profile_service, + )); + + let conversation_id = Uuid::new_v4(); + service + .begin_stream_for_test(conversation_id) + .expect("begin_stream should succeed"); + let stream_id = service + .stream_id_for_test(conversation_id) + .expect("the reserved stream should be readable back"); + let (active_streams, steering_queues) = service.stream_registries_for_test(); + + Self { + service, + conversations, + conversation_service, + conversation_id, + stream_id, + active_streams, + steering_queues, + cancel: CancellationToken::new(), + } + } + + /// The history the first turn is seeded with. + fn initial_messages() -> Vec { + vec![ + LlmMessage::system("be helpful"), + LlmMessage::user("original prompt"), + ] + } + + /// Queue a steering message through the real service path. + async fn steer(&self, text: &str) -> Uuid { + ChatService::steer( + self.service.as_ref(), + self.conversation_id, + text.to_string(), + ) + .await + .expect("steering a running turn must be accepted") + } + + /// Run this send's turns through the delivery loop and finalize. + async fn run_turns(&self, run_turn: R) + where + R: FnMut(Vec) -> TurnFuture, + { + let ctx = SteeringDeliveryContext { + finalize: streaming::StreamFinalizeContext { + conversation_service: &self.conversation_service, + conversation_id: self.conversation_id, + stream_id: self.stream_id, + active_streams: &self.active_streams, + steering_queues: &self.steering_queues, + }, + cancel: &self.cancel, + model_label: Self::MODEL_LABEL, + }; + run_steered_turns_and_finalize(&ctx, no_compression(), Self::initial_messages(), run_turn) + .await; + } + + fn is_streaming(&self) -> bool { + ChatService::is_streaming_for(self.service.as_ref(), self.conversation_id) + } +} + +/// A runner that logs each turn's seed history and replies with the next +/// scripted completed transcript. +fn scripted_runner( + log: &TurnLog, + replies: &'static [&'static str], +) -> impl FnMut(Vec) -> TurnFuture { + let log = log.clone(); + move |messages: Vec| { + let index = { + let mut turns = log.lock().expect("turn log poisoned"); + turns.push(messages); + turns.len() - 1 + }; + let reply = replies + .get(index) + .copied() + .expect("the turn runner ran out of scripted replies"); + Box::pin(std::future::ready(completed_turn(reply))) as TurnFuture + } +} + +fn new_turn_log() -> TurnLog { + Arc::new(StdMutex::new(Vec::new())) +} + +/// The seed histories the runner recorded, consuming the log. +fn recorded(log: &TurnLog) -> Vec> { + log.lock().expect("turn log poisoned").clone() +} + +/// An empty queue must leave a send exactly as it is today: one turn, one +/// completion, and the assistant output persisted once. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-005 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn empty_steering_queue_runs_a_single_turn_and_finalizes_once() { + let _steering_bus_guard = lock_steering_bus().await; + let fixture = DeliveryFixture::new(); + let log = new_turn_log(); + + let ((), events) = events_during(fixture.conversation_id, async { + fixture + .run_turns(scripted_runner(&log, &["only answer"])) + .await; + }) + .await; + + let turns = recorded(&log); + assert_eq!(turns.len(), 1, "an empty queue must run exactly one turn"); + assert_eq!( + shape(&turns[0]), + shape(&DeliveryFixture::initial_messages()), + "the single turn must be seeded with the prepared history, unchanged" + ); + + assert_eq!( + persisted_shape(&fixture.conversations).await, + vec![(MessageRole::Assistant, "only answer".to_string())], + "the assistant output must be persisted exactly once, with nothing else" + ); + let persisted = fixture.conversations.messages.read().await.clone(); + assert!( + !persisted[0].interrupted, + "a cleanly finished turn must not be marked interrupted" + ); + assert_eq!( + persisted[0].model_id.as_deref(), + Some(DeliveryFixture::MODEL_LABEL) + ); + assert!( + fixture.conversations.context_state.read().await.is_some(), + "normal finalization must still persist context state" + ); + + assert_eq!( + completion_count(&events), + 1, + "an empty queue must emit exactly one StreamCompleted, got {events:?}" + ); + assert!( + delivered_ids(&events).is_empty(), + "an empty queue must deliver no steering, got {events:?}" + ); + assert!( + !fixture.is_streaming(), + "finalization must release the conversation's stream slot" + ); +} + +/// A queued steer chains a second turn whose history carries the steer as a +/// user message after the first turn's assistant output. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-005 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn queued_steer_chains_a_second_turn_carrying_the_steer_text() { + let _steering_bus_guard = lock_steering_bus().await; + let fixture = DeliveryFixture::new(); + fixture.steer("focus on the parser").await; + let log = new_turn_log(); + + fixture + .run_turns(scripted_runner(&log, &["first answer", "second answer"])) + .await; + + let turns = recorded(&log); + assert_eq!(turns.len(), 2, "a queued steer must chain a second turn"); + assert_eq!( + shape(&turns[1]), + vec![ + (LlmRole::System, "be helpful".to_string()), + (LlmRole::User, "original prompt".to_string()), + (LlmRole::Assistant, "first answer".to_string()), + (LlmRole::User, "focus on the parser".to_string()), + ], + "the second turn must be seeded with the finished turn plus the steer" + ); +} + +/// Two steers queued against the same boundary arrive in submission order. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-005 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn two_queued_steers_are_delivered_in_fifo_order() { + let _steering_bus_guard = lock_steering_bus().await; + let fixture = DeliveryFixture::new(); + fixture.steer("first steer").await; + fixture.steer("second steer").await; + let log = new_turn_log(); + + fixture + .run_turns(scripted_runner(&log, &["first answer", "second answer"])) + .await; + + let turns = recorded(&log); + assert_eq!( + turns.len(), + 2, + "queued steers must chain one follow-up turn" + ); + assert_eq!( + shape(&turns[1]), + vec![ + (LlmRole::System, "be helpful".to_string()), + (LlmRole::User, "original prompt".to_string()), + (LlmRole::Assistant, "first answer".to_string()), + (LlmRole::User, "first steer".to_string()), + (LlmRole::User, "second steer".to_string()), + ], + "both steers must be seeded in FIFO order" + ); + assert_eq!( + persisted_shape(&fixture.conversations).await, + vec![ + (MessageRole::Assistant, "first answer".to_string()), + (MessageRole::User, "first steer".to_string()), + (MessageRole::User, "second steer".to_string()), + (MessageRole::Assistant, "second answer".to_string()), + ], + "persistence must record the steers in FIFO order too" + ); +} + +/// Every delivered steer announces itself once, under the id its +/// `SteeringQueued` reported. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-005 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn steering_delivered_is_emitted_once_per_steer_with_the_queued_id() { + let _steering_bus_guard = lock_steering_bus().await; + let fixture = DeliveryFixture::new(); + let log = new_turn_log(); + + let ((first_id, second_id), events) = events_during(fixture.conversation_id, async { + let first_id = fixture.steer("first steer").await; + let second_id = fixture.steer("second steer").await; + fixture + .run_turns(scripted_runner(&log, &["first answer", "second answer"])) + .await; + (first_id, second_id) + }) + .await; + + assert_eq!( + delivered_ids(&events), + vec![first_id, second_id], + "each steer must be delivered once, under the id steer() returned, got {events:?}" + ); +} + +/// A cancelled turn never reaches a delivery boundary: it drains nothing and +/// chains nothing, even though it finished cleanly. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-006 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cancelled_turn_drains_nothing_and_chains_no_follow_up() { + let _steering_bus_guard = lock_steering_bus().await; + let fixture = DeliveryFixture::new(); + fixture.steer("too late").await; + let log = new_turn_log(); + let log_for_runner = log.clone(); + let cancel = fixture.cancel.clone(); + + let ((), events) = events_during(fixture.conversation_id, async { + fixture + .run_turns(move |messages: Vec| { + log_for_runner + .lock() + .expect("turn log poisoned") + .push(messages); + // The user stopped the turn while it was finishing, so the + // transcript is clean but the token has fired. + cancel.cancel(); + Box::pin(std::future::ready(completed_turn("interrupted answer"))) as TurnFuture + }) + .await; + }) + .await; + + assert_eq!( + recorded(&log).len(), + 1, + "a cancelled turn must not chain a follow-up" + ); + assert!( + delivered_ids(&events).is_empty(), + "a cancelled turn must deliver no steering, got {events:?}" + ); + assert!( + !persisted_shape(&fixture.conversations) + .await + .iter() + .any(|(role, content)| *role == MessageRole::User && content == "too late"), + "a cancelled turn must not persist the queued steer" + ); +} + +/// A failed turn drains nothing and chains nothing; its partial output is +/// still finalized as interrupted. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-006 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn failed_turn_drains_nothing_and_chains_no_follow_up() { + let _steering_bus_guard = lock_steering_bus().await; + let fixture = DeliveryFixture::new(); + fixture.steer("never delivered").await; + let log = new_turn_log(); + let log_for_runner = log.clone(); + + let ((), events) = events_during(fixture.conversation_id, async { + fixture + .run_turns(move |messages: Vec| { + log_for_runner + .lock() + .expect("turn log poisoned") + .push(messages); + Box::pin(std::future::ready(streaming::StreamTranscript { + response_text: "partial answer".to_string(), + error: Some("provider reset the connection".to_string()), + ..streaming::StreamTranscript::default() + })) as TurnFuture + }) + .await; + }) + .await; + + assert_eq!( + recorded(&log).len(), + 1, + "a failed turn must not chain a follow-up" + ); + assert!( + delivered_ids(&events).is_empty(), + "a failed turn must deliver no steering, got {events:?}" + ); + assert_eq!( + completion_count(&events), + 0, + "a failed turn must not emit StreamCompleted, got {events:?}" + ); + let persisted = fixture.conversations.messages.read().await.clone(); + assert_eq!( + persisted.len(), + 1, + "a failed turn must persist only its own partial output" + ); + assert!( + persisted[0].interrupted, + "a failed turn's output must carry the interrupted marker" + ); +} + +/// The steering user message lands between the assistant output that +/// preceded it and the assistant output it produced, so a reload replays the +/// turn in the order the user experienced it. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-007 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn steering_message_persists_between_the_assistant_outputs() { + let _steering_bus_guard = lock_steering_bus().await; + let fixture = DeliveryFixture::new(); + fixture.steer("adjust course").await; + let log = new_turn_log(); + let log_for_runner = log.clone(); + + fixture + .run_turns(move |messages: Vec| { + let index = { + let mut turns = log_for_runner.lock().expect("turn log poisoned"); + turns.push(messages); + turns.len() - 1 + }; + let transcript = if index == 0 { + streaming::StreamTranscript { + response_text: "before the steer".to_string(), + thinking_text: "weighing options".to_string(), + tool_calls: vec![crate::llm::tools::ToolUse::new( + "call-1", + "web_search", + serde_json::json!({"query": "rust"}), + )], + tool_results: vec![crate::llm::tools::ToolResult::success("call-1", "ok")], + completed: true, + ..streaming::StreamTranscript::default() + } + } else { + completed_turn("after the steer") + }; + Box::pin(std::future::ready(transcript)) as TurnFuture + }) + .await; + + assert_eq!( + persisted_shape(&fixture.conversations).await, + vec![ + (MessageRole::Assistant, "before the steer".to_string()), + (MessageRole::User, "adjust course".to_string()), + (MessageRole::Assistant, "after the steer".to_string()), + ], + "the steer must be ordered after the preceding output and before the following one" + ); + + let persisted = fixture.conversations.messages.read().await.clone(); + assert!( + persisted[0].tool_calls.is_some() && persisted[0].tool_results.is_some(), + "the preceding turn's tool transcript must survive the chained turn" + ); + + let turns = recorded(&log); + let second = &turns[1]; + let preceding = &second[second.len() - 2]; + assert_eq!( + preceding.thinking_content.as_deref(), + Some("weighing options"), + "the chained history must carry the finished turn's thinking" + ); + assert_eq!( + preceding.tool_uses.len(), + 1, + "the chained history must carry the finished turn's tool calls" + ); + assert_eq!( + preceding.tool_results.len(), + 1, + "the chained history must carry the finished turn's tool results" + ); +} + +/// A chained send completes once, not once per turn. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-005 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn stream_completed_is_emitted_once_across_a_chained_send() { + let _steering_bus_guard = lock_steering_bus().await; + let fixture = DeliveryFixture::new(); + let log = new_turn_log(); + + let ((), events) = events_during(fixture.conversation_id, async { + fixture.steer("keep going").await; + fixture + .run_turns(scripted_runner(&log, &["first answer", "second answer"])) + .await; + }) + .await; + + assert_eq!(recorded(&log).len(), 2, "the send must run two turns"); + assert_eq!( + completion_count(&events), + 1, + "a chained send must emit exactly one StreamCompleted, got {events:?}" + ); + assert_eq!( + persisted_shape(&fixture.conversations).await, + vec![ + (MessageRole::Assistant, "first answer".to_string()), + (MessageRole::User, "keep going".to_string()), + (MessageRole::Assistant, "second answer".to_string()), + ], + "each turn's output must be persisted exactly once" + ); +} + +/// A user who keeps steering cannot chain turns forever: the send stops at +/// `MAX_STEERING_TURNS`. +/// +/// Every follow-up turn steers again through the real service path, which +/// only succeeds while the conversation still holds its stream slot — so this +/// also pins that the slot survives the chain. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-005 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn chain_stops_at_the_turn_cap_when_steering_keeps_refilling() { + let _steering_bus_guard = lock_steering_bus().await; + let fixture = DeliveryFixture::new(); + fixture.steer("steer 0").await; + let log = new_turn_log(); + let log_for_runner = log.clone(); + let service = fixture.service.clone(); + let conversation_id = fixture.conversation_id; + + let ((), events) = events_during(conversation_id, async { + fixture + .run_turns(move |messages: Vec| { + let index = { + let mut turns = log_for_runner.lock().expect("turn log poisoned"); + turns.push(messages); + turns.len() + }; + let service = service.clone(); + Box::pin(async move { + ChatService::steer(service.as_ref(), conversation_id, format!("steer {index}")) + .await + .expect("the conversation must still be steerable mid-chain"); + completed_turn("answer") + }) as TurnFuture + }) + .await; + }) + .await; + + assert_eq!( + recorded(&log).len(), + MAX_STEERING_TURNS, + "a refilling queue must stop the chain at the turn cap" + ); + assert_eq!( + completion_count(&events), + 1, + "a capped chain must still emit exactly one StreamCompleted, got {events:?}" + ); +} + +/// A turn that stops at a real tool approval, and what it was told. +/// +/// The first turn takes the waiter, announces that it has reached the gate, +/// and blocks until someone decides. `decided` therefore distinguishes "the +/// steer resolved the approval" from "the user did". +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-008 +struct ApprovalProbe { + waiter: StdMutex>, + reached: tokio::sync::Notify, + decided: AtomicBool, + approved: AtomicBool, +} + +impl ApprovalProbe { + fn new(waiter: crate::llm::client_agent::ApprovalWaiter) -> Arc { + Arc::new(Self { + waiter: StdMutex::new(Some(waiter)), + reached: tokio::sync::Notify::new(), + decided: AtomicBool::new(false), + approved: AtomicBool::new(false), + }) + } + + /// A turn runner whose first turn blocks on the approval decision. + fn runner(self: &Arc, log: &TurnLog) -> impl FnMut(Vec) -> TurnFuture { + let probe = self.clone(); + let log = log.clone(); + move |messages: Vec| { + let index = { + let mut turns = log.lock().expect("turn log poisoned"); + turns.push(messages); + turns.len() - 1 + }; + let probe = probe.clone(); + Box::pin(async move { + if index == 0 { + probe.await_decision().await; + completed_turn("ran the tool") + } else { + completed_turn("followed the steer") + } + }) as TurnFuture + } + } + + async fn await_decision(&self) { + let pending = self + .waiter + .lock() + .expect("waiter slot poisoned") + .take() + .expect("the first turn owns the approval waiter"); + self.reached.notify_one(); + let decision = pending.wait().await.expect("the approval must resolve"); + self.approved.store(decision, Ordering::SeqCst); + self.decided.store(true, Ordering::SeqCst); + } +} + +/// A steer submitted while a tool approval is pending neither resolves nor +/// cancels that approval, and lands only after the decision lets the turn +/// reach its boundary. +/// +/// @plan PLAN-20260903-ISSUE222.P02 +/// @requirement REQ-222-008 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn steer_during_a_pending_approval_waits_for_the_decision() { + let _steering_bus_guard = lock_steering_bus().await; + let fixture = DeliveryFixture::new(); + let log = new_turn_log(); + let request_id = Uuid::new_v4().to_string(); + let probe = ApprovalProbe::new(fixture.service.approval_gate.wait_for_approval( + request_id.clone(), + "WriteFile".to_string(), + fixture.conversation_id, + )); + let steer_id = StdMutex::new(None); + + let (((), ()), events) = events_during(fixture.conversation_id, async { + tokio::join!(fixture.run_turns(probe.runner(&log)), async { + probe.reached.notified().await; + *steer_id.lock().expect("steer slot poisoned") = + Some(fixture.steer("write the other file instead").await); + + // Let the waiting turn observe a decision it must not have been + // given. + tokio::task::yield_now().await; + assert!( + !probe.decided.load(Ordering::SeqCst), + "a steer must not resolve a pending tool approval" + ); + + fixture + .service + .resolve_tool_approval(request_id.clone(), ToolApprovalResponseAction::ProceedOnce) + .await + .expect("resolving the approval must succeed"); + }) + }) + .await; + + assert!( + probe.approved.load(Ordering::SeqCst), + "the approval must resolve from the user's decision, approved" + ); + assert!( + events + .iter() + .all(|event| !matches!(event, ChatEvent::StreamCancelled { .. })), + "a steer must not cancel the turn holding the approval, got {events:?}" + ); + + let turns = recorded(&log); + assert_eq!( + turns.len(), + 2, + "the steer must be delivered after the approved turn reaches its boundary" + ); + assert_eq!( + shape(&turns[1]).last().cloned(), + Some((LlmRole::User, "write the other file instead".to_string())), + "the chained turn must carry the steer queued during the approval" + ); + assert_eq!( + delivered_ids(&events), + vec![steer_id + .lock() + .expect("steer slot poisoned") + .expect("the driver must have queued a steer")], + "the steer must be announced as delivered exactly once, got {events:?}" + ); +} diff --git a/src/services/chat_impl/tests/steering_delivery/discard.rs b/src/services/chat_impl/tests/steering_delivery/discard.rs new file mode 100644 index 0000000..707a0a4 --- /dev/null +++ b/src/services/chat_impl/tests/steering_delivery/discard.rs @@ -0,0 +1,339 @@ +//! The terminal state of a queued steering message that is never delivered. +//! +//! A steer the service accepted is rendered as a waiting entry, and the view +//! only stops rendering it when something says what became of it. Delivery +//! says that. So must every path that ends a turn with entries still queued, +//! or the user is left watching an instruction that will never be acted on. +//! +//! Two of those paths are driven here: a steer accepted in the window between +//! the delivery loop's last drain and the release of the stream slot, and a +//! steer whose own persistence fails. Both are driven from inside a +//! conversation-service write, so the window is a fact of the call order +//! rather than something a sleep hopes to hit. +//! +//! @plan PLAN-20260903-ISSUE222.P06 +//! @requirement REQ-222-003 +//! @requirement REQ-222-007 + +use super::*; +use std::sync::{OnceLock, Weak}; + +/// A steer submitted from inside a write the turn makes while tearing down. +/// +/// Holds the service weakly: the service owns the conversation service that +/// owns this, so an owning handle would close a cycle that never drops. +struct LateSteer { + service: OnceLock>, + conversation_id: OnceLock, + text: &'static str, + steer_id: StdMutex>, +} + +impl LateSteer { + fn new(text: &'static str) -> Arc { + Arc::new(Self { + service: OnceLock::new(), + conversation_id: OnceLock::new(), + text, + steer_id: StdMutex::new(None), + }) + } + + /// Point this at the fixture's service, once the fixture exists. + fn attach(&self, fixture: &DeliveryFixture) { + self.service + .set(Arc::downgrade(&fixture.service)) + .unwrap_or_else(|_| panic!("the late steer must be attached exactly once")); + self.conversation_id + .set(fixture.conversation_id) + .expect("the late steer must be attached exactly once"); + } + + /// Steer the running turn through the real service path, recording the + /// id it was accepted under. + /// + /// Going through `ChatService::steer` rather than the queue directly is + /// the point: it proves the conversation still passes `is_streaming_for` + /// at this moment, which is what makes the window reachable in + /// production. + async fn fire(&self) { + let service = self + .service + .get() + .expect("the late steer must be attached before the turn runs") + .upgrade() + .expect("the fixture keeps the service alive for the whole turn"); + let conversation_id = *self + .conversation_id + .get() + .expect("the late steer must be attached before the turn runs"); + + let steer_id = ChatService::steer(service.as_ref(), conversation_id, self.text.to_string()) + .await + .expect("the turn is still Running here, so the steer must be accepted"); + *self.steer_id.lock().expect("steer slot poisoned") = Some(steer_id); + } + + fn steer_id(&self) -> Option { + *self.steer_id.lock().expect("steer slot poisoned") + } +} + +/// What the interposed conversation service does to the turn writing through +/// it. +enum Interference { + /// Refuse to persist user messages. Assistant output still lands, so the + /// chain is intact right up to the steering message that cannot be + /// recorded. + RefuseUserMessages, + /// Steer the conversation from inside `update_context_state`, which + /// finalization calls after the delivery loop's final drain and before it + /// releases the stream slot. + SteerDuringTeardown(Arc), +} + +/// A conversation service that delegates to the mock and interferes with one +/// write on the way through. +struct InterferingConversationService { + inner: Arc, + interference: Interference, +} + +impl InterferingConversationService { + /// Wrap `inner`, ready to hand to a fixture as its conversation service. + fn interposed( + inner: Arc, + interference: Interference, + ) -> Arc { + Arc::new(Self { + inner, + interference, + }) + } +} + +#[async_trait::async_trait] +impl crate::services::ConversationService for InterferingConversationService { + async fn create( + &self, + title: Option, + model_profile_id: Uuid, + ) -> Result { + self.inner.create(title, model_profile_id).await + } + + async fn load(&self, id: Uuid) -> Result { + self.inner.load(id).await + } + + async fn list_metadata( + &self, + limit: Option, + offset: Option, + ) -> Result, ServiceError> { + self.inner.list_metadata(limit, offset).await + } + + async fn add_message( + &self, + conversation_id: Uuid, + message: Message, + ) -> Result { + if matches!(self.interference, Interference::RefuseUserMessages) + && message.role == MessageRole::User + { + return Err(ServiceError::Storage( + "simulated steering persistence failure".to_string(), + )); + } + self.inner.add_message(conversation_id, message).await + } + + async fn search( + &self, + query: &str, + limit: Option, + offset: Option, + ) -> Result, ServiceError> { + self.inner.search(query, limit, offset).await + } + + async fn message_count(&self, conversation_id: Uuid) -> Result { + self.inner.message_count(conversation_id).await + } + + async fn update_context_state( + &self, + id: Uuid, + state: &crate::models::ContextState, + ) -> Result<(), ServiceError> { + if let Interference::SteerDuringTeardown(late) = &self.interference { + late.fire().await; + } + self.inner.update_context_state(id, state).await + } + + async fn get_context_state( + &self, + id: Uuid, + ) -> Result, ServiceError> { + self.inner.get_context_state(id).await + } + + async fn rename(&self, id: Uuid, new_title: String) -> Result<(), ServiceError> { + self.inner.rename(id, new_title).await + } + + async fn delete(&self, id: Uuid) -> Result<(), ServiceError> { + self.inner.delete(id).await + } + + async fn set_active(&self, id: Uuid) -> Result<(), ServiceError> { + self.inner.set_active(id).await + } + + async fn get_active(&self) -> Result, ServiceError> { + self.inner.get_active().await + } + + async fn get_messages(&self, conversation_id: Uuid) -> Result, ServiceError> { + self.inner.get_messages(conversation_id).await + } + + async fn update( + &self, + id: Uuid, + title: Option, + model_profile_id: Option, + ) -> Result { + self.inner.update(id, title, model_profile_id).await + } +} + +/// A steer accepted after the delivery loop's last drain, while the turn is +/// still `Running`, is announced as discarded rather than dropped. +/// +/// The steer is submitted from inside `update_context_state`: finalization +/// calls that after the final drain and before it releases the stream slot, +/// which is precisely the window the entry used to vanish in. The service +/// accepts it — `SteeringQueued` proves the view was told to render it — and +/// then nothing is ever going to deliver it, so something has to say so. +/// +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-003 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_steer_accepted_during_teardown_is_announced_as_discarded() { + let _steering_bus_guard = lock_steering_bus().await; + let conversations = Arc::new(MockConversationService::new(Uuid::new_v4())); + let late = LateSteer::new("too late to be delivered"); + let fixture = DeliveryFixture::with_conversation_service( + conversations.clone(), + InterferingConversationService::interposed( + conversations, + Interference::SteerDuringTeardown(late.clone()), + ), + ); + late.attach(&fixture); + let log = new_turn_log(); + + let ((), events) = events_during(fixture.conversation_id, async { + fixture + .run_turns(scripted_runner(&log, &["only answer"])) + .await; + }) + .await; + + let steer_id = late + .steer_id() + .expect("the teardown write must have queued a steer"); + + assert_eq!( + recorded(&log).len(), + 1, + "a steer that arrives during teardown must not chain a turn" + ); + assert_eq!( + queued_ids(&events), + vec![steer_id], + "the late steer must be accepted and announced as queued, so the view renders it, \ + got {events:?}" + ); + assert_eq!( + discarded_ids(&events), + vec![steer_id], + "a steer accepted after the last drain must be announced as discarded, got {events:?}" + ); + assert!( + delivered_ids(&events).is_empty(), + "a steer no turn ever handed to the model must not be reported delivered, got {events:?}" + ); + assert!( + !fixture.is_streaming(), + "finalization must still release the conversation's stream slot" + ); +} + +/// A steering message the store refuses is not chained over. +/// +/// Persisting the steer is what keeps the chained history equal to what a +/// reload rebuilds. When that write fails, seeding the model with the text +/// anyway would run a turn over a conversation the database does not have, so +/// the chain stops here. The entry is announced as discarded, not delivered: +/// the view withdraws it either way, and reporting delivery would claim the +/// instruction reached the model when it did not. +/// +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-003 +/// @requirement REQ-222-007 +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_steer_that_cannot_be_persisted_is_discarded_and_chains_nothing() { + let _steering_bus_guard = lock_steering_bus().await; + let conversations = Arc::new(MockConversationService::new(Uuid::new_v4())); + let fixture = DeliveryFixture::with_conversation_service( + conversations.clone(), + InterferingConversationService::interposed(conversations, Interference::RefuseUserMessages), + ); + let log = new_turn_log(); + + // One scripted reply: a follow-up turn would run out and panic, so + // "chains nothing" is enforced by the runner as well as asserted. + let ((first, second), events) = events_during(fixture.conversation_id, async { + let first = fixture.steer("first steer").await; + let second = fixture.steer("second steer").await; + fixture + .run_turns(scripted_runner(&log, &["only answer"])) + .await; + (first, second) + }) + .await; + + assert_eq!( + recorded(&log).len(), + 1, + "a steer the store refused must not seed a follow-up turn" + ); + assert!( + delivered_ids(&events).is_empty(), + "a steer that was never recorded must not be reported delivered, got {events:?}" + ); + assert_eq!( + discarded_ids(&events), + vec![first, second], + "the refused steer and every entry behind it must be announced as discarded, \ + got {events:?}" + ); + assert_eq!( + persisted_shape(&fixture.conversations).await, + vec![(MessageRole::Assistant, "only answer".to_string())], + "only the assistant output the turn actually produced may be recorded, and only once" + ); + assert_eq!( + completion_count(&events), + 1, + "the turn itself finished, so the send must still finalize once, got {events:?}" + ); + assert!( + !fixture.is_streaming(), + "finalization must still release the conversation's stream slot" + ); +} diff --git a/src/services/chat_impl/tests/stream_failure.rs b/src/services/chat_impl/tests/stream_failure.rs index 138cd3f..803a31c 100644 --- a/src/services/chat_impl/tests/stream_failure.rs +++ b/src/services/chat_impl/tests/stream_failure.rs @@ -46,6 +46,15 @@ fn chat_event_conversation_id(event: &ChatEvent) -> Uuid { } | ChatEvent::MessageSaved { conversation_id, .. + } + | ChatEvent::SteeringQueued { + conversation_id, .. + } + | ChatEvent::SteeringDelivered { + conversation_id, .. + } + | ChatEvent::SteeringDiscarded { + conversation_id, .. } => *conversation_id, } } diff --git a/src/services/chat_impl/tests/stream_failure/finalization.rs b/src/services/chat_impl/tests/stream_failure/finalization.rs index 4bec976..c026ce5 100644 --- a/src/services/chat_impl/tests/stream_failure/finalization.rs +++ b/src/services/chat_impl/tests/stream_failure/finalization.rs @@ -21,15 +21,22 @@ fn finalize_context<'a>( conversation_id: Uuid, stream_id: Uuid, active_streams: &'a Arc>>, + steering_queues: &'a SteeringQueues, ) -> streaming::StreamFinalizeContext<'a> { streaming::StreamFinalizeContext { conversation_service, conversation_id, stream_id, active_streams, + steering_queues, } } +/// An empty steering registry, for finalize paths that never queue a steer. +fn no_steering_queues() -> SteeringQueues { + Arc::new(StdMutex::new(HashMap::new())) +} + /// A `CompressionResult` that leaves the conversation's context untouched /// apart from what the finalize path itself writes. fn no_compression() -> CompressionResult { @@ -66,11 +73,14 @@ async fn interrupted_finalize_persists_partial_output_as_interrupted() { }; streaming::finalize_interrupted_stream( - &conversation_service, - conversation_id, - stream_id, + &finalize_context( + &conversation_service, + conversation_id, + stream_id, + &active_streams, + &no_steering_queues(), + ), &transcript, - &active_streams, "Interrupted Model", ) .await; @@ -138,11 +148,14 @@ async fn interrupted_finalize_skips_persistence_when_no_assistant_output() { .expect("active_streams poisoned") .insert(empty_conversation, active_stream_entry(empty_stream)); streaming::finalize_interrupted_stream( - &conversation_service, - empty_conversation, - empty_stream, + &finalize_context( + &conversation_service, + empty_conversation, + empty_stream, + &active_streams, + &no_steering_queues(), + ), &streaming::StreamTranscript::default(), - &active_streams, "Interrupted Model", ) .await; @@ -167,11 +180,14 @@ async fn interrupted_finalize_skips_persistence_when_no_assistant_output() { ..streaming::StreamTranscript::default() }; streaming::finalize_interrupted_stream( - &conversation_service, - tool_only_conversation, - tool_only_stream, + &finalize_context( + &conversation_service, + tool_only_conversation, + tool_only_stream, + &active_streams, + &no_steering_queues(), + ), &tool_only, - &active_streams, "Interrupted Model", ) .await; @@ -217,11 +233,14 @@ async fn interrupted_finalize_persists_thinking_only_output() { }; streaming::finalize_interrupted_stream( - &conversation_service, - conversation_id, - stream_id, + &finalize_context( + &conversation_service, + conversation_id, + stream_id, + &active_streams, + &no_steering_queues(), + ), &transcript, - &active_streams, "Interrupted Model", ) .await; @@ -262,9 +281,13 @@ async fn normal_finalize_persists_message_not_interrupted_and_emits_completion() }; streaming::finalize_stream_task( - &conversation_service, - conversation_id, - stream_id, + &finalize_context( + &conversation_service, + conversation_id, + stream_id, + &active_streams, + &no_steering_queues(), + ), CompressionResult { messages: vec![], phase: crate::models::CompressionPhase::None, @@ -274,7 +297,6 @@ async fn normal_finalize_persists_message_not_interrupted_and_emits_completion() estimated_tokens: 0, }, transcript, - &active_streams, "Normal Model", ) .await; @@ -350,6 +372,7 @@ async fn finalize_dispatch_completed_persists_updates_context_and_emits_completi conversation_id, stream_id, &active_streams, + &no_steering_queues(), ), no_compression(), transcript, @@ -430,6 +453,7 @@ async fn finalize_dispatch_errored_incomplete_persists_interrupted_without_conte conversation_id, stream_id, &active_streams, + &no_steering_queues(), ), no_compression(), transcript, @@ -506,6 +530,7 @@ async fn finalize_dispatch_benign_incomplete_persists_nothing_and_clears_stream_ conversation_id, stream_id, &active_streams, + &no_steering_queues(), ), no_compression(), transcript, diff --git a/src/ui_gpui/views/chat_view/command.rs b/src/ui_gpui/views/chat_view/command.rs index 2567dc9..2151be9 100644 --- a/src/ui_gpui/views/chat_view/command.rs +++ b/src/ui_gpui/views/chat_view/command.rs @@ -12,7 +12,7 @@ //! //! @plan PLAN-20250130-GPUIREDUX.P04 -use super::state::{ApprovalBubbleState, ToolApprovalBubble}; +use super::state::{ApprovalBubbleState, QueuedSteeringEntry, ToolApprovalBubble}; use super::ChatView; use crate::events::types::{ToolApprovalResponseAction, UserEvent}; use crate::presentation::view_command::{ToolApprovalContext, ViewCommand}; @@ -69,6 +69,38 @@ impl ChatView { } } + /// Record the user's decision on a tool approval and retire the bubble. + /// + /// Extracted from `handle_command` unchanged; the dispatch had grown past + /// the length the complexity gate allows. + fn handle_tool_approval_resolved( + &mut self, + conversation_id: uuid::Uuid, + request_id: &str, + approved: bool, + cx: &mut gpui::Context, + ) { + // Find the bubble containing this request_id in the owning conversation bucket. + if let Some(bubbles) = self.state.approval_bubbles.get_mut(&conversation_id) { + if let Some(bubble) = bubbles + .iter_mut() + .find(|b| b.request_ids.iter().any(|id| id == request_id)) + { + bubble.state = if approved { + ApprovalBubbleState::Approved + } else { + ApprovalBubbleState::Denied + }; + } + // Remove resolved bubbles so they don't accumulate. + bubbles.retain(|b| b.state == ApprovalBubbleState::Pending); + if bubbles.is_empty() { + self.state.approval_bubbles.remove(&conversation_id); + } + } + cx.notify(); + } + /// Handle YOLO mode activation - auto-approve any pending tool approval bubbles. fn handle_yolo_mode_changed(&mut self, active: bool, cx: &mut gpui::Context) { self.state.yolo_mode = active; @@ -116,6 +148,10 @@ impl ChatView { if let Some(conversation_id) = cleared_conversation_id { self.state.approval_bubbles.remove(&conversation_id); } + // Queued steers belong to the turn that was just cleared away. + // @plan PLAN-20260903-ISSUE222.P04 + // @requirement REQ-222-003 + self.state.queued_steering.clear(); self.state.chat_autoscroll_enabled = true; self.scroll_transcript_to_bottom(); self.state.sync_conversation_title_from_active(); @@ -123,6 +159,113 @@ impl ChatView { cx.notify(); } + /// Show a steering message as waiting for the running turn's boundary. + /// + /// Scoped to the conversation on screen the way the approval commands + /// are: `queued_steering` describes the transcript being rendered, and a + /// steer accepted for another conversation is not part of it. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-003 + fn handle_steering_queued( + &mut self, + conversation_id: uuid::Uuid, + steer_id: uuid::Uuid, + text: String, + cx: &mut gpui::Context, + ) { + if self.state.active_conversation_id != Some(conversation_id) { + return; + } + self.state + .queued_steering + .push(QueuedSteeringEntry::new(steer_id, text)); + self.maybe_scroll_chat_to_bottom(); + cx.notify(); + } + + /// Stop showing a steering message as waiting: it reached the model. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-003 + fn handle_steering_delivered( + &mut self, + conversation_id: uuid::Uuid, + steer_id: uuid::Uuid, + cx: &mut gpui::Context, + ) { + if self.state.active_conversation_id != Some(conversation_id) { + return; + } + self.state + .queued_steering + .retain(|entry| entry.id != steer_id); + cx.notify(); + } + + /// Stop showing a steering message as waiting: the turn it was waiting on + /// ended, so it is never reaching the model. + /// + /// The removal is the one delivery performs, because the entry's job is + /// the same either way: it says an instruction is still pending. Once + /// that stops being true the entry has to go, or it waits on screen for + /// the rest of the session. + /// + /// @plan PLAN-20260903-ISSUE222.P06 + /// @requirement REQ-222-003 + fn handle_steering_discarded( + &mut self, + conversation_id: uuid::Uuid, + steer_id: uuid::Uuid, + cx: &mut gpui::Context, + ) { + if self.state.active_conversation_id != Some(conversation_id) { + return; + } + self.state + .queued_steering + .retain(|entry| entry.id != steer_id); + cx.notify(); + } + + /// A steer the service refused. + /// + /// This withdraws no queued entry. A refusal means the service queued + /// nothing, so there is none under it to remove, and removing one anyway + /// would take away a steer that is still waiting. The accompanying + /// `ShowError` is what the user sees. + /// + /// What it does do is give the words back. Submitting clears the + /// composer without waiting for an answer, so a refused steer would + /// otherwise be typed, taken away and never seen again. The text goes + /// back only into the conversation it was typed in, and only into a + /// composer that is still empty: the round trip leaves room for the + /// user to have moved to another conversation or started something + /// new, and whatever is on their screen now outranks the draft the + /// service turned down. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @plan PLAN-20260903-ISSUE222.P08 + /// @requirement REQ-222-002 + /// @requirement REQ-222-004 + fn handle_steering_rejected( + &mut self, + conversation_id: uuid::Uuid, + error: &str, + text: String, + cx: &mut gpui::Context, + ) { + tracing::warn!(%conversation_id, "Steering message refused: {error}"); + if self.state.active_conversation_id != Some(conversation_id) + || !self.state.input_text.is_empty() + { + return; + } + self.state.input_text = text; + self.state.cursor_position = self.state.input_text.len(); + cx.notify(); + } + fn handle_conversation_search_results( &mut self, results: Vec, @@ -203,25 +346,33 @@ impl ChatView { request_id, approved, } => { - // Find the bubble containing this request_id in the owning conversation bucket. - if let Some(bubbles) = self.state.approval_bubbles.get_mut(&conversation_id) { - if let Some(bubble) = bubbles - .iter_mut() - .find(|b| b.request_ids.contains(&request_id)) - { - bubble.state = if approved { - ApprovalBubbleState::Approved - } else { - ApprovalBubbleState::Denied - }; - } - // Remove resolved bubbles so they don't accumulate. - bubbles.retain(|b| b.state == ApprovalBubbleState::Pending); - if bubbles.is_empty() { - self.state.approval_bubbles.remove(&conversation_id); - } - } - cx.notify(); + self.handle_tool_approval_resolved(conversation_id, &request_id, approved, cx); + } + ViewCommand::SteeringQueued { + conversation_id, + steer_id, + text, + } => { + self.handle_steering_queued(conversation_id, steer_id, text, cx); + } + ViewCommand::SteeringDelivered { + conversation_id, + steer_id, + } => { + self.handle_steering_delivered(conversation_id, steer_id, cx); + } + ViewCommand::SteeringDiscarded { + conversation_id, + steer_id, + } => { + self.handle_steering_discarded(conversation_id, steer_id, cx); + } + ViewCommand::SteeringRejected { + conversation_id, + error, + text, + } => { + self.handle_steering_rejected(conversation_id, &error, text, cx); } ViewCommand::YoloModeChanged { active } => { self.handle_yolo_mode_changed(active, cx); diff --git a/src/ui_gpui/views/chat_view/composer_submit.rs b/src/ui_gpui/views/chat_view/composer_submit.rs new file mode 100644 index 0000000..e4e2642 --- /dev/null +++ b/src/ui_gpui/views/chat_view/composer_submit.rs @@ -0,0 +1,140 @@ +//! What a composer submit means, and what it does. +//! +//! The Send button and the Enter key are the same gesture reached two ways. +//! Both ask [`ChatView::composer_submit`] what the current state makes of the +//! composer text and then hand the answer to [`ChatView::submit_composer`], +//! so a rule added to one is a rule added to both. +//! +//! @plan PLAN-20260903-ISSUE222.P04 +//! @requirement REQ-222-002 + +use super::state::StreamingState; +use super::ChatView; +use crate::events::types::UserEvent; +use uuid::Uuid; + +/// The meaning of a composer submit in the view's current state. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-002 +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) enum ComposerSubmit { + /// There is nothing to submit: the composer holds no non-whitespace text, + /// or a running turn has no conversation to name. + Nothing, + /// Nothing is running, so the text starts a turn. + Send(String), + /// A turn is running, so the text joins it instead of starting a second. + Steer { conversation_id: Uuid, text: String }, +} + +impl ChatView { + /// Reads the composer and the streaming state, without changing either. + /// + /// A steer names the same conversation the Stop button would cancel, + /// because it joins that turn. With no conversation selected there is no + /// turn to join and no id to send, so there is nothing to submit. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-002 + pub(super) fn composer_submit(&self) -> ComposerSubmit { + if self.state.input_text.trim().is_empty() { + return ComposerSubmit::Nothing; + } + let text = self.state.input_text.clone(); + + if !matches!(self.state.streaming, StreamingState::Streaming { .. }) { + return ComposerSubmit::Send(text); + } + + self.state + .active_conversation_id + .map_or(ComposerSubmit::Nothing, |conversation_id| { + ComposerSubmit::Steer { + conversation_id, + text, + } + }) + } + + /// Applies whatever the current state makes of the composer text. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-002 + pub(super) fn submit_composer(&mut self, cx: &mut gpui::Context) { + match self.composer_submit() { + ComposerSubmit::Nothing => {} + ComposerSubmit::Send(text) => { + tracing::info!("Composer submit - emitting SendMessage: {}", text); + self.send_message_and_start_streaming(text, cx); + } + ComposerSubmit::Steer { + conversation_id, + text, + } => { + tracing::info!(%conversation_id, "Composer submit - steering the running turn"); + self.steer_streaming(conversation_id, text, cx); + } + } + } + + /// Offers the composer text to the turn already running. + /// + /// The streaming state is deliberately untouched: the turn keeps going, + /// and the service reports back through `SteeringQueued` (or refuses with + /// `SteeringRejected`). Nothing here cancels anything. + /// + /// The composer clears now rather than when the service answers, so + /// typing ahead never waits on a round trip. A refusal carries the text + /// back on `SteeringRejected` and the view puts it into the composer + /// again if the user has not started something else, so the optimism + /// costs them nothing. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @plan PLAN-20260903-ISSUE222.P08 + /// @requirement REQ-222-002 + /// @requirement REQ-222-006 + fn steer_streaming( + &mut self, + conversation_id: Uuid, + text: String, + cx: &mut gpui::Context, + ) { + self.emit(UserEvent::SteerStreaming { + conversation_id, + text, + }); + self.state.input_text.clear(); + self.state.cursor_position = 0; + self.state.chat_autoscroll_enabled = true; + self.state.conversation_dropdown_open = false; + self.state.profile_dropdown_open = false; + self.state.conversation_title_editing = false; + self.maybe_scroll_chat_to_bottom(); + cx.notify(); + } + + pub(super) fn send_message_and_start_streaming( + &mut self, + text: String, + cx: &mut gpui::Context, + ) { + self.emit(UserEvent::SendMessage { + text, + conversation_id: self.conversation_id, + }); + self.state.input_text.clear(); + self.state.cursor_position = 0; + self.state.chat_autoscroll_enabled = true; + self.state.conversation_dropdown_open = false; + self.state.profile_dropdown_open = false; + self.state.conversation_title_editing = false; + self.state.streaming = StreamingState::Streaming { + content: String::new(), + done: false, + }; + self.refresh_transcript_selection_revisions(); + self.maybe_scroll_chat_to_bottom(); + cx.notify(); + } +} diff --git a/src/ui_gpui/views/chat_view/message_selection.rs b/src/ui_gpui/views/chat_view/message_selection.rs index 68050c6..d55bbb8 100644 --- a/src/ui_gpui/views/chat_view/message_selection.rs +++ b/src/ui_gpui/views/chat_view/message_selection.rs @@ -303,7 +303,13 @@ impl super::ChatView { ); Some((self.streaming_selection_content_key(), leaves)) } - TranscriptRow::Approval(_) => None, + // Chrome, not transcript content: an approval prompt and a + // steering message that has not reached the model yet are + // both absent from a copied transcript. + // + // @plan PLAN-20260903-ISSUE222.P04 + // @requirement REQ-222-003 + TranscriptRow::Approval(_) | TranscriptRow::QueuedSteering(_) => None, }) .collect(); Arc::new(TranscriptCopyDocument::new(messages)) @@ -319,7 +325,12 @@ impl super::ChatView { let content_key = match row { TranscriptRow::Message(index) => self.message_selection_content_key(index), TranscriptRow::Streaming => self.streaming_selection_content_key(), - TranscriptRow::Approval(_) => return None, + // No content key, so no selection: dragging across an approval + // prompt or a queued steering entry selects neither. + // + // @plan PLAN-20260903-ISSUE222.P04 + // @requirement REQ-222-003 + TranscriptRow::Approval(_) | TranscriptRow::QueuedSteering(_) => return None, }; Some(TranscriptSelectionContext { scroll_offset, diff --git a/src/ui_gpui/views/chat_view/mod.rs b/src/ui_gpui/views/chat_view/mod.rs index 18721c0..8b77e20 100644 --- a/src/ui_gpui/views/chat_view/mod.rs +++ b/src/ui_gpui/views/chat_view/mod.rs @@ -11,6 +11,7 @@ //! @requirement REQ-GPUI-003 mod command; +mod composer_submit; mod emoji; mod focus; @@ -20,6 +21,7 @@ mod render; mod render_bars; #[cfg(test)] mod render_bars_tests; +mod render_composer; #[cfg(test)] mod selection_autoscroll_tests; @@ -38,8 +40,8 @@ mod transcript; // ── Re-exports so downstream consumers (mod.rs, tests, main_panel.rs) ── // see the same type paths as before extraction. pub use state::{ - ApprovalBubbleState, ChatMessage, ChatState, GroupedOperation, MessageRole, StreamingState, - ToolApprovalBubble, + ApprovalBubbleState, ChatMessage, ChatState, GroupedOperation, MessageRole, + QueuedSteeringEntry, StreamingState, ToolApprovalBubble, }; use crate::events::types::UserEvent; @@ -193,6 +195,7 @@ impl ChatView { self.state.messages.len(), approval_bubbles, &self.state.streaming, + self.state.queued_steering.len(), ) } @@ -803,6 +806,15 @@ impl ChatView { } /// Handle enter key (called from `MainPanel`) + /// + /// The overlay early-returns come first and are unrelated to submitting: + /// an inline rename, the conversation dropdown and the profile dropdown + /// each own Enter while they are open. Everything past them is a composer + /// submit, which `submit_composer` decides for both Enter and the Send + /// button. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-002 pub fn handle_enter(&mut self, cx: &mut gpui::Context) { if self.state.conversation_title_editing { self.submit_rename_conversation(cx); @@ -819,40 +831,7 @@ impl ChatView { return; } - if matches!(self.state.streaming, StreamingState::Streaming { .. }) { - tracing::info!("ChatView::handle_enter ignored while stream is active"); - return; - } - - if !self.state.input_text.trim().is_empty() { - let text = self.state.input_text.clone(); - tracing::info!("ChatView::handle_enter - emitting SendMessage: {}", text); - self.send_message_and_start_streaming(text, cx); - } - } - - pub(super) fn send_message_and_start_streaming( - &mut self, - text: String, - cx: &mut gpui::Context, - ) { - self.emit(UserEvent::SendMessage { - text, - conversation_id: self.conversation_id, - }); - self.state.input_text.clear(); - self.state.cursor_position = 0; - self.state.chat_autoscroll_enabled = true; - self.state.conversation_dropdown_open = false; - self.state.profile_dropdown_open = false; - self.state.conversation_title_editing = false; - self.state.streaming = StreamingState::Streaming { - content: String::new(), - done: false, - }; - self.refresh_transcript_selection_revisions(); - self.maybe_scroll_chat_to_bottom(); - cx.notify(); + self.submit_composer(cx); } } @@ -875,3 +854,17 @@ mod mod_tests; #[cfg(test)] #[path = "performance_tests.rs"] mod performance_tests; + +/// Composer controls for mid-turn steering. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +#[cfg(test)] +#[path = "steering_tests.rs"] +mod steering_tests; + +/// Queued steering entries in the transcript. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +#[cfg(test)] +#[path = "steering_queue_tests.rs"] +mod steering_queue_tests; diff --git a/src/ui_gpui/views/chat_view/render.rs b/src/ui_gpui/views/chat_view/render.rs index f4d14da..d306af9 100644 --- a/src/ui_gpui/views/chat_view/render.rs +++ b/src/ui_gpui/views/chat_view/render.rs @@ -1,10 +1,12 @@ //! Chat view content render subtrees. //! -//! Contains `render_chat_area`, message rendering helpers, `render_input_bar`, -//! and the root `impl Render for ChatView`. These are the content-area methods -//! below the navigation bars. +//! Contains `render_chat_area`, message rendering helpers, and the root +//! `impl Render for ChatView`. These are the content-area methods below the +//! navigation bars. The composer row moved to `render_composer` when this +//! file reached the length CI rejects. //! //! @plan PLAN-20260325-ISSUE11B.P02 +//! @plan PLAN-20260903-ISSUE222.P04 use super::emoji::strip_emojis; use super::state::{ApprovalBubbleState, ChatMessage, MessageRole, StreamingState}; @@ -344,9 +346,57 @@ impl ChatView { selection.expect("streaming row requires selectable content"), ) .into_any_element(), + TranscriptRow::QueuedSteering(index) => { + Self::render_queued_steering(index, &self.state.queued_steering[index].text) + } } } + /// Render a steering message that is waiting for the turn's boundary. + /// + /// It sits on the user's side of the transcript because the user wrote + /// it, but it is deliberately not a user bubble: an outline and a label + /// say it has not been said to the model yet, so a queued steer is never + /// mistaken for one the agent has already read. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-003 + fn render_queued_steering(index: usize, text: &str) -> gpui::AnyElement { + div() + .id(SharedString::from(format!("queued-steering-{index}"))) + .debug_selector(move || format!("chat-queued-steering-{index}")) + .w_full() + .flex() + .justify_end() + .child( + div() + .max_w(px(300.0)) + .px(px(10.0)) + .py(px(8.0)) + .rounded(px(12.0)) + .bg(Theme::bg_darker()) + .border_1() + .border_color(Theme::border()) + .flex() + .flex_col() + .gap(px(Theme::SPACING_XS)) + .child( + div() + .text_size(px(Theme::font_size_small())) + .text_color(Theme::text_muted()) + .child("Queued for this turn"), + ) + .child( + div() + .text_size(px(Theme::font_size_mono())) + .text_color(Theme::text_secondary()) + .whitespace_normal() + .child(text.to_string()), + ), + ) + .into_any_element() + } + /// Render the streaming assistant message bubble. fn render_streaming_message( &self, @@ -609,209 +659,6 @@ impl ChatView { ), ) } - - #[allow(clippy::too_many_lines)] - pub(super) fn render_input_bar(&self, cx: &mut gpui::Context) -> impl IntoElement { - let is_streaming = matches!(self.state.streaming, StreamingState::Streaming { .. }); - let input_text = self.state.input_text.clone(); - let has_text = !input_text.trim().is_empty(); - let focus_handle = self.focus_handle.clone(); - - let wrapped_line_count = if input_text.is_empty() { - 1 - } else { - input_text - .split('\n') - .map(|line| { - let len = line.chars().count(); - if len == 0 { - 1 - } else { - let approx_chars_per_line = 65usize; - len.div_ceil(approx_chars_per_line) - } - }) - .sum::() - .max(1) - }; - - let max_composer_height = 150.0; - let min_composer_height = 44.0; - let line_height = Theme::font_size_mono().mul_add(0.4, Theme::font_size_mono()); - #[allow(clippy::cast_precision_loss)] - let computed_height = (wrapped_line_count as f32).mul_add(line_height, 14.0); - let input_box_height = computed_height.clamp(min_composer_height, max_composer_height); - let focused = self.composer_has_focus(cx); - let text_content = self.composer_display_text(cx); - - div() - .id("input-bar-container") - .w_full() - .flex() - .debug_selector(|| "chat-input-bar".to_string()) - .items_end() - .justify_between() - .min_h(px(56.0)) - .gap(px(Theme::SPACING_SM)) - .p(px(Theme::SPACING_MD)) - .bg(Theme::bg_darker()) - .border_t_1() - .border_color(Theme::bg_dark()) - .overflow_hidden() - .on_mouse_down( - MouseButton::Left, - cx.listener(|this, _, _window, cx| { - if this.sidebar_search_focused(cx) { - this.set_sidebar_search_focused(false, cx); - cx.notify(); - } - }), - ) - .child(Self::render_composer_field( - focus_handle, - input_box_height, - max_composer_height, - line_height, - &input_text, - text_content, - focused, - cx, - )) - .child(self.render_send_stop_button(is_streaming, has_text, cx)) - } - - #[allow(clippy::too_many_arguments)] - fn render_composer_field( - focus_handle: gpui::FocusHandle, - input_box_height: f32, - max_composer_height: f32, - line_height: f32, - input_text: &str, - text_content: String, - focused: bool, - cx: &mut gpui::Context, - ) -> impl IntoElement { - div() - .id("input-field") - .debug_selector(|| "chat-input-field".to_string()) - .flex_1() - .min_w(px(0.0)) - .h(px(input_box_height)) - .max_h(px(max_composer_height)) - .px(px(Theme::SPACING_SM)) - .py(px(7.0)) - .bg(Theme::bg_darkest()) - .border_1() - .border_color(if focused { - Theme::accent() - } else { - Theme::border() - }) - .rounded(px(Theme::RADIUS_MD)) - .overflow_x_hidden() - .overflow_y_scroll() - .cursor_text() - .on_mouse_down( - MouseButton::Left, - cx.listener(move |this, _, window, cx| { - window.activate_window(); - window.focus(&focus_handle, cx); - this.focus_composer_dismissing_overlays(cx); - }), - ) - .child( - div() - .w_full() - .text_size(px(Theme::font_size_mono())) - .line_height(px(line_height)) - .text_color(if input_text.is_empty() && !focused { - Theme::text_secondary() - } else { - Theme::text_primary() - }) - .whitespace_normal() - .child(text_content), - ) - } - - /// Send/Stop button with event emission. - /// @plan PLAN-20250130-GPUIREDUX.P04 - #[allow(clippy::unused_self)] // cx.listener borrows the entity, not &self directly - fn render_send_stop_button( - &self, - is_streaming: bool, - has_text: bool, - cx: &mut gpui::Context, - ) -> impl IntoElement { - div() - .id(if is_streaming { "stop-btn" } else { "send-btn" }) - .debug_selector(|| { - if is_streaming { - "chat-stop-button".to_string() - } else { - "chat-send-button".to_string() - } - }) - .flex_shrink_0() - .min_h(px(36.0)) - .px(px(Theme::SPACING_MD)) - .py(px(Theme::SPACING_SM)) - .rounded(px(Theme::RADIUS_MD)) - .cursor_pointer() - .when(is_streaming, |d| { - d.bg(Theme::error()) - .text_color(Theme::selection_fg()) - .child("Stop") - .on_mouse_down( - MouseButton::Left, - cx.listener(|this, _, window, cx| { - if let Some(conversation_id) = this.state.active_conversation_id { - tracing::info!("Stop button clicked - emitting StopStreaming"); - this.emit(UserEvent::StopStreaming { conversation_id }); - } - this.state.streaming = StreamingState::Idle; - this.refresh_transcript_selection_revisions(); - this.maybe_scroll_chat_to_bottom(); - // Refocus the composer so keyboard input works - // immediately after stopping — without this, - // GPUI leaves focus on the now-vanished Stop - // button div, making all text inputs unresponsive - // until the popup is toggled. - this.focus_composer(cx); - window.focus(&this.focus_handle, cx); - cx.notify(); - }), - ) - }) - .when(!is_streaming && has_text, |d| { - d.bg(Theme::bg_dark()) - .text_color(Theme::text_primary()) - .hover(|s| s.bg(Theme::bg_darker())) - .child("Send") - .on_mouse_down( - MouseButton::Left, - cx.listener(|this, _, _window, cx| { - if matches!(this.state.streaming, StreamingState::Streaming { .. }) { - tracing::info!("Send button ignored while stream is active"); - return; - } - let text = this.state.input_text.clone(); - if !text.trim().is_empty() { - tracing::info!( - "Send button clicked - emitting SendMessage: {}", - text - ); - this.send_message_and_start_streaming(text, cx); - } - }), - ) - }) - .when(!is_streaming && !has_text, |d| { - d.bg(Theme::bg_dark()) - .text_color(Theme::text_secondary()) - .child("Send") - }) - } } impl ChatView { diff --git a/src/ui_gpui/views/chat_view/render_composer.rs b/src/ui_gpui/views/chat_view/render_composer.rs new file mode 100644 index 0000000..c3a3251 --- /dev/null +++ b/src/ui_gpui/views/chat_view/render_composer.rs @@ -0,0 +1,224 @@ +//! The composer row: the text field and the controls beside it. +//! +//! While a turn runs the row carries two controls, not one. Stop ends the +//! turn; Send joins it. They are separate elements so neither can hide the +//! other, and the whole row lives here rather than in `render.rs` because +//! that file is at the length CI rejects. +//! +//! @plan PLAN-20260903-ISSUE222.P04 +//! @requirement REQ-222-001 + +use super::state::StreamingState; +use super::ChatView; +use crate::events::types::UserEvent; +use crate::ui_gpui::theme::Theme; +use gpui::{div, prelude::*, px, MouseButton}; + +impl ChatView { + #[allow(clippy::too_many_lines)] + pub(super) fn render_input_bar(&self, cx: &mut gpui::Context) -> impl IntoElement { + let is_streaming = matches!(self.state.streaming, StreamingState::Streaming { .. }); + let input_text = self.state.input_text.clone(); + let has_text = !input_text.trim().is_empty(); + let focus_handle = self.focus_handle.clone(); + + let wrapped_line_count = if input_text.is_empty() { + 1 + } else { + input_text + .split('\n') + .map(|line| { + let len = line.chars().count(); + if len == 0 { + 1 + } else { + let approx_chars_per_line = 65usize; + len.div_ceil(approx_chars_per_line) + } + }) + .sum::() + .max(1) + }; + + let max_composer_height = 150.0; + let min_composer_height = 44.0; + let line_height = Theme::font_size_mono().mul_add(0.4, Theme::font_size_mono()); + #[allow(clippy::cast_precision_loss)] + let computed_height = (wrapped_line_count as f32).mul_add(line_height, 14.0); + let input_box_height = computed_height.clamp(min_composer_height, max_composer_height); + let focused = self.composer_has_focus(cx); + let text_content = self.composer_display_text(cx); + + div() + .id("input-bar-container") + .w_full() + .flex() + .debug_selector(|| "chat-input-bar".to_string()) + .items_end() + .justify_between() + .min_h(px(56.0)) + .gap(px(Theme::SPACING_SM)) + .p(px(Theme::SPACING_MD)) + .bg(Theme::bg_darker()) + .border_t_1() + .border_color(Theme::bg_dark()) + .overflow_hidden() + .on_mouse_down( + MouseButton::Left, + cx.listener(|this, _, _window, cx| { + if this.sidebar_search_focused(cx) { + this.set_sidebar_search_focused(false, cx); + cx.notify(); + } + }), + ) + .child(Self::render_composer_field( + focus_handle, + input_box_height, + max_composer_height, + line_height, + &input_text, + text_content, + focused, + cx, + )) + // Stop only exists while there is a turn to stop; Send is always + // there, so a running turn shows both. + // @requirement REQ-222-001 + .when(is_streaming, |d| d.child(self.render_stop_button(cx))) + .child(self.render_send_button(has_text, cx)) + } + + #[allow(clippy::too_many_arguments)] + fn render_composer_field( + focus_handle: gpui::FocusHandle, + input_box_height: f32, + max_composer_height: f32, + line_height: f32, + input_text: &str, + text_content: String, + focused: bool, + cx: &mut gpui::Context, + ) -> impl IntoElement { + div() + .id("input-field") + .debug_selector(|| "chat-input-field".to_string()) + .flex_1() + .min_w(px(0.0)) + .h(px(input_box_height)) + .max_h(px(max_composer_height)) + .px(px(Theme::SPACING_SM)) + .py(px(7.0)) + .bg(Theme::bg_darkest()) + .border_1() + .border_color(if focused { + Theme::accent() + } else { + Theme::border() + }) + .rounded(px(Theme::RADIUS_MD)) + .overflow_x_hidden() + .overflow_y_scroll() + .cursor_text() + .on_mouse_down( + MouseButton::Left, + cx.listener(move |this, _, window, cx| { + window.activate_window(); + window.focus(&focus_handle, cx); + this.focus_composer_dismissing_overlays(cx); + }), + ) + .child( + div() + .w_full() + .text_size(px(Theme::font_size_mono())) + .line_height(px(line_height)) + .text_color(if input_text.is_empty() && !focused { + Theme::text_secondary() + } else { + Theme::text_primary() + }) + .whitespace_normal() + .child(text_content), + ) + } + + /// Ends the running turn. + /// + /// Rendered only while a turn runs, and unchanged in what it does: emit + /// `StopStreaming`, drop the view out of streaming, and put the keyboard + /// back in the composer. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-001 + #[allow(clippy::unused_self)] // cx.listener borrows the entity, not &self directly + fn render_stop_button(&self, cx: &mut gpui::Context) -> impl IntoElement { + div() + .id("stop-btn") + .debug_selector(|| "chat-stop-button".to_string()) + .flex_shrink_0() + .min_h(px(36.0)) + .px(px(Theme::SPACING_MD)) + .py(px(Theme::SPACING_SM)) + .rounded(px(Theme::RADIUS_MD)) + .cursor_pointer() + .bg(Theme::error()) + .text_color(Theme::selection_fg()) + .child("Stop") + .on_mouse_down( + MouseButton::Left, + cx.listener(|this, _, window, cx| { + if let Some(conversation_id) = this.state.active_conversation_id { + tracing::info!("Stop button clicked - emitting StopStreaming"); + this.emit(UserEvent::StopStreaming { conversation_id }); + } + this.state.streaming = StreamingState::Idle; + this.refresh_transcript_selection_revisions(); + this.maybe_scroll_chat_to_bottom(); + // Refocus the composer so keyboard input works + // immediately after stopping — without this, + // GPUI leaves focus on the now-vanished Stop + // button div, making all text inputs unresponsive + // until the popup is toggled. + this.focus_composer(cx); + window.focus(&this.focus_handle, cx); + cx.notify(); + }), + ) + } + + /// Submits the composer. + /// + /// Rendered in every state, because what a submit means is decided by + /// `submit_composer`, not by which button is on screen: idle it starts a + /// turn, mid-turn it steers the one already running. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-001 + /// @requirement REQ-222-002 + #[allow(clippy::unused_self)] // cx.listener borrows the entity, not &self directly + fn render_send_button(&self, has_text: bool, cx: &mut gpui::Context) -> impl IntoElement { + div() + .id("send-btn") + .debug_selector(|| "chat-send-button".to_string()) + .flex_shrink_0() + .min_h(px(36.0)) + .px(px(Theme::SPACING_MD)) + .py(px(Theme::SPACING_SM)) + .rounded(px(Theme::RADIUS_MD)) + .cursor_pointer() + .bg(Theme::bg_dark()) + .child("Send") + .when(!has_text, |d| d.text_color(Theme::text_secondary())) + .when(has_text, |d| { + d.text_color(Theme::text_primary()) + .hover(|s| s.bg(Theme::bg_darker())) + .on_mouse_down( + MouseButton::Left, + cx.listener(|this, _, _window, cx| { + this.submit_composer(cx); + }), + ) + }) + } +} diff --git a/src/ui_gpui/views/chat_view/snapshot.rs b/src/ui_gpui/views/chat_view/snapshot.rs index 2811b3c..c0b9f2b 100644 --- a/src/ui_gpui/views/chat_view/snapshot.rs +++ b/src/ui_gpui/views/chat_view/snapshot.rs @@ -133,6 +133,10 @@ impl ChatView { let previous_messages_empty = self.state.messages.is_empty(); self.carry_draft_across_selection(previous_conversation_id, selected_conversation_id); + self.drop_queued_steering_across_selection( + previous_conversation_id, + selected_conversation_id, + ); self.state.conversations = conversations; self.state.active_conversation_id = selected_conversation_id; @@ -223,6 +227,26 @@ impl ChatView { } } + /// Drop queued steering entries when the selection moves. + /// + /// `queued_steering` describes the transcript on screen, so it cannot + /// follow the user into another conversation the way a draft does: the + /// entries belong to a turn that is still running somewhere else, and + /// they come back with that conversation's own `SteeringQueued` events + /// only if the service still holds them. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-003 + fn drop_queued_steering_across_selection( + &mut self, + previous_conversation_id: Option, + selected_conversation_id: Option, + ) { + if previous_conversation_id != selected_conversation_id { + self.state.queued_steering.clear(); + } + } + /// Adopt the generation carried by `load_state`, reporting whether the view is now /// looking at a different conversation or a newer selection than before. fn adopt_selection_generation( diff --git a/src/ui_gpui/views/chat_view/state.rs b/src/ui_gpui/views/chat_view/state.rs index 0a38b7d..e2ece38 100644 --- a/src/ui_gpui/views/chat_view/state.rs +++ b/src/ui_gpui/views/chat_view/state.rs @@ -211,6 +211,33 @@ impl ToolApprovalBubble { } } +/// A steering message accepted by the service and waiting for the running +/// turn to reach its boundary. +/// +/// `text` is `Arc` for the same reason `ChatMessage::content` is: the +/// transcript clones every row it paints, and a queued entry is repainted on +/// each streaming token. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-003 +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct QueuedSteeringEntry { + /// The id the service issued, which `SteeringDelivered` names to withdraw + /// this entry. + pub id: Uuid, + pub text: Arc, +} + +impl QueuedSteeringEntry { + #[must_use] + pub fn new(id: Uuid, text: impl Into) -> Self { + Self { + id, + text: Arc::new(text.into()), + } + } +} + /// Main chat state container #[allow(clippy::struct_excessive_bools)] #[derive(Clone)] @@ -251,6 +278,16 @@ pub struct ChatState { pub marked_range: Option>, /// Inline tool approval bubbles pending or resolved in this session, keyed by conversation. pub approval_bubbles: HashMap>, + /// Steering messages accepted for the active conversation's running turn + /// and not yet delivered, in the order they were submitted. + /// + /// Scoped to the active conversation rather than keyed by one: the view + /// renders these inline in the transcript it is showing, and drops them + /// when the selection moves elsewhere. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-003 + pub queued_steering: Vec, /// Whether YOLO mode (auto-approve all) is currently active. pub yolo_mode: bool, /// Whether the sidebar is visible (popout mode only). @@ -299,6 +336,7 @@ impl Default for ChatState { chat_autoscroll_enabled: true, marked_range: None, approval_bubbles: HashMap::new(), + queued_steering: Vec::new(), yolo_mode: false, sidebar_visible: true, sidebar_search_query: String::new(), diff --git a/src/ui_gpui/views/chat_view/steering_queue_tests.rs b/src/ui_gpui/views/chat_view/steering_queue_tests.rs new file mode 100644 index 0000000..7a25552 --- /dev/null +++ b/src/ui_gpui/views/chat_view/steering_queue_tests.rs @@ -0,0 +1,587 @@ +//! Queued steering entries in the transcript (issue #222). +//! +//! A steer the service accepted has to be visible while it waits, gone once +//! it lands, and absent from everything the transcript treats as content. +//! These drive the real `handle_command` dispatch and the real Cmd+A path, +//! so an entry that lingers, leaks into another conversation, or shifts +//! selection identity fails here. +//! +//! @plan PLAN-20260903-ISSUE222.P04 +//! @requirement REQ-222-003 + +#![allow(clippy::future_not_send)] +#![allow(deprecated)] + +use super::state::{ChatMessage, ChatState, StreamingState}; +use super::transcript::TranscriptRow; +use super::ChatView; +use crate::presentation::view_command::{ConversationSummary, ViewCommand}; +use crate::ui_gpui::app_store::{ChatStoreSnapshot, ConversationLoadState, StreamingStoreSnapshot}; +use chrono::Utc; +use gpui::{AppContext, Entity, KeyDownEvent, Keystroke, Modifiers, TestAppContext}; +use gpui_selection_vendor::{TextSelection, TextSelectionContentKey}; +use uuid::Uuid; + +fn mid_turn_state(conversation_id: Uuid) -> ChatState { + ChatState { + active_conversation_id: Some(conversation_id), + messages: vec![ChatMessage::user("stable")], + streaming: StreamingState::Streaming { + content: "partial".to_string(), + done: false, + }, + ..ChatState::default() + } +} + +fn queued(conversation_id: Uuid, steer_id: Uuid, text: &str) -> ViewCommand { + ViewCommand::SteeringQueued { + conversation_id, + steer_id, + text: text.to_string(), + } +} + +/// The refusal the presenter sends, carrying the text the user submitted. +/// +/// @plan PLAN-20260903-ISSUE222.P08 +/// @requirement REQ-222-002 +fn rejected(conversation_id: Uuid, error: &str, text: &str) -> ViewCommand { + ViewCommand::SteeringRejected { + conversation_id, + error: error.to_string(), + text: text.to_string(), + } +} + +fn queued_texts(view: &ChatView) -> Vec { + view.state + .queued_steering + .iter() + .map(|entry| (*entry.text).clone()) + .collect() +} + +fn queued_rows(view: &ChatView) -> Vec { + view.transcript_rows() + .into_iter() + .filter(|row| matches!(row, TranscriptRow::QueuedSteering(_))) + .collect() +} + +fn ready_snapshot_for(conversation_id: Uuid, title: &str) -> ChatStoreSnapshot { + ChatStoreSnapshot { + selected_conversation_id: Some(conversation_id), + selected_conversation_title: title.to_string(), + selection_generation: 1, + load_state: ConversationLoadState::Ready { + conversation_id, + generation: 1, + }, + transcript: Vec::new(), + streaming: StreamingStoreSnapshot::default(), + conversations: vec![ConversationSummary { + id: conversation_id, + title: title.to_string(), + updated_at: Utc::now(), + message_count: 0, + preview: None, + }], + } +} + +// ── REQ-222-003: an accepted steer is visible until it lands ───────────── + +/// The whole point of a queued entry is that the user can see the +/// instruction is waiting, and see it stop waiting. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-003 +#[gpui::test] +fn a_queued_steer_appears_in_the_transcript_until_it_is_delivered(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let steer_id = Uuid::new_v4(); + let view = cx.new(|cx| ChatView::new(mid_turn_state(conversation_id), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.handle_command( + queued(conversation_id, steer_id, "use the cached index"), + cx, + ); + + assert_eq!(queued_texts(view), vec!["use the cached index".to_string()]); + assert_eq!( + queued_rows(view), + vec![TranscriptRow::QueuedSteering(0)], + "a waiting steer must have a row of its own in the transcript" + ); + + view.handle_command( + ViewCommand::SteeringDelivered { + conversation_id, + steer_id, + }, + cx, + ); + + assert!( + view.state.queued_steering.is_empty(), + "a delivered steer has been said to the model, so it stops waiting" + ); + assert!(queued_rows(view).is_empty()); + }); + }); +} + +/// Two steers wait in the order they were submitted, and delivering the +/// first leaves the second alone. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-003 +#[gpui::test] +fn queued_steers_keep_submission_order_and_are_withdrawn_one_at_a_time(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let first = Uuid::new_v4(); + let second = Uuid::new_v4(); + let view = cx.new(|cx| ChatView::new(mid_turn_state(conversation_id), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.handle_command(queued(conversation_id, first, "first"), cx); + view.handle_command(queued(conversation_id, second, "second"), cx); + assert_eq!( + queued_texts(view), + vec!["first".to_string(), "second".to_string()] + ); + + view.handle_command( + ViewCommand::SteeringDelivered { + conversation_id, + steer_id: first, + }, + cx, + ); + + assert_eq!( + queued_texts(view), + vec!["second".to_string()], + "delivery withdraws the entry it names and only that one" + ); + }); + }); +} + +/// A delivery id the view never queued belongs to something else. Removing +/// on a miss would silently drop a steer the user is still waiting on. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-003 +#[gpui::test] +fn delivery_of_an_unknown_id_withdraws_nothing(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let steer_id = Uuid::new_v4(); + let view = cx.new(|cx| ChatView::new(mid_turn_state(conversation_id), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.handle_command(queued(conversation_id, steer_id, "still waiting"), cx); + + view.handle_command( + ViewCommand::SteeringDelivered { + conversation_id, + steer_id: Uuid::new_v4(), + }, + cx, + ); + + assert_eq!( + queued_texts(view), + vec!["still waiting".to_string()], + "an unmatched delivery must leave the queue alone" + ); + }); + }); +} + +/// A discarded steer is never going to reach the model, so the entry +/// standing for it has to go. Left alone it waits on screen forever, which +/// is exactly the bug the discard event exists to close. +/// +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-003 +#[gpui::test] +fn a_discarded_steer_is_withdrawn_from_the_transcript(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let discarded = Uuid::new_v4(); + let kept = Uuid::new_v4(); + let view = cx.new(|cx| ChatView::new(mid_turn_state(conversation_id), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.handle_command(queued(conversation_id, discarded, "never delivered"), cx); + view.handle_command(queued(conversation_id, kept, "still waiting"), cx); + assert_eq!( + queued_rows(view), + vec![ + TranscriptRow::QueuedSteering(0), + TranscriptRow::QueuedSteering(1) + ], + "both steers must be waiting before one of them is discarded" + ); + + view.handle_command( + ViewCommand::SteeringDiscarded { + conversation_id, + steer_id: discarded, + }, + cx, + ); + + assert_eq!( + queued_texts(view), + vec!["still waiting".to_string()], + "a discard withdraws the entry it names and only that one" + ); + assert_eq!( + queued_rows(view), + vec![TranscriptRow::QueuedSteering(0)], + "the discarded entry must stop occupying a transcript row" + ); + }); + }); +} + +/// A discard id the view never queued belongs to something else. Removing on +/// a miss would take away a steer that is still on its way to the model. +/// +/// @plan PLAN-20260903-ISSUE222.P06 +/// @requirement REQ-222-003 +#[gpui::test] +fn a_discard_of_an_unknown_id_withdraws_nothing(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let steer_id = Uuid::new_v4(); + let view = cx.new(|cx| ChatView::new(mid_turn_state(conversation_id), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.handle_command(queued(conversation_id, steer_id, "still waiting"), cx); + + view.handle_command( + ViewCommand::SteeringDiscarded { + conversation_id, + steer_id: Uuid::new_v4(), + }, + cx, + ); + + assert_eq!( + queued_texts(view), + vec!["still waiting".to_string()], + "an unmatched discard must leave the queue alone" + ); + }); + }); +} + +/// A refused steer was never queued, so there is no entry under it to +/// withdraw. Removing anything here would take away a different steer that +/// the service is still holding. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-004 +#[gpui::test] +fn a_rejection_withdraws_no_queued_entry(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let steer_id = Uuid::new_v4(); + let view = cx.new(|cx| ChatView::new(mid_turn_state(conversation_id), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.handle_command(queued(conversation_id, steer_id, "accepted earlier"), cx); + + view.handle_command( + rejected(conversation_id, "Steering queue is full", "one too many"), + cx, + ); + + assert_eq!( + queued_texts(view), + vec!["accepted earlier".to_string()], + "a refusal concerns a steer that was never queued" + ); + }); + }); +} + +/// A refused steer gives the user their words back. +/// +/// Submitting clears the composer before the service has accepted anything, +/// so the rejection carries the only remaining copy of the text. Dropping it +/// would mean a full queue or a turn that ended mid-send silently destroys +/// what the user typed. +/// +/// @plan PLAN-20260903-ISSUE222.P08 +/// @requirement REQ-222-002 +#[gpui::test] +fn a_rejection_restores_the_text_into_an_empty_composer(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let view = cx.new(|cx| ChatView::new(mid_turn_state(conversation_id), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + // The composer cleared optimistically when Send was pressed. + assert!(view.state.input_text.is_empty()); + + view.handle_command( + rejected( + conversation_id, + "No active turn to steer", + "check the parser first", + ), + cx, + ); + + assert_eq!( + view.state.input_text, "check the parser first", + "a refused steer must come back to the composer it was typed in" + ); + assert_eq!( + view.state.cursor_position, + "check the parser first".len(), + "the caret must sit at the end of the restored text, ready to keep typing" + ); + }); + }); +} + +/// Restoring must never overwrite something newer. +/// +/// The refusal takes a round trip through the service, and the user is free +/// to type during it. What they are typing now is what they mean; the +/// refused text is older and loses. +/// +/// @plan PLAN-20260903-ISSUE222.P08 +/// @requirement REQ-222-002 +#[gpui::test] +fn a_rejection_leaves_newer_composer_text_alone(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let view = cx.new(|cx| ChatView::new(mid_turn_state(conversation_id), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.state.input_text = "a newer thought".to_string(); + view.state.cursor_position = "a newer".len(); + + view.handle_command( + rejected(conversation_id, "Steering queue is full", "the older steer"), + cx, + ); + + assert_eq!( + view.state.input_text, "a newer thought", + "a refusal must not clobber what the user is typing now" + ); + assert_eq!( + view.state.cursor_position, + "a newer".len(), + "leaving the text alone means leaving the caret where it was" + ); + }); + }); +} + +/// A refusal answers a steer for one conversation, and the user may have +/// gone elsewhere while it travelled. The words belonged where they were +/// typed; restoring them into the conversation the user is reading now +/// would inject a draft they never wrote there. +/// +/// @plan PLAN-20260903-ISSUE222.P08 +/// @requirement REQ-222-002 +#[gpui::test] +fn a_rejection_for_a_conversation_the_user_left_restores_nothing(cx: &mut TestAppContext) { + let first = Uuid::new_v4(); + let second = Uuid::new_v4(); + let view = cx.new(|cx| ChatView::new(ChatState::default(), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.apply_store_snapshot(ready_snapshot_for(first, "First"), cx); + view.apply_store_snapshot(ready_snapshot_for(second, "Second"), cx); + assert!( + view.state.input_text.is_empty(), + "the composer must start clean in the conversation switched to" + ); + + view.handle_command( + rejected(first, "No active turn to steer", "meant for the first"), + cx, + ); + + assert!( + view.state.input_text.is_empty(), + "a refusal for a conversation the user left must not land in the one being read" + ); + }); + }); +} + +/// Queued entries describe the transcript on screen. A steer accepted for +/// another conversation is not part of it. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-003 +#[gpui::test] +fn a_steer_for_another_conversation_is_not_shown_here(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let view = cx.new(|cx| ChatView::new(mid_turn_state(conversation_id), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.handle_command( + queued(Uuid::new_v4(), Uuid::new_v4(), "meant for elsewhere"), + cx, + ); + + assert!( + view.state.queued_steering.is_empty(), + "another conversation's steer must not appear in this transcript" + ); + }); + }); +} + +/// Switching away drops the entries with the transcript they annotated, the +/// way the cleared conversation's approval bubbles go. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-003 +#[gpui::test] +fn switching_conversations_clears_queued_entries(cx: &mut TestAppContext) { + let first = Uuid::new_v4(); + let second = Uuid::new_v4(); + let view = cx.new(|cx| ChatView::new(ChatState::default(), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.apply_store_snapshot(ready_snapshot_for(first, "First"), cx); + view.handle_command(queued(first, Uuid::new_v4(), "for the first"), cx); + assert_eq!(queued_texts(view), vec!["for the first".to_string()]); + + view.apply_store_snapshot(ready_snapshot_for(second, "Second"), cx); + + assert!( + view.state.queued_steering.is_empty(), + "a queued steer must not follow the user into another conversation" + ); + }); + }); +} + +/// Clearing a conversation takes its queued steers with it. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-003 +#[gpui::test] +fn clearing_the_conversation_clears_queued_entries(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let view = cx.new(|cx| ChatView::new(mid_turn_state(conversation_id), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.handle_command(queued(conversation_id, Uuid::new_v4(), "waiting"), cx); + assert_eq!(view.state.queued_steering.len(), 1); + + view.handle_command(ViewCommand::ConversationCleared, cx); + + assert!( + view.state.queued_steering.is_empty(), + "the turn these were waiting for has been cleared away" + ); + }); + }); +} + +// ── Selection identity must not skew ───────────────────────────────────── + +fn cmd_a() -> KeyDownEvent { + KeyDownEvent { + keystroke: Keystroke { + modifiers: Modifiers { + platform: true, + ..Modifiers::default() + }, + ..Keystroke::parse("a").expect("a keystroke") + }, + is_held: false, + prefer_character_input: false, + } +} + +fn transcript_keys(view: &ChatView) -> Vec { + vec![ + view.message_selection_content_key(0), + view.streaming_selection_content_key(), + ] +} + +/// Installs a window selection standing in for a user drag, because +/// `select_all_for_focused_surface` reaches the transcript only when one +/// already exists. +fn seed_drag_selection(window: &mut gpui::Window, cx: &mut gpui::Context) { + TextSelection::select_all(&[TextSelectionContentKey::new(999_999)], "drag", window, cx); +} + +/// Select-all is where row identity, leaf counting and the copy document all +/// have to agree. A queued entry adds a row, so if it were counted as +/// content the keys and the copied text would both skew. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-003 +#[gpui::test] +fn a_queued_entry_leaves_select_all_identical(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let view: Entity = cx.new(|cx| ChatView::new(mid_turn_state(conversation_id), cx)); + let mut visual_cx = cx.add_empty_window().clone(); + + visual_cx.update(|window, app| { + view.update(app, |view: &mut ChatView, cx| { + seed_drag_selection(window, cx); + view.handle_key_down(&cmd_a(), window, cx); + let keys_before = TextSelection::selected_content_keys(window, cx); + let text_before = TextSelection::selected_text(window, cx); + assert_eq!(keys_before, Some(transcript_keys(view))); + assert_eq!(text_before, "stable\n\npartial▋"); + + view.handle_command( + queued(conversation_id, Uuid::new_v4(), "do not copy me"), + cx, + ); + seed_drag_selection(window, cx); + view.handle_key_down(&cmd_a(), window, cx); + + assert_eq!( + TextSelection::selected_content_keys(window, cx), + keys_before, + "a queued entry must not add, remove or renumber a selection key" + ); + assert_eq!( + TextSelection::selected_text(window, cx), + text_before, + "a queued steer has not been said yet, so it is not part of the transcript" + ); + }); + }); +} diff --git a/src/ui_gpui/views/chat_view/steering_tests.rs b/src/ui_gpui/views/chat_view/steering_tests.rs new file mode 100644 index 0000000..f2349fe --- /dev/null +++ b/src/ui_gpui/views/chat_view/steering_tests.rs @@ -0,0 +1,463 @@ +//! Mid-turn steering from the chat composer (issue #222). +//! +//! Two controls share the composer row while a turn runs: Stop ends it, Send +//! joins it. These drive the real rendered buttons and the real key handler, +//! so a control that disappears, a click that reaches the wrong branch, or a +//! submit that restarts the turn all fail here. +//! +//! @plan PLAN-20260903-ISSUE222.P04 + +#![allow(clippy::future_not_send)] +#![allow(deprecated)] + +use super::state::{ChatState, StreamingState}; +use super::ChatView; +use crate::events::types::UserEvent; +use crate::presentation::view_command::ViewCommand; +use crate::ui_gpui::bridge::GpuiBridge; +use gpui::{ + px, size, Entity, KeyDownEvent, Keystroke, Modifiers, Pixels, Point, Render, TestAppContext, + VisualTestContext, +}; +use std::sync::Arc; +use uuid::Uuid; + +fn make_chat_bridge() -> (Arc, flume::Receiver) { + let (user_tx, user_rx) = flume::bounded(16); + let (_view_tx, view_rx) = flume::bounded(16); + (Arc::new(GpuiBridge::new(user_tx, view_rx)), user_rx) +} + +fn enter_event() -> KeyDownEvent { + KeyDownEvent { + keystroke: Keystroke::parse("enter").expect("enter keystroke"), + is_held: false, + prefer_character_input: false, + } +} + +/// A conversation whose turn is running, with `input` already typed. +fn mid_turn_state(conversation_id: Uuid, input: &str) -> ChatState { + ChatState { + active_conversation_id: Some(conversation_id), + streaming: StreamingState::Streaming { + content: "partial answer".to_string(), + done: false, + }, + input_text: input.to_string(), + cursor_position: input.len(), + ..ChatState::default() + } +} + +/// A conversation with nothing running, with `input` already typed. +fn idle_state(conversation_id: Uuid, input: &str) -> ChatState { + ChatState { + active_conversation_id: Some(conversation_id), + input_text: input.to_string(), + cursor_position: input.len(), + ..ChatState::default() + } +} + +/// Opens a window whose root is the chat view, so `debug_bounds` reports the +/// composer controls the user would actually click. +fn mount_chat_view( + cx: &mut TestAppContext, + state: ChatState, +) -> ( + Entity, + flume::Receiver, + &mut VisualTestContext, +) { + let (bridge, user_rx) = make_chat_bridge(); + let (view, window_cx) = cx.add_window_view(move |_window, cx| { + let mut view = ChatView::new(state, cx); + view.set_bridge(bridge); + view + }); + window_cx.simulate_resize(size(px(780.0), px(600.0))); + window_cx.run_until_parked(); + (view, user_rx, window_cx) +} + +/// Redraws the view so `debug_bounds` reflects the current state. +fn redraw(view: &Entity, window_cx: &mut VisualTestContext) { + window_cx.update(|window, app| { + view.update(app, |view: &mut ChatView, cx| { + cx.notify(); + let _ = view.render(window, cx); + }); + }); + window_cx.run_until_parked(); +} + +fn with_view( + view: &Entity, + window_cx: &mut VisualTestContext, + read: impl FnOnce(&ChatView) -> R, +) -> R { + window_cx.update(|_window, app| read(view.read(app))) +} + +fn press_enter(view: &Entity, window_cx: &mut VisualTestContext) { + window_cx.update(|window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.handle_key_down(&enter_event(), window, cx); + }); + }); + window_cx.run_until_parked(); +} + +fn click(window_cx: &mut VisualTestContext, selector: &'static str) { + let center: Point = window_cx + .debug_bounds(selector) + .unwrap_or_else(|| panic!("{selector} must be rendered")) + .center(); + window_cx.simulate_click(center, Modifiers::default()); + window_cx.run_until_parked(); +} + +fn emitted(user_rx: &flume::Receiver) -> Vec { + let mut events = Vec::new(); + while let Ok(event) = user_rx.try_recv() { + events.push(event); + } + events +} + +// ── REQ-222-001: Stop is a control of its own ──────────────────────────── + +/// A running turn offers both controls at once. Before this, Send was +/// replaced by Stop, so the only way to redirect the agent was to kill the +/// turn and lose what it had produced. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-001 +#[gpui::test] +fn a_running_turn_shows_stop_beside_send(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let (view, _user_rx, window_cx) = mount_chat_view(cx, mid_turn_state(conversation_id, "wait")); + redraw(&view, window_cx); + + assert!( + window_cx.debug_bounds("chat-stop-button").is_some(), + "a running turn must offer a Stop control" + ); + assert!( + window_cx.debug_bounds("chat-send-button").is_some(), + "a running turn must keep the Send control, or steering has no button" + ); +} + +/// With nothing running there is nothing to stop, so Stop is absent and only +/// Send remains — the composer the user has always seen. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-001 +#[gpui::test] +fn an_idle_conversation_shows_only_send(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let (view, _user_rx, window_cx) = mount_chat_view(cx, idle_state(conversation_id, "hello")); + redraw(&view, window_cx); + + assert!( + window_cx.debug_bounds("chat-stop-button").is_none(), + "there is no turn to stop, so Stop must not be rendered" + ); + assert!( + window_cx.debug_bounds("chat-send-button").is_some(), + "an idle composer must still offer Send" + ); +} + +/// Stop keeps doing exactly what it did: end the turn, leave the composer +/// text alone, and hand the keyboard back. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-001 +#[gpui::test] +fn clicking_stop_still_ends_the_turn(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let (view, user_rx, window_cx) = mount_chat_view(cx, mid_turn_state(conversation_id, "wait")); + redraw(&view, window_cx); + + click(window_cx, "chat-stop-button"); + + assert_eq!( + emitted(&user_rx), + vec![UserEvent::StopStreaming { conversation_id }], + "Stop must still cancel the turn it names" + ); + with_view(&view, window_cx, |view| { + assert_eq!( + view.state.streaming, + StreamingState::Idle, + "Stop resets the view's streaming state" + ); + assert_eq!( + view.state.input_text, "wait", + "Stop must not consume the composer text" + ); + }); +} + +// ── REQ-222-002: Send steers a running turn ────────────────────────────── + +/// Clicking Send mid-turn joins the turn instead of starting a new one, and +/// the composer empties the way it does for any accepted submit. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-002 +#[gpui::test] +fn clicking_send_mid_turn_steers_the_running_turn(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let (view, user_rx, window_cx) = + mount_chat_view(cx, mid_turn_state(conversation_id, "use the cached index")); + redraw(&view, window_cx); + + click(window_cx, "chat-send-button"); + + assert_eq!( + emitted(&user_rx), + vec![UserEvent::SteerStreaming { + conversation_id, + text: "use the cached index".to_string(), + }], + "Send mid-turn must steer, and must not start a second turn" + ); + with_view(&view, window_cx, |view| { + assert_eq!( + view.state.input_text, "", + "an accepted steer clears the composer" + ); + assert_eq!(view.state.cursor_position, 0); + }); +} + +/// Enter is the same submit as the button; routing them separately is how +/// the two drift apart. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-002 +#[gpui::test] +fn pressing_enter_mid_turn_steers_the_running_turn(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let (view, user_rx, window_cx) = mount_chat_view( + cx, + mid_turn_state(conversation_id, "check the config first"), + ); + + press_enter(&view, window_cx); + + assert_eq!( + emitted(&user_rx), + vec![UserEvent::SteerStreaming { + conversation_id, + text: "check the config first".to_string(), + }], + "Enter mid-turn must steer, and must not start a second turn" + ); + with_view(&view, window_cx, |view| { + assert_eq!(view.state.input_text, ""); + assert_eq!(view.state.cursor_position, 0); + }); +} + +/// Steering is additive: the turn it joins keeps running, so the view's +/// streaming state must survive the submit untouched. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-002 +/// @requirement REQ-222-006 +#[gpui::test] +fn steering_leaves_the_turn_running(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let (view, user_rx, window_cx) = mount_chat_view(cx, mid_turn_state(conversation_id, "and")); + + let before = with_view(&view, window_cx, |view| view.state.streaming.clone()); + press_enter(&view, window_cx); + + with_view(&view, window_cx, |view| { + assert_eq!( + view.state.streaming, before, + "a steer must not restart, finish or clear the turn it joins" + ); + }); + let events = emitted(&user_rx); + assert!( + events + .iter() + .any(|event| matches!(event, UserEvent::SteerStreaming { .. })), + "the submit under test must have steered, got {events:?}" + ); + assert!( + !events + .iter() + .any(|event| matches!(event, UserEvent::StopStreaming { .. })), + "a steer must never cancel the turn on the user's behalf" + ); +} + +/// Whitespace is not an instruction. It was a no-op mid-turn before and +/// stays one, through both controls. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-002 +#[gpui::test] +fn blank_input_submits_nothing_mid_turn(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let (view, user_rx, window_cx) = + mount_chat_view(cx, mid_turn_state(conversation_id, " \n ")); + redraw(&view, window_cx); + + click(window_cx, "chat-send-button"); + press_enter(&view, window_cx); + + assert_eq!( + emitted(&user_rx), + Vec::new(), + "blank input must submit nothing while a turn runs" + ); + with_view(&view, window_cx, |view| { + assert_eq!( + view.state.streaming, + StreamingState::Streaming { + content: "partial answer".to_string(), + done: false, + }, + "blank input must not disturb the running turn" + ); + assert_eq!( + view.state.input_text, " \n ", + "a refused submit leaves the composer as typed" + ); + }); +} + +/// The same rule while idle, which is where it already held. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-002 +#[gpui::test] +fn blank_input_submits_nothing_while_idle(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let (view, user_rx, window_cx) = mount_chat_view(cx, idle_state(conversation_id, " \n ")); + redraw(&view, window_cx); + + click(window_cx, "chat-send-button"); + press_enter(&view, window_cx); + + assert_eq!( + emitted(&user_rx), + Vec::new(), + "blank input must submit nothing while idle" + ); + with_view(&view, window_cx, |view| { + assert_eq!( + view.state.streaming, + StreamingState::Idle, + "a refused submit must not start a turn" + ); + }); +} + +// ── REQ-222-003: the queued entry is on screen ─────────────────────────── + +/// A queued steer has to be visible, not merely recorded: the point of the +/// entry is that the user can see the instruction is waiting its turn. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-003 +#[gpui::test] +fn a_queued_steer_is_painted_in_the_transcript(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let (view, _user_rx, window_cx) = mount_chat_view(cx, mid_turn_state(conversation_id, "")); + redraw(&view, window_cx); + assert!( + window_cx.debug_bounds("chat-queued-steering-0").is_none(), + "nothing is queued yet, so no entry should be on screen" + ); + + window_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, cx| { + view.handle_command( + ViewCommand::SteeringQueued { + conversation_id, + steer_id: Uuid::new_v4(), + text: "hold off on the refactor".to_string(), + }, + cx, + ); + }); + }); + redraw(&view, window_cx); + + assert!( + window_cx.debug_bounds("chat-queued-steering-0").is_some(), + "a waiting steer must be painted in the transcript" + ); +} + +// ── Regression guard: the idle submit path is unchanged ────────────────── + +/// The idle path is the one users take all day; steering must not disturb it. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-002 +#[gpui::test] +fn clicking_send_while_idle_still_sends_a_message(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let (view, user_rx, window_cx) = mount_chat_view(cx, idle_state(conversation_id, "first ask")); + window_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, _cx| { + view.conversation_id = Some(conversation_id); + }); + }); + redraw(&view, window_cx); + + click(window_cx, "chat-send-button"); + + assert_eq!( + emitted(&user_rx), + vec![UserEvent::SendMessage { + text: "first ask".to_string(), + conversation_id: Some(conversation_id), + }], + "an idle Send must still start a turn" + ); + with_view(&view, window_cx, |view| { + assert_eq!( + view.state.streaming, + StreamingState::Streaming { + content: String::new(), + done: false, + }, + "an idle Send still moves the view into streaming" + ); + }); +} + +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-002 +#[gpui::test] +fn pressing_enter_while_idle_still_sends_a_message(cx: &mut TestAppContext) { + let conversation_id = Uuid::new_v4(); + let (view, user_rx, window_cx) = mount_chat_view(cx, idle_state(conversation_id, "first ask")); + window_cx.update(|_window, app| { + view.update(app, |view: &mut ChatView, _cx| { + view.conversation_id = Some(conversation_id); + }); + }); + + press_enter(&view, window_cx); + + assert_eq!( + emitted(&user_rx), + vec![UserEvent::SendMessage { + text: "first ask".to_string(), + conversation_id: Some(conversation_id), + }], + "an idle Enter must still start a turn" + ); +} diff --git a/src/ui_gpui/views/chat_view/transcript.rs b/src/ui_gpui/views/chat_view/transcript.rs index 13fd241..51c2b2a 100644 --- a/src/ui_gpui/views/chat_view/transcript.rs +++ b/src/ui_gpui/views/chat_view/transcript.rs @@ -12,14 +12,29 @@ pub(super) enum TranscriptRow { Message(usize), Approval(usize), Streaming, + /// A steering message waiting for the running turn's boundary, indexed + /// into `ChatState::queued_steering`. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-003 + QueuedSteering(usize), } +/// Flattens the transcript into the rows the virtualized list paints. +/// +/// Queued steering entries sit after the live stream block: they were typed +/// after everything above them and have not reached the model yet, so they +/// are the newest thing in the conversation and belong last. +/// +/// @plan PLAN-20260903-ISSUE222.P04 +/// @requirement REQ-222-003 pub(super) fn build_transcript_rows( message_count: usize, approval_bubbles: &[ToolApprovalBubble], streaming: &StreamingState, + queued_steering_count: usize, ) -> Vec { - let mut rows = Vec::with_capacity(message_count + 2); + let mut rows = Vec::with_capacity(message_count + 2 + queued_steering_count); rows.extend((0..message_count).map(TranscriptRow::Message)); if let Some((index, _)) = approval_bubbles @@ -34,6 +49,8 @@ pub(super) fn build_transcript_rows( rows.push(TranscriptRow::Streaming); } + rows.extend((0..queued_steering_count).map(TranscriptRow::QueuedSteering)); + rows } @@ -103,7 +120,13 @@ pub(super) fn transcript_row_leaf_count( displayed_message_thinking(message, show_thinking, filter_emoji), ) } - TranscriptRow::Approval(_) => return 0, + // Approvals and queued steering entries are chrome, not transcript + // content: neither is selectable or copyable, so neither contributes + // a leaf and neither shifts the document order of the rows around it. + // + // @plan PLAN-20260903-ISSUE222.P04 + // @requirement REQ-222-003 + TranscriptRow::Approval(_) | TranscriptRow::QueuedSteering(_) => return 0, TranscriptRow::Streaming => { let content = match streaming { StreamingState::Streaming { content, .. } => content.as_str(), @@ -353,7 +376,7 @@ mod tests { #[test] fn empty_transcript_has_no_rows() { - assert!(build_transcript_rows(0, &[], &StreamingState::Idle).is_empty()); + assert!(build_transcript_rows(0, &[], &StreamingState::Idle, 0).is_empty()); } #[test] @@ -366,6 +389,7 @@ mod tests { content: "partial".to_string(), done: false, }, + 0, ), vec![ TranscriptRow::Message(0), @@ -388,7 +412,7 @@ mod tests { }; assert_eq!( - build_transcript_rows(1, &bubbles, &streaming), + build_transcript_rows(1, &bubbles, &streaming, 0), vec![ TranscriptRow::Message(0), TranscriptRow::Approval(1), @@ -397,6 +421,53 @@ mod tests { ); } + /// Queued steers are the newest thing in the conversation: typed after + /// everything above them and not yet seen by the model. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-003 + #[test] + fn queued_steering_rows_follow_the_streaming_row() { + let bubbles = vec![approval(ApprovalBubbleState::Pending)]; + let streaming = StreamingState::Streaming { + content: "partial".to_string(), + done: false, + }; + + assert_eq!( + build_transcript_rows(1, &bubbles, &streaming, 2), + vec![ + TranscriptRow::Message(0), + TranscriptRow::Approval(0), + TranscriptRow::Streaming, + TranscriptRow::QueuedSteering(0), + TranscriptRow::QueuedSteering(1), + ] + ); + } + + /// A queued steer contributes no selectable leaf, so it cannot shift the + /// document order of the rows it follows. + /// + /// @plan PLAN-20260903-ISSUE222.P04 + /// @requirement REQ-222-003 + #[test] + fn queued_steering_rows_contribute_no_leaves() { + let messages = vec![ChatMessage::user("go")]; + + assert_eq!( + transcript_row_leaf_count( + TranscriptRow::QueuedSteering(0), + &messages, + &StreamingState::Idle, + false, + true, + Some("thinking"), + ), + 0 + ); + } + #[test] fn document_orders_are_prefix_sums_in_transcript_row_order() { assert_eq!(derive_document_orders(&[2, 3, 1]), vec![0, 2, 5]); diff --git a/tests/chat_presenter_coverage_tests.rs b/tests/chat_presenter_coverage_tests.rs index c7a8be9..1676dab 100644 --- a/tests/chat_presenter_coverage_tests.rs +++ b/tests/chat_presenter_coverage_tests.rs @@ -279,6 +279,10 @@ impl ConversationService for MockConversationService { struct MockChatService { fail_send: Mutex>, cancelled: AtomicBool, + /// `(conversation_id, text)` for every `steer` call, in order. + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-006 + steered: Mutex>, } impl MockChatService { @@ -286,6 +290,7 @@ impl MockChatService { Self { fail_send: Mutex::new(None), cancelled: AtomicBool::new(false), + steered: Mutex::new(Vec::new()), } } @@ -321,6 +326,13 @@ impl ChatService for MockChatService { false } + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-006 + async fn steer(&self, conversation_id: Uuid, text: String) -> Result { + self.steered.lock().await.push((conversation_id, text)); + Ok(Uuid::new_v4()) + } + async fn resolve_tool_approval( &self, _request_id: String, @@ -850,6 +862,14 @@ async fn stop_streaming_invokes_chat_service_cancel() { let _ = collect_commands(&mut view_rx).await; assert!(chat_service.cancelled.load(Ordering::SeqCst)); + // Stop and steer are separate controls: stopping a turn must not queue a + // steering message. + // @plan PLAN-20260903-ISSUE222.P01 + // @requirement REQ-222-006 + assert!( + chat_service.steered.lock().await.is_empty(), + "StopStreaming must not reach ChatService::steer" + ); } #[tokio::test] diff --git a/tests/e2e_presenter_chat.rs b/tests/e2e_presenter_chat.rs index bef1700..55ab282 100644 --- a/tests/e2e_presenter_chat.rs +++ b/tests/e2e_presenter_chat.rs @@ -374,6 +374,18 @@ async fn test_chat_presenter_error_handling() { false } + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-004 + async fn steer( + &self, + _conversation_id: Uuid, + _text: String, + ) -> Result { + Err(personal_agent::services::ServiceError::Validation( + "No active turn to steer".to_string(), + )) + } + async fn resolve_tool_approval( &self, _request_id: String, diff --git a/tests/presenter_selection_and_settings_tests.rs b/tests/presenter_selection_and_settings_tests.rs index 98f09e5..098db20 100644 --- a/tests/presenter_selection_and_settings_tests.rs +++ b/tests/presenter_selection_and_settings_tests.rs @@ -158,6 +158,14 @@ impl ChatService for MockChatService { false } + /// @plan PLAN-20260903-ISSUE222.P01 + /// @requirement REQ-222-004 + async fn steer(&self, _conversation_id: Uuid, _text: String) -> Result { + Err(ServiceError::Validation( + "No active turn to steer".to_string(), + )) + } + async fn resolve_tool_approval( &self, _request_id: String,