Skip to content

refactor(gooddata-eval): centralize agentic outcome fields; fix exit_reason and tool_calls gaps - #1846

Merged
Tomkess merged 1 commit into
feat/agentic-best-run-latencyfrom
feat/agentic-exit-reason-and-outcome-helper
Oct 5, 2026
Merged

Tomkess merged 1 commit into
feat/agentic-best-run-latencyfrom
feat/agentic-exit-reason-and-outcome-helper

Conversation

@Tomkess

@Tomkess Tomkess commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #1845. After shipping best_run_latency_s, I ran a comprehensive
comparison across all 11 agentic evaluators (alert_skill, anomaly_detection,
conversation, dashboard_skill, general_question, guardrail, kda_skill,
metric_skill, search_tool, visualization, what_if) and live production JSON
output to check whether the same field-is-missing-from-some-kinds pattern recurs
elsewhere. It does, twice more, both confirmed in real result files before this PR:

  1. exit_reason (the LoopExit enum) — invented specifically so a run that hit
    max_iterations stops being indistinguishable from a genuine refusal (every
    downstream check is produced_output and <check>, so an exhausted run reports
    every field as False either way). Wired into 5 of 8 multi-turn kinds. Missing
    from anomaly_detection, dashboard_skill, what_if.
  2. detail's tool_calls key — timeline_detail() builds both
    latency_breakdown and tool_calls from the same events so they stay
    index-aligned, but roughly half the evaluators called build_latency_breakdown()
    directly instead and silently never got a tool_calls key.

Root cause of both (and of best_run_latency_s before them, and of .timings
before that — see AgenticAssertionError's own docstring, which documents this
happening once already): every evaluator hand-writes the same ~7-10 line block
copying reasoning_steps/conversation_id/response_id/detail/runs_passed/
runs_effective/(timings)/best_run_latency_s onto either a raised exception or
the returned AgenticEvalOutcome. 11 independent copies of the same logic — a new
universal field needs 11 edits, and nothing fails loudly when one is skipped. There's
a test asserting every kind is dispatched (test_agentic_runner.py's
assert covered == set(AGENTIC_TEST_KINDS)), but none asserting every kind's result
carries the same fields.

What changed

  • New core/agentic/_outcome.py:
    • agentic_detail(tool_call_events, reasoning_step_events, **kind_specific) — every
      evaluator's detail dict now goes through this, so the tool_calls gap can't
      recur by omission.
    • raise_agentic_failure(exception_cls, message, ...) / agentic_success(...) —
      the shared tail that used to be hand-rolled. exception_cls is typed
      type[AgenticAssertionError], which is what makes the bare-annotation fields
      (reasoning_steps, detail, etc.) type-check without # type: ignore.
  • All 11 evaluators migrated to call these instead of constructing the
    exception/outcome by hand. Per-evaluator duplicate-field-assignment count: ~85
    lines total before this PR, 3 after
    — the 3 remaining are general_question's
    and guardrail's "no judge verdict for any run" branches, which raise a bare
    JudgeResponseError (a RuntimeError, not an AgenticAssertionError subclass) and
    so can't route through raise_agentic_failure; left as direct attribute sets,
    unchanged from before.
  • anomaly_detection.py, dashboard_skill.py, what_if.py each gained an
    exit_reason: LoopExit field on their per-run result dataclass, set at every break
    point in their run loop. dashboard_skill.py's loop has no send_message
    try/except and no simulated-user call, so it only ever produces SUCCESS,
    AGENT_SILENT, or the BUDGET_EXHAUSTED default — never CHAT_ERROR or
    SIMULATED_USER_FAILED, unlike the other two.

Explicitly NOT in this PR

  • agentic_dashboard_summary not existing on master at all. Checked: your pin
    is on integration/eval-rebuild, which carries 2,180+ lines of unmerged diffs
    across 19 files (a whole separate forecasting.py evaluator, a new
    _failed_runs.py module, and substantial rewrites already in flight) — a
    pre-existing release effort of its own. Porting just dashboard_summary.py here
    would duplicate or conflict with that work rather than fix anything.
  • A ~9% gap in agentic_guardrail's .reasoning.json sidecar capture, where runs
    that genuinely had reasoning steps (per detail.latency_breakdown) still got no
    sidecar written. Investigated as far as: ruled out a --no-reasoning config
    difference (same run_id, mixed result within it); ruled out SSE-level divergence
    (ChatResult.reasoning_steps and .reasoning_step_events are built from the exact
    same accumulator list in sse_client.py, so they can't disagree there). The
    remaining cause needs a live repro with actual reasoning content, not static source
    reading — not fixed here rather than guessed at.

Test plan

  • Full gooddata-eval suite: 1382 passed
  • ruff check / ruff format --check: clean
  • ty check: clean (one real issue found and fixed along the way — the
    exception_cls parameter needed type[AgenticAssertionError], not
    type[Exception], for the bare-annotation attributes to type-check at all;
    and conversation_id needed to stay non-optional str to match
    AgenticAssertionError's own declared type, distinct from
    AgenticEvalOutcome's optional one)
  • Extended tests for anomaly_detection, dashboard_skill, what_if assert the
    actual exit_reason value at each new break point (SUCCESS, AGENT_SILENT,
    BUDGET_EXHAUSTED, CHAT_ERROR) — not just that existing tests still pass,
    which they already did even before exit_reason was wired in correctly, since
    none of them asserted on it
  • Extended tests for general_question, kda_skill, search_tool assert the
    new tool_calls key is present in detail

🤖 Generated with Claude Code

…reason and tool_calls gaps

Comprehensive analysis of the 11 agentic evaluators (alert_skill, anomaly_detection,
conversation, dashboard_skill, general_question, guardrail, kda_skill, metric_skill,
search_tool, visualization, what_if) found the same unfinished-rollout pattern that
caused best_run_latency_s (#1845) recurring twice more, live in production data:

1. exit_reason (LoopExit) -- invented specifically so a run that hit max_iterations
   stops looking identical to a genuine refusal -- was wired into 5 of 8 multi-turn
   kinds. Missing from anomaly_detection, dashboard_skill, what_if. All three now
   track it through every break point in their run loop (SUCCESS/AGENT_SILENT/
   CHAT_ERROR/SIMULATED_USER_FAILED/BUDGET_EXHAUSTED default), mirroring the exact
   pattern already proven in kda_skill.py/alert_skill.py.

2. detail's tool_calls key -- timeline_detail() builds both latency_breakdown and
   tool_calls from the same events so they stay index-aligned, but roughly half the
   evaluators called build_latency_breakdown() directly and silently never got a
   tool_calls key. Every evaluator's detail dict now goes through one shared
   `agentic_detail()` call, so this can't happen again by omission.

3. The root cause of 1 and 2 (and best_run_latency_s before them): every evaluator
   hand-wrote the same ~7-10 line block copying reasoning_steps/conversation_id/
   response_id/detail/runs_passed/runs_effective/(timings)/best_run_latency_s either
   onto a raised exception or into the returned AgenticEvalOutcome -- 11 independent
   copies of the same logic, so a new universal field needed 11 edits, and nothing
   failed loudly when one was missed (AgenticAssertionError's own docstring already
   documents this happening once before, to `.timings`).

   New core/agentic/_outcome.py centralizes it: `agentic_detail()` for the detail
   dict, `raise_agentic_failure()`/`agentic_success()` for the common fields. All 11
   evaluators now call these instead of hand-rolling. Per-evaluator duplicate field
   assignments: ~85 lines total before, 3 after (the 2 remaining are general_question's
   and guardrail's "no judge verdict at all" branches, which raise a bare
   JudgeResponseError -- not an AgenticAssertionError subclass, so they can't route
   through raise_agentic_failure; left as direct attribute sets, same as before).

Explicitly NOT addressed here (see PR description for why):
- agentic_dashboard_summary not existing on master at all -- it's part of a much
  larger (2000+ line) unmerged integration branch with its own in-flight PRs; porting
  it here would duplicate/conflict with that separate effort.
- A ~9% gap in agentic_guardrail's .reasoning.json sidecar capture for runs that did
  have reasoning steps -- traced as far as ruling out config differences and SSE-level
  field divergence (reasoningSteps/reasoningStepEvents are built from the same
  accumulator list, so they can't diverge there), but the remaining cause needs a live
  repro with real reasoning content, not static source reading.

Verified: full gooddata-eval suite (1382 passed), ruff lint/format, and ty type-check
all clean. New/extended tests assert real exit_reason values (not just that nothing
broke) for every break point added, plus tool_calls presence, across all three
newly-covered kinds.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 5, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0dda85a8-144f-4583-ae01-068fb7a98d90

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@Tomkess
Tomkess merged commit c41f542 into feat/agentic-best-run-latency Oct 5, 2026
1 check passed
@Tomkess
Tomkess deleted the feat/agentic-exit-reason-and-outcome-helper branch October 5, 2026 20:46
Tomkess added a commit that referenced this pull request Oct 5, 2026
…build

#1845 (carrying #1846) centralizes the common tail of every evaluate_agentic_*
into _outcome.py; #1816 centralizes each kind's per-run detail into a
_run_detail builder. Both restructure the same ten evaluators, so every
return site conflicted.

Both land, and they fit together better than either alone:

- The shared tail now carries `failed_runs`. That is the field whose
  omission this branch has been patching kind by kind -- anomaly detection,
  forecasting, dashboard summary and the report skill each shipped without
  it -- and it is exactly the failure mode _outcome.py's own docstring
  describes for `timings` and `best_run_latency_s`. One tail means a kind
  cannot forget it again.
- Each kind keeps its `_run_detail`, so a failing run is still described by
  the same keys as the winning one, and `build_failed_runs` is wired in all
  ten.
- The all-ungraded JudgeResponseError branch in guardrail and
  general_question moved below the `failed_runs` build. It used to raise
  before the records existed; a broken judge is precisely when they are
  worth having. general_question also keeps its item timings on that error.
- The structural guard learns the new shape: attaching via
  raise_agentic_failure counts, alongside setting the field directly or
  going through a kind's own _attach_diagnostics.

#1831's gate imports in what_if were restored -- the import hunk resolved to
the incoming side, which predates that fix.

1578 passed, 2 skipped. ruff clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tomkess added a commit that referenced this pull request Oct 8, 2026
#1816 landed on master, so master now carries the hand-rolled per-evaluator
failure tail that #1846's _outcome.py refactor replaced here. Twelve source
files conflicted on exactly that: ours is the centralised form of theirs, so
ours wins everywhere. The structural guards in test_agentic_runner.py are what
confirm it -- they accept raise_agentic_failure() as one of the three ways a
kind can attach its records, and every multi-run kind still passes.

test_agentic_runner.py conflicted the usual way, ours a superset of master's.
Verified by AST rather than by reading: nothing defined on master is missing
here, and no definition is duplicated.

One defect the merge introduced silently, with no conflict to mark it:
test_trace_linker.py's _EVALUATE_FUNCS gained duplicate entries for
anomaly_detection, dashboard_summary and forecasting. Git auto-merged two
versions of a growing list by appending both. pytest refused to collect the
file over duplicate parametrize ids, which is the only reason it surfaced --
a list of tuples has no syntax error to trip over.

1771 passed, 2 skipped. The 16 fewer than before are precisely the 4 duplicate
entries times the 4 tests parametrized over that list.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant