ci(email): derive the scorecard-refresh budget from measured times, guard the published basis#2107
Conversation
…add basis guard (#2094) email_scorecard_refresh.yml has never completed a single run (0/100): timeout-minutes: 90 was a hand-typed guess against a measured ~2h workload (run 29433392568), and lowering the sample count never helped since the three fixed-size judge evals alone cost more than the benchmark. Rewrites the workflow around two workflow_dispatch profiles: - full (limit>=249, experiments>=committed n_runs): reproduces the committed card's 250x3 methodology; the only profile allowed to commit. - subset (limit<249): an end-to-end smoke/dry-run that never commits. Every long step now carries its own timeout-minutes derived from the measured table in the job comment, so an overrun fails at a named step instead of a silent job-level cancelled. A pre step reads the committed card's ctx_size/n_runs stamp and fails fast (before any eval spend) on a basis mismatch unless rebaseline=true names a reason. The push: trigger is removed (0/100 successes means it never provided real coverage). Promotes scorecard_gate._env_ctx_size to a public env_ctx_size so the pre step can read the committed ctx stamp without re-deriving the recipe.environment.ctx_size path, with a unit test for the legacy unstamped-card case (the real current state of the committed card).
The header claimed test_email_agent_eval.yml was "red as of #2093". That attribution is wrong: #2093 was the FIX for the specification.html drift guard (54837cd, already on main), not a cause of the weekly's failures. The weekly is red -- its last scheduled run (2026-07-13, 29235382831) failed -- but that predates #2093 and the cause is unestablished. State the observation, not an unverified cause.
…s.create sites (#2094) Adds failing tests for ClaudeClient's new temperature kwarg: constructor storage, all six messages.create call sites (get_completion, get_completion_with_usage, analyze_file HTML/binary, analyze_file_with_usage text/binary) forwarding temperature only when set, and the three make_claude_judge factories (action_item_quality, briefing_quality, draft_quality) pinning temperature=0.0.
The eval judges graded at the Anthropic API's default temperature because ClaudeClient never sent one. With generation already pinned at temp 0, the judge was the last uncontrolled variance source in the scorecard -- a tripwire that can flip on the grader's own sampling is not a tripwire. ClaudeClient now takes an optional temperature, forwarded to all six messages.create sites only when set, and the three make_claude_judge factories pass 0.0. Left as None the kwarg is omitted entirely, so today's behavior is preserved exactly for callers that never asked for a pin -- pdf_document_generator constructs ClaudeClient to *generate* synthetic fixture content, where temp-0 would collapse output diversity. That is why the pin is judge-scoped rather than a class default. Docs: eval-scorecard.mdx and EVALUATION.md still described the refresh workflow as push-triggered with a binary commit/fail outcome; both now describe the dispatch-only full/subset profiles and the basis guard.
github-actions
Bot
added
documentation
Evidence 3 — subset end-to-end: the incident, re-run against this fixRun 29461875126 — dispatched
Measured step times vs the ceilings this PR derives
Every step landed inside its derived ceiling, with the judge legs (37:31) dominating as predicted — they are fixed-size and do not shrink with Anomaly worth its own issue (not introduced here, not fixed here): the briefing judge finished in 0:39 against a ~11-15 min estimate — and it did the same on the incident run (0:36, 29457164041). Two independent runs agreeing suggests the briefing leg isn't performing a real judged evaluation, which would mean one of the three judged metrics on the published card is not what it claims. Flagged on #2094; out of scope for this PR. Evidence 4 (full-corpus, AC-1's terminal step) is running — 29464281335, dispatched |
kovtcharov
left a comment
kovtcharov
left a comment
There was a problem hiding this comment.
Verified locally on the branch: the two new test suites pass (64 tests — judge-client hardening + gate basis checks). The design earns the approve on its own: the basis guard runs pre-spend and fails naming both bases + the exact re-dispatch command; SUBSET runs are structurally unable to commit (gates skipped by if:, card restored, loud notice) rather than trusted not to; rebaseline requires a stated reason; and dispatch against main is refused. This closes exactly the gap flagged in the #1900 closure caveats — the gate compared only the number, never the measurement basis, and a lucky 25-email run proved it two hours ago. The honest HISTORY comment (0/100 completions, per-limit breakdown from a real run) is the kind of archaeology that keeps the next editor from re-typing a hand-guessed timeout.
Two non-blocking notes: (1) the FULL profile's ~10.5h wall-clock on the single serial lemonade-eval slot means a full refresh monopolizes eval CI for a workday — the slot-occupancy warning covers it, but consider a scheduled monthly full run so refreshes don't rely on someone remembering the etiquette; (2) the workflow itself is only provable post-merge via dispatch — recommend a SUBSET dry-run as the first action after merging, before any FULL run.
Evidence 4 — full-corpus run: AC-1 satisfied via the commit pathRun 29464281335 — I predicted this would reject; it did not, and the correction is worth stating plainly. I expected ~0.796 against the 0.80 floor. That figure came from a single-run measurement. The proper 3-run full-corpus measurement at 16K is:
So moving the card onto the production envelope did not lower the headline — it slightly raised it and made it traceable. This also resolves the open worry from #2090 that 16K might be sub-bar for triage: at n=3 it is 0.8453, comfortably clear. The re-baselined card now rides in this PR's diff ( Briefing-judge anomaly reproduced a third time: 41s on this full run (vs 39s subset, 36s incident). Three independent runs agree, so the briefing metric on the card is almost certainly not a real judged evaluation — the re-baselined card carries that suspect report-only value forward. It does not affect the gated aggregate (which comes from the triage benchmark, not the judge), but it needs its own fix. Flagged on #2094. |
|
Verdict: Approve with suggestions This turns the email scorecard-refresh workflow from "reliably dies / reliably publishes a lucky number" into a manual-dispatch job with a fail-fast basis guard, per-step timeouts, and a full/subset split where only a full-corpus run can ever commit a card. The judge-temperature pin and the guard logic are well-designed and thoroughly tested — the core change is sound. Two non-blocking things worth a look before this ships:
🔍 Technical details🟡 Important1. Subset benchmark timeout is under-derived —
Using the PR's own full-profile rate (3 × 249 × 45s ≈ 560 min ⇒ ~187 min/experiment for 249 emails), 25 emails ≈ ~19 min per experiment × 3 ≈ ~56 min — over the 45-min step ceiling (and it feeds the 240-min job budget on line 201, also derived at or, better, force 2. Published SCORECARD.md ships Windows-backslash paths + a stale "whole corpus" claim ( The card was generated on the Separately, Neither blocks merge, but both degrade a card whose whole purpose is to be the honest, reproducible published figure. Strengths
|
|
Follow-ups filed from this PR's validation, both deliberately out of scope here: #2121 (briefing judge ~40s / likely not judging — its number ships on the card) and #2122 (six workflows share the stx runner outside |
3 participants
Closes #2094
Why this matters
The email agent's published quality card — the accuracy figure integrators rely on — is refreshed by an automated workflow that was supposed to reject any change making quality worse. Two hours ago that workflow did the opposite: it auto-committed a card claiming 100% accuracy measured on 25 emails in a single run, silently replacing the real figure (83.4%, measured over 249 emails, repeated 3 times). Its regression check approved the swap because 1.0 beats 0.834 — it compares only the number, never the basis the number was measured on, so a small lucky sample reads as an improvement. That card is sitting on an open, merge-ready PR right now (flagged in #2060);
mainis still clean.That incident reframes this issue. #2094 was filed because the workflow had never once completed — a hand-typed
timeout-minutes: 90against a ~2h workload. #2089's interim limit cut (60→25) made it fast enough to finish, which means the failure mode changed rather than went away: a workflow that reliably died became one that reliably publishes a meaningless number. The loud failure was the safer bug.After this PR: a subset-sized run physically cannot commit a card, a run that would downgrade the published basis (fewer emails, fewer repeats, or a different context window) is rejected in seconds before spending any eval time, a sub-bar card can never reach the published surface, and an overrun fails at a named step instead of a silent job-level
cancelled. The per-push trigger — which fired the incident above, unasked, from an unrelated refactor branch mergingmain— is gone; refresh is now a deliberate dispatch, and the budget is derived from measured step times with the arithmetic recorded in the workflow.Evidence
1. The bug this prevents, firing in production — run 29457164041 (old workflow,
main's version) auto-committed2e21edb9:Under this PR that run is a SUBSET profile: it evaluates, prints
SUBSET RUN -- card NOT committed, restores the card, and exits 0.2. Basis guard rejects a downgrade before spending eval time — run 29458595361, dispatched
limit=250 experiments=3 ctx_size=16384 rebaseline=false:Failed at the named step
Resolve version, committed basis, and fail fast on a basis mismatchin 6 seconds — the whole run cost ~2 minutes instead of ~2 hours. This is AC-3 (loud, step-named failure) and the AC-4 guard in one artifact.3. Subset end-to-end + 4. full-corpus terminal step — dispatched; posted as a follow-up comment.
Budget derivation (AC-2) — measured, not guessed
Ceilings derive from run 29433392568 and are recorded in the workflow header beside the arithmetic. The judge legs are fixed-size (19/21/11 cases) and don't shrink with
limit, so they dominate a subset run: tonight's 50-minute run spent 34:25 of it in judges.The old value, 90, was never derived from a measurement. The job timeout is now only a backstop — every long step carries its own ceiling so an overrun names itself.
Tests
python -m pytest tests/unit/eval/ -q→ 471 passed, 1 deselected (pre-existing worktree.envfailure, reproduced identically atorigin/main)python -m pytest tests/unit/eval/test_claude_judge.py -v→ 28 passed; 18 new tests cover all 6messages.createsites across 4 public methods. Tests were authored against the acceptance criteria before the implementation existed, and pin the judge totemperature=0.0— with agent generation already at temp 0, the judge's own sampling was the last uncontrolled variance sourcepython util/lint.py --all→ Black / isort / Pylint / Flake8 OKactionlint→ 31→30 findings, zero new (remainder are pre-existing PowerShell-parsed-as-bash false positives already onmain). Static only — it cannot prove the ternaries evaluate correctly or that PS 5.1 survives the string encoding; the dispatched runs above are the real validationNotes for the reviewer
SCORECARD.mdin an unrelated PR bypasses every guard here.test_api,test_embeddings,test_lemonade_server,test_rag,build_cpp,test_examples) share thestxrunner label outside thelemonade-evalconcurrency group, and most runcleanup-lemonade.ps1, which force-kills port 13305 — so an unrelated PR can kill a long eval mid-run. Widening the group is the right fix and needs its own issue.scorecard_gate.pyis touched additively only:_env_ctx_size→ publicenv_ctx_size(+4 tests), because the guard must read the ctx stamp fromrecipe.environment, and re-deriving that path in workflow PowerShell is how this class of bug gets written twice. No gate semantics, thresholds, or flags changed.