From e95d0630189d7b9875dd2442b80b56550304890c Mon Sep 17 00:00:00 2001 From: Ehsan Barkhordar Date: Fri, 17 Jul 2026 03:57:05 +0000 Subject: [PATCH] Compaction: clear server-side tool uses before client-side removal shifts indices (#4528) CompactionEdit captures client-side and server-side tool use indices in one enumeration of the message list. With keep_tool_inputs=False the client-side pass deletes tool messages, shifting every later message down, but the server-side indices from that same enumeration were consumed afterwards with nothing rebasing them. Server-side clearing replaces messages without changing the list length, so applying it before the client-side pass keeps both sets of indices valid. --- CHANGELOG.md | 1 + src/inspect_ai/model/_compaction/edit.py | 11 ++- tests/model/test_compaction_edit.py | 110 +++++++++++++++++++++++ 3 files changed, 119 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05d2c912be..7ee051eeb2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased +- Compaction: Fix `CompactionEdit` clearing server-side tool uses at stale message indices when `keep_tool_inputs=False` removes client-side tool messages, which raised `IndexError` or wrote a tool use into an unrelated message. (#4528) - Logging: Local eval (`.eval`) and JSON (`.json`) log files are now written atomically (temp file + `fsync` + rename), preventing corruption from interrupted writes such as disk-full or process crash. (#2949) - Checkpointing: In-sandbox restic artifacts moved from the world-listable `/opt/inspect-restic` to root-only `/root/.cache/inspect`, and egress scratch files no longer land in `/tmp`, so agents no longer see evidence of checkpointing. - Eval: `EvalSample` and `EvalSampleSummary` now record `turn_count` and the sample's token limit (`token_limit`, `token_limit_type`, and metered `token_limit_usage`). diff --git a/src/inspect_ai/model/_compaction/edit.py b/src/inspect_ai/model/_compaction/edit.py index f569381100..85d2dcaa69 100644 --- a/src/inspect_ai/model/_compaction/edit.py +++ b/src/inspect_ai/model/_compaction/edit.py @@ -180,6 +180,14 @@ async def compact( if t[0] == "server" and isinstance(t[3], ContentToolUse) ] + # Clear server-side tools first. Both index lists were captured by the single + # enumeration in Phase 2, and clearing a client-side tool use with + # keep_tool_inputs=False deletes a message, which invalidates every index at or + # after it. Server-side clearing replaces messages without changing the list + # length, so applying it before the client-side pass keeps both sets of indices + # valid (the client-side pass already guards itself by iterating in reverse). + result = _apply_server_tool_clearing(result, server_to_clear) + # Clear client-side tools (process in reverse to preserve indices) for assistant_idx, tool_call, tool_idx in reversed(client_to_clear): if self.keep_tool_inputs: @@ -196,9 +204,6 @@ async def compact( tool_call, ) - # Clear server-side tools - result = _apply_server_tool_clearing(result, server_to_clear) - # Phase 4: Clear content from memory tool calls (if memory integration active) if self.memory: result = clear_memory_content(result) diff --git a/tests/model/test_compaction_edit.py b/tests/model/test_compaction_edit.py index 41ae9e368f..6d2b3d8d26 100644 --- a/tests/model/test_compaction_edit.py +++ b/tests/model/test_compaction_edit.py @@ -1272,3 +1272,113 @@ async def test_no_trailing_assistant_after_compaction( assert compacted[-1].role != "assistant", ( "Compacted messages must not end with an assistant message" ) + + +# Tests for server-side tool clearing when client-side clearing removes messages +async def test_server_tool_clearing_after_client_message_removal( + tool_call_1: ToolCall, +) -> None: + """Test server-side clearing with keep_tool_inputs=False, which deletes messages. + + The tool use indices are captured in a single enumeration. With + keep_tool_inputs=False the client-side pass deletes the tool message, so any + server-side index after it must still resolve to the message it was captured + from rather than to whatever shifted into its place. + """ + web_search = ContentToolUse( + tool_type="web_search", + id="ws1", + name="web_search", + arguments='{"query": "hello"}', + result='[{"title": "Result"}]', + ) + + strategy = CompactionEdit( + keep_thinking_turns="all", keep_tool_uses=0, keep_tool_inputs=False + ) + + messages: list[ChatMessage] = [ + ChatMessageUser(content="Start"), + ChatMessageAssistant(content="Using tool", tool_calls=[tool_call_1]), + # Removed by the client-side pass, shifting every later message down one + ChatMessageTool( + content="Weather: Sunny", tool_call_id="tool1", function="get_weather" + ), + ChatMessageAssistant(content=[ContentText(text="Searching"), web_search]), + ChatMessageUser(content="Follow up"), + ] + + compacted, _ = await strategy.compact(get_model("mockllm/model"), messages, []) + + # The server-side tool use is cleared, in the message that actually carried it + assistant = next( + m + for m in compacted + if isinstance(m, ChatMessageAssistant) + and isinstance(m.content, list) + and any(isinstance(c, ContentToolUse) for c in m.content) + ) + assert isinstance(assistant.content, list) + tool_use = next(c for c in assistant.content if isinstance(c, ContentToolUse)) + assert tool_use.id == "ws1" + assert tool_use.result == TOOL_RESULT_REMOVED + + +async def test_server_tool_clearing_does_not_write_into_other_messages( + tool_call_1: ToolCall, + tool_call_2: ToolCall, +) -> None: + """Test that server-side clearing never writes a tool use into another message. + + Two client-side removals shift the server-side message down by two, which lands + the captured index on a later assistant message that has list content. That + message must keep its own content, and must not gain a tool use it never made. + """ + web_search = ContentToolUse( + tool_type="web_search", + id="ws1", + name="web_search", + arguments='{"query": "hello"}', + result='[{"title": "Result"}]', + ) + + strategy = CompactionEdit( + keep_thinking_turns="all", keep_tool_uses=0, keep_tool_inputs=False + ) + + messages: list[ChatMessage] = [ + ChatMessageUser(content="Start"), + ChatMessageAssistant(content="First tool", tool_calls=[tool_call_1]), + ChatMessageTool( + content="Weather: Sunny", tool_call_id="tool1", function="get_weather" + ), + ChatMessageAssistant(content="Second tool", tool_calls=[tool_call_2]), + ChatMessageTool( + content="Time: 12:00", tool_call_id="tool2", function="get_time" + ), + ChatMessageAssistant(content=[ContentText(text="Searching"), web_search]), + ChatMessageUser(content="More"), + ChatMessageAssistant(content=[ContentText(text="A"), ContentText(text="B")]), + ChatMessageUser(content="Follow up"), + ] + + compacted, _ = await strategy.compact(get_model("mockllm/model"), messages, []) + + # The later assistant message keeps its own content and gains no tool use + other = compacted[-2] + assert isinstance(other, ChatMessageAssistant) + assert isinstance(other.content, list) + assert [c.text for c in other.content if isinstance(c, ContentText)] == ["A", "B"] + assert not any(isinstance(c, ContentToolUse) for c in other.content) + + # Exactly one tool use survives anywhere, and it is the cleared original + tool_uses = [ + c + for m in compacted + if isinstance(m.content, list) + for c in m.content + if isinstance(c, ContentToolUse) + ] + assert len(tool_uses) == 1 + assert tool_uses[0].id == "ws1" + assert tool_uses[0].result == TOOL_RESULT_REMOVED