Skip to content

Commit 134fe42

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 134fe42

2 files changed

Lines changed: 239 additions & 0 deletions

File tree

crates/goose/src/agents/moim.rs

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

crates/goose/src/context_mgmt/mod.rs

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -825,4 +825,110 @@ 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) =
898+
crate::conversation::fix_conversation(compacted.clone());
899+
900+
assert!(
901+
issues.is_empty(),
902+
"Compacted conversation + new tool pair should have no issues, but found: {:?}",
903+
issues
904+
);
905+
906+
// The new tool pair should be intact in the visible messages
907+
let visible = fixed.agent_visible_messages();
908+
let has_new_tool_request = visible.iter().any(|m| {
909+
m.content.iter().any(|c| {
910+
matches!(c, MessageContent::ToolRequest(tr) if tr.id == "new_tool_1")
911+
})
912+
});
913+
let has_new_tool_response = visible.iter().any(|m| {
914+
m.content.iter().any(|c| {
915+
matches!(c, MessageContent::ToolResponse(tr) if tr.id == "new_tool_1")
916+
})
917+
});
918+
assert!(has_new_tool_request, "New tool request should be in visible messages");
919+
assert!(has_new_tool_response, "New tool response should be in visible messages");
920+
921+
// The old tool pairs should still exist as non-visible messages
922+
let all_msgs = fixed.messages();
923+
let non_visible_tool_count = all_msgs.iter().filter(|m| {
924+
!m.metadata.agent_visible
925+
&& m.content.iter().any(|c| {
926+
matches!(c, MessageContent::ToolRequest(_) | MessageContent::ToolResponse(_))
927+
})
928+
}).count();
929+
assert!(
930+
non_visible_tool_count > 0,
931+
"Old tool pairs should be preserved as non-visible messages"
932+
);
933+
}
828934
}

0 commit comments

Comments
 (0)