From 72efd50069ae55445f3218f95b1d9532fac17870 Mon Sep 17 00:00:00 2001
From: Erik Nilsen <enilsen16@live.com>
Date: Tue, 12 May 2026 13:47:16 -0700
Subject: [PATCH] fix(goose): never append unsigned thinking deltas to a signed
 block
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The previous coalesce guard merged a new delta whenever either side
had an empty signature. In signature-at-end streams (e.g. Anthropic's
content_block deltas, where the closing chunk carries `signature`),
the first text of the *next* thinking block arrives unsigned and was
appended to the previous block's already-signed content. Replay would
then send that body under the wrong signature and fail provider-side
validation of per-block signatures.

Tighten the predicate to require that either `last` is unsigned (still
mid-block) or the signatures match. An unsigned delta following a
signed block falls through to the default arm and lands as a new
`MessageContent::Thinking` item. The previous
`adopts_latest_signature` test is replaced with two narrower tests:
`adopts_signature_on_closing_delta` (unsigned → signed merges into
the closing delta) and `unsigned_thinking_after_signed_starts_new_block`
(the signature-at-end case Codex flagged).

Addresses Codex P1 on #9162 (conversation/mod.rs:63).

Signed-off-by: Erik Nilsen <enilsen16@live.com>
---
 crates/goose/src/conversation/mod.rs | 63 +++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 11 deletions(-)

diff --git a/crates/goose/src/conversation/mod.rs b/crates/goose/src/conversation/mod.rs
index 24ed6fbefc..2b65bbf792 100644
--- a/crates/goose/src/conversation/mod.rs
+++ b/crates/goose/src/conversation/mod.rs
@@ -58,10 +58,16 @@ impl Conversation {
                     Some(MessageContent::Thinking(ref mut last)),
                     Some(MessageContent::Thinking(new)),
                 ) if message.content.len() == 1
-                    && (new.signature.is_empty()
-                        || last.signature.is_empty()
-                        || new.signature == last.signature) =>
+                    && (last.signature.is_empty() || new.signature == last.signature) =>
                 {
+                    // Merge cases:
+                    //   - `last` is still unsigned (block in progress) — append
+                    //     and adopt `new.signature` if it's the closing delta.
+                    //   - signatures match — same block continuing.
+                    // An unsigned delta arriving after a signed block belongs
+                    // to the next block (signature-at-end streams emit the
+                    // first text of block N+1 before its signature), so the
+                    // outer match arm falls through to push it separately.
                     last.thinking.push_str(&new.thinking);
                     if !new.signature.is_empty() {
                         last.signature = new.signature.clone();
@@ -1294,10 +1300,12 @@ mod tests {
     }
 
     #[test]
-    fn test_push_thinking_adopts_latest_signature() {
+    fn test_push_thinking_adopts_signature_on_closing_delta() {
         use crate::conversation::message::MessageContent;
 
         let mut conv = Conversation::empty();
+        // Streamed shape for one signed block: text deltas accumulate while
+        // unsigned; the closing delta carries the signature.
         conv.push(
             Message::assistant()
                 .with_thinking("a", "")
@@ -1308,24 +1316,57 @@ mod tests {
                 .with_thinking("b", "sig1")
                 .with_id("turn-1"),
         );
-        conv.push(
-            Message::assistant()
-                .with_thinking("c", "")
-                .with_id("turn-1"),
-        );
 
         let content = &conv.messages()[0].content;
         assert_eq!(content.len(), 1);
         match &content[0] {
             MessageContent::Thinking(t) => {
-                assert_eq!(t.thinking, "abc");
-                // Non-empty signature wins; later empty signature does not clobber.
+                assert_eq!(t.thinking, "ab");
                 assert_eq!(t.signature, "sig1");
             }
             other => panic!("expected Thinking, got {:?}", other),
         }
     }
 
+    #[test]
+    fn test_push_unsigned_thinking_after_signed_starts_new_block() {
+        use crate::conversation::message::MessageContent;
+
+        let mut conv = Conversation::empty();
+        conv.push(
+            Message::assistant()
+                .with_thinking("first body", "sig1")
+                .with_id("turn-1"),
+        );
+        // A second thinking block begins; in signature-at-end streams the
+        // first text arrives before the block's signature, so the new
+        // unsigned delta must NOT be appended to the already-signed block —
+        // otherwise the closing signature later would replay text under
+        // the wrong signature.
+        conv.push(
+            Message::assistant()
+                .with_thinking("second body start", "")
+                .with_id("turn-1"),
+        );
+
+        let content = &conv.messages()[0].content;
+        assert_eq!(
+            content.len(),
+            2,
+            "unsigned delta must not merge into a signed block: {:?}",
+            content
+        );
+        match (&content[0], &content[1]) {
+            (MessageContent::Thinking(a), MessageContent::Thinking(b)) => {
+                assert_eq!(a.thinking, "first body");
+                assert_eq!(a.signature, "sig1");
+                assert_eq!(b.thinking, "second body start");
+                assert_eq!(b.signature, "");
+            }
+            other => panic!("unexpected content shape: {:?}", other),
+        }
+    }
+
     #[test]
     fn test_push_keeps_distinct_signed_thinking_blocks_separate() {
         use crate::conversation::message::MessageContent;