Skip to content

Commit 81bcac5

Browse files
committed
fix: widen MOIM allowlist to apply fix_conversation repairs for orphaned tool blocks
Production death loop: after tool-heavy responses, orphaned tool_use/tool_result blocks cause persistent Anthropic API 400 errors. fix_conversation correctly detects and repairs these orphans, but MOIM's allowlist only permitted merge operations. Any other fix — including orphan removal — caused MOIM to discard the repair and return the original broken conversation to the provider. Widens the allowlist to also accept orphan removal, empty message cleanup, leading/trailing assistant removal, and text content merging. These are all legitimate repairs that fix_conversation should be allowed to apply regardless of whether MOIM insertion triggered the detection.
1 parent abadb87 commit 81bcac5

3 files changed

Lines changed: 249 additions & 26 deletions

File tree

crates/goose/src/agents/moim.rs

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,12 @@ pub async fn inject_moim(
3535
let has_unexpected_issues = issues.iter().any(|issue| {
3636
!issue.contains("Merged consecutive user messages")
3737
&& !issue.contains("Merged consecutive assistant messages")
38+
&& !issue.contains("Merged text content")
39+
&& !issue.contains("Removed orphaned tool response")
40+
&& !issue.contains("Removed orphaned tool request")
41+
&& !issue.contains("Removed empty message")
42+
&& !issue.contains("Removed leading assistant message")
43+
&& !issue.contains("Removed trailing assistant message")
3844
});
3945

4046
if has_unexpected_issues {
@@ -170,4 +176,130 @@ mod tests {
170176
"MOIM should be in message before latest assistant message"
171177
);
172178
}
179+
180+
// After the allowlist fix: MOIM now applies the fix when fix_conversation detects an
181+
// orphaned tool request, removing it from the conversation.
182+
#[tokio::test]
183+
async fn test_moim_fixes_orphaned_tool_request() {
184+
let temp_dir = tempfile::tempdir().unwrap();
185+
let em = ExtensionManager::new_without_provider(temp_dir.path().to_path_buf());
186+
let working_dir = PathBuf::from("/test/dir");
187+
188+
let broken_conv = Conversation::new_unvalidated(vec![
189+
Message::user().with_text("Do something"),
190+
Message::assistant()
191+
.with_text("I'll call a tool")
192+
.with_tool_request(
193+
"orphan_tool_1",
194+
Ok(CallToolRequestParams {
195+
meta: None,
196+
task: None,
197+
name: "some_tool".into(),
198+
arguments: None,
199+
}),
200+
),
201+
]);
202+
203+
// Control: fix_conversation alone correctly removes the orphaned tool request.
204+
let (_, issues) = fix_conversation(Conversation::new_unvalidated(
205+
broken_conv.messages().clone(),
206+
));
207+
assert!(
208+
issues
209+
.iter()
210+
.any(|i| i.contains("Removed orphaned tool request")),
211+
"fix_conversation alone should detect the orphan, but issues were: {:?}",
212+
issues
213+
);
214+
215+
// inject_moim now applies the fix (orphan removal is in the allowlist).
216+
let result = inject_moim("test-session-id", broken_conv.clone(), &em, &working_dir).await;
217+
let msgs = result.messages();
218+
219+
// The orphaned ToolRequest should be removed.
220+
let has_orphan = msgs.iter().any(|m| {
221+
m.content.iter().any(|c| {
222+
matches!(c, crate::conversation::message::MessageContent::ToolRequest(tr) if tr.id == "orphan_tool_1")
223+
})
224+
});
225+
assert!(
226+
!has_orphan,
227+
"Orphaned tool request should have been removed by MOIM's fix_conversation"
228+
);
229+
230+
// MOIM should have been injected (conversation was fixed).
231+
let has_moim = msgs.iter().any(|m| {
232+
m.content
233+
.iter()
234+
.any(|c| c.as_text().is_some_and(|t| t.contains("<info-msg>")))
235+
});
236+
assert!(
237+
has_moim,
238+
"MOIM should have been injected after fixing the orphan"
239+
);
240+
}
241+
242+
// After the allowlist fix: MOIM now fixes the cancellation scenario — removes the
243+
// orphaned tool request and empty user message, preventing the Anthropic 400 error.
244+
#[tokio::test]
245+
async fn test_moim_fixes_cancellation_orphan() {
246+
let temp_dir = tempfile::tempdir().unwrap();
247+
let em = ExtensionManager::new_without_provider(temp_dir.path().to_path_buf());
248+
let working_dir = PathBuf::from("/test/dir");
249+
250+
// Simulates what the agent produces after cancellation:
251+
// - Valid prior exchange
252+
// - Assistant issues a tool call
253+
// - Pre-allocated user message was never populated (cancellation fired first)
254+
let broken_conv = Conversation::new_unvalidated(vec![
255+
Message::user().with_text("Search for something"),
256+
Message::assistant().with_text("I searched and found results"),
257+
Message::user().with_text("Now do something else"),
258+
Message::assistant()
259+
.with_text("I'll call a tool")
260+
.with_tool_request(
261+
"cancelled_tool",
262+
Ok(CallToolRequestParams {
263+
meta: None,
264+
task: None,
265+
name: "some_tool".into(),
266+
arguments: None,
267+
}),
268+
),
269+
// Pre-allocated Message::user() never populated due to cancellation.
270+
Message::user(),
271+
]);
272+
273+
let result = inject_moim("test-session-id", broken_conv.clone(), &em, &working_dir).await;
274+
let msgs = result.messages();
275+
276+
// The orphaned ToolRequest should be removed.
277+
let has_orphan = msgs.iter().any(|m| {
278+
m.content.iter().any(|c| {
279+
matches!(c, crate::conversation::message::MessageContent::ToolRequest(tr) if tr.id == "cancelled_tool")
280+
})
281+
});
282+
assert!(
283+
!has_orphan,
284+
"Orphaned tool request should have been removed by MOIM's fix_conversation"
285+
);
286+
287+
// The empty user message should be removed.
288+
let has_empty = msgs.iter().any(|m| m.content.is_empty());
289+
assert!(
290+
!has_empty,
291+
"Empty user message should have been removed by MOIM's fix_conversation"
292+
);
293+
294+
// MOIM should have been injected.
295+
let has_moim = msgs.iter().any(|m| {
296+
m.content
297+
.iter()
298+
.any(|c| c.as_text().is_some_and(|t| t.contains("<info-msg>")))
299+
});
300+
assert!(
301+
has_moim,
302+
"MOIM should have been injected after fixing the cancellation orphan"
303+
);
304+
}
173305
}

crates/goose/src/context_mgmt/mod.rs

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -825,4 +825,121 @@ mod tests {
825825
let result = tool_id_to_summarize(&updated_conversation, 3);
826826
assert!(result.is_none(), "Nothing left to summarize");
827827
}
828+
829+
// Control test: after compaction + appending new tool calls, fix_conversation
830+
// produces zero issues. Proves compaction output is structurally clean and
831+
// compatible with the fix pipeline when new tool pairs are added.
832+
#[tokio::test]
833+
async fn test_compaction_output_with_new_tool_pair_is_valid() {
834+
let response_message = Message::assistant().with_text("<mock summary>");
835+
let provider = MockProvider::new(response_message, 1000);
836+
837+
// Pre-compaction conversation: user text, 3 tool pairs, user text at end
838+
let mut messages = vec![Message::user().with_text("Help me with a task")];
839+
for i in 0..3 {
840+
messages.push(Message::assistant().with_tool_request(
841+
format!("old_tool_{}", i),
842+
Ok(CallToolRequestParams {
843+
meta: None,
844+
task: None,
845+
name: "some_tool".into(),
846+
arguments: None,
847+
}),
848+
));
849+
messages.push(Message::user().with_tool_response(
850+
format!("old_tool_{}", i),
851+
Ok(rmcp::model::CallToolResult {
852+
content: vec![RawContent::text(format!("result {}", i)).no_annotation()],
853+
structured_content: None,
854+
is_error: Some(false),
855+
meta: None,
856+
}),
857+
));
858+
}
859+
messages.push(Message::user().with_text("Now do something else"));
860+
861+
let conversation = Conversation::new_unvalidated(messages);
862+
let (compacted, _usage) =
863+
compact_messages(&provider, "test-session-id", &conversation, false)
864+
.await
865+
.unwrap();
866+
867+
// Simulate post-compaction agent loop: LLM makes a new tool call, tool executes
868+
let new_asst = Message::assistant()
869+
.with_text("I'll run a new tool")
870+
.with_tool_request(
871+
"new_tool_1",
872+
Ok(CallToolRequestParams {
873+
meta: None,
874+
task: None,
875+
name: "new_tool".into(),
876+
arguments: None,
877+
}),
878+
);
879+
let new_tool_resp = Message::user().with_tool_response(
880+
"new_tool_1",
881+
Ok(rmcp::model::CallToolResult {
882+
content: vec![RawContent::text("new result").no_annotation()],
883+
structured_content: None,
884+
is_error: Some(false),
885+
meta: None,
886+
}),
887+
);
888+
// Rebuild conversation from compacted messages + new tool pair.
889+
// Can't use Conversation::push() because it merges by ID, and we need
890+
// the messages to be separate entries.
891+
let mut all_msgs = compacted.messages().clone();
892+
all_msgs.push(new_asst);
893+
all_msgs.push(new_tool_resp);
894+
let compacted = Conversation::new_unvalidated(all_msgs);
895+
896+
// fix_conversation should produce zero issues — the conversation is valid
897+
let (fixed, issues) = crate::conversation::fix_conversation(compacted.clone());
898+
899+
assert!(
900+
issues.is_empty(),
901+
"Compacted conversation + new tool pair should have no issues, but found: {:?}",
902+
issues
903+
);
904+
905+
// The new tool pair should be intact in the visible messages
906+
let visible = fixed.agent_visible_messages();
907+
let has_new_tool_request = visible.iter().any(|m| {
908+
m.content
909+
.iter()
910+
.any(|c| matches!(c, MessageContent::ToolRequest(tr) if tr.id == "new_tool_1"))
911+
});
912+
let has_new_tool_response = visible.iter().any(|m| {
913+
m.content
914+
.iter()
915+
.any(|c| matches!(c, MessageContent::ToolResponse(tr) if tr.id == "new_tool_1"))
916+
});
917+
assert!(
918+
has_new_tool_request,
919+
"New tool request should be in visible messages"
920+
);
921+
assert!(
922+
has_new_tool_response,
923+
"New tool response should be in visible messages"
924+
);
925+
926+
// The old tool pairs should still exist as non-visible messages
927+
let all_msgs = fixed.messages();
928+
let non_visible_tool_count = all_msgs
929+
.iter()
930+
.filter(|m| {
931+
!m.metadata.agent_visible
932+
&& m.content.iter().any(|c| {
933+
matches!(
934+
c,
935+
MessageContent::ToolRequest(_) | MessageContent::ToolResponse(_)
936+
)
937+
})
938+
})
939+
.count();
940+
assert!(
941+
non_visible_tool_count > 0,
942+
"Old tool pairs should be preserved as non-visible messages"
943+
);
944+
}
828945
}

ui/desktop/package-lock.json

Lines changed: 0 additions & 26 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)