feat(eval): compare a run against a saved baseline and fail on regression - #3946
feat(eval): compare a run against a saved baseline and fail on regression#3946dwin-gharibi wants to merge 2 commits into
Conversation
…or new eval matrix
aheritier
left a comment
There was a problem hiding this comment.
Thanks for this — the problem is real and the semantics write-up is genuinely useful. CI is green on 5480ffa (build-and-test, lint, windows-tests, license-check, both build-image). Three blocking issues, the first of which means the feature cannot currently be used end to end.
[blocking] --baseline cannot read anything docker agent eval writes
LoadBaseline (pkg/evaluation/baseline.go:103) expects an EvalRun. The eval command never writes one: it writes a RunOutput via SaveRunSessionsJSON (cmd/root/eval.go:143 → pkg/evaluation/save.go:441). SaveRunJSON (pkg/evaluation/save.go:402-405), which the description presents as the producer, has no non-test callers and is documented as "kept for backward compatibility and debugging purposes".
RunOutput.Duration is a string (pkg/evaluation/types.go:127) while EvalRun.Duration is a time.Duration (pkg/evaluation/types.go:117), so the load fails outright:
path, _ := SaveRunSessionsJSON(run, dir) // exactly what the eval command writes
_, err := LoadBaseline(path)LoadBaseline(that file) -> parsing baseline ".../yesterday.json":
json: cannot unmarshal string into Go struct field EvalRun.duration of type time.Duration
Even with the duration fixed, RunOutput has no results key, so the baseline would load with zero results (see the next point).
The file that does exist already carries what the gate needs: summary for the aggregate rates, and sessions[].eval_result.passed for per-eval pass/fail. Reading RunOutput would make the "no new format" claim true, and would also resolve the third issue below. Alternatively add an explicit producer (--save-baseline) and document it — but as it stands there is no supported way to obtain a baseline file.
[blocking] The gate fails open
LoadBaseline accepts any JSON object. A baseline with no results yields all-zero Metrics, so every rate is skipped by the has-flag guards (pkg/evaluation/baseline.go:158) and the failure rate by pkg/evaluation/baseline.go:167 — and the gate reports success while every evaluation in the current run fails:
os.WriteFile(p, []byte(`{"name":"not-an-eval-run"}`), 0o600)
base, err := LoadBaseline(p) // err == nil
cur := &EvalRun{Results: []Result{
{InputPath: "a", SizeExpected: "M", Size: "S"},
{InputPath: "b", Error: "boom"},
}}
PrintComparison(os.Stdout, Compare(base, cur, 0))Baseline comparison (tolerance 0.000)
METRIC BASELINE CURRENT DELTA
· total cost 0.000 0.000 +0.000
Changed evaluations
a absent → fail
b absent → fail
✅ No regression against baseline
The same holds when the current run has no evaluations (e.g. an --only pattern that matches nothing): nothing gates. A CI gate must fail closed — reject a baseline with zero results, and reject a comparison where either side has no evaluations or no metric in common.
[blocking] resultPassed contradicts the project's own definition of pass
Result.checkResults (pkg/evaluation/types.go:70-77) is what the eval output prints (pkg/evaluation/progress.go:94) and what the saved eval_result.passed records (pkg/evaluation/save.go:451). It requires ToolCallsScore >= 1.0 when a tool-call expectation is declared. resultPassed (pkg/evaluation/baseline.go:283) requires only >= ToolCallsExpected:
r := Result{InputPath: "a", ToolCallsExpected: 0.8, ToolCallsScore: 0.9}
_, failures := r.checkResults() // [tool calls score 0.90] → printed as FAIL
resultPassed(r) // true → gate says passThis defeats the PR's central guarantee. An evaluation whose printed status flips from pass to fail is recorded as no change, and the aggregate movement is then absorbed by the tolerance:
base := &EvalRun{Results: []Result{{InputPath: "a", ToolCallsExpected: 0.8, ToolCallsScore: 1.0}}}
cur := &EvalRun{Results: []Result{{InputPath: "a", ToolCallsExpected: 0.8, ToolCallsScore: 0.9}}}
// printed: baseline failures=[] -> current failures=[tool calls score 0.90]
Compare(base, cur, 0.2).Regressed // false, changes=[]TestResultPassed currently pins this divergence rather than catching it. Reuse checkResults (len(failures) == 0) instead of a second definition — one notion of "passed" in the package, and the gate can no longer drift from the product.
[should-fix] A failed run prints "✅ No regression"
cmd/root/eval.go:162 computes the comparison even when evalErr != nil, discarding only the error. A partial run's missing evaluations register as absent and do not gate, so a broken run prints ✅ No regression against baseline and then exits non-zero — contradictory output. Skip the comparison entirely unless evalErr == nil.
[should-fix] Flag tables not updated
Both tables that enumerate every docker agent eval flag are missing the two new ones: docs/features/evaluation/index.md:159-170 and docs/features/cli/index.md:458-469. The semantics that need to be discoverable (tolerance governs aggregates only; a pass → fail always gates; cost is informational; an added failing evaluation gates via the aggregate) currently live only in Compare's doc comment, where a user configuring CI will not see them.
[optional] Tolerance upper bound
--regression-tolerance is documented as (0-1) and negatives are clamped (pkg/evaluation/baseline.go:138), but values above 1 are accepted — --regression-tolerance 10 silently disables the aggregate gate. Rejecting > 1 would make that a startup error instead.
Worth doing
The underlying gap is real and the judgement calls are the right ones: a tolerance is necessary for a judge-scored suite, cost must not gate, and an absent metric must not read as a drop from zero. Commits are signed and conventional, tests follow package conventions and go test ./pkg/evaluation/ ./cmd/root/ passes locally. Once the comparison is grounded on the file the command actually writes and reuses checkResults, I'd like to see this land — the Comparison JSON tags leaving room for a later --json is a nice touch.
Adds
--baselineand--regression-tolerancetodocker agent eval. After the run, the results arecompared against a previously saved run and the command exits non-zero if quality regressed — so
"did my prompt edit make this worse?" becomes a CI gate instead of a scrollback comparison.
Closes #3945.
!marks a gating regression,·marks an informational metric.No new format
The baseline is just an
EvalRunJSON thatSaveRunJSONalready writes on every run. There isnothing new to produce and nothing to migrate — yesterday's
results/*-eval.jsonis a valid baselinetoday.
LoadBaselinereads it back, and there's a round-trip test asserting a saved run loads withidentical metrics.
The three semantics worth reviewing
These are judgement calls, and getting them wrong makes the gate either useless or unusable.
1. Tolerance governs aggregate rates only; a pass → fail transition always gates.
An LLM judge does not return the same score twice, so without a tolerance a gate built on this flaps,
gets disabled, and is then ignored. But if the tolerance could also absorb an evaluation going from
passing to failing, the gate would be worthless — that transition is the exact signal it exists to
catch.
So
--regression-tolerance 0.99still cannot hide a breakage. Both halves are pinned by tests(
ToleranceAbsorbsSmallDrops,PassToFailGatesRegardlessOfTolerance), and the interaction isspelled out in
Compare's doc comment because it is genuinely surprising.2. A metric absent on either side is skipped, not compared against zero.
Adding the first
size_expectedto a suite would otherwise look like a drop from 1.0 to nothing.Metricstherefore carriesHasSizes/HasTools/HasRelevancealongside the rates: withoutthat, "no size expectations declared" and "every size expectation failed" both read as
0.0and agate cannot tell them apart.
3. Cost is reported but never gates.
A provider price change is not a quality regression, and a gate that fires on it gets turned off. It
is marked
Informationaland printed with·.One consequence I want to flag rather than bury: adding a new failing evaluation lowers the
aggregate rate and therefore gates, even though no existing evaluation regressed. I think that is
right — a suite that got worse should say so — but it means "commit a known-failing eval as a TODO"
needs a tolerance bump or a fix.
Compare's doc says this, andAddedFailingEvalGatesViaTheAggregateRatepins it.Structure
pkg/evaluation/baseline.go— all pure functions, no I/O exceptLoadBaseline:MetricsOf(*EvalRun) Metrics— derives rates from the same fieldscomputeSummaryalready scores,so the gate cannot drift from what the summary prints
Compare(baseline, current, tolerance) ComparisonresultPassed(Result)— mirrors the expectationscomputeSummarychecks; an expectation that wasnot declared is not a failure
PrintComparison(io.Writer, Comparison)Comparisonis JSON-tagged throughout, so a--jsonmode is a later flag rather than a rewrite.cmd/root/eval.go— two flags and acheckBaselinemethod. When the run itself errored, thaterror keeps the exit code: an eval that could not execute is a more fundamental problem than a
regression.
Repeated inputs (
--repeat > 1) collapse pessimistically — if any repetition of an input failed, theevaluation counts as failed. A flaky pass is not a pass.
Tests
pkg/evaluation/baseline_test.go(16 tests): rate derivation and the Has… flags; nil-run safety;no-change; quality drop gates; tolerance absorbs noise; pass→fail gates through a large tolerance;
improvement never gates; failure-rate climb gates; cost is informational; absent metric skipped;
added/removed evals do not gate on their own; an added failing eval gates via the aggregate;
regressions sort first; repeated inputs collapse pessimistically;
SaveRunJSON→LoadBaselineround-trip; both load error paths; print output for regressed and clean; JSON round-trip;
resultPassedper expectation kind.cmd/root/eval_baseline_test.go(6 tests): no-baseline is a no-op; regression returns an errorand prints; clean run succeeds; tolerance is plumbed end to end; a missing baseline file fails
loudly rather than silently skipping the gate; flags registered with a default of 0.
Two of these tests started as wrong assertions of mine and are worth calling out, because both
uncovered semantics rather than bugs: I initially expected the tolerance to absorb a pass→fail flip,
and expected an added failing eval not to gate. Both behaviours are defensible as implemented, so the
tests now pin the real semantics and the doc comment explains them.
Verification
Toolchain
go1.26.5, darwin/arm64.go test ./pkg/evaluation/ ./cmd/root/golangci-lint run ./pkg/evaluation/... ./cmd/root/...(v2.12.2, CI's pin)go run ./lint .go build ./...,gofmt -ldocker agent eval --helpgo test ./...pkg/teamloaderfails — pre-existing (Google Cloud ADC), unrelated