feat(pilot): behavioural scorers, feedback ground truth, and an eval dataset
Adds the evaluation layer on top of the review traces: five deterministic
scores describing how the reviewer behaved, a bridge that turns human reactions
into ground truth, and a dataset seeded from the reviews already run.
The two are kept apart on purpose. feedback.db has recorded 113 reviews and
zero reactions, resolutions or replies — nobody has ever responded to a bot
comment — so an accuracy metric cannot be built yet. The scorers therefore
measure behaviour, which is computable from data in hand, and feedback_scores
turns verdicts into scores the moment any arrive.
eval_scores.py emits finding_rate, severity_info_ratio, severity_max,
dropped_findings and cost_per_finding into the same ingestion batch as the
trace. Undefined values are omitted rather than reported as zero: an info ratio
over a silent review is undefined, and charting it as 0 would read as perfect
calibration.
dropped_findings needed a parser change. Both parsers silently discard findings
with an unusable path/line, which made a model emitting garbage locations
indistinguishable from one that found nothing. last_parse_dropped() exposes the
delta, read at parse time — after apply_repo_config the drops are the config
working as intended, not the model misbehaving.
feedback_scores.py scores the session ("{repo}#{pr}"), because feedback arrives
days later against a PR and nothing records which re-run produced which
comment. review_acceptance is absent rather than 0 when nothing was engaged.
eval_bootstrap.py registers the score configs, seeds the pragent-reviews
dataset, and can backfill scores onto traces that predate the scorers.
expectedOutput is the reviewer's own prior output, flagged
labelled_by_human: false — a regression baseline, not verified truth.
Also fixes a silent telemetry failure: the ingestion endpoint answers 207 when
only some events succeed, so a batch with every event rejected still looked
like success. Score events were missing the required per-event timestamp and
ingested nothing while reporting 207. _warn_on_rejected_events now logs the
per-event errors under LANGFUSE_DEBUG.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude
@@ -0,0 +1,123 @@
|
||||
# Evaluation — scorers, ground truth, and the dataset
|
||||
|
||||
Langfuse already receives one trace per review (`README-langfuse.md`). This is
|
||||
the layer on top: numbers attached to those traces that say how the reviewer
|
||||
*behaved*, and the beginnings of a ground-truth signal that says whether it was
|
||||
*right*.
|
||||
|
||||
Those two things are deliberately kept apart, because only one of them exists
|
||||
yet.
|
||||
|
||||
## What could and could not be built
|
||||
|
||||
`feedback.db` has recorded 113 reviews across 4 repos. It has recorded **zero**
|
||||
reactions, zero thread resolutions and zero replies. The harvester, the schema
|
||||
and the daily analyzer are all working; nobody has ever reacted to a bot
|
||||
comment.
|
||||
|
||||
That rules out an accuracy metric today. Correctness needs labels, and a
|
||||
judge scored against no labels is theatre. So the scorers here measure
|
||||
behaviour, which is computable from data already in hand, and a separate
|
||||
bridge exists to turn human reactions into scores the moment any arrive.
|
||||
|
||||
## The five behavioural scores
|
||||
|
||||
Emitted with every review by `eval_scores.py`, folded into the same ingestion
|
||||
batch as the trace so they cost no extra request.
|
||||
|
||||
| score | type | what a change in it means |
|
||||
|---|---|---|
|
||||
| `finding_rate` | NUMERIC | Findings posted. 0 is the restraint case — good on clean code, a failure when the run degraded. Only the rate over time separates those. |
|
||||
| `severity_info_ratio` | NUMERIC 0–1 | Share of findings the model rated `info`/`trivial`. Rising = the model is hedging rather than committing. `None` when the review was silent: a ratio over an empty set is undefined, and charting it as 0 would read as perfect calibration. |
|
||||
| `severity_max` | CATEGORICAL | Highest severity surfaced, `none` when silent. Categorical because "did this ever surface something serious" is the real question, and a mean of severity ranks answers nothing. |
|
||||
| `dropped_findings` | NUMERIC | Findings the model emitted that the parser rejected for an unusable `path`/`line`. This is the only score here that measures the model's raw output. |
|
||||
| `cost_per_finding` | NUMERIC | Equivalent USD per finding. A cheaper model that finds nothing is not cheaper. |
|
||||
|
||||
### Why `dropped_findings` needed a change to the parser
|
||||
|
||||
`parse_findings` and `parse_review_output` discard any finding with a missing or
|
||||
unusable location. That happens silently, so a model emitting ten findings at
|
||||
invalid locations was indistinguishable from a model that found nothing — both
|
||||
produce an empty list. `ai_review.last_parse_dropped()` exposes the delta,
|
||||
recorded at parse time.
|
||||
|
||||
It must be read at parse time specifically: by the time findings reach
|
||||
`_emit_langfuse`, `apply_repo_config` has already filtered them by
|
||||
`severity_threshold` and `max_findings`, and those drops are the config working
|
||||
as intended, not the model misbehaving.
|
||||
|
||||
## Ground truth: `feedback_scores.py`
|
||||
|
||||
Turns `feedback.db` into two session-level scores, keyed on `"{repo}#{pr}"`
|
||||
(which is what `langfuse_trace` already sets as `sessionId`).
|
||||
|
||||
| score | meaning |
|
||||
|---|---|
|
||||
| `review_engagement` | Share of a PR's findings that drew any human reaction, resolution or reply. **Watch this first** — every quality number is vapour until it moves off 0. |
|
||||
| `review_acceptance` | Net verdict over engaged findings, −1 to +1. Absent, not 0, when nothing was engaged: zero would claim humans judged the review neutral, when the truth is nobody looked. |
|
||||
|
||||
Session-level rather than trace-level because feedback arrives days later
|
||||
against a PR, and nothing in `feedback.db` records which re-run of the reviewer
|
||||
produced which comment. The session is both the available join and the honest
|
||||
granularity.
|
||||
|
||||
Score ids are `uuid5(namespace, repo#pr#name)`, so the daily backfill updates
|
||||
rather than duplicates.
|
||||
|
||||
## The dataset
|
||||
|
||||
`pragent-reviews`, one item per PR the reviewer has run on, seeded by
|
||||
`eval_bootstrap.py` from `feedback.db`.
|
||||
|
||||
`expectedOutput` is **the reviewer's own prior output**, not human-verified
|
||||
truth — every item carries `metadata.labelled_by_human: false`. Read it as a
|
||||
regression baseline: re-run a candidate model over these PRs and the diff
|
||||
against this column is the behaviour change. Promoting an item to real ground
|
||||
truth means a human editing it in the dataset view after re-reading the PR.
|
||||
|
||||
## Running it
|
||||
|
||||
```bash
|
||||
# once per project: score configs + dataset (+ score historical traces)
|
||||
python3 pilot/eval_bootstrap.py --db /data/feedback.db --backfill-traces
|
||||
|
||||
# ship feedback verdicts (runs daily from the feedback CronJob)
|
||||
python3 pilot/feedback_scores.py --db /data/feedback.db
|
||||
```
|
||||
|
||||
Both need `LANGFUSE_HOST`, `LANGFUSE_PUBLIC_KEY`, `LANGFUSE_SECRET_KEY`. In
|
||||
cluster they come from the `pragent-langfuse` Secret and point at the ClusterIP
|
||||
— never the NodePort, whose oauth2-proxy 302s ingestion to Logto and drops it.
|
||||
|
||||
## Gotcha: HTTP 207 is not success
|
||||
|
||||
The ingestion endpoint answers `207 Multi-Status` when *some* events failed, so
|
||||
a batch where **every** event was rejected still returns 207. An early version
|
||||
of these scorers omitted the required per-event `timestamp` and silently
|
||||
ingested nothing while reporting success. `langfuse_trace._warn_on_rejected_events`
|
||||
now logs the per-event errors under `LANGFUSE_DEBUG=1`. If scores are missing,
|
||||
check that before anything else.
|
||||
|
||||
## What the first run showed
|
||||
|
||||
Backfilled over 42 existing traces and 13 PRs:
|
||||
|
||||
```
|
||||
cost_per_finding n=42 mean=0.3133 min=0.0880 max=0.9042
|
||||
finding_rate n=42 mean=0.4762 min=0.0000 max=4.0000
|
||||
severity_info_ratio n=14 mean=0.0000
|
||||
review_engagement n=14 mean=0.0000
|
||||
severity_max {none: 28, medium: 11, high: 1, critical: 2}
|
||||
```
|
||||
|
||||
Two things worth keeping:
|
||||
|
||||
- **The reviewer is not info-heavy.** `feedback.db` shows 61 of 62 findings at
|
||||
`INFO`, which looked like a badly calibrated model. It is not: `severity_max`
|
||||
reads `medium`/`high`/`critical` on every trace that found anything, and
|
||||
`severity_info_ratio` is flat 0. The `INFO` in the DB comes from
|
||||
`feedback_harvest._parse_severity`, which defaults to `INFO` when its regex
|
||||
misses the severity badge in the rendered comment. The DB severity is a
|
||||
re-parse artifact; the score reads the model's structured output directly.
|
||||
- **28 of 42 reviews found nothing** (67%), and **engagement is flat zero**. The
|
||||
first is not yet interpretable without the second.
|
||||
Reference in New Issue
Block a user