Conversation
|
Warning Review limit reached
This review includes 5 billable files and costs up to $1.25. Or wait 31 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 (5)
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 |
|
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 83.08% 83.25% +0.16%
==========================================
Files 330 331 +1
Lines 21763 22025 +262
==========================================
+ Hits 18082 18336 +254
- Misses 3681 3689 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
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: no label in the workspace I measured against names two granularities. Its multi-word date labels are camelCase, so they tokenize to a single word and are safe — the same attribute in snake_case would not be. Latent, not live, and not worth filing upstream on this evidence. 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. |
Scores the anomaly-detection skill end to end, with the granularity map aligned to what the service actually accepts and whole-conversation latency and cost rather than the goal turn's alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
213b39f to
4515325
Compare
#1799 landed on master, so the two sibling evaluators now register side by side. All three conflicts are the same shape -- each branch adds an entry for its own kind to the registered-kinds set, a parametrized test list and the trace-linker list -- and both entries are kept. The dispatch chains did not collide. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…alone The evaluator computed pass^K and then never read it: the verdict asked `if not summary.pass_at_k`, so `--gate power` on a flaky item reported a pass on the strength of one good run out of K. That is the exact case the gate exists to catch, and the report claimed the opposite of what happened. Forecasting now takes `gate` like every other multi-run kind, asks `gate_passed(...)` for the verdict, and carries the gate's own note into the assertion message -- which matters here because the message body describes the BEST run, and under pass^K that can be a run which passed. It also stamps the gate on the run metadata and publishes pass@K/pass^K/gate_passed to Langfuse, so a gated run is readable there rather than only in the exit code. The default is unchanged. Without --gate the behaviour is pass@K exactly as before, which the added tests pin alongside the power case. Two sibling kinds are still unwired -- agentic_what_if, already on master, and agentic_anomaly_detection in #1801 -- and neither evaluator accepts `gate` yet. Left for a follow-up that can cover both together rather than widening this PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`run_agentic_anomaly_detection` already computed `pass_power_k`; the evaluator never read it, asking `if not summary.pass_at_k` for the verdict, so `--gate power` on a flaky item reported a pass on the strength of one good run out of K. Wired the same way as the other multi-run kinds: the evaluator takes `gate`, the dispatch passes it, `gate_passed(...)` decides, the gate's note goes into the assertion message -- which matters because the body describes the BEST run, and under pass^K that can be one that passed -- and pass@K/pass^K/gate_passed reach Langfuse alongside the run metadata stamp. The default is unchanged: without --gate this is pass@K exactly as before, pinned by a test beside the power case. The power test fails against the previous code. Found by CodeRabbit on #1798. Forecasting is fixed there; what-if, which is already on master, in #1831. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…alone The evaluator computed pass^K and then never read it: the verdict asked `if not summary.pass_at_k`, so `--gate power` on a flaky item reported a pass on the strength of one good run out of K. That is the exact case the gate exists to catch, and the report claimed the opposite of what happened. Forecasting now takes `gate` like every other multi-run kind, asks `gate_passed(...)` for the verdict, and carries the gate's own note into the assertion message -- which matters here because the message body describes the BEST run, and under pass^K that can be a run which passed. It also stamps the gate on the run metadata and publishes pass@K/pass^K/gate_passed to Langfuse, so a gated run is readable there rather than only in the exit code. The default is unchanged. Without --gate the behaviour is pass@K exactly as before, which the added tests pin alongside the power case. Two sibling kinds are still unwired -- agentic_what_if, already on master, and agentic_anomaly_detection in #1801 -- and neither evaluator accepts `gate` yet. Left for a follow-up that can cover both together rather than widening this PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 0cf23ca)
`run_agentic_what_if` already computed `pass_power_k`; the evaluator never read it, asking `if not summary.pass_at_k` for the verdict. `--gate power` on a flaky item therefore reported a pass on the strength of one good run out of K -- the exact case the gate exists to catch, reporting the opposite of what happened. The evaluator now takes `gate` like every other multi-run kind, asks `gate_passed(...)`, and carries the gate's note into the assertion message, which matters here because the message body describes the BEST run and under pass^K that can be one that passed. It also stamps the gate on the run metadata and publishes pass@K/pass^K/gate_passed, so a gated run is readable in Langfuse rather than only in the exit code. The default is unchanged: without --gate this is pass@K exactly as before, pinned by a test alongside the power case. The power test fails against the previous code. Found by CodeRabbit on #1798, where the same gap was fixed for forecasting. `agentic_anomaly_detection` has it too and is fixed on its own branch, #1801, because that evaluator does not exist on master yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 313505a)
2 participants
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 a live development workspace: 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/order_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/order_dateDAY— grading a run the service never ranlabel/order_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
day_of_quarter.monthtokenizes to{day, of, 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: no label in the workspace this was measured against names two granularities. Its multi-word date labels are camelCase, so they tokenize to one word and are safe; the same attribute in snake_case would 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