eval: second/third servers + baseline regression gate - #7
Conversation
Deliver hillclimb #1 (2nd & 3rd servers) and the "catch regressions fast" lever from the ranked backlog, both on the unchanged eval package. Multi-server: the product-specific launch details move into a server registry in the eval command (fizzy, hey, basecamp), so a new server is one map entry, not code. fizzy and hey list+describe hermetically from their vendored catalogs; hey lands with a committed corpus (testdata/scenarios/hey.json) generated straight from its own describe surface and a real haiku run (12/12, $0.0183, results/hey-v0.jsonl) — zero eval-package changes. basecamp-mcp's stdio authenticates eagerly (it fetches authorization.json before serving), so it is wired as a credentialed/--live target and marked non-hermetic until the cassette player can stub its startup. Regression gate: --baseline compares a fresh run to a prior JSONL cell by cell, keyed on (model, scenario_id), and exits nonzero on a score drop, newly-failing, or safety regression; improvements and corpus edits are reported, never gated. The hermetic eval-smoke now gates against a committed fake baseline, so CI catches a shifted catalog surface at zero cost.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Pull request overview
Adds multi-server eval configuration and baseline regression comparisons while keeping the core eval package product-agnostic.
Changes:
- Adds Hey and Basecamp server profiles plus a Hey corpus/result.
- Adds JSONL baseline comparison and regression reporting.
- Gates the fake/oracle smoke run against a committed baseline.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
Makefile |
Adds baseline comparison to eval smoke. |
eval/testdata/scenarios/hey.json |
Adds the Hey scenario corpus. |
eval/testdata/results/fake-oracle.jsonl |
Adds the fake oracle baseline. |
eval/results/hey-v0.jsonl |
Records the Hey Haiku run. |
eval/README.md |
Documents servers, runs, and regression gating. |
eval/cmd/eval/main.go |
Adds server registry and baseline CLI support. |
eval/cmd/eval/main_test.go |
Tests registry and environment behavior. |
eval/baseline.go |
Implements baseline loading, comparison, and rendering. |
eval/baseline_test.go |
Tests regression classifications and keying. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $(GO) run ./eval/cmd/eval --server fake --backend oracle --n 12 --seed 1 \ | ||
| --out /tmp/eval-smoke.jsonl \ | ||
| --baseline eval/testdata/results/fake-oracle.jsonl |
There was a problem hiding this comment.
Fixed in f7c1f08: the smoke now runs a pinned corpus (testdata/scenarios/fake.json) graded against the current fake catalog, so a renamed action, an added required param, or a changed enum invalidates a committed gold and fails as a newly-failing regression. Verified by renaming a fake action -> nonzero exit.
| if err != nil { | ||
| return "" | ||
| } | ||
| return strings.Join(append([]string{path}, prof.args...), " ") |
There was a problem hiding this comment.
Fixed in f7c1f08: registry defaults now return an argv slice (serverFields) instead of joining the resolved path into a string and re-splitting it, so a binary under a directory with spaces spawns correctly. Explicit --server-cmd/EVAL_*_CMD are still tokenized with quote handling. Test added.
| model pass params safety in_tok out_tok cost_usd | ||
| haiku 12/12 12/12 12/12 19199 291 $0.0165 | ||
| server tools/actions model pass params safety cost_usd | ||
| fizzy boards, cards haiku 12/12 12/12 12/12 $0.0165 results/fizzy-v0.jsonl |
There was a problem hiding this comment.
Fixed in f7c1f08: the runs table now lists the domains the fizzy corpus actually samples — boards, cards, columns, comments, steps, users.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e33793a929
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if *baseline != "" { | ||
| bf, err := os.Open(*baseline) |
There was a problem hiding this comment.
When --baseline names the same file as --out—including the natural case where --out is omitted and its default eval/results/<server>-v0.jsonl matches the supplied baseline—the earlier os.Create(outPath) truncates and rewrites the baseline before this open. The comparison consequently reads the current run as its own baseline, silently reports no regression, and destroys the prior results; load the baseline before creating the output file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 060ad1c: the baseline is now loaded before os.Create(outPath). Verified with --out == --baseline: the prior run is read first, the regression is detected (exit 1), and the file then holds the new run.
| $(GO) run ./eval/cmd/eval --server fake --backend oracle --n 12 --seed 1 \ | ||
| --out /tmp/eval-smoke.jsonl \ | ||
| --baseline eval/testdata/results/fake-oracle.jsonl |
There was a problem hiding this comment.
This target still generates scenarios from the current fake catalog, while CompareToBaseline deliberately treats removed and added scenario keys as non-gating. If an action is renamed or removed, the oracle passes the newly generated corpus and the comparison merely reports the old key as removed and the replacement as added, so this command exits successfully despite the comment's promise that a silent fake-catalog surface change fails CI. Use a pinned scenario corpus here or make structural additions/removals gate this smoke target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f7c1f08: pinned corpus (testdata/scenarios/fake.json) so the committed golds grade against the current catalog. Verified a renamed fake action fires the gate.
|
|
||
| # Real cheap-model run against a product's stdio server. fizzy and hey are | ||
| # hermetic (structural only; the server never reaches a backend): | ||
| go build -o /tmp/hey-mcp github.com/basecamp/hey-mcp-server/cmd/hey-mcp |
There was a problem hiding this comment.
Running this documented command from /workspace/mcp fails with no required module provides package github.com/basecamp/hey-mcp-server/cmd/hey-mcp because that product module is intentionally absent from this toolkit's module graph. The Go CLI does not resolve and add an unrequired module for go build; use a versioned go install ...@<version> with an appropriate GOBIN, or run the build from a checkout of the HEY repository instead.
AGENTS.md reference: AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 060ad1c: the README now builds the HEY binary from a checkout of its own repo; the product module is intentionally absent from the toolkit graph, as you note.
There was a problem hiding this comment.
💡 Codex Review
Line 106 in e33793a
Read the baseline before truncating the output
When --baseline resolves to outPath—notably --baseline eval/results/hey-v0.jsonl while leaving --out at its default—os.Create truncates the prior baseline and writes the current run before the baseline is opened. The comparison then reads the new results, always reports no regression, and destroys the committed prior run; load the baseline before creating the output or reject paths that reference the same file.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $(GO) run ./eval/cmd/eval --server fake --backend oracle --n 12 --seed 1 \ | ||
| --out /tmp/eval-smoke.jsonl \ | ||
| --baseline eval/testdata/results/fake-oracle.jsonl |
There was a problem hiding this comment.
In the eval-smoke workflow, this command regenerates both the oracle's gold answers and the evaluated scenarios from the current fake catalog. The oracle therefore gives every retained action a score of 1; renamed, added, or removed actions surface only as added/removed cells, which CompareToBaseline deliberately does not gate, while schema or annotation changes retain the same IDs and still score 1. Consequently, the catalog changes this target claims to block will pass CI; use committed scenarios or a catalog fingerprint, or make corpus drift gating for this smoke run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f7c1f08 — same as the pinned-corpus change above: eval-smoke now grades a committed corpus against the live catalog, so schema/rename/enum drift under an unchanged id scores 0 and gates.
| func LoadBaseline(r io.Reader) (*Baseline, error) { | ||
| b := &Baseline{cells: map[string]Record{}} | ||
| sc := bufio.NewScanner(r) | ||
| sc.Buffer(make([]byte, 0, 64*1024), 4*1024*1024) | ||
| for sc.Scan() { |
There was a problem hiding this comment.
This introduces the baseline store and comparison as new shared toolkit machinery, but this repository only accepts shared code after equivalent implementations have been proven by duplication in two product instances and then extracted with minimal adaptation. The reviewed change contains the first implementation rather than an extraction, so prove it independently in two product servers before moving the duplicated code here.
AGENTS.md reference: AGENTS.md:L8-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining this one, with reasoning. The extract-after-duplication rule governs product-domain machinery that products implement (gateway, catalog, mcptest). The eval package is not that: it is deliberately product-agnostic, toolkit-native infrastructure established by #6 and the program design doc — it reads any server's wire surface and is inherited by every server for free, with no product-side implementation to duplicate first. baseline.go extends that eval loop (a --baseline compare over its own JSONL), so there is nothing to prove-by-duplication in two products. Happy to revisit if we decide the eval loop itself should move out of the toolkit.
| case rec.Score < prev.Score: | ||
| cmp.Regressions = append(cmp.Regressions, Regression{ | ||
| Model: rec.Model, ScenarioID: rec.ScenarioID, Kind: KindScoreDrop, | ||
| OldScore: prev.Score, NewScore: rec.Score, | ||
| }) |
There was a problem hiding this comment.
For records produced by Grade, Score is always either 0 or 1, so after the preceding prev.Score >= 1 && rec.Score < 1 case, this score-drop branch cannot be reached by a real evaluation. A scenario that was already failing can therefore lose additional correctness dimensions—for example, going from valid to invalid parameters—without the advertised score-drop gate noticing; compare the individual grading dimensions or store a genuinely fractional score.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 060ad1c: added a dimension-regression gate — an already-failing cell (score stays 0) that loses tool/action/params true->false now gates. The fractional score-drop branch is kept for the future judge layer, with a comment noting it is unreachable under today's binary grader.
…racy - eval-smoke now runs a PINNED corpus (testdata/scenarios/fake.json), so the committed golds are graded against the current fake catalog: a renamed action, an added required param, or a changed enum invalidates a gold and fails as a newly-failing regression (nonzero exit). Regenerating the corpus each run let the oracle drift with the schema and gated nothing. Verified: renaming a fake action fires the gate. - Resolve the server argv as a slice (serverFields) instead of joining the PATH-resolved binary into a string and re-splitting it, so a binary under a directory with spaces still spawns. Explicit --server-cmd / EVAL_<PRODUCT>_CMD are still tokenized (quotes honored). - --baseline help now names safety regressions; README fizzy row lists the domains its corpus actually samples (boards, cards, columns, comments, steps, users).
- Load --baseline BEFORE creating the --out file. When they name the same path (compare-then-overwrite, or the default out coinciding with the baseline), os.Create truncated the baseline first, so the comparison read the just-written run and reported no regression while destroying the prior results. Verified: same-file run now detects the regression. - Add a dimension regression: today's grader scores a binary 0/1, so an already-failing cell that loses a correctness dimension (tool/action/ params true->false) kept score 0 and slipped past the score compare. Such a cell now gates. The fractional score-drop branch stays for a future judge layer. - Fix the README HEY build command (the product module is absent from the toolkit graph; build from the product checkout).
There was a problem hiding this comment.
💡 Codex Review
Line 124 in 060ad1c
Preserve a same-path baseline when the run regresses
When --baseline and --out resolve to the same file, a failing run is written over the known-good baseline before CompareToBaseline returns errRegression; the next invocation then compares against the regressed data and can pass, while the prior results are lost. Fresh evidence in this revision is that the baseline is now loaded before this call, fixing self-comparison, but os.Create still truncates it before the regression decision; compare first and preserve the file on failure, or reject identical paths.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Model: rec.Model, ScenarioID: rec.ScenarioID, Kind: KindDimension, | ||
| Detail: dim, OldScore: prev.Score, NewScore: rec.Score, | ||
| }) | ||
| } else if rec.Score > prev.Score { |
There was a problem hiding this comment.
When an already-failing cell fixes one correctness dimension but still fails another—for example, tool_match changes from false to true while action_match remains false—both scores remain 0, so this condition does not add the cell to Improved and the comparison reports no change. Since regressions at an unchanged score are handled dimension-by-dimension, the symmetric improvement case also needs to inspect those dimensions for the advertised comparison report to be accurate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d4877f6: added dimensionImproved as the mirror of dimensionRegressed, so an already-failing cell that fixes one correctness dimension (e.g. tool_match false->true) now appears in the non-gating improvement report. Improvements still never gate.
| // same-cell drop — surfaced, not silently dropped. | ||
|
|
||
| // baselineKey identifies one comparable cell. | ||
| func baselineKey(model, scenarioID string) string { return model + "\x00" + scenarioID } |
There was a problem hiding this comment.
When a pinned corpus edits nl_framing, gold parameters, or readonly_framed while retaining its scenario_id, this key treats the changed task as the same comparison cell. A model failure caused by the new framing can therefore be reported as a regression even though corpus edits are documented as non-gating, while two successful but materially different tasks can be reported as unchanged; persist and key on a scenario-content fingerprint, or reject records whose scenario identity metadata differs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this is exactly the next backlog increment — scenario-set / three-SHA rows: stamping a scenario-content (and catalog/SDK) fingerprint on each record so a same-id-but-edited task is keyed as a distinct cell (and a catalog-driven regeneration floats to its layer). It needs a new field on Record, so I'm keeping it out of this PR rather than half-adding it. In the intended workflow it doesn't bite: you compare runs of the same committed corpus (the smoke pins it), and the corpus is regenerated only on deliberate catalog drift. Tracked as the follow-up.
| if *baseline != "" { | ||
| bf, err := os.Open(*baseline) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| base, err = eval.LoadBaseline(bf) |
There was a problem hiding this comment.
When --backend api or cli is used with a missing, unreadable, or malformed --baseline, the entire model run has already completed before this open is attempted, so the command incurs every model call and only then exits without producing the requested comparison. Loading and validating this input immediately after flag parsing would fail before any paid work or server startup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d4877f6: --baseline is now loaded and validated right after flag parsing, before the server spawns or any model call — a missing/unreadable/empty baseline fails immediately, not after a billable run. Verified with a nonexistent path (exits before the run).
| if err := sc.Err(); err != nil { | ||
| return nil, err | ||
| } | ||
| return b, nil |
There was a problem hiding this comment.
When the supplied baseline file is empty, LoadBaseline returns a valid baseline with zero cells; the subsequent comparison classifies every current record as added, HasRegression remains false, and the merge-blocking command exits successfully without comparing anything. An explicitly requested baseline with no records cannot provide a regression signal and should be rejected as invalid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d4877f6: LoadBaseline now errors on zero records, so an empty or truncated baseline fails loudly instead of vacuously passing the gate (every cell would otherwise be 'added').
- Load and validate --baseline right after flag parsing, before spawning a server or making any paid model call, so a missing/unreadable/empty baseline fails immediately instead of after a full billable run. - Reject an empty baseline: a file with zero records would classify every cell as "added" and pass a merge gate vacuously, silently disarming it. - Report dimension-only improvements symmetrically with the dimension regression gate, so the (non-gating) improvement list is accurate when an already-failing cell fixes one correctness dimension.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4877f66f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err := json.Unmarshal([]byte(line), &rec); err != nil { | ||
| return nil, fmt.Errorf("decode baseline record: %w", err) | ||
| } | ||
| b.cells[baselineKey(rec.Model, rec.ScenarioID)] = rec |
There was a problem hiding this comment.
When a baseline contains syntactically valid but structurally invalid JSON such as {}, unmarshalling succeeds and inserts a record under the blank model/scenario key. That makes len(b.cells) nonzero, so the empty-baseline check is bypassed; every real result is then classified as added and the regression gate exits successfully. Validate required record fields before inserting each baseline cell so a damaged or wrong JSONL file cannot silently disable the gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e0d1ff8: LoadBaseline now rejects any record missing model or scenario_id at parse time, so a stray {} or truncated line fails loudly instead of inserting a blank-key cell and slipping past the empty-baseline guard. Test added.
A syntactically valid but identity-less line (a stray {} or a truncated
record) unmarshalled to a blank model/scenario_id, inserted under a blank
key, and made len(cells) nonzero — bypassing the empty-baseline guard so
every real result classified as "added" and the gate passed vacuously.
Reject any baseline record missing model or scenario_id at parse time, so
a corrupt JSONL fails loudly instead of silently disarming the gate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0d1ff805a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| prev, ok := base.cells[key] | ||
| if !ok { | ||
| cmp.Added = append(cmp.Added, rec.Model+"/"+rec.ScenarioID) | ||
| continue |
There was a problem hiding this comment.
When the current run has an empty scenario file or no (model, scenario_id) overlap with the baseline—for example, the baseline contains haiku but the run accidentally uses another label—every baseline cell is classified as removed and every current cell as added, so HasRegression() remains false and the command exits successfully without performing any comparison. Reject a comparison with zero matched cells, just as LoadBaseline now rejects an empty baseline, to prevent silently disabling the gate.
Useful? React with 👍 / 👎.
| $(GO) run ./eval/cmd/eval --server fake --backend oracle \ | ||
| --scenarios eval/testdata/scenarios/fake.json \ | ||
| --out /tmp/eval-smoke.jsonl \ | ||
| --baseline eval/testdata/results/fake-oracle.jsonl |
There was a problem hiding this comment.
After the pinned-corpus fix, the workflow in .github/workflows/eval.yml still passes when a fake action only loses its ReadOnly or Idempotent annotation. The oracle returns the same pinned gold, Grade only uses the live Destructive bit for its annotation check, and the record's class comes from the pinned scenario, so both runs retain score 1 and this target exits zero despite the safety metadata regression. Compare the live annotations with pinned metadata or include them in a catalog fingerprint.
Useful? React with 👍 / 👎.
| } | ||
| // An explicitly requested baseline with no records cannot provide a | ||
| // regression signal: comparing against it would classify every cell as | ||
| // "added" and pass a merge gate vacuously. Reject it so an empty or |
There was a problem hiding this comment.
If a baseline contains the same (model, scenario_id) more than once—for example, because two runs were concatenated into the documented append-only JSONL store—this assignment silently keeps only the last record. A later failing duplicate can overwrite an earlier passing result, allowing the same failure in the current run to compare equal and pass the gate. Reject duplicate keys, or define and validate an explicit run-selection policy, rather than silently discarding baseline records.
Useful? React with 👍 / 👎.
Stacked on #6 (
eval-loop-v0). Delivers the ranked top of the hillclimb backlog — 2nd & 3rd servers — plus the catch-regressions-fast lever, both on the unchanged eval package.Multi-server (backlog #1)
Product-specific launch details move into a server registry in the eval command (
cmd/eval/main.go), so a new server is one map entry, not code.fakefizzyfizzy-mcp stdio --writesheyhey-mcp stdiobasecampbasecamp-mcp stdiohey lands as a real second product server with a committed corpus (
testdata/scenarios/hey.json, generated straight from hey's own describe surface — reads/writes/idempotent/destructive across four domains) and a real haiku run: 12/12 pass, $0.0183 (results/hey-v0.jsonl). Zero eval-package changes — that is the proof of product-agnosticism.Honest basecamp boundary:
basecamp-mcp's stdio authenticates eagerly (it fetchesauthorization.jsonbefore serving the transport), so unlike fizzy/hey it cannot list+describe hermetically. It is wired into the registry as a credentialed/--livetarget and marked non-hermetic; making it hermetic is the cassette player (hillclimb #2).Regression gate (backlog #4/#5)
--baseline <prior.jsonl>compares a fresh run cell by cell, keyed on(model, scenario_id):make eval-smokenow gates the hermetic fake+oracle run against a committed baseline (testdata/results/fake-oracle.jsonl), so CI catches a shifted catalog surface at $0.The runs
Tests / CI
Hermetic, $0. New unit tests cover the baseline comparison (each regression kind, per-model keying, corpus edits never gating) and the server registry (env override, dummy-token injection that never overwrites a real token, hermeticity flags).
make vet test-race buildgreen;go mod tidy -diffclean (no new deps).Do not merge — shared toolkit, human merge.