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 == messages
Since _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 < 3 is False → skipped
The else branch introduced in #2026 implicitly fixes this, but there's no test to pin the behavior.
Proposed changes
- Apply
copy.deepcopy to 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)
Related: #2025, #2026
Summary
While reviewing #2026 I found two test quality issues in
tests/strands/session/test_repository_session_manager.pyfor_fix_broken_tool_use.1.
test_fix_broken_tool_use_ignores_last_messagedoesn't actually guard against mutationThe test asserts:
Since
_fix_broken_tool_usemutates the list in-place and returns the same object,fixed_messages is messagesis alwaysTrue. This meansfixed_messages == messagespasses 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 skipWhen consecutive orphaned
toolUsemessages exist without a trailing non-toolUse message,insert()at L306 shifts subsequent messages, causing the finaltoolUseto become the new "last message" mid-iteration. Theindex + 1 < len(messages)guard then skips it.Example:
index=0: toolUse(A) →insert(1, toolResult(A))→ list becomes[A, resultA, B](len=3)index=2: toolUse(B) →index+1=3 < 3isFalse→ skippedThe
elsebranch introduced in #2026 implicitly fixes this, but there's no test to pin the behavior.Proposed changes
copy.deepcopyto the existing last-message testRelated: #2025, #2026