feat(gooddata-eval): add the agentic anomaly-detection evaluator - #1801
feat(gooddata-eval): add the agentic anomaly-detection evaluator#1801Tomkess wants to merge 7 commits into
Conversation
The anomaly skill is enabled on the eval org and reachable today (confirmed live:
one turn -- search, build the chart, detect), but nothing evaluates it.
execute_anomaly_detection takes only visualization_ref and max_points. There is no
threshold, no sensitivity, no expected anomaly, so unlike forecasting and what-if
the tool call carries almost no assertable intent. What is assertable is the chart
the detection ran on: the measure and the time granularity are the whole of "did
it look at the right series", and getting either wrong makes the result
meaningless however well the detection performed.
expected_output:
{"metric": "metric/spend", "granularity": "MONTH"}
The flagged-point count is reported but never asserted. Whether a real series
contains anomalies is a property of the data, not the agent, so a fixture
demanding some would start failing the day the warehouse refreshed.
One deliberate divergence from the service. gen-ai infers granularity by
tokenizing the field reference into a SET and returning the first match it
iterates -- and its token map sends both "date" and "month" to a granularity, so
`label/process_date.month` resolves to MONTH or DAY depending on hash order. This
takes the last recognised token instead, which reads the dotted suffix and is
deterministic. The divergence only appears on references naming two
granularities, where the service's own answer is unstable; worth reporting
upstream separately.
LoopExit is deliberately not used -- it lands with #1789, which is still open.
23 tests. 803 passed, lint and format clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 2 billable files and costs up to $0.50. Or wait 45 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR adds the ChangesAgentic anomaly detection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant evaluate_agentic_anomaly_detection
participant ChatClient
participant AnomalyTools
participant Langfuse
CLI->>evaluate_agentic_anomaly_detection: dispatch anomaly evaluation
evaluate_agentic_anomaly_detection->>ChatClient: run question and clarification turns
ChatClient->>AnomalyTools: request visualization and anomaly detection
AnomalyTools-->>evaluate_agentic_anomaly_detection: return tool traces
evaluate_agentic_anomaly_detection->>Langfuse: record trace scores and quality data
evaluate_agentic_anomaly_detection-->>CLI: return AgenticEvalOutcome
Merge Risk: ⚪ Minimal · up to The change adds anomaly-detection evaluation with clarification handling, scoring, diagnostics, and CLI registration. Current evidence indicates the behavior is covered and mergeable with normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
A rabbit reads each line, Comment |
…detection Two of the three findings on #1798 are structural and apply here unchanged. Tool calls were extracted from the current turn only. The agent may build the chart on one turn and detect on the next, and reading a single turn dropped the series the detection actually ran on, failing a correct run for having no metric or granularity. Extraction now reads every turn accumulated so far. Unasserted content checks were published to Langfuse as BOOLEAN 1. They are True internally so they cannot fail a run, but reporting that as a score claims the evaluator verified something it never looked at. Only checks named in ev.asserted are now scored. The third finding (unchecked confidence/seasonality) was forecasting-specific. 1 test added, verified to fail against the previous version. 804 passed, lint and format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review on the sibling PR #1798 surfaced two findings that are structural and applied here unchanged. Fixed in ``, before this PR was reviewed. Tool calls were extracted from the current turn only. The agent asks a disambiguation question before building anything, so the create call and the execute call can land on different turns — reading a single turn dropped the object the execution actually ran on, and the evaluator then failed a correct run for having no content to check. Extraction now reads every turn accumulated so far. Unasserted content checks were published to Langfuse as BOOLEAN 1. They are One test added, verified to fail against the previous version. Lint and format clean. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1801 +/- ##
==========================================
+ Coverage 82.27% 82.45% +0.18%
==========================================
Files 282 283 +1
Lines 20326 20584 +258
==========================================
+ Hits 16723 16973 +250
- Misses 3603 3611 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Third structural finding from the sibling PRs' review, applied here. latency_sec used run.turn_wall_clock_sec, which is the goal turn alone and excludes the clarification turns that got there -- understating the item's real elapsed cost on exactly the runs where it matters. It now prefers pt.latency, which covers the conversation, and falls back to the goal turn. That is what 7 of the 8 existing kinds already do; kda_skill is the outlier and documents its own reason, and this copied it without re-checking. cost_usd was gated on ev.triggered, so a run that answered without ever reaching the tool reported no cost despite having spent tokens. The gate is gone. 804 passed, lint and format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit claimed gen-ai's granularity inference was ambiguous for a
label like process_date.month, because "date" and "month" both map. That was
wrong, and the fault was here: the service's token map has no "date" key. This
module invented one, and also dropped the service's "hour". The map is now copied
verbatim, so the two agree.
Two consequences of the invented key, both fixed:
label/process_date the service matches nothing and the tool refuses the
call rather than guessing daily. This scored DAY, so a
run the service never ran would have been graded
against a granularity it never used.
label/process_date.hour HOUR was unrecognised here and now resolves.
The last-token rule stays, but for a narrower and real reason: a reference naming
TWO granularities -- a snake_case first_day_quarter.month, whose tokens contain
day, quarter and month -- does make the service's set iteration order decide, and
a scorer must not be a coin flip even where the thing it scores is one. No label
in the eval workspace names two (0 of 558 checked), so this is latent rather
than live.
"week_us" is kept though tokenization can never reach it (it splits into "week"
and "us"), so the map stays a copy rather than an edit.
Tests updated to the corrected behaviour, plus one pinning that a bare date
attribute resolves to None. 806 passed, lint and format clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Correction — the "service bug" I reported in this PR was not one, and the fault was mine. I claimed gen-ai's granularity inference was ambiguous for _TOKEN_TO_GRANULARITY: dict[str, str] = {
"hour": "HOUR", "day": "DAY", "week": "WEEK", "week_us": "WEEK",
"month": "MONTH", "quarter": "QUARTER", "year": "YEAR",
}This module invented The invented key was also two real bugs in the evaluator, not just a wrong claim:
What survives is real but much narrower. A reference naming two granularities does leave the service's answer to set iteration order — a snake_case I measured the scope rather than assuming it this time: 0 of 558 labels in the eval workspace name two granularities. The Worth stating plainly: I asserted a defect in someone else's service from a misread of my own code, and put it in a PR body and a commit message. The check that caught it was reading the service's map again instead of trusting my note about it. 805 passed, lint and format clean. |
…lure paths
codecov flagged 47 uncovered lines on this patch. The percentage is not the point
-- the largest uncovered block was the deferred Langfuse scoring, which includes
the ev.asserted gating and the latency/cost change made in response to review.
Those were behavioural fixes shipped with no test at all.
Covered now, by capturing the deferred callable and running it against a fake
context:
- only checks the fixture pinned are scored, an unasserted one is absent
rather than published as a BOOLEAN 1
- both content checks scored when both are pinned
- cost is reported even when the tool was never reached
Plus the smaller gaps: the clarification prompt dropping absent hints instead of
asserting a literal None, bare-string fields in metric and granularity reading,
a chat error keeping what its partial_result carried, and a simulated-user
failure ending the run without raising.
82% -> 97% on the module. 8 lines remain, all inside the OpenAI call itself.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merges master, which added a guard requiring every kind to hand the scored item's question to the linker -- a score is otherwise readable only by resolving its conversation back to the item. This kind predates the guard and did not. 949 passed, lint and format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Third of three evaluators for skills that ship in the product but have no eval coverage. Siblings: #1798 (forecasting), #1799 (what-if analysis).
Why now
Probed live against
micdiagnose-dev: the skill is enabled and completes in a single turn — search, build a monthly-spend chart,execute_anomaly_detection. Nothing evaluates it.Honest about what can be checked
execute_anomaly_detectiontakes onlyvisualization_refandmax_points. No threshold, no sensitivity, no expected anomaly — so unlike forecasting and what-if, the tool call itself carries almost no assertable intent.What is assertable is the chart the detection ran on. The measure and the time granularity are the whole of "did it look at the right series", and getting either wrong makes the result meaningless however well the detection performed.
{"metric": "metric/spend", "granularity": "MONTH"}The flagged-point count is reported but never asserted. Whether a real series contains anomalies is a property of the data, not of the agent — a fixture demanding some would start failing the day the warehouse refreshed. The live probe returned
point_count: 0and the agent correctly said so; that is a pass.If the team would rather not add another kind whose signal is mostly "the chain ran" (the same limitation
kda_skillcarries, open in the eval repo since August), this is the one of the three to drop. Forecasting and what-if check real correctness; this one checks targeting.A correction, and a much narrower real issue
An earlier revision of this PR claimed gen-ai's granularity inference was ambiguous for
label/process_date.month, becausedateandmonthboth map to a granularity. That was wrong, and the fault was in this module, not the service. gen-ai's token map has nodatekey:This module invented
"date": "DAY"and dropped the service's"hour". Fixed in5a57f66c— the map is now copied verbatim. Two consequences of the invented key, both real bugs in the evaluator:label/process_dateDAY— grading a run the service never ranlabel/process_date.hourHOURWhat survives is narrower and genuine. A reference naming two granularities does let set iteration order decide the service's answer — a snake_case
first_day_quarter.monthtokenizes to{first, day, quarter, month}, three of which map. The last-token rule stays for that reason: a scorer must not be a coin flip even where the thing it scores is one.Scope, measured rather than asserted: 0 of 558 labels in the eval workspace name two granularities. This workspace's
FirstDayQuarter.*labels are camelCase, so they tokenize to one word and are safe; the same attribute namedfirst_day_quarterwould not be. So it is latent, not live, and I would not file it upstream on this evidence alone.Not included, on purpose
LoopExit/exit_reason— lands with #1789, still open.Tests
23, including: extraction pairing a detection with the chart it followed, the ambiguous-label case above, a field token winning over a disagreeing filter (matching the service's order), finding no anomalies still passing, and a chat error on a later run not discarding the earlier one.
803 passed, lint and format clean.
Merge note
Touches the same three files as the siblings —
cli/agentic_runner.pyplus the_ALL_AGENTIC_KIND_CASESand_EVALUATE_FUNCSstaleness guards. Whichever merges first, the others need a trivial rebase on those lists.🤖 Generated with Claude Code
Summary by CodeRabbit