From b058ccbdda8c35b4a7e8305aac3d8ecebf4f65af Mon Sep 17 00:00:00 2001 From: Adam Dalloul <47503782+Adam-Dalloul@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:44:49 -0700 Subject: [PATCH 1/4] fix(chat): steer a draft with image attachments instead of refusing it The composer's "insert into current turn" entry went dark the moment an image was staged, and a draft with file badges was silently rerouted to the queue. The whole steering chain narrowed the draft to a bare string (ConnectionCommand::Steer carried text, build_steer_params hardcoded a single text block), so those gates were the honest option. The wire never had that limit: the claude adapter's _session/steering handler feeds the prompt array through the same conversion as session/prompt, images included. Steer now carries the draft's PromptInputBlocks end to end. The composer sends the full block list whenever the draft holds more than plain text, with the display text as the recorded note; the backend maps the blocks with map_prompt_blocks, the exact session/prompt encoding, and re-hydrates uploaded file:// markers like /acp_prompt does, so web and remote mode work unchanged. The prompt ledger fingerprints the steered blocks the same way a prompt's are, and a text-only steer stays byte-identical to before. The pull channel still delivers plain text, so a blocks-bearing note on a session that downgraded mid-race is rejected with NoActiveTurn and the composer's existing fallback queues the whole draft, attachments included; an attachment is never silently dropped. Steering while an upload is still settling gets the same toast a plain send does, and the now-unused steerAttachmentsUnsupported string is gone from all ten locales. --- src-tauri/src/acp/background_watch.rs | 11 +- src-tauri/src/acp/connection.rs | 82 +++++++--- src-tauri/src/acp/manager.rs | 149 +++++++++++++++--- src-tauri/src/acp/types.rs | 2 +- src-tauri/src/commands/feedback.rs | 8 +- src-tauri/src/web/handlers/feedback.rs | 8 +- src/components/chat/message-input.test.tsx | 141 ++++++++++++++++- src/components/chat/message-input.tsx | 59 +++---- .../conversation-detail-panel.tsx | 7 +- src/hooks/use-session-feedback.test.ts | 19 ++- src/hooks/use-session-feedback.ts | 21 ++- src/i18n/messages/ar.json | 1 - src/i18n/messages/de.json | 1 - src/i18n/messages/en.json | 1 - src/i18n/messages/es.json | 1 - src/i18n/messages/fr.json | 1 - src/i18n/messages/ja.json | 1 - src/i18n/messages/ko.json | 1 - src/i18n/messages/pt.json | 1 - src/i18n/messages/zh-CN.json | 1 - src/i18n/messages/zh-TW.json | 1 - src/lib/api.ts | 17 +- 22 files changed, 432 insertions(+), 102 deletions(-) diff --git a/src-tauri/src/acp/background_watch.rs b/src-tauri/src/acp/background_watch.rs index 82a16057f6..f797711a07 100644 --- a/src-tauri/src/acp/background_watch.rs +++ b/src-tauri/src/acp/background_watch.rs @@ -184,12 +184,11 @@ impl PromptLedger { false } - /// Fingerprint a bare string. Used for `_session/steering` injections, - /// which reach the agent outside `session/prompt` yet still land in the - /// transcript as a user record that [`group_into_turns`] reads as the - /// start of a turn — one the wire is already rendering, so it must - /// classify foreground like any prompt (see the `Steer` arm in - /// `connection.rs`). + /// Fingerprint a bare string — test convenience over + /// [`Self::record_prompt_blocks`]. (The `_session/steering` arm in + /// `connection.rs` used to be the production caller; it now records the + /// steered blocks directly, since a steered draft can carry attachments.) + #[cfg(test)] pub(crate) fn record_text(&self, text: &str) { self.record_prompt_blocks(&[crate::acp::types::PromptInputBlock::Text { text: text.to_string(), diff --git a/src-tauri/src/acp/connection.rs b/src-tauri/src/acp/connection.rs index 581e190642..d9d5eb307c 100644 --- a/src-tauri/src/acp/connection.rs +++ b/src-tauri/src/acp/connection.rs @@ -960,16 +960,18 @@ pub enum ConnectionCommand { reply: tokio::sync::oneshot::Sender>, }, - /// Inject a live-feedback note into the RUNNING turn over the ACP + /// Inject a live-feedback message into the RUNNING turn over the ACP /// `_session/steering` extension (native push channel — see - /// `manager::submit_feedback`). The loop does the protocol round-trip - /// only and replies the parsed outcome; recording the note + the - /// `FeedbackSubmitted` broadcast happen in the manager's - /// cancellation-shielded task, mirroring Fork's protocol/persistence - /// split. The idle arm replies `Err(NoActiveTurn)` so the oneshot can - /// never hang. + /// `manager::submit_feedback`). Carries the same `PromptInputBlock`s a + /// normal prompt does (text plus image attachments), mapped onto the wire + /// with the same conversion, so a steered draft keeps its attachments. + /// The loop does the protocol round-trip only and replies the parsed + /// outcome; recording the note + the `FeedbackSubmitted` broadcast happen + /// in the manager's cancellation-shielded task, mirroring Fork's + /// protocol/persistence split. The idle arm replies `Err(NoActiveTurn)` + /// so the oneshot can never hang. Steer { - text: String, + blocks: Vec, reply: tokio::sync::oneshot::Sender>, }, Disconnect, @@ -3148,9 +3150,9 @@ fn build_grok_set_model_params( async fn send_steer_request( cx: &ConnectionTo, session_id: &SessionId, - text: &str, + blocks: &[PromptInputBlock], ) -> Result { - let params = build_steer_params(session_id.0.as_ref(), text); + let params = build_steer_params(session_id.0.as_ref(), blocks); let untyped_req = UntypedMessage::new("_session/steering", params).map_err(|e| { AcpError::protocol(format!("Failed to build steering request: {e}")) })?; @@ -3162,15 +3164,19 @@ async fn send_steer_request( parse_steer_outcome(&raw) } -/// Build the `_session/steering` params. The prompt is a single text block -/// (codeg steering is text-only), and `_meta.steering.idleBehavior = -/// "promptRequired"` opts into the turn-end-race contract: a turn that -/// settled first yields `{outcome:"promptRequired"}` WITHOUT consuming the -/// content, so the host resubmits it through a normal `session/prompt`. -fn build_steer_params(session_id: &str, text: &str) -> serde_json::Value { +/// Build the `_session/steering` params. The prompt carries the caller's +/// blocks through [`map_prompt_blocks`] — the SAME conversion a +/// `session/prompt` uses — so a steered draft's image attachments reach the +/// adapter in the exact encoding its prompt path already accepts (a plain +/// note is still a single text block, as before). `_meta.steering +/// .idleBehavior = "promptRequired"` opts into the turn-end-race contract: a +/// turn that settled first yields `{outcome:"promptRequired"}` WITHOUT +/// consuming the content, so the host resubmits it through a normal +/// `session/prompt`. +fn build_steer_params(session_id: &str, blocks: &[PromptInputBlock]) -> serde_json::Value { serde_json::json!({ "sessionId": session_id, - "prompt": [{ "type": "text", "text": text }], + "prompt": map_prompt_blocks(blocks.to_vec()), "_meta": { "steering": { "idleBehavior": "promptRequired" } }, }) } @@ -8700,7 +8706,7 @@ async fn run_conversation_loop<'a>( let _ = reply.send(landed); } } - Some(ConnectionCommand::Steer { text, reply }) => { + Some(ConnectionCommand::Steer { blocks, reply }) => { // Protocol round-trip only — the manager's // cancellation-shielded task records the // note + broadcasts `FeedbackSubmitted` @@ -8712,7 +8718,7 @@ async fn run_conversation_loop<'a>( // commands, not session updates. A dead // receiver is fine — the reply is then // moot (teardown), nothing to unwind. - let outcome = send_steer_request(&cx, &sid, &text).await; + let outcome = send_steer_request(&cx, &sid, &blocks).await; // A steered message still lands in the // agent's OWN transcript as a user record, // which `group_into_turns` reads as the @@ -8736,7 +8742,7 @@ async fn run_conversation_loop<'a>( // — the overlay is the only place its work // can surface at all. if matches!(outcome, Ok(SteerOutcome::Injected)) { - prompt_ledger.record_text(&text); + prompt_ledger.record_prompt_blocks(&blocks); } let _ = reply.send(outcome); } @@ -8973,7 +8979,7 @@ async fn run_conversation_loop<'a>( let _ = reply.send(landed); } } - Some(ConnectionCommand::Steer { text: _, reply }) => { + Some(ConnectionCommand::Steer { blocks: _, reply }) => { // Steering only means something for a RUNNING turn. Reply — // never drop — so the manager's shielded task can't hang on // the oneshot; the caller falls back to a normal prompt (the @@ -13583,7 +13589,12 @@ mod tests { #[test] fn build_steer_params_shape_carries_the_prompt_required_opt_in() { - let params = build_steer_params("sess-1", "use the staging db"); + let params = build_steer_params( + "sess-1", + &[crate::acp::types::PromptInputBlock::Text { + text: "use the staging db".into(), + }], + ); assert_eq!(params["sessionId"], "sess-1"); assert_eq!(params["prompt"][0]["type"], "text"); assert_eq!(params["prompt"][0]["text"], "use the staging db"); @@ -13592,6 +13603,33 @@ mod tests { assert_eq!(params["_meta"]["steering"]["idleBehavior"], "promptRequired"); } + #[test] + fn build_steer_params_maps_image_blocks_like_a_prompt() { + // A steered draft with an attachment must hit the wire in the SAME + // encoding `session/prompt` uses (`map_prompt_blocks`): the adapter's + // steering handler feeds the array through its normal prompt + // conversion, so ACP camelCase (`mimeType`) is what it reads. + let params = build_steer_params( + "sess-1", + &[ + crate::acp::types::PromptInputBlock::Text { + text: "match this mock".into(), + }, + crate::acp::types::PromptInputBlock::Image { + data: "aGk=".into(), + mime_type: "image/png".into(), + uri: None, + }, + ], + ); + assert_eq!(params["prompt"][0]["type"], "text"); + assert_eq!(params["prompt"][0]["text"], "match this mock"); + assert_eq!(params["prompt"][1]["type"], "image"); + assert_eq!(params["prompt"][1]["data"], "aGk="); + assert_eq!(params["prompt"][1]["mimeType"], "image/png"); + assert_eq!(params["_meta"]["steering"]["idleBehavior"], "promptRequired"); + } + #[test] fn parse_steer_outcome_maps_the_wire_strings_and_rejects_unknowns() { assert_eq!( diff --git a/src-tauri/src/acp/manager.rs b/src-tauri/src/acp/manager.rs index 21dbc3b457..1c9bcfffe8 100644 --- a/src-tauri/src/acp/manager.rs +++ b/src-tauri/src/acp/manager.rs @@ -2469,10 +2469,15 @@ impl ConnectionManager { /// steer and the note would strand (the frontend falls back to an ordinary /// prompt). The append rides `emit_with_state` so `SessionState.feedback`, /// the ring buffer, and every attached client stay in lockstep. + /// `blocks`, when present, is the full prompt-block draft (text plus + /// image attachments) to deliver on the native wire instead of the bare + /// `text` — `text` then serves as the recorded note. Only the native + /// channel can carry blocks; see the pull-path gate below. pub async fn submit_feedback( &self, conn_id: &str, text: String, + blocks: Option>, ) -> Result { let trimmed = text.trim(); if trimmed.is_empty() { @@ -2484,6 +2489,7 @@ impl ConnectionManager { ))); } let text = trimmed.to_string(); + let blocks = blocks.filter(|b| !b.is_empty()); let (state, cmd_tx, emitter) = { let connections = self.connections.lock().await; let conn = connections @@ -2512,7 +2518,19 @@ impl ConnectionManager { } if native { - return Self::submit_feedback_native(conn_id, state, cmd_tx, emitter, text).await; + return Self::submit_feedback_native(conn_id, state, cmd_tx, emitter, text, blocks) + .await; + } + + // The pull tool delivers plain text (`PendingFeedback`), so a draft + // carrying attachment blocks cannot ride it without silently dropping + // them. This only arises when the channel downgraded between the + // frontend's channel read and this call (startedNewTurn latch); + // `NoActiveTurn` is the rejection the caller already maps to its + // queue fallback, which re-routes the WHOLE draft — attachments + // included — as the next turn's prompt. + if blocks.is_some() { + return Err(AcpError::NoActiveTurn); } let item = FeedbackItem::new_pending( @@ -2567,6 +2585,7 @@ impl ConnectionManager { cmd_tx: tokio::sync::mpsc::Sender, emitter: EventEmitter, text: String, + blocks: Option>, ) -> Result { // Cheap pre-flight, NOT the authoritative check (that's the loop's // idle arm replying `NoActiveTurn`): skip the round-trip when no turn @@ -2574,13 +2593,31 @@ impl ConnectionManager { if !state.read().await.turn_in_flight { return Err(AcpError::NoActiveTurn); } + // The wire payload: the caller's full draft when it carried blocks + // (attachments included), else the recorded text as a single block — + // byte-identical to the historical text-only steer. Uploaded-image + // markers (web / remote mode) are re-hydrated exactly like a prompt's, + // AFTER the admission checks above so a rejected steer never triggers + // file reads, and BEFORE the shield below so a failure aborts with no + // side effects. + let wire_blocks = match blocks { + Some(mut blocks) => { + crate::acp::prompt_hydration::hydrate_prompt_blocks( + &mut blocks, + &crate::paths::codeg_uploads_root(), + ) + .await?; + blocks + } + None => vec![PromptInputBlock::Text { text: text.clone() }], + }; let conn_id_for_task = conn_id.to_string(); let handle = tokio::spawn(async move { let outcome: Result = async { let (reply_tx, reply_rx) = tokio::sync::oneshot::channel(); cmd_tx .send(ConnectionCommand::Steer { - text: text.clone(), + blocks: wire_blocks, reply: reply_tx, }) .await @@ -7395,7 +7432,7 @@ mod tests { // (e.g. its session started before the feature was enabled), even mid-turn. let state = mgr.get_state("c1").await.unwrap(); state.write().await.turn_in_flight = true; - let err = mgr.submit_feedback("c1", "note".into()).await.unwrap_err(); + let err = mgr.submit_feedback("c1", "note".into(), None).await.unwrap_err(); assert!(matches!(err, AcpError::FeedbackDisabled)); assert!(state.read().await.feedback.is_empty()); } @@ -7407,7 +7444,7 @@ mod tests { .await; // Tool available but no turn in flight → nothing to steer. set_feedback_tool_available(&mgr, "c1").await; - let err = mgr.submit_feedback("c1", "note".into()).await.unwrap_err(); + let err = mgr.submit_feedback("c1", "note".into(), None).await.unwrap_err(); assert!(matches!(err, AcpError::NoActiveTurn)); // And nothing was appended. let state = mgr.get_state("c1").await.unwrap(); @@ -7418,7 +7455,7 @@ mod tests { async fn submit_feedback_missing_connection_errors() { let mgr = ConnectionManager::new(); let err = mgr - .submit_feedback("nope", "note".into()) + .submit_feedback("nope", "note".into(), None) .await .unwrap_err(); assert!(matches!(err, AcpError::ConnectionNotFound(_))); @@ -7431,7 +7468,7 @@ mod tests { .await; mark_feedback_ready(&mgr, "c1").await; let item = mgr - .submit_feedback("c1", " use UserService ".into()) + .submit_feedback("c1", " use UserService ".into(), None) .await .unwrap(); assert_eq!(item.status, FeedbackStatus::Pending); @@ -7451,16 +7488,16 @@ mod tests { mark_feedback_ready(&mgr, "c1").await; // Empty / whitespace-only → rejected, nothing appended. for empty in ["", " ", "\n\t "] { - let err = mgr.submit_feedback("c1", empty.into()).await.unwrap_err(); + let err = mgr.submit_feedback("c1", empty.into(), None).await.unwrap_err(); assert!(matches!(err, AcpError::InvalidFeedback(_))); } // Oversized → rejected. let huge = "x".repeat(MAX_FEEDBACK_CHARS + 1); - let err = mgr.submit_feedback("c1", huge).await.unwrap_err(); + let err = mgr.submit_feedback("c1", huge, None).await.unwrap_err(); assert!(matches!(err, AcpError::InvalidFeedback(_))); // Exactly at the bound is accepted. let at_bound = "y".repeat(MAX_FEEDBACK_CHARS); - assert!(mgr.submit_feedback("c1", at_bound).await.is_ok()); + assert!(mgr.submit_feedback("c1", at_bound, None).await.is_ok()); let state = mgr.get_state("c1").await.unwrap(); assert_eq!(state.read().await.feedback.len(), 1, "only the valid note stuck"); } @@ -7475,16 +7512,16 @@ mod tests { } /// Play the connection loop's role: receive one `Steer` command and reply - /// the given outcome. Returns the text the command carried. + /// the given outcome. Returns the blocks the command carried. fn answer_steer( mut rx: tokio::sync::mpsc::Receiver, outcome: Result, - ) -> tokio::task::JoinHandle { + ) -> tokio::task::JoinHandle> { tokio::spawn(async move { match rx.recv().await { - Some(ConnectionCommand::Steer { text, reply }) => { + Some(ConnectionCommand::Steer { blocks, reply }) => { let _ = reply.send(outcome); - text + blocks } _ => panic!("expected a Steer command"), } @@ -7504,12 +7541,18 @@ mod tests { set_feedback_tool_available(&mgr, "c1").await; let fake_loop = answer_steer(rx, Ok(SteerOutcome::Injected)); - let item = mgr.submit_feedback("c1", " ship it ".into()).await.unwrap(); + let item = mgr.submit_feedback("c1", " ship it ".into(), None).await.unwrap(); assert_eq!(item.status, FeedbackStatus::Delivered); assert!(item.delivered_at.is_some()); assert_eq!(item.text, "ship it"); - // The wire carried the trimmed text. - assert_eq!(fake_loop.await.unwrap(), "ship it"); + // The wire carried the trimmed text as a single block (a blocks-less + // submit stays byte-identical to the historical text-only steer). + assert_eq!( + fake_loop.await.unwrap(), + vec![PromptInputBlock::Text { + text: "ship it".into() + }] + ); let state = mgr.get_state("c1").await.unwrap(); { @@ -7524,6 +7567,64 @@ mod tests { ); } + #[tokio::test] + async fn native_submit_with_blocks_carries_the_draft_and_records_the_text() { + // A draft with an image attachment steers as its full block list (the + // wire payload) while the recorded note stays the display text — the + // strip/snapshot/broadcast never carry image bytes. + let mgr = ConnectionManager::new(); + let rx = mgr + .insert_test_connection_live("c1", AgentType::ClaudeCode, None, EventEmitter::Noop) + .await; + mark_native_steering_ready(&mgr, "c1").await; + let fake_loop = answer_steer(rx, Ok(SteerOutcome::Injected)); + + let draft = vec![ + PromptInputBlock::Text { + text: "make it match this mock".into(), + }, + PromptInputBlock::Image { + data: "aGk=".into(), + mime_type: "image/png".into(), + uri: None, + }, + ]; + let item = mgr + .submit_feedback("c1", "make it match this mock".into(), Some(draft.clone())) + .await + .unwrap(); + assert_eq!(item.status, FeedbackStatus::Delivered); + assert_eq!(item.text, "make it match this mock"); + // The wire carried the caller's blocks verbatim, attachment included. + assert_eq!(fake_loop.await.unwrap(), draft); + } + + #[tokio::test] + async fn pull_submit_with_blocks_rejects_instead_of_dropping_attachments() { + // The pull tool delivers plain text, so a blocks-bearing note on a + // pull-only session (native downgraded mid-race) must reject with + // NoActiveTurn — the caller's queue fallback re-routes the whole + // draft — rather than deliver the text and silently drop the image. + let mgr = ConnectionManager::new(); + mgr.insert_test_connection("c1", AgentType::ClaudeCode, None, EventEmitter::Noop) + .await; + mark_feedback_ready(&mgr, "c1").await; + let draft = vec![PromptInputBlock::Image { + data: "aGk=".into(), + mime_type: "image/png".into(), + uri: None, + }]; + let err = mgr + .submit_feedback("c1", "1 attachment".into(), Some(draft)) + .await + .unwrap_err(); + assert!(matches!(err, AcpError::NoActiveTurn)); + // Nothing recorded: the content is still draft-owned. + let state = mgr.get_state("c1").await.unwrap(); + assert!(state.read().await.feedback.is_empty()); + assert!(mgr.read_pending_feedback("c1").await.is_empty()); + } + #[tokio::test] async fn native_submit_prompt_required_maps_to_no_active_turn_and_records_nothing() { let mgr = ConnectionManager::new(); @@ -7533,7 +7634,7 @@ mod tests { mark_native_steering_ready(&mgr, "c1").await; let fake_loop = answer_steer(rx, Ok(SteerOutcome::PromptRequired)); - let err = mgr.submit_feedback("c1", "note".into()).await.unwrap_err(); + let err = mgr.submit_feedback("c1", "note".into(), None).await.unwrap_err(); assert!(matches!(err, AcpError::NoActiveTurn)); let _ = fake_loop.await; @@ -7557,7 +7658,7 @@ mod tests { // The adapter ignored the opt-in: content consumed → recorded // Delivered (never resent), and the session downgrades to pull. - let item = mgr.submit_feedback("c1", "note one".into()).await.unwrap(); + let item = mgr.submit_feedback("c1", "note one".into(), None).await.unwrap(); assert_eq!(item.status, FeedbackStatus::Delivered); let _ = fake_loop.await; let state = mgr.get_state("c1").await.unwrap(); @@ -7569,7 +7670,7 @@ mod tests { // The NEXT note rides the pull path: lands Pending, no Steer command // (the loop receiver was consumed above — a native attempt would fail // on the dead channel, so an Ok(Pending) proves the pull branch ran). - let second = mgr.submit_feedback("c1", "note two".into()).await.unwrap(); + let second = mgr.submit_feedback("c1", "note two".into(), None).await.unwrap(); assert_eq!(second.status, FeedbackStatus::Pending); let pending = mgr.read_pending_feedback("c1").await; assert_eq!(pending.len(), 1); @@ -7602,7 +7703,7 @@ mod tests { } }); - let item = mgr.submit_feedback("c1", "late note".into()).await.unwrap(); + let item = mgr.submit_feedback("c1", "late note".into(), None).await.unwrap(); assert_eq!(item.status, FeedbackStatus::Delivered); let _ = fake_loop.await; assert_eq!(state.read().await.feedback.len(), 1); @@ -7644,7 +7745,7 @@ mod tests { // caller future. let timed = tokio::time::timeout( std::time::Duration::from_millis(100), - mgr.submit_feedback("c1", "shielded note".into()), + mgr.submit_feedback("c1", "shielded note".into(), None), ) .await; assert!( @@ -7684,7 +7785,7 @@ mod tests { mark_native_steering_ready(&mgr, "c1").await; // feedback_tool_available stays false. let fake_loop = answer_steer(rx, Ok(SteerOutcome::Injected)); - let item = mgr.submit_feedback("c1", "no tool needed".into()).await.unwrap(); + let item = mgr.submit_feedback("c1", "no tool needed".into(), None).await.unwrap(); assert_eq!(item.status, FeedbackStatus::Delivered); let _ = fake_loop.await; } @@ -8023,8 +8124,8 @@ mod tests { mgr.insert_test_connection("c1", AgentType::ClaudeCode, None, EventEmitter::Noop) .await; mark_feedback_ready(&mgr, "c1").await; - let a = mgr.submit_feedback("c1", "a".into()).await.unwrap(); - let b = mgr.submit_feedback("c1", "b".into()).await.unwrap(); + let a = mgr.submit_feedback("c1", "a".into(), None).await.unwrap(); + let b = mgr.submit_feedback("c1", "b".into(), None).await.unwrap(); // READ returns both pending notes (insert order) WITHOUT mutating state. let pending = mgr.read_pending_feedback("c1").await; diff --git a/src-tauri/src/acp/types.rs b/src-tauri/src/acp/types.rs index bec6264f93..9c926bf83a 100644 --- a/src-tauri/src/acp/types.rs +++ b/src-tauri/src/acp/types.rs @@ -1,7 +1,7 @@ use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] #[serde(tag = "type", rename_all = "snake_case")] pub enum PromptInputBlock { Text { diff --git a/src-tauri/src/commands/feedback.rs b/src-tauri/src/commands/feedback.rs index 5be0de9b04..e07951d35b 100644 --- a/src-tauri/src/commands/feedback.rs +++ b/src-tauri/src/commands/feedback.rs @@ -129,6 +129,11 @@ pub async fn set_feedback_settings( /// web handler mirrors this. Returns the stored note so the caller can render it /// optimistically (it also arrives via the `FeedbackSubmitted` event). /// +/// `blocks` (optional) is the full prompt-block draft when the note carries +/// image attachments; `text` stays the recorded/display form. Only the native +/// `_session/steering` channel can deliver blocks — the manager rejects them +/// on the pull path so an attachment is never silently dropped. +/// /// The gate lives in `ConnectionManager::submit_feedback`, keyed on the /// connection's actual `check_user_feedback` capability (not the possibly /// later-toggled global setting). Rejections the frontend recognizes: @@ -139,9 +144,10 @@ pub async fn set_feedback_settings( pub async fn submit_session_feedback( connection_id: String, text: String, + blocks: Option>, manager: tauri::State<'_, crate::acp::manager::ConnectionManager>, ) -> Result { - manager.submit_feedback(&connection_id, text).await + manager.submit_feedback(&connection_id, text, blocks).await } #[cfg(test)] diff --git a/src-tauri/src/web/handlers/feedback.rs b/src-tauri/src/web/handlers/feedback.rs index 10948623e8..721adf2641 100644 --- a/src-tauri/src/web/handlers/feedback.rs +++ b/src-tauri/src/web/handlers/feedback.rs @@ -51,6 +51,12 @@ pub async fn set_feedback_settings( pub struct SubmitSessionFeedbackParams { pub connection_id: String, pub text: String, + /// Full prompt-block draft when the note carries image attachments + /// (native steering only); `text` stays the recorded/display form. Web / + /// remote-mode uploads arrive as empty-payload `file://` markers, exactly + /// like `/acp_prompt`, and are re-hydrated server-side. + #[serde(default)] + pub blocks: Option>, } pub async fn submit_session_feedback( @@ -61,7 +67,7 @@ pub async fn submit_session_feedback( // in `submit_feedback`; recoverable rejections map to 4xx below. let item = state .connection_manager - .submit_feedback(¶ms.connection_id, params.text) + .submit_feedback(¶ms.connection_id, params.text, params.blocks) .await .map_err(|e| { let message = e.to_string(); diff --git a/src/components/chat/message-input.test.tsx b/src/components/chat/message-input.test.tsx index c469de545e..5fa29d63b1 100644 --- a/src/components/chat/message-input.test.tsx +++ b/src/components/chat/message-input.test.tsx @@ -97,6 +97,34 @@ vi.mock("@/lib/transport", () => ({ vi.mock("@/lib/turn-busy", () => ({ isNoActiveTurnRejection: vi.fn(() => false), })) +// Nothing here mounts a Toaster, so toasts would vanish silently — record them +// instead. The steering tests assert the uploading gate's honest signal. +vi.mock("sonner", () => ({ + toast: { error: vi.fn(), info: vi.fn(), success: vi.fn(), dismiss: vi.fn() }, +})) +// Wrap-mock (rich-composer pattern above): render the REAL attachments hook, +// but let a test stage image attachments — the drop/paste pipelines that +// normally populate them need real files and upload endpoints. +type ComposerAttachmentsApi = ReturnType< + typeof import("./composer/use-composer-attachments").useComposerAttachments +> +const attachmentsOverride = vi.hoisted(() => ({ + current: null as Partial | null, +})) +vi.mock("./composer/use-composer-attachments", async (importOriginal) => { + const actual = + await importOriginal() + return { + ...actual, + useComposerAttachments: ( + ...args: Parameters + ) => { + const real = actual.useComposerAttachments(...args) + const override = attachmentsOverride.current + return override ? { ...real, ...override } : real + }, + } +}) // virtua renders 0 rows under jsdom — render children directly so the large // (searchable + virtualized) model list is exercisable here too. vi.mock("virtua", async () => { @@ -971,6 +999,7 @@ describe("MessageInput native steering (insert into current turn)", () => { afterEach(() => { cleanup() composerHandle.current = null + attachmentsOverride.current = null vi.clearAllMocks() }) @@ -1044,7 +1073,10 @@ describe("MessageInput native steering (insert into current turn)", () => { await user.click( await screen.findByRole("menuitem", { name: MI.steerIntoTurn }) ) - await waitFor(() => expect(onSteer).toHaveBeenCalledWith("go left")) + // A plain-text draft steers as text alone — no blocks payload. + await waitFor(() => + expect(onSteer).toHaveBeenCalledWith("go left", undefined) + ) // Unsettled: the draft must survive until the backend confirms. expect(serializeDocToText(editor.state.doc)).toContain("go left") @@ -1106,4 +1138,111 @@ describe("MessageInput native steering (insert into current turn)", () => { expect(onEnqueue).not.toHaveBeenCalled() expect(serializeDocToText(editor.state.doc)).toContain("keep me") }) + + const stagedImage = { + id: "att-1", + type: "image" as const, + data: "aGk=", + uri: null, + name: "mock.png", + mimeType: "image/png", + } + const stagedImageBlock = { + type: "image" as const, + data: "aGk=", + mime_type: "image/png", + } + + it("steers a draft with an image attachment, blocks included", async () => { + // The whole point of block steering: the entry stays enabled with an + // image staged, and the handler ships the SAME block list a normal send + // would build — text prose plus the attachment — with the prose as the + // recorded note. + const user = userEvent.setup() + attachmentsOverride.current = { + attachments: [stagedImage], + imagePromptBlocks: () => [stagedImageBlock], + } + const onSteer = vi.fn().mockResolvedValue(undefined) + const editor = await mountPrompting({ onSteer }) + typeDraft(editor, "match this mock") + await waitFor(() => + expect(screen.getByLabelText(MI.steerIntoTurn)).toBeInTheDocument() + ) + + await user.click(screen.getByLabelText(MI.steerIntoTurn)) + const item = await screen.findByRole("menuitem", { + name: MI.steerIntoTurn, + }) + expect(item).not.toHaveAttribute("aria-disabled", "true") + await user.click(item) + await waitFor(() => + expect(onSteer).toHaveBeenCalledWith("match this mock", [ + { type: "text", text: "match this mock" }, + stagedImageBlock, + ]) + ) + // Confirmed: the draft clears like any successful steer. + await waitFor(() => + expect(serializeDocToText(editor.state.doc)).not.toContain( + "match this mock" + ) + ) + }) + + it("steers an image-only draft with the attachment summary as the note", async () => { + // No prose to record, so the note falls back to the draft's display text + // (what the queue chip would have shown) while the wire still carries the + // real image block. + const user = userEvent.setup() + attachmentsOverride.current = { + attachments: [stagedImage], + imagePromptBlocks: () => [stagedImageBlock], + } + const onSteer = vi.fn().mockResolvedValue(undefined) + await mountPrompting({ onSteer }) + await waitFor(() => + expect(screen.getByLabelText(MI.steerIntoTurn)).toBeInTheDocument() + ) + + await user.click(screen.getByLabelText(MI.steerIntoTurn)) + await user.click( + await screen.findByRole("menuitem", { name: MI.steerIntoTurn }) + ) + await waitFor(() => + expect(onSteer).toHaveBeenCalledWith("Attached 1 attachment", [ + stagedImageBlock, + ]) + ) + }) + + it("blocks steering while an image upload is still settling", async () => { + // Same guard as a plain send: an unsettled upload has no server-side uri + // to hydrate from. The gate must be its own honest toast — the enqueue + // fallback would otherwise ship a bytes-less marker block. + const user = userEvent.setup() + const { toast } = await import("sonner") + attachmentsOverride.current = { + attachments: [{ ...stagedImage, uploading: true }], + hasUploadingImage: true, + imagePromptBlocks: () => [stagedImageBlock], + } + const onSteer = vi.fn() + const editor = await mountPrompting({ onSteer }) + typeDraft(editor, "wait for it") + await waitFor(() => + expect(screen.getByLabelText(MI.steerIntoTurn)).toBeInTheDocument() + ) + + await user.click(screen.getByLabelText(MI.steerIntoTurn)) + await user.click( + await screen.findByRole("menuitem", { name: MI.steerIntoTurn }) + ) + expect(onSteer).not.toHaveBeenCalled() + expect(vi.mocked(toast.error)).toHaveBeenCalledWith( + enMessages.Folder.chat.messageInput.attachUploadInProgress + ) + // Draft and attachment stay put for the retry. + expect(serializeDocToText(editor.state.doc)).toContain("wait for it") + }) }) diff --git a/src/components/chat/message-input.tsx b/src/components/chat/message-input.tsx index 605cdc8045..5f367524a4 100644 --- a/src/components/chat/message-input.tsx +++ b/src/components/chat/message-input.tsx @@ -214,13 +214,15 @@ interface MessageInputProps { * draft synchronously (clears on click); the parent re-queues it if the fork * can't run, so it is never lost. */ onForkSend?: (draft: PromptDraft, modeId?: string | null) => void - /** Inject the draft's TEXT into the RUNNING turn (native live-feedback - * steering). Present only on sessions whose feedback channel is native — - * when absent, the prompting branch renders its historical Stop-only form. - * Awaited: resolve = injected + recorded (clear the draft); reject = - * failure, where a turn-end `NoActiveTurn` race falls back to the queue - * and anything else keeps the draft. */ - onSteer?: (text: string) => Promise + /** Inject the draft into the RUNNING turn (native live-feedback steering). + * Present only on sessions whose feedback channel is native — when absent, + * the prompting branch renders its historical Stop-only form. `text` is + * the recorded/display form; `blocks` carries the full draft whenever it + * holds more than plain text (image attachments, file badges), encoded + * exactly like a normal send. Awaited: resolve = injected + recorded + * (clear the draft); reject = failure, where a turn-end `NoActiveTurn` + * race falls back to the queue and anything else keeps the draft. */ + onSteer?: (text: string, blocks?: PromptInputBlock[]) => Promise /** Open the live-feedback dialog (from the "+" menu). When omitted the entry * is hidden (feature off). */ onAddFeedback?: () => void @@ -1294,14 +1296,19 @@ export function MessageInput({ // the synchronous send/enqueue/fork paths: the draft clears ONLY once the // backend confirms the injection was recorded — a turn-end race falls back // to the queue (the note is never lost), any other failure keeps the draft - // for retry. Steering is text-only: a draft carrying non-text blocks (file - // badges) is queued whole instead of being silently stripped; image - // attachments disable the menu entry at render (which also keeps unsettled - // uploads out of this path — the enqueue fallback below bypasses - // `handleSend`'s uploading gate). + // for retry. A draft that holds more than plain text (image attachments, + // file badges) steers as its full block list — the same encoding a normal + // send uses, which the native wire carries verbatim — with the display text + // as the recorded note; nothing is silently stripped. Unsettled uploads are + // gated here exactly like `handleSend` (no server-side uri to hydrate from + // yet), since the enqueue fallback below bypasses its gate. const [steering, setSteering] = useState(false) const handleSteerClick = useCallback(async () => { if (!onSteer || steering) return + if (hasUploadingImage) { + toast.error(tAttach("attachUploadInProgress")) + return + } const draft = buildDraft() if (!draft) return const enqueueInstead = () => { @@ -1310,18 +1317,19 @@ export function MessageInput({ resetComposer() toast.info(t("steerQueuedInstead")) } - if (draft.blocks.some((b) => b.type !== "text")) { - enqueueInstead() - return - } - const text = draft.blocks - .map((b) => (b.type === "text" ? b.text : "")) - .join("\n") - .trim() + const blocks = draft.blocks.some((b) => b.type !== "text") + ? draft.blocks + : undefined + const text = blocks + ? draft.displayText + : draft.blocks + .map((b) => (b.type === "text" ? b.text : "")) + .join("\n") + .trim() if (!text) return setSteering(true) try { - await onSteer(text) + await onSteer(text, blocks) resetComposer() } catch (err) { if (isNoActiveTurnRejection(err)) { @@ -1336,6 +1344,8 @@ export function MessageInput({ }, [ onSteer, steering, + hasUploadingImage, + tAttach, buildDraft, onEnqueue, showModeSelector, @@ -1695,12 +1705,7 @@ export function MessageInput({ void handleSteerClick()} - disabled={steering || attachments.length > 0} - title={ - attachments.length > 0 - ? t("steerAttachmentsUnsupported") - : undefined - } + disabled={steering} > {t("steerIntoTurn")} diff --git a/src/components/conversations/conversation-detail-panel.tsx b/src/components/conversations/conversation-detail-panel.tsx index 09a308dde6..bd70c65cf8 100644 --- a/src/components/conversations/conversation-detail-panel.tsx +++ b/src/components/conversations/conversation-detail-panel.tsx @@ -111,6 +111,7 @@ import { type MessageTurn, type PlanApprovalAnswer, type PromptDraft, + type PromptInputBlock, type QuestionAnswer, type UserMessageBlock, } from "@/lib/types" @@ -1949,10 +1950,12 @@ const ConversationTabView = memo(function ConversationTabView({ // Composer "insert into current turn" (native steering only). Rethrows — // MessageInput owns the enqueue fallback and draft-preservation policy, so // this wrapper must not swallow the turn-end race the way `submit` does. + // `blocks` rides along when the draft carries attachments (images steer + // too); `text` stays the recorded/display form. const feedbackSteer = feedback.steer const handleSteer = useCallback( - async (text: string) => { - await feedbackSteer(text) + async (text: string, blocks?: PromptInputBlock[]) => { + await feedbackSteer(text, blocks) }, [feedbackSteer] ) diff --git a/src/hooks/use-session-feedback.test.ts b/src/hooks/use-session-feedback.test.ts index 3349393162..fd9e587750 100644 --- a/src/hooks/use-session-feedback.test.ts +++ b/src/hooks/use-session-feedback.test.ts @@ -307,9 +307,26 @@ describe("useSessionFeedback", () => { await act(async () => { await result.current.steer("go left") }) - expect(mockSubmit).toHaveBeenCalledWith("c1", "go left") + expect(mockSubmit).toHaveBeenCalledWith("c1", "go left", undefined) expect(result.current.notes.map((n) => n.id)).toContain("st1") + // A draft with attachments hands its full block list through untouched — + // the API layer owns upload-marker stripping, the backend the channel + // gate; the hook adds nothing. + const blocks = [ + { type: "text" as const, text: "match this" }, + { + type: "image" as const, + data: "aGk=", + mime_type: "image/png", + }, + ] + mockSubmit.mockResolvedValueOnce(note("st2", "match this", "delivered")) + await act(async () => { + await result.current.steer("match this", blocks) + }) + expect(mockSubmit).toHaveBeenCalledWith("c1", "match this", blocks) + const noTurn = new Error("no turn") mockSubmit.mockRejectedValueOnce(noTurn) mockIsNoTurn.mockReturnValue(true) diff --git a/src/hooks/use-session-feedback.ts b/src/hooks/use-session-feedback.ts index 04d70076d6..84ebed0db2 100644 --- a/src/hooks/use-session-feedback.ts +++ b/src/hooks/use-session-feedback.ts @@ -27,7 +27,11 @@ import { useAcpEvent } from "@/contexts/acp-connections-context" import { acpGetSessionSnapshot, submitSessionFeedback } from "@/lib/api" import { toErrorMessage } from "@/lib/app-error" import { isNoActiveTurnRejection } from "@/lib/turn-busy" -import type { ConnectionStatus, FeedbackItem } from "@/lib/types" +import type { + ConnectionStatus, + FeedbackItem, + PromptInputBlock, +} from "@/lib/types" /** Merge snapshot-hydrated notes with live ones, keyed by id; live entries win * (they carry the most recent status). Snapshot order first, live-only after. */ @@ -77,8 +81,12 @@ export interface UseSessionFeedback { * every failure — including the turn-end `NoActiveTurn` race — is * RETHROWN untouched (no toast, no reroute). The composer owns its own * fallback (enqueue) and draft-preservation policy, which `submit`'s - * dialog-shaped error handling would preempt. */ - steer: (text: string) => Promise + * dialog-shaped error handling would preempt. `blocks` carries the full + * draft when it holds attachments (native wire only; the backend's + * `NoActiveTurn` rejection on the pull path reroutes it to the queue + * whole, so an attachment is never silently dropped); `text` stays the + * recorded/display form. */ + steer: (text: string, blocks?: PromptInputBlock[]) => Promise } export function useSessionFeedback({ @@ -354,12 +362,15 @@ export function useSessionFeedback({ // queue on `NoActiveTurn`, keep the draft on real failures. Shared with // `submit`: the optimistic note append and the channel reconciliation. const steer = useCallback( - async (rawText: string): Promise => { + async ( + rawText: string, + blocks?: PromptInputBlock[] + ): Promise => { const text = rawText.trim() if (!text || !connectionId) { throw new Error("nothing to steer") } - const item = await submitSessionFeedback(connectionId, text) + const item = await submitSessionFeedback(connectionId, text, blocks) // A resolution that landed after a connection switch must not touch the // new connection's channel state or note list (reconcileChannel guards // itself too; the append needs the same protection). diff --git a/src/i18n/messages/ar.json b/src/i18n/messages/ar.json index b292df9446..e5a9b1fbda 100644 --- a/src/i18n/messages/ar.json +++ b/src/i18n/messages/ar.json @@ -2860,7 +2860,6 @@ "steerIntoTurn": "إدراج في الدور الحالي", "steerQueuedInstead": "أُضيفت إلى قائمة الانتظار — ستُرسل مع الدور التالي.", "steerFailed": "تعذّر الإدراج في الدور الحالي", - "steerAttachmentsUnsupported": "نص فقط — المسودات التي تحتوي على مرفقات تمر عبر قائمة الانتظار.", "slashCommands": "أوامر الشرطة المائلة", "slashSearchPlaceholder": "البحث عن الأوامر...", "slashSearchEmpty": "لا توجد أوامر مطابقة", diff --git a/src/i18n/messages/de.json b/src/i18n/messages/de.json index 57b70186a4..f5d3ec9b18 100644 --- a/src/i18n/messages/de.json +++ b/src/i18n/messages/de.json @@ -2860,7 +2860,6 @@ "steerIntoTurn": "In laufenden Turn einfügen", "steerQueuedInstead": "Stattdessen in die Warteschlange gestellt – wird mit dem nächsten Turn gesendet.", "steerFailed": "Konnte nicht in den laufenden Turn eingefügt werden", - "steerAttachmentsUnsupported": "Nur Text – Entwürfe mit Anhängen laufen über die Warteschlange.", "slashCommands": "Slash-Befehle", "slashSearchPlaceholder": "Befehle suchen...", "slashSearchEmpty": "Keine passenden Befehle", diff --git a/src/i18n/messages/en.json b/src/i18n/messages/en.json index 875c7361ae..f753ab0489 100644 --- a/src/i18n/messages/en.json +++ b/src/i18n/messages/en.json @@ -2862,7 +2862,6 @@ "steerIntoTurn": "Insert into current turn", "steerQueuedInstead": "Queued instead — it will be sent with the next turn.", "steerFailed": "Couldn't insert into the current turn", - "steerAttachmentsUnsupported": "Text only — drafts with attachments go through the queue.", "slashCommands": "Slash commands", "slashSearchPlaceholder": "Search commands...", "slashSearchEmpty": "No matching commands", diff --git a/src/i18n/messages/es.json b/src/i18n/messages/es.json index 77a742a896..f337f025cb 100644 --- a/src/i18n/messages/es.json +++ b/src/i18n/messages/es.json @@ -2860,7 +2860,6 @@ "steerIntoTurn": "Insertar en el turno actual", "steerQueuedInstead": "Se puso en cola: se enviará con el siguiente turno.", "steerFailed": "No se pudo insertar en el turno actual", - "steerAttachmentsUnsupported": "Solo texto: los borradores con adjuntos pasan por la cola.", "slashCommands": "Comandos de barra", "slashSearchPlaceholder": "Buscar comandos...", "slashSearchEmpty": "Sin comandos coincidentes", diff --git a/src/i18n/messages/fr.json b/src/i18n/messages/fr.json index b4c432fd76..56d9d0d325 100644 --- a/src/i18n/messages/fr.json +++ b/src/i18n/messages/fr.json @@ -2860,7 +2860,6 @@ "steerIntoTurn": "Insérer dans le tour en cours", "steerQueuedInstead": "Mis en file d'attente — il sera envoyé au tour suivant.", "steerFailed": "Impossible d'insérer dans le tour en cours", - "steerAttachmentsUnsupported": "Texte uniquement — les brouillons avec pièces jointes passent par la file d'attente.", "slashCommands": "Commandes slash", "slashSearchPlaceholder": "Rechercher des commandes...", "slashSearchEmpty": "Aucune commande correspondante", diff --git a/src/i18n/messages/ja.json b/src/i18n/messages/ja.json index fe51a74131..9aff807028 100644 --- a/src/i18n/messages/ja.json +++ b/src/i18n/messages/ja.json @@ -2860,7 +2860,6 @@ "steerIntoTurn": "現在のターンに挿入", "steerQueuedInstead": "代わりにキューに追加しました。次のターンで送信されます。", "steerFailed": "現在のターンに挿入できませんでした", - "steerAttachmentsUnsupported": "テキストのみ対応です。添付ファイル付きの下書きはキューをご利用ください。", "slashCommands": "スラッシュコマンド", "slashSearchPlaceholder": "コマンドを検索...", "slashSearchEmpty": "一致するコマンドがありません", diff --git a/src/i18n/messages/ko.json b/src/i18n/messages/ko.json index 08a75a9cb4..6527082bb8 100644 --- a/src/i18n/messages/ko.json +++ b/src/i18n/messages/ko.json @@ -2860,7 +2860,6 @@ "steerIntoTurn": "현재 턴에 삽입", "steerQueuedInstead": "대신 대기열에 추가되었습니다. 다음 턴에 전송됩니다.", "steerFailed": "현재 턴에 삽입하지 못했습니다", - "steerAttachmentsUnsupported": "텍스트만 지원됩니다. 첨부 파일이 있는 초안은 대기열을 이용하세요.", "slashCommands": "슬래시 명령", "slashSearchPlaceholder": "명령 검색...", "slashSearchEmpty": "일치하는 명령이 없습니다", diff --git a/src/i18n/messages/pt.json b/src/i18n/messages/pt.json index 8c27886a48..9843daac72 100644 --- a/src/i18n/messages/pt.json +++ b/src/i18n/messages/pt.json @@ -2860,7 +2860,6 @@ "steerIntoTurn": "Inserir no turno atual", "steerQueuedInstead": "Adicionado à fila — será enviado no próximo turno.", "steerFailed": "Não foi possível inserir no turno atual", - "steerAttachmentsUnsupported": "Somente texto — rascunhos com anexos vão pela fila.", "slashCommands": "Comandos de barra", "slashSearchPlaceholder": "Buscar comandos...", "slashSearchEmpty": "Nenhum comando correspondente", diff --git a/src/i18n/messages/zh-CN.json b/src/i18n/messages/zh-CN.json index f7ecdcc22c..073b29e435 100644 --- a/src/i18n/messages/zh-CN.json +++ b/src/i18n/messages/zh-CN.json @@ -2862,7 +2862,6 @@ "steerIntoTurn": "插入当前回合", "steerQueuedInstead": "已转入队列——将随下一回合发送。", "steerFailed": "无法插入当前回合", - "steerAttachmentsUnsupported": "仅支持纯文本——带附件的草稿请走队列。", "slashCommands": "斜杠命令", "slashSearchPlaceholder": "搜索命令...", "slashSearchEmpty": "没有匹配的命令", diff --git a/src/i18n/messages/zh-TW.json b/src/i18n/messages/zh-TW.json index 9dd20a906c..31b43a08bd 100644 --- a/src/i18n/messages/zh-TW.json +++ b/src/i18n/messages/zh-TW.json @@ -2860,7 +2860,6 @@ "steerIntoTurn": "插入目前回合", "steerQueuedInstead": "已轉入佇列——將隨下一回合傳送。", "steerFailed": "無法插入目前回合", - "steerAttachmentsUnsupported": "僅支援純文字——帶附件的草稿請走佇列。", "slashCommands": "斜線命令", "slashSearchPlaceholder": "搜尋命令...", "slashSearchEmpty": "沒有符合的指令", diff --git a/src/lib/api.ts b/src/lib/api.ts index bbe39e833e..8144ba5768 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -4859,14 +4859,29 @@ export async function setFeedbackSettings( * steering path). Returns the stored note (it also arrives via the * `feedback_submitted` event). Rejects when no turn is in flight — callers * detect that with `isNoActiveTurnRejection` and fall back to a normal prompt. + * + * `blocks` (optional) is the full prompt-block draft when the note carries + * image attachments; `text` stays the recorded/display form. Blocks ride the + * native `_session/steering` wire only — the backend rejects them on the pull + * path (same `NoActiveTurn` fallback) so an attachment is never silently + * dropped. Uploaded payloads are stripped to their `file://` markers in every + * HTTP-body mode, exactly like `acpPrompt`; the backend re-hydrates them. */ export async function submitSessionFeedback( connectionId: string, - text: string + text: string, + blocks?: PromptInputBlock[] | null ): Promise { return getTransport().call("submit_session_feedback", { connectionId, text, + blocks: + blocks && blocks.length > 0 + ? stripUploadedImagePayloads( + blocks, + !isDesktop() || getActiveRemoteConnectionId() !== null + ) + : null, }) } From 87ee4b9f597d0571bbf2090cd7332a2154d89a71 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 4 Sep 2026 15:41:06 +0800 Subject: [PATCH 2/4] fix(chat): thread the steered blocks through the whole composer chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `onSteer` widened to `(text, blocks?)` in `MessageInput` and in the panel's handler, but `ConversationShell` and `ChatInput` — the two layers the prop actually travels through — still declared the one-argument form. The optional second parameter keeps that assignable, so tsc reports nothing and the leak would only appear the day either layer wraps the callback instead of forwarding it. Also lock down the two claims the change rests on: the text-only steer's wire shape is now asserted by exact equality rather than field probes, and the enqueue fallback is asserted to carry the image block, not just the prose. --- src-tauri/src/acp/connection.rs | 11 +++++-- src/components/chat/chat-input.tsx | 13 +++++--- src/components/chat/conversation-shell.tsx | 13 +++++--- src/components/chat/message-input.test.tsx | 36 ++++++++++++++++++++++ 4 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src-tauri/src/acp/connection.rs b/src-tauri/src/acp/connection.rs index d9d5eb307c..be6a48bd23 100644 --- a/src-tauri/src/acp/connection.rs +++ b/src-tauri/src/acp/connection.rs @@ -13596,8 +13596,15 @@ mod tests { }], ); assert_eq!(params["sessionId"], "sess-1"); - assert_eq!(params["prompt"][0]["type"], "text"); - assert_eq!(params["prompt"][0]["text"], "use the staging db"); + // EXACT equality, not field probes: routing a text-only note through + // `map_prompt_blocks` must stay byte-identical to the hand-built + // `[{type,text}]` this used to emit. A future schema bump that starts + // serializing `annotations`/`_meta` as null would change the wire for + // every existing steer, and a field probe would not notice. + assert_eq!( + params["prompt"], + serde_json::json!([{ "type": "text", "text": "use the staging db" }]) + ); // The opt-in is what keeps the idle race host-owned — its absence // would regress to detached `startedNewTurn` turns. assert_eq!(params["_meta"]["steering"]["idleBehavior"], "promptRequired"); diff --git a/src/components/chat/chat-input.tsx b/src/components/chat/chat-input.tsx index 3403ec48a3..19ecdb92f1 100644 --- a/src/components/chat/chat-input.tsx +++ b/src/components/chat/chat-input.tsx @@ -59,12 +59,17 @@ interface ChatInputProps { onSaveQueueEdit?: (draft: PromptDraft) => void onCancelQueueEdit?: () => void onForkSend?: (draft: PromptDraft, modeId?: string | null) => void - /** Inject the draft's text into the RUNNING turn over the native steering - * channel. Present only when the session's live-feedback channel is native + /** Inject the draft into the RUNNING turn over the native steering channel. + * Present only when the session's live-feedback channel is native * (`useSessionFeedback().channel === "native"`); resolves once recorded, * rejects on any failure (incl. the turn-end race) so MessageInput can run - * its own enqueue fallback / draft preservation. */ - onSteer?: (text: string) => Promise + * its own enqueue fallback / draft preservation. `blocks` carries the full + * draft when it holds more than plain text (image attachments, file + * badges); `text` stays the recorded/display form. Must stay in sync with + * `MessageInputProps.onSteer` — the optional second parameter makes a + * stale one-arg declaration here assignable, so tsc would NOT catch a + * wrapper that silently drops the blocks. */ + onSteer?: (text: string, blocks?: PromptInputBlock[]) => Promise onAddFeedback?: () => void feedbackAddDisabled?: boolean /** diff --git a/src/components/chat/conversation-shell.tsx b/src/components/chat/conversation-shell.tsx index 9b43959ff4..c64fef9f27 100644 --- a/src/components/chat/conversation-shell.tsx +++ b/src/components/chat/conversation-shell.tsx @@ -117,10 +117,15 @@ interface ConversationShellProps { onSaveQueueEdit?: (draft: PromptDraft) => void onCancelQueueEdit?: () => void onForkSend?: (draft: PromptDraft, modeId?: string | null) => void - /** Inject the draft's text into the RUNNING turn (native live-feedback - * steering). Present only for sessions on the native channel; threaded - * straight through to the composer. */ - onSteer?: (text: string) => Promise + /** Inject the draft into the RUNNING turn (native live-feedback steering). + * Present only for sessions on the native channel; threaded straight + * through to the composer. `blocks` carries the full draft when it holds + * more than plain text (image attachments, file badges); `text` stays the + * recorded/display form. Must stay in sync with `MessageInputProps.onSteer` + * — the optional second parameter makes a stale one-arg declaration here + * assignable, so tsc would NOT catch a wrapper that silently drops the + * blocks. */ + onSteer?: (text: string, blocks?: PromptInputBlock[]) => Promise /** Optional banner pinned to the top of the panel, above the message area * (e.g. the "restart to apply" config-stale banner). Renders nothing when * omitted. */ diff --git a/src/components/chat/message-input.test.tsx b/src/components/chat/message-input.test.tsx index 5fa29d63b1..f6512c0f69 100644 --- a/src/components/chat/message-input.test.tsx +++ b/src/components/chat/message-input.test.tsx @@ -1245,4 +1245,40 @@ describe("MessageInput native steering (insert into current turn)", () => { // Draft and attachment stay put for the retry. expect(serializeDocToText(editor.state.doc)).toContain("wait for it") }) + + it("queues the attachment too when the blocks steer is rejected", async () => { + // The load-bearing half of "attachments are never silently dropped": the + // backend rejects a blocks-bearing note on the pull path (and on the + // turn-end race) with NoActiveTurn, and this fallback has to re-route the + // WHOLE draft — image included — not just the prose. + const user = userEvent.setup() + const { isNoActiveTurnRejection } = await import("@/lib/turn-busy") + vi.mocked(isNoActiveTurnRejection).mockReturnValue(true) + attachmentsOverride.current = { + attachments: [stagedImage], + imagePromptBlocks: () => [stagedImageBlock], + } + const onSteer = vi.fn().mockRejectedValue(new Error("no active turn")) + const onEnqueue = vi.fn() + const editor = await mountPrompting({ onSteer, onEnqueue }) + typeDraft(editor, "late note") + await waitFor(() => + expect(screen.getByLabelText(MI.steerIntoTurn)).toBeInTheDocument() + ) + + await user.click(screen.getByLabelText(MI.steerIntoTurn)) + await user.click( + await screen.findByRole("menuitem", { name: MI.steerIntoTurn }) + ) + + await waitFor(() => expect(onEnqueue).toHaveBeenCalled()) + const [draft] = onEnqueue.mock.calls[0] + expect(draft.blocks).toEqual([ + { type: "text", text: "late note" }, + stagedImageBlock, + ]) + await waitFor(() => + expect(serializeDocToText(editor.state.doc)).not.toContain("late note") + ) + }) }) From 3c4bddd796574cd41d44962ec9c01a373e59f336 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 4 Sep 2026 16:03:59 +0800 Subject: [PATCH 3/4] fix(acp): stop a hydrating steer from riding the next turn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attachment hydration is the one await `submit_feedback_native` puts between the `turn_in_flight` admission check and the enqueue, and it runs for as long as reading the uploads takes. The loop's idle arm covers "the turn ended" — it replies `NoActiveTurn` and the composer queues the whole draft — but it cannot cover "the next turn started in the meantime": the flag reads true either way, so the loop is in its active arm and injects the note into a turn the user never aimed at, recorded `Delivered` while the composer clears. `turn_in_flight` says only that some turn is running, never which one, so give the state a turn identity: `SessionState.turns_completed`, bumped next to the `turn_in_flight` clear in the `TurnComplete` handler — the single production site that ends a turn. A steer captures it during admission and re-checks it after hydration; a change means the admitted turn is over and the note takes the caller's queue fallback, which re-routes the whole draft, attachment included. A counter rather than the existing `pending_user_message_started_at`: that stamp only exists once a turn has published a user message, and `user_message` is `None` for delegation children and unbound conversations, so those turns would have carried no identity at all. The counter is monotonic rather than an exact turn count — `TurnComplete` has three emitters and a repeat can land on a settled turn — and only inequality is ever read. --- src-tauri/src/acp/manager.rs | 109 +++++++++++++++++++++++++++-- src-tauri/src/acp/session_state.rs | 22 ++++++ 2 files changed, 127 insertions(+), 4 deletions(-) diff --git a/src-tauri/src/acp/manager.rs b/src-tauri/src/acp/manager.rs index 1c9bcfffe8..7034237e0b 100644 --- a/src-tauri/src/acp/manager.rs +++ b/src-tauri/src/acp/manager.rs @@ -137,6 +137,27 @@ struct SpawnDedupKey { /// genuinely broken. pub(crate) const SPAWN_HANDSHAKE_TIMEOUT_SECS: u64 = 60; +/// Whether the turn a steer was admitted against is no longer the turn now in +/// flight — the guard `submit_feedback_native` applies across attachment +/// hydration, the one await between admission and the enqueue. +/// +/// Both halves are needed. `turn_in_flight` alone cannot see "turn N ended and +/// N+1 started while we hydrated" — it reads true both times. +/// `SessionState.turns_completed` closes exactly that: it moves only on +/// `TurnComplete`, so it is stable for a turn's whole life and differs across +/// turns, whatever the new turn did to the flag. It is also independent of +/// whether the turn ever published a user message, which +/// `pending_user_message_started_at` is not (`user_message` is `None` for +/// delegation children and unbound conversations, so those turns would have +/// carried no identity at all). +fn steered_turn_changed( + admitted_turns_completed: u64, + now_in_flight: bool, + now_turns_completed: u64, +) -> bool { + !now_in_flight || now_turns_completed != admitted_turns_completed +} + /// Read the spawn-handshake timeout from `CODEG_ACP_SPAWN_HANDSHAKE_TIMEOUT_SECS`, /// falling back to `SPAWN_HANDSHAKE_TIMEOUT_SECS`. Returns the configured /// `Duration`. Tests can construct the manager with a custom value via @@ -2589,10 +2610,15 @@ impl ConnectionManager { ) -> Result { // Cheap pre-flight, NOT the authoritative check (that's the loop's // idle arm replying `NoActiveTurn`): skip the round-trip when no turn - // is in flight at all. - if !state.read().await.turn_in_flight { - return Err(AcpError::NoActiveTurn); - } + // is in flight at all. The counter read alongside it identifies WHICH + // turn this steer was admitted against — see the re-check below. + let admitted_turns_completed = { + let s = state.read().await; + if !s.turn_in_flight { + return Err(AcpError::NoActiveTurn); + } + s.turns_completed + }; // The wire payload: the caller's full draft when it carried blocks // (attachments included), else the recorded text as a single block — // byte-identical to the historical text-only steer. Uploaded-image @@ -2607,6 +2633,27 @@ impl ConnectionManager { &crate::paths::codeg_uploads_root(), ) .await?; + // Hydration is the ONLY await this path puts between admission + // and the enqueue, and it runs for as long as reading the + // uploads takes. The loop's idle arm already covers "the turn + // ended" (it replies `NoActiveTurn`), but it cannot cover "the + // NEXT turn started in the meantime": the loop would then be + // in its active arm and inject the note into a turn the user + // never aimed at, recorded `Delivered` while the composer + // clears. Re-check the admitted turn's identity so that case + // takes the caller's queue fallback instead — which re-routes + // the whole draft, attachment included. + let changed = { + let s = state.read().await; + steered_turn_changed( + admitted_turns_completed, + s.turn_in_flight, + s.turns_completed, + ) + }; + if changed { + return Err(AcpError::NoActiveTurn); + } blocks } None => vec![PromptInputBlock::Text { text: text.clone() }], @@ -7599,6 +7646,60 @@ mod tests { assert_eq!(fake_loop.await.unwrap(), draft); } + #[test] + fn a_steer_admitted_against_one_turn_does_not_ride_the_next_one() { + // The guard `submit_feedback_native` applies across attachment + // hydration — the one await between admission and the enqueue. The + // loop's idle arm covers "the turn ended"; only this covers "the next + // turn started", which would otherwise have the loop inject the note + // into a turn the user never aimed at. + // + // Same turn throughout — the overwhelmingly common case. + assert!(!steered_turn_changed(3, true, 3)); + // The turn ended and a NEW one started: still in flight, so the flag + // alone says nothing. This is the case nothing else catches. + assert!(steered_turn_changed(3, true, 4)); + // The turn simply ended (the loop's idle arm would also catch this). + assert!(steered_turn_changed(3, false, 4)); + // A repeat `TurnComplete` double-counts; only inequality is read, so + // the verdict is the same. + assert!(steered_turn_changed(3, true, 5)); + // First turn of a connection: the counter starts at zero and carries + // identity from the very first turn, with no "unknown" window. + assert!(!steered_turn_changed(0, true, 0)); + assert!(steered_turn_changed(0, true, 1)); + } + + #[tokio::test] + async fn turn_complete_moves_the_turn_identity_the_steer_guard_reads() { + // The guard above is only as good as the counter under it: prove + // `TurnComplete` — the single production clear of `turn_in_flight` — + // is what moves it, so "the turn I was admitted against is over" is + // observable even once a NEXT turn has set the flag again. + let mgr = ConnectionManager::new(); + mgr.insert_test_connection("c1", AgentType::ClaudeCode, None, EventEmitter::Noop) + .await; + let state = mgr.get_state("c1").await.unwrap(); + let admitted = { + let mut s = state.write().await; + s.turn_in_flight = true; + s.turns_completed + }; + state.write().await.apply_event(&AcpEvent::TurnComplete { + session_id: "ext".into(), + stop_reason: "end_turn".into(), + agent_type: "claude_code".into(), + }); + // A next turn re-sets the flag, exactly as `send_prompt_inner` does. + state.write().await.turn_in_flight = true; + + let s = state.read().await; + assert!( + steered_turn_changed(admitted, s.turn_in_flight, s.turns_completed), + "an in-flight flag that belongs to the NEXT turn must not read as the admitted one" + ); + } + #[tokio::test] async fn pull_submit_with_blocks_rejects_instead_of_dropping_attachments() { // The pull tool delivers plain text, so a blocks-bearing note on a diff --git a/src-tauri/src/acp/session_state.rs b/src-tauri/src/acp/session_state.rs index 7e9b0da602..a51bc2ccba 100644 --- a/src-tauri/src/acp/session_state.rs +++ b/src-tauri/src/acp/session_state.rs @@ -511,6 +511,23 @@ pub struct SessionState { /// not part of the client-visible snapshot. pub turn_in_flight: bool, + /// How many `TurnComplete`s this connection has applied — the turn's + /// IDENTITY, paired with `turn_in_flight`. `turn_in_flight` alone only says + /// "some turn is running"; a caller that admitted itself against turn N and + /// then awaited something cannot tell, on waking, whether it is still + /// looking at turn N or at an N+1 that started meanwhile. Comparing this + /// counter answers that: it moves only when a turn ends, so it is stable + /// for a turn's whole life and differs across turns. + /// + /// Incremented unconditionally next to the `turn_in_flight` clear below — + /// `TurnComplete` has three emitters and a repeat can land on an already + /// settled turn, so this is a monotonic marker, not an exact turn count. + /// Only inequality is ever read. Not serialized: backend-internal, like + /// `turn_in_flight`. Sole consumer today is + /// `ConnectionManager::submit_feedback_native`, which re-checks it across + /// attachment hydration so a steered note cannot ride into the next turn. + pub turns_completed: u64, + /// Whether the most recently completed turn ended via a stop reason other /// than `"end_turn"` (cancelled, refusal, max_tokens, max_turn_requests, /// empty, unknown — the same "abnormal ending" bucket `connection.rs` @@ -602,6 +619,7 @@ impl SessionState { pending_user_message: None, pending_user_message_started_at: None, turn_in_flight: false, + turns_completed: 0, last_turn_ended_abnormally: false, config_stale: false, config_stale_kind: None, @@ -1009,6 +1027,10 @@ impl SessionState { // cancel, stop-reason — emit TurnComplete; disconnect/error // discard the state entirely, so no stale flag can outlive them.) self.turn_in_flight = false; + // Same edge, the identity half: anyone holding "the turn I was + // admitted against" can now see that it is gone, even if a new + // turn sets `turn_in_flight` again before they look. + self.turns_completed = self.turns_completed.saturating_add(1); // NOTE: `active_delegations` is intentionally NOT cleared here. // A running delegation's child runs in the background long after // the parent's `delegate_to_agent` tool call returns and this From a727c063b25dd17c4c021bbbd657e7fe9d43e43b Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 4 Sep 2026 21:37:06 +0800 Subject: [PATCH 4/4] docs(chat): retire the last fork-send mention from the steering comment `830ea832` removed fork-and-send and `8eb05f8e` swept its comments, but the steering handler's note still contrasted itself with "the synchronous send/enqueue/fork paths". There is no fork path in the composer any more. --- src/components/chat/message-input.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/chat/message-input.tsx b/src/components/chat/message-input.tsx index dd57444fe6..de21cfa85f 100644 --- a/src/components/chat/message-input.tsx +++ b/src/components/chat/message-input.tsx @@ -1256,7 +1256,7 @@ export function MessageInput({ ]) // Mid-turn "insert into current turn" (native steering). Awaited, unlike - // the synchronous send/enqueue/fork paths: the draft clears ONLY once the + // the synchronous send/enqueue paths: the draft clears ONLY once the // backend confirms the injection was recorded — a turn-end race falls back // to the queue (the note is never lost), any other failure keeps the draft // for retry. A draft that holds more than plain text (image attachments,