From 2c162109c5d32c16e1f237e7dbf7564f8a81f5aa Mon Sep 17 00:00:00 2001 From: dparkmit24 <163079241+dparkmit24@users.noreply.github.com> Date: Wed, 5 Aug 2026 18:57:51 -0700 Subject: [PATCH] test(agent): pin the colliding case and state why the rule is shaped this way The occurrence selector tells a per-call list from a single call's list value by length alone. One test pinned the side where that is unambiguous -- a 3-element list against 2 calls, replayed whole. The side that decides whether the rule is safe is the other one: a tool called n times whose single stored value is a list of length n. That case was decided by the rule and described by no test, so it read as an oversight rather than a decision. Pin it at both readers. The replay reader in base_agent_runner and the display reader on MessageAgentThought each get a two-call record whose one stored value is a two-element list, asserting that call 1 reads element 0 and call 2 reads element 1 -- what the rule does today. The name says what the case concedes rather than what it asserts. The docstrings say why the asymmetry is tolerable. observation values are always str: ToolEngine.agent_invoke is typed -> tuple[str, list[str], ToolInvokeMeta] and both runners store element 0, so a list under a tool name is not a shape any writer produces and the length check is defensive there. tool_input values are json.loads of the model's arguments with no shape check, so a legacy list-valued input is possible in principle, and that is the side the collision can reach. The selector is defined twice, identically, because models/ importing from core/agent/ is the worse layering trade and the reverse is odd. Neither copy is in the wrong place, so each now names the other and says the duplication is deliberate -- enough for a future editor to find both. No behaviour change: the condition, the ordering and the fallback are untouched. Co-Authored-By: Claude Opus 5 (1M context) --- api/core/agent/base_agent_runner.py | 13 ++++++++++++ api/models/model.py | 13 ++++++++++++ .../core/agent/test_base_agent_runner.py | 21 +++++++++++++++++++ api/tests/unit_tests/models/test_model.py | 17 +++++++++++++++ 4 files changed, 64 insertions(+) diff --git a/api/core/agent/base_agent_runner.py b/api/core/agent/base_agent_runner.py index 8fdd9b9d63b..c8bf7173161 100644 --- a/api/core/agent/base_agent_runner.py +++ b/api/core/agent/base_agent_runner.py @@ -57,6 +57,19 @@ def _select_tool_occurrence(value: Any, occurrence: int, occurrences: int) -> An order. Records written before those calls were kept apart store a single value for the tool name, and every occurrence replays it — the behaviour those records were written with. + + `observation` values are always `str`: `ToolEngine.agent_invoke` is typed + `-> tuple[str, list[str], ToolInvokeMeta]` and the runners store element 0, + so a list there is not a shape any writer produces and the length check is + defensive. `tool_input` values are `json.loads` of the model's `arguments` + with no shape check, so a legacy list-valued input is possible in principle; + on a length collision the rule reads per call, not whole — + `test_a_legacy_list_of_matching_length_is_read_per_call_not_whole` pins it. + + The same function is defined, identically, in `models/model.py`. + The duplication is deliberate: `models/` importing from `core/agent/` is the + worse layering trade and the reverse is odd, so neither copy is in the wrong + place and neither should move. A change to this rule must be applied in both. """ if occurrences > 1 and isinstance(value, list) and len(value) == occurrences: return value[occurrence] diff --git a/api/models/model.py b/api/models/model.py index 83a7b88b913..64040495820 100644 --- a/api/models/model.py +++ b/api/models/model.py @@ -2523,6 +2523,19 @@ def _select_tool_occurrence(value: Any, occurrence: int, occurrences: int) -> An order. Records written before those calls were kept apart store a single value for the tool name, and every occurrence replays it — the behaviour those records were written with. + + `observation` values are always `str`: `ToolEngine.agent_invoke` is typed + `-> tuple[str, list[str], ToolInvokeMeta]` and the runners store element 0, + so a list there is not a shape any writer produces and the length check is + defensive. `tool_input` values are `json.loads` of the model's `arguments` + with no shape check, so a legacy list-valued input is possible in principle; + on a length collision the rule reads per call, not whole — + `test_a_legacy_list_of_matching_length_is_read_per_call_not_whole` pins it. + + The same function is defined, identically, in `core/agent/base_agent_runner.py`. + The duplication is deliberate: `models/` importing from `core/agent/` is the + worse layering trade and the reverse is odd, so neither copy is in the wrong + place and neither should move. A change to this rule must be applied in both. """ if occurrences > 1 and isinstance(value, list) and len(value) == occurrences: return value[occurrence] diff --git a/api/tests/unit_tests/core/agent/test_base_agent_runner.py b/api/tests/unit_tests/core/agent/test_base_agent_runner.py index 67c2dc8b7b4..b7fd1037fdc 100644 --- a/api/tests/unit_tests/core/agent/test_base_agent_runner.py +++ b/api/tests/unit_tests/core/agent/test_base_agent_runner.py @@ -760,3 +760,24 @@ class TestOrganizeHistoryRepeatedTools: assert [call.function.name for call in assistant.tool_calls] == ["search", "calculator"] assert [response.content for response in responses] == ["search result", "2"] + + def test_a_legacy_list_of_matching_length_is_read_per_call_not_whole( + self, runner: BaseAgentRunner, mock_db_session, mocker: MockerFixture + ): + # the other half of the same decision. Length is the only signal the + # reader has, so a single stored value that is itself a list as long as + # the call count is indistinguishable from one value per call, and is + # read as one value per call. A legacy row whose one value happened to + # be a two-element list is therefore split across the two calls instead + # of replayed whole. + thought = mocker.MagicMock( + tool="search;search", + tool_input=json.dumps({"search": ["a", "b"]}), + observation=json.dumps({"search": ["x", "y"]}), + thought="thinking", + ) + + assistant, responses = self._replay(runner, mock_db_session, mocker, thought) + + assert [json.loads(call.function.arguments) for call in assistant.tool_calls] == ["a", "b"] + assert [response.content for response in responses] == ["x", "y"] diff --git a/api/tests/unit_tests/models/test_model.py b/api/tests/unit_tests/models/test_model.py index ca7d3092534..8810a910b9c 100644 --- a/api/tests/unit_tests/models/test_model.py +++ b/api/tests/unit_tests/models/test_model.py @@ -243,6 +243,23 @@ def test_a_stored_list_that_is_not_one_value_per_call_is_replayed_whole(): assert thought.tool_outputs_per_call == [["x", "y", "z"], ["x", "y", "z"]] +def test_a_legacy_list_of_matching_length_is_read_per_call_not_whole(): + # the other half of the same decision. Length is the only signal the reader + # has, so a single stored value that is itself a list as long as the call + # count is indistinguishable from one value per call, and is read as one + # value per call. A legacy row whose one value happened to be a two-element + # list is therefore split across the two calls instead of replayed whole. + thought = _agent_thought( + tool="search;search", + tool_input=json.dumps({"search": ["a", "b"]}), + observation=json.dumps({"search": ["x", "y"]}), + tool_meta_str=json.dumps({"search": {"time_cost": 1}}), + ) + + assert thought.tool_inputs_per_call == ["a", "b"] + assert thought.tool_outputs_per_call == ["x", "y"] + + def test_a_tool_missing_from_the_payload_reads_empty(): thought = _agent_thought( tool="search;calculator",