Skip to content

fix(eval): label a sub-agent's reply with its own name for the judge - #8094

Open
YusefSyed wants to merge 1 commit into
google:mainfrom
YusefSyed:fix/multi-turn-final-reply-author
Open

YusefSyed wants to merge 1 commit into
google:mainfrom
YusefSyed:fix/multi-turn-final-reply-author

Conversation

@YusefSyed

Copy link
Copy Markdown
Contributor

Link to Issue or Description of Change

2. Or, if no issue exists, describe the change:

Problem:
rubric_based_multi_turn_trajectory_quality_v1 builds a dialogue for the judge
in which every line carries the agent that produced it. The line for a turn's
final reply is labelled with invocation_events[0].author, the first agent
that acted in the turn. After a transfer that is the agent that handed off,
not the one that answered, so the judge reads a sub-agent's reply as the root
agent's.

A rubric about which agent answers, for example "the router never answers a
billing question itself", is then judged on the wrong name. The tool call
lines of the same turn are labelled correctly, so the dialogue contradicts
itself.

Repro (no model calls; a scripted BaseLlm makes router call
transfer_to_agent and billing_agent answer; the session events go through
EvaluationGenerator.convert_events_to_eval_invocations and then into the
evaluator's dialogue):

session events:
  user: ['text']
  router: ['function_call']
  router: ['function_response']
  billing_agent: ['text']

dialogue sent to the judge:
USER TURN 1: How much do I owe?
AGENT (router) TURN 1 (tool call): transfer_to_agent({"agent_name": "billing_agent"})
AGENT (router) TURN 1 (tool output): transfer_to_agent -> {"result": null}
AGENT (router) TURN 1: You owe $42.

Solution:
Label the reply with the author of the event that carried it.
convert_events_to_eval_invocations keeps that event in invocation_events
and moves its content to final_response, so it is the last agent event left
without content. A new helper in llm_as_judge_utils.py,
get_final_response_author, returns its author.

The helper does not simply take the last event: a usage report or another
agent's tool call can follow the reply, and a hand-written invocation can hold
a user event. An invocation with no event for the reply falls back to its last
agent event, and one without agent events keeps the existing "agent" label.
For a single agent the label is the same as before.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Added tests for the helper in test_llm_as_judge_utils.py (after a transfer,
with a later usage report or tool call of another agent, with no event for the
reply, with no agent events) and
test_final_reply_is_attributed_to_the_agent_that_wrote_it, which checks the
whole dialogue for a router to billing_agent transfer.

Before the fix the dialogue test fails (1 failed, 11 passed). After, on Python
3.11 and 3.13:

$ pytest tests/unittests/evaluation/test_llm_as_judge_utils.py tests/unittests/evaluation/test_rubric_based_multi_turn_trajectory_evaluator.py
49 passed
$ pytest tests/unittests/evaluation
1324 passed, 27 warnings

pre-commit run over the commit passes, and mypy reports no issues in the
two changed modules. tox over the whole suite was not run.

Manual End-to-End (E2E) Tests:

The scripted two-agent run above, after the fix:

dialogue sent to the judge:
USER TURN 1: How much do I owe?
AGENT (router) TURN 1 (tool call): transfer_to_agent({"agent_name": "billing_agent"})
AGENT (router) TURN 1 (tool output): transfer_to_agent -> {"result": null}
AGENT (billing_agent) TURN 1: You owe $42.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

rubric_based_final_response_quality_v1 picks the agent the same way when it
looks up developer instructions for the judge. That is left out to keep this
to one concern and can follow as a separate PR.

The multi-turn trajectory metric shows the judge each turn's final reply
under the name of the first agent that acted in the turn. After a transfer
that is the agent that handed off, not the one that answered, so a
billing_agent reply was shown as "AGENT (router)" and a rubric about which
agent answers was judged on the wrong name.

Label the reply with the author of the event that carried it. That event
stays in the invocation events with its content moved to final_response, so
it is the last agent event left without content; when there is none, the
last agent event is used.

@lxcxjxhx lxcxjxhx left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reverse scan is the right way to find the event whose content was moved: convert_events_to_eval_invocations empties exactly one agent event, and looking from the end skips anything that came after it. The docstring names that dependency, which is what makes the heuristic readable.

The risk worth naming is that this is a heuristic coupled to another function's internals. get_final_response_author is correct because convert_events_to_eval_invocations moves the content off the final-response event and leaves the event in place. Nothing enforces that: if the conversion later changes to drop the event entirely, or to keep the content and copy it, this function silently returns a different author rather than failing. The docstring says what it assumes; a test that constructs the "content moved off the last agent event" shape directly would keep the assumption honest.

Concretely: a test that builds InvocationEvents with [user, agent_a(content), agent_b(no content)] and asserts agent_b — plus one where a content-bearing event follows the empty one, asserting the empty one still wins. Those two pin the scan order and the "first from the end" choice; without them, a future change to the loop passes review by looking plausible.

Checked and correct: the user filter is case-insensitive, so "User" / "USER" are excluded too; and an invocation with no agent events returns None rather than raising, which is what the callers need.

Minor: agent_events[-1].author if agent_events else None at the end could be agent_events[-1].author guarded by the earlier if not agent_events — as written it re-checks a condition the loop above already established, but it is harmless and clearer than nesting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants