-
Notifications
You must be signed in to change notification settings - Fork 753
Test: _fix_broken_tool_use tests don't guard against in-place mutation; missing coverage for insert-induced edge case #2028
Description
Summary
While reviewing #2026 I found two test quality issues in tests/strands/session/test_repository_session_manager.py for _fix_broken_tool_use.
1. test_fix_broken_tool_use_ignores_last_message doesn't actually guard against mutation
The test asserts:
fixed_messages = session_manager._fix_broken_tool_use(messages)
assert fixed_messages == messagesSince _fix_broken_tool_use mutates the list in-place and returns the same object, fixed_messages is messages is always True. This means fixed_messages == messages passes regardless of whether the method appended anything. The test would still pass even after applying the fix from #2026.
Fix: Use copy.deepcopy(messages) to snapshot the original before calling the method, and compare against the snapshot.
2. No test for insert()-induced last-message skip
When consecutive orphaned toolUse messages exist without a trailing non-toolUse message, insert() at L306 shifts subsequent messages, causing the final toolUse to become the new "last message" mid-iteration. The index + 1 < len(messages) guard then skips it.
Example:
messages = [
{"role": "assistant", "content": [{"toolUse": {"toolUseId": "A", ...}}]},
{"role": "assistant", "content": [{"toolUse": {"toolUseId": "B", ...}}]},
]index=0: toolUse(A) →insert(1, toolResult(A))→ list becomes[A, resultA, B](len=3)index=2: toolUse(B) →index+1=3 < 3isFalse→ skipped
The else branch introduced in #2026 implicitly fixes this, but there's no test to pin the behavior.
Proposed changes
- Apply
copy.deepcopyto the existing last-message test - Add a test for the consecutive orphaned toolUse scenario (insert-induced skip)
- Add a test for the single-message edge case (conversation is just one orphaned toolUse)