mirror of
https://github.com/aaif-goose/goose.git
synced 2026-07-03 14:10:03 +02:00
fix(goose): coalesce streaming Thinking deltas + list available tools on not-found (#9162)
Signed-off-by: Erik Nilsen <enilsen16@live.com>
This commit is contained in:
@@ -1558,9 +1558,18 @@ impl ExtensionManager {
|
||||
}
|
||||
}
|
||||
|
||||
let available = tools
|
||||
.iter()
|
||||
.map(|t| t.name.as_ref())
|
||||
.collect::<Vec<&str>>()
|
||||
.join(", ");
|
||||
|
||||
Err(ErrorData::new(
|
||||
ErrorCode::RESOURCE_NOT_FOUND,
|
||||
format!("Tool '{}' not found", tool_name),
|
||||
format!(
|
||||
"Tool '{}' not found. Available tools: [{}]",
|
||||
tool_name, available
|
||||
),
|
||||
None,
|
||||
))
|
||||
}
|
||||
@@ -2418,6 +2427,35 @@ mod tests {
|
||||
assert!(!tool_names.iter().any(|n| n.starts_with("ext_b__")));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_resolve_tool_error_includes_available_tools() {
|
||||
let temp_dir = tempfile::tempdir().unwrap();
|
||||
let extension_manager =
|
||||
ExtensionManager::new_without_provider(temp_dir.path().to_path_buf());
|
||||
|
||||
extension_manager
|
||||
.add_mock_extension("ext_a".to_string(), Arc::new(MockClient {}))
|
||||
.await;
|
||||
|
||||
let result = extension_manager
|
||||
.resolve_tool("test-session-id", "definitely_not_a_real_tool")
|
||||
.await;
|
||||
let err = match result {
|
||||
Ok(_) => panic!("resolve_tool should fail for an unknown name"),
|
||||
Err(e) => e,
|
||||
};
|
||||
|
||||
let msg = err.message.to_string();
|
||||
assert!(
|
||||
msg.contains("definitely_not_a_real_tool"),
|
||||
"error should echo the bad name; got: {msg}"
|
||||
);
|
||||
assert!(
|
||||
msg.contains("ext_a__"),
|
||||
"error should list at least one real tool name; got: {msg}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_remove_untrusted_mcp_app_meta_strips_spoofed_payload() {
|
||||
let mut result = CallToolResult::success(vec![]);
|
||||
|
||||
@@ -54,6 +54,25 @@ impl Conversation {
|
||||
{
|
||||
last.text.push_str(&new.text);
|
||||
}
|
||||
(
|
||||
Some(MessageContent::Thinking(ref mut last)),
|
||||
Some(MessageContent::Thinking(new)),
|
||||
) if message.content.len() == 1
|
||||
&& (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();
|
||||
}
|
||||
}
|
||||
(_, _) => {
|
||||
last.content.extend(message.content);
|
||||
}
|
||||
@@ -1254,4 +1273,160 @@ mod tests {
|
||||
assert_eq!(fixed_messages[5].as_concat_text(), "Non-vis C");
|
||||
assert!(!fixed_messages[5].metadata.agent_visible);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_push_coalesces_thinking_deltas() {
|
||||
use crate::conversation::message::MessageContent;
|
||||
|
||||
let mut conv = Conversation::empty();
|
||||
for fragment in ["I ", "should ", "think ", "about ", "this."] {
|
||||
conv.push(
|
||||
Message::assistant()
|
||||
.with_thinking(fragment, "")
|
||||
.with_id("turn-1"),
|
||||
);
|
||||
}
|
||||
|
||||
assert_eq!(conv.messages().len(), 1);
|
||||
let content = &conv.messages()[0].content;
|
||||
assert_eq!(content.len(), 1);
|
||||
match &content[0] {
|
||||
MessageContent::Thinking(t) => {
|
||||
assert_eq!(t.thinking, "I should think about this.");
|
||||
assert_eq!(t.signature, "");
|
||||
}
|
||||
other => panic!("expected single Thinking block, got {:?}", other),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
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", "")
|
||||
.with_id("turn-1"),
|
||||
);
|
||||
conv.push(
|
||||
Message::assistant()
|
||||
.with_thinking("b", "sig1")
|
||||
.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, "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;
|
||||
|
||||
let mut conv = Conversation::empty();
|
||||
conv.push(
|
||||
Message::assistant()
|
||||
.with_thinking("block A", "sig-A")
|
||||
.with_id("turn-1"),
|
||||
);
|
||||
conv.push(
|
||||
Message::assistant()
|
||||
.with_thinking("block B", "sig-B")
|
||||
.with_id("turn-1"),
|
||||
);
|
||||
|
||||
let content = &conv.messages()[0].content;
|
||||
assert_eq!(
|
||||
content.len(),
|
||||
2,
|
||||
"two distinct signed blocks must not coalesce: {:?}",
|
||||
content
|
||||
);
|
||||
match (&content[0], &content[1]) {
|
||||
(MessageContent::Thinking(a), MessageContent::Thinking(b)) => {
|
||||
assert_eq!(a.thinking, "block A");
|
||||
assert_eq!(a.signature, "sig-A");
|
||||
assert_eq!(b.thinking, "block B");
|
||||
assert_eq!(b.signature, "sig-B");
|
||||
}
|
||||
other => panic!("unexpected content shape: {:?}", other),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_push_does_not_coalesce_multi_block_thinking_message() {
|
||||
use crate::conversation::message::MessageContent;
|
||||
|
||||
let mut conv = Conversation::empty();
|
||||
conv.push(
|
||||
Message::assistant()
|
||||
.with_thinking("first", "")
|
||||
.with_id("turn-1"),
|
||||
);
|
||||
|
||||
// Multi-block message must NOT coalesce into the existing thinking
|
||||
// block — the merge arm requires `message.content.len() == 1`.
|
||||
let mut multi = Message::assistant().with_thinking("second", "");
|
||||
multi = multi.with_text("and now text").with_id("turn-1");
|
||||
conv.push(multi);
|
||||
|
||||
let content = &conv.messages()[0].content;
|
||||
assert_eq!(content.len(), 3);
|
||||
match (&content[0], &content[1], &content[2]) {
|
||||
(MessageContent::Thinking(a), MessageContent::Thinking(b), MessageContent::Text(c)) => {
|
||||
assert_eq!(a.thinking, "first");
|
||||
assert_eq!(b.thinking, "second");
|
||||
assert_eq!(c.text, "and now text");
|
||||
}
|
||||
other => panic!("unexpected content shape: {:?}", other),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user