mirror of
https://github.com/aaif-goose/goose.git
synced 2026-07-03 14:10:03 +02:00
fix(goose): never append unsigned thinking deltas to a signed block
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>
This commit is contained in:
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user