feat(e2e): evaluate scorecard gates, score families, and baseline comparison - #181
feat(e2e): evaluate scorecard gates, score families, and baseline comparison#181ahrav wants to merge 1 commit into
Conversation
…parison A release reviewer must see every gate and every metric slot on every run, including the ones no lane can observe, or a missing probe reads as a passing one. These stages therefore never shorten their output. Gates come from one producer table with four row states: a gate without a producing lane is not-observed rather than omitted, and a throwing extractor becomes an errored row without copying its message. The injection gate reads metamorphic evidence only under complete pre-registered canary coverage. Score families always emit their full slot list: a lane that is missing or did not finish yields not-measured slots with a reason code, and an unfinished paired-delta lane still contributes its run-health and exclusion counts to reliability. Derived rates are arithmetic over counts the lane published. Baseline comparison shifts each current paired interval by the baseline point estimate, lists every wholly adverse pair, and blocks on outside-floor and family-missing rows.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![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: 481a602992
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const covered = new Set(lane.report.coverage.filter((entry) => entry.applied >= 1).map((entry) => entry.scenarioId)); | ||
| if (!bundle.policy.injectionCanaryScenarioIds.every((scenarioId) => covered.has(scenarioId))) { |
There was a problem hiding this comment.
When an applicable canary pair errors before producing a scored observation, coverage.applied is still positive, and the loader can retain the metamorphic lane as present whenever tierInvalidReason is null. The producer itself treats non-scored entries and coverage violations as failures in metamorphic-eval/report.ts, but this check ignores both and reports passed with zero hits despite never successfully observing the canary. Require scored, violation-free coverage for each policy canary before treating zero hits as a pass.
Useful? React with 👍 / 👎.
| const rates = report.evidence.scenarios | ||
| .filter((scenario) => scenario.partition === "holdout" && scenario.mode === mode) | ||
| .map((scenario) => scenario.metrics.duplicateRateAt50) | ||
| .filter((rate): rate is number => rate !== null); |
There was a problem hiding this comment.
For reports containing diagnostic lane-restricted cases, this path includes those scenarios in the release duplicate-rate metric, while the adjacent quality-metric path calls aggregateReportQuality, which explicitly removes cases whose CaseEvidence.laneRestricted is true. A high- or low-duplicate diagnostic case can therefore change only the duplicate-rate scorecard slots even though it is excluded from the report's gate aggregates; apply the same case-level exclusion before averaging.
Useful? React with 👍 / 👎.
| const currentFamilies = new Set(current.map((row) => row.familyId)); | ||
| const missingFamilies = [...new Set(baseline.familyEstimates.map((row) => row.familyId))] | ||
| .filter((familyId) => !currentFamilies.has(familyId)); |
There was a problem hiding this comment.
When the current paired-delta lane is missing or incomplete, buildScoreFamilies supplies an empty familyEstimates array; with a present baseline, this code consequently marks every historical family as a blocking family-missing regression. That turns an evidence-availability failure into fabricated adverse deltas and inflates blockingRegressionCount, even though the lane status already records the real failure. The comparison needs the current evidence status and should emit an appropriate limitation instead of missing-family rows unless a present run actually omitted the family.
Useful? React with 👍 / 👎.
| export function familyEstimateRows(report: PairedDeltaReport): FamilyEstimateRow[] { | ||
| return report.body.analysis.endpoints | ||
| .flatMap((estimate) => estimate.families.map((family): FamilyEstimateRow => ({ | ||
| endpoint: estimate.endpoint as FamilyEstimateRow["endpoint"], |
There was a problem hiding this comment.
A parsed paired-delta report can contain any DeltaEndpoint in body.analysis.endpoints because parseEndpointEstimates accepts DELTA_ENDPOINTS, and evidence conformance only requires that the configured primary endpoint appear somewhere. This cast therefore lets a validly parsed retrieval, formation, or representation endpoint escape as a FamilyEstimateRow, although the scorecard contract accepts only PRIMARY_ENDPOINTS; the generated scorecard will later fail its own parser. Filter or reject non-primary endpoints before constructing these rows rather than hiding the wider input type with a cast.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const currentFamilies = new Set(current.map((row) => row.familyId)); | ||
| const missingFamilies = [...new Set(baseline.familyEstimates.map((row) => row.familyId))] | ||
| .filter((familyId) => !currentFamilies.has(familyId)); |
There was a problem hiding this comment.
Bug: missing-family detection dedupes by familyId alone, not (endpoint, familyId)
The docstring above says pairing happens on the (endpoint, estimate family) key, and deltas is indeed keyed that way via estimateKey. But missingFamilies here only checks currentFamilies.has(familyId) — ignoring endpoint.
Concretely: baseline has fam-x at both mc-on-vs-mc-off and mc-on-vs-compaction. If the current release drops fam-x at mc-on-vs-compaction but still has it at mc-on-vs-mc-off, currentFamilies still contains fam-x, so no family-missing row is emitted — and the dropped pair is also absent from deltas (which only iterates current). A real per-endpoint regression (evidence silently vanishing for one endpoint) slips past blockingRegressionCount/promotionAllowed undetected.
Consider deduping missing families by estimateKey(row) (endpoint + familyId) instead of familyId alone.
| .filter((rate): rate is number => rate !== null); | ||
| return ratio(mean(rates), "no-holdout-queries"); | ||
| } | ||
| const aggregate = gateAggregates(aggregateReportQuality(report)).find((entry) => entry.mode === mode); |
There was a problem hiding this comment.
Efficiency: full macro-aggregation recomputed per slot
retrievalReading calls aggregateReportQuality(report) + gateAggregates(...) from scratch on every invocation. laneSlot calls this reader once per retrieval MetricSlotId, and 8 of the 14 retrieval ids (recall-at-10/50, reciprocal-rank, ndcg-at-10 × explicit/automatic) hit this branch — so the full scenario-grouping/aggregation pass over the report reruns 8x per scorecard build for the same input. Since the result depends only on report (not id), consider hoisting it out — compute once in buildScoreFamilies/section and pass it into the reader, or memoize per-report.
| return ratio(mean(rates), "no-holdout-queries"); | ||
| } | ||
| const aggregate = gateAggregates(aggregateReportQuality(report)).find((entry) => entry.mode === mode); | ||
| if (aggregate === undefined) return { reason: "no-holdout-queries" }; |
There was a problem hiding this comment.
Misleading reason code when metric is null but holdout queries exist
This only handles the case where no aggregate exists for the mode at all. But aggregate can be found while a specific metric on it (e.g. recallAt10) is null — meanOrNull in metrics.ts returns null when no judged-relevant documents exist within the cutoff, even though queryCount/groupCount are non-zero. That path falls through to ratio(value, "no-holdout-queries") below (line 131), so it reports reason: "no-holdout-queries" even though holdout queries did exist — just none had relevant judgments. Worth a distinct reason code (e.g. no-relevant-judgments) so consumers of reason can tell the two failure modes apart.
| } | ||
|
|
||
| export function hardGateFailures(rows: readonly GateRow[]): GateId[] { | ||
| return rows.filter((entry) => entry.status !== "passed").map((entry) => entry.gateId).sort(); |
There was a problem hiding this comment.
Duplicated logic, currently unused outside its own test
This is the same computation report-contract.ts's deriveOutcome already does inline: input.gates.filter((row) => row.status !== "passed").map((row) => row.gateId).sort() (report-contract.ts:332), and parseScorecardReport's cross-field check re-derives it a third time (report-contract.ts:401). Right now hardGateFailures here isn't imported anywhere outside gates.test.ts.
If the definition of a "hard" gate failure ever changes (e.g. treating errored differently from not-observed), it's easy to update one copy and miss the others, producing a report whose safetyGates rows disagree with outcome.hardGateFailures. Worth having deriveOutcome call this exported helper instead of recomputing it, or dropping the export if it's not meant to be the shared implementation yet.
Review summaryReviewed the three new scorecard modules (
No security concerns identified; this is internal eval tooling with no untrusted input surface in the diff. |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Reviewed by gemini-3.7-flash · Input: 298.3K · Output: 16.5K · Cached: 620.5K |
1 participant
Summary
Stack 4/6 of the release scorecard. Three pure stages over the evidence bundle.
gates.ts:GATE_SOURCES: Record<GateId, Extractor | null>;nullyieldsnot-observed/no-producing-lane. Five rows always, in fixed order, four states. A throwing extractor becomeserrored/extractor-threwwithout copying the message.gate-injection-promotedreads metamorphic evidence only when every pre-registered canary scenario has an applied transform. No exemption path.families.ts: five fixed score families, every slot emitted every run asmeasuredornot-measuredwith a reason (lane-missing,lane-incomplete,producer-pending, ...). Derived rates are arithmetic over published counts (false-authoritative memory rate = summed matches / summed visible claims; duplicate rate = mean per-scenarioduplicateRateAt50over holdout). An unfinished paired-delta lane still contributes run-health and exclusion counts to reliability.comparison.ts: pairs each current(endpoint, estimate family)with the baseline scorecard's row;delta= current − baseline point, interval = current paired interval shifted by the baseline point. Wholly adverse pairs are listed;outside-floorandfamily-missingrows block. No baseline → every deltano-baselinewith its absolute value plus a limitation.Testing
bun run test:scorecard-unit(39 passed at this head), incl. AE3/AE4/AE5/AE6/AE7 from the plan and permuted-input byte stability.Stack
Depends on #180. Next: #182.