Repository navigation
Python: fix(eval): scope local tool checks to the response side of the conversation split - #10186
tang-ontheway wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The reusable conversation builder in test_local_eval.py violates the required test-helper layout and must be moved to a helper module or converted to an injected fixture in conftest.py.
1 open issue
What changed in this PR
This PR fixes #10091 by making local tool checks respect EvalItem’s conversation split, preventing earlier tool calls from incorrectly satisfying later-turn evaluations.
Changes:
- Scope all three local tool checks to response-side messages through a shared private helper.
- Update docstrings to describe split-aware behavior.
- Add regression tests for per-turn isolation,
FULLsplits, and unchanged single-turn behavior.
| File | Description |
|---|---|
python/packages/core/tests/core/test_local_eval.py |
Adds regression coverage for split-aware tool checks. |
python/packages/core/agent_framework/_evaluation.py |
Restricts tool checks to response-side messages and updates documentation. |
Review effort: Balanced
| @staticmethod | ||
| def _two_turn_conversation() -> list[Message]: |
|
tang-ontheway please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
| expected = set(tool_names) | ||
| called: set[str] = set() | ||
| for msg in item.conversation: | ||
| for msg in _response_messages(item): |
There was a problem hiding this comment.
tang-ontheway, this response-side scan treats the last role="user" message as the turn boundary, but AgentLoopMiddleware injects user-role nudge/progress messages between iterations in its default aggregated response. A tool call from an earlier loop iteration then falls on the query side and tool_called_check / expected-tool-call checks report it missing. Please distinguish real conversation turns from loop-injected messages (or scope this only to explicit multi-turn items) and add a loop-aggregated regression.
|
Reporter of #10091 here: this fixes my case. I checked out this branch (1997df4) and ran the reproduction from #10091 against it and against
The first turn, which made the call, still passes all three. I also applied the tests from my closed #10245 on top of this branch: all 105 tests in I used Claude (Anthropic) to help run these checks; I reviewed the results myself. |
The agent loop injects ``user``-role messages (nudges, progress summaries) between iterations of a single run. In the default aggregated response those messages sit between the iterations, so the response-side scan treated the last one as the turn boundary and dropped the tool calls of earlier iterations onto the query side. - ``AgentLoopMiddleware`` now tags injected messages with ``LOOP_INJECTED_MESSAGE_PROPERTY`` in ``additional_properties`` (also on the streaming updates that surface them). - The built-in splits and ``per_turn_items`` skip loop-injected messages, so they never act as a turn boundary. - Move the reusable conversation builders to a helper module and add loop-aggregated regression coverage.
|
Thanks Eduard van Valkenburg (@eavanvalkenburg) — good catch, and thanks Qiming Bao (@14H034160212) for running the reproduction. Loop-injected messages vs. real turns.
With that, the response side of a loop-aggregated conversation starts after the real user query, so a tool call made in the first iteration is still found by Regression coverage. New tests in Test layout. The reusable conversation builders moved out of the test module into Qiming Bao (@14H034160212) — on the table you posted: the second turn now fails the tool checks because that turn made no tool call, which is the point of #10091 (the check is scoped to the turn under evaluation instead of the whole conversation). The first turn still passes all three, and the tests from #10245 pass unchanged on this head — the only behaviour change since |

Motivation & Context
tool_called_check,tool_calls_presentandtool_call_args_matchscanneditem.conversation— the whole conversation — for function calls. Every other derived value onEvalItem(query,response) and the Foundry provider (item.split_messages(effective_split)) uses the item's split strategy.As a result, in multi-turn evaluation a tool call made in an earlier turn satisfies the tool check for a later turn. With
EvalItem.per_turn_items(...), a turn in which the agent made no tool call still passedtool_called_check("get_weather"),tool_calls_presentandtool_call_args_match, because the call from turn 1 sits on the query side of turn 2's split.That contradicts the per-turn design described in ADR-0023 and in
per_turn_items:response_messages"contains the agent's actions up to the next user message", which "lets you evaluate each response independently". ADR-0023 also states: "Changing thesplit_strategyconsistently changes all derived values." For a CI gate, the outcome is a silent false pass on exactly the turns that per-turn evaluation is meant to pinpoint.Description & Review Guide
What are the major changes?
_response_messages(item)that returnsitem.split_messages()[1]— the response side of the item's split, the same messagesitem.responseuses.tool_called_checkand_extract_tool_calls(used bytool_calls_presentandtool_call_args_match) now read those messages instead ofitem.conversation.FULLsplit that still counts the whole trajectory, and an unchanged single-turn conversation.What is the impact of these changes?
LAST_TURNsplit behave exactly as before, because the query side there contains only the user message.What do you want reviewers to focus on?
item.split_messages(effective_split)) should stay as they are.Related Issue
Fixes #10091
AI Assistance
AI assistance details: An AI coding assistant helped audit the evaluation module, reproduce the issue, write the regression tests, and draft this description. The analysis, the reproduction and the test results were verified by me against
mainat commitdc6617a.Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.