From 7b666057e686024f366eee975833b65cb99d29d3 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 9 Jun 2026 20:36:46 -0400 Subject: [PATCH] =?UTF-8?q?fix(acp):=20review=20fixes=20=E2=80=94=20transp?= =?UTF-8?q?ort-error=20death=20notices,=20threaded=20messages,=20keepalive?= =?UTF-8?q?=20arm?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review findings from Thufir on PR #935: - Post death notice on transport-error respawns (Io, WriteTimeout, Timeout, Protocol) — same user-facing silence as idle timeout - Thread death notices into the original conversation via e-tag reply so users know which task died in busy channels - Add explicit "keepalive" match arm in handle_session_update for documentation clarity - Update idle-reset comment to clarify belt-and-suspenders intent Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/sprout-acp/src/acp.rs | 6 ++++-- crates/sprout-acp/src/lib.rs | 30 +++++++++++++++++++++++++++++- crates/sprout-acp/src/relay.rs | 16 ++++++++++++++-- 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/crates/sprout-acp/src/acp.rs b/crates/sprout-acp/src/acp.rs index 495f5873d..483e158d2 100644 --- a/crates/sprout-acp/src/acp.rs +++ b/crates/sprout-acp/src/acp.rs @@ -854,8 +854,9 @@ impl AcpClient { match method { "session/update" => { if self.handle_session_update(&msg) { - // Tool call started — explicitly reset idle clock. - // The agent will be silent while the tool executes. + // Belt-and-suspenders — general reset already fired + // above, this is defense-in-depth in case the general + // reset is later narrowed. tracing::debug!("idle clock reset: tool call started"); idle_deadline = Instant::now() + idle_timeout; } @@ -963,6 +964,7 @@ impl AcpClient { ); false } + "keepalive" => false, other => { tracing::debug!(target: "acp::update", "session/update: {other}"); false diff --git a/crates/sprout-acp/src/lib.rs b/crates/sprout-acp/src/lib.rs index 0fe3934d3..019773f0d 100644 --- a/crates/sprout-acp/src/lib.rs +++ b/crates/sprout-acp/src/lib.rs @@ -1996,6 +1996,15 @@ fn handle_prompt_result( .retain(|_, meta| meta.agent_index != agent_index); debug_assert_eq!(before, pool.task_map().len() + 1); + // Extract thread root from the batch before it's consumed by requeue. + // Used by death notices to thread the message into the original conversation. + let thread_root: Option = result + .batch + .as_ref() + .and_then(|b| b.events.first()) + .map(|e| queue::parse_thread_tags(&e.event)) + .and_then(|tags| tags.root_event_id); + // Requeue BEFORE mark_complete: requeue() sets retry_after with a future // deadline, and mark_complete() checks for it to decide whether to preserve // retry_counts. If mark_complete runs first, retry_counts is cleared and @@ -2087,7 +2096,7 @@ fn handle_prompt_result( // Post a visible death notice to the channel so humans know why // the agent went silent. if let Some(ch) = channel_id { - match relay.build_death_notice(ch, death_message) { + match relay.build_death_notice(ch, death_message, thread_root.as_deref()) { Ok(event) => { if let Err(e) = relay.try_publish_event(event) { tracing::warn!("failed to publish death notice: {e}"); @@ -2154,6 +2163,25 @@ fn handle_prompt_result( "transport/protocol error — respawning agent" ); emit_turn_error(&e.to_string()); + + // Post a visible death notice for transport errors too. + if let Some(ch) = channel_id { + match relay.build_death_notice( + ch, + "Agent connection lost (transport error)", + thread_root.as_deref(), + ) { + Ok(event) => { + if let Err(e) = relay.try_publish_event(event) { + tracing::warn!("failed to publish death notice: {e}"); + } + } + Err(e) => { + tracing::warn!("failed to build death notice: {e}"); + } + } + } + let index = result.agent.index; let slot_history = &mut crash_history[index]; if !spawn_respawn_task( diff --git a/crates/sprout-acp/src/relay.rs b/crates/sprout-acp/src/relay.rs index 89d7ffbe4..738957618 100644 --- a/crates/sprout-acp/src/relay.rs +++ b/crates/sprout-acp/src/relay.rs @@ -750,13 +750,25 @@ impl HarnessRelay { /// Build a channel message (kind:9) for death notices — posted when the /// agent session ends due to timeout or unexpected exit. - pub fn build_death_notice(&self, channel_id: Uuid, message: &str) -> Result { + /// If `thread_root` is provided, the message is threaded as a reply. + pub fn build_death_notice( + &self, + channel_id: Uuid, + message: &str, + thread_root: Option<&str>, + ) -> Result { use sprout_core::kind::KIND_STREAM_MESSAGE; let h_tag = Tag::parse(["h", &channel_id.to_string()]) .map_err(|e| RelayError::AuthFailed(e.to_string()))?; + let mut tags = vec![h_tag]; + if let Some(root_id) = thread_root { + let e_tag = Tag::parse(["e", root_id, "", "reply"]) + .map_err(|e| RelayError::AuthFailed(e.to_string()))?; + tags.push(e_tag); + } let event = EventBuilder::new(Kind::Custom(KIND_STREAM_MESSAGE as u16), message) - .tags([h_tag]) + .tags(tags) .sign_with_keys(&self.keys)?; Ok(event) }