fix(e2e): harden the scorecard per parallel review - #183
Conversation
…e parsers The three lane parsers and the scorecard each re-derived boolean, number, text, nullable, and count-record checks, and three of them restated the seven-key system tuple by hand. Two copies had already drifted: one count record accepted zero where another required one, and one interval parser rejected inverted bounds where another did not. A tuple field added in one place would have gone unnoticed in the others, and the tuple is compared by fingerprint as a build identity. Move the shared checks into the contract primitives and parse the system tuple through one function that also bounds each field to the shape the runner writes, so a lane string with prose in it cannot reach a report. The paired-delta parser now admits only the endpoint partition the estimator writes into each list and rejects duplicate estimate keys, and report builders sort by code unit rather than locale so identical runs fingerprint identically on every host.
…every guard with a test Review found three ways evidence the policy rejected could still shape the report, and a set of report guards no test exercised. A paired-delta or identity-projected lane that ran under other settings or another build is a different experiment, so its report is dropped rather than kept for reliability counts; only a run that did not finish keeps its counts. The holdout adapter now requires the paired-delta lane to be present before certifying paired facts. Baseline comparison keys on the (endpoint, family) pair, so an estimate that vanishes at one endpoint is a blocking family-missing row instead of no row. Observed lane identities are admitted through the policy's own parser, retrieval slots read one scored holdout population, and parity counts only completed cases. The shared privacy allowlist no longer exempts generic key names; the retrieval release-fingerprint record is exempted by its parent key so other artifacts keep the hash check. Tests now drive the production outcome deriver to an allowed promotion at the tolerance boundary, mutate every cross-field guard of the report parser, prove the loader reads no lane file before freeze binding, and cover a failed gate, a non-comparable baseline, and the remaining pre-registration checks.
Remove duplicated scorecard helpers and dead exports so each shared behavior has one owner: one factory builds the contract error classes, interval parsing and closed vocabularies live beside the types they describe, and the operator script relies on node:util for flag parsing. Make the test selection fail when the script test is missing instead of filtering the gap out of the assertion meant to catch it.
|
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: 0f10498970
ℹ️ 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".
| for (const [innerKey, inner] of Object.entries(child)) { | ||
| if (typeof inner === "string") scanString(inner, `${path}.${segment}.${innerKey}`, forbidden, violations, true); |
There was a problem hiding this comment.
When an artifact contains releaseFingerprints, this special branch scans only each property's value and never scans innerKey, unlike the normal object traversal. Consequently, sensitive names such as {"releaseFingerprints":{"/home/alice/secret":"safe"}} produce no privacy violation, and if the value itself triggers a violation its diagnostic path can expose the unredacted key. Apply the usual key scan/redaction before scanning each direct fingerprint value.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let values: Partial<Record<keyof typeof OPTIONS, string>>; | ||
| try { | ||
| values = parseNodeArgs({ args: [...argv], strict: true, allowPositionals: false, options: OPTIONS }).values; | ||
| } catch (error) { |
There was a problem hiding this comment.
Bug: duplicate-flag rejection silently dropped by the node:util.parseArgs migration.
parseNodeArgs is called without multiple: true for any option, and Node's parseArgs treats a repeated string option as last-value-wins, not an error — strict: true only guards unknown options/type mismatches. So run-scorecard --baseline a.json --baseline b.json (or any other flag given twice) now silently resolves to the last value instead of throwing, in a script that gates releases.
The old behavior was covered by a test asserting /given twice/ on --out twice — it was deleted in this PR rather than replaced with an equivalent check under the new parser, so the regression has no test coverage.
Consider validating argv for duplicate flags before/after calling parseNodeArgs, or accepting this as an intentional behavior change (and documenting it in the PR description if so).
| function scoredHoldoutScenarios(report: BenchmarkReport, mode: QueryMode): BenchmarkReport["evidence"]["scenarios"] { | ||
| const laneRestricted = new Set(report.evidence.cases.filter((entry) => entry.laneRestricted).map((entry) => entry.caseId)); | ||
| return report.evidence.scenarios.filter((scenario) => | ||
| scenario.partition === "holdout" && scenario.mode === mode && !laneRestricted.has(scenario.queryId.split(":", 1)[0]!)); |
There was a problem hiding this comment.
Duplicated exclusion logic risks drift. scoredHoldoutScenarios re-implements the "exclude lane-restricted case scenarios" filter that already exists in aggregateReportQuality() (packages/plugin/scripts/retrieval-benchmark/report.ts, ~line 278). Both compute the same laneRestricted set from report.evidence.cases and filter evidence.scenarios on it, but as two separate implementations. If the exclusion rule changes (e.g., what counts as "lane-restricted", or how queryId is parsed to a case id), it's easy to update one and miss the other, silently producing an inconsistent duplicate-rate-at-50 reading vs. the other retrieval aggregates.
Worth factoring into one shared helper that both call sites use.
Review summaryReviewed the diff for this scorecard-hardening PR (30 files: shared parsing primitives extraction, correctness fixes to lane/comparison logic, and complexity reduction). The refactor is internally consistent — traced the de Morgan's-law rewrites in Two findings posted inline, most-severe first:
No security issues found (the privacy-allowlist narrowing in |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if (measured !== (row.endpoint !== null && row.noiseLabel !== null && row.delta !== null && row.interval !== null)) { | ||
| fail(`${label}: shape-invalid`); | ||
| } | ||
| if (measured !== (row.noiseLabel !== null && row.delta !== null && row.interval !== null)) fail(`${label}: shape-invalid`); |
There was a problem hiding this comment.
WARNING: Partial non-null measured fields bypass validation for family-missing rows
When measured is false (row.kind === "family-missing"), the condition (row.noiseLabel !== null && row.delta !== null && row.interval !== null) checks only whether all three fields are simultaneously non-null. If 1 or 2 fields are non-null (for example, delta: -0.5 while interval: null and noiseLabel: null), the compound conjunction evaluates to false, causing measured !== false to evaluate to false !== false and pass validation.
For family-missing rows, all three measured fields must be null, whereas for adverse-interval rows all three must be non-null.
| if (measured !== (row.noiseLabel !== null && row.delta !== null && row.interval !== null)) fail(`${label}: shape-invalid`); | |
| if (measured ? (row.noiseLabel === null || row.delta === null || row.interval === null) : (row.noiseLabel !== null || row.delta !== null || row.interval !== null)) fail(`${label}: shape-invalid`); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| fail("report.body.outcome.blockingRegressionCount: cross-field-invalid"); | ||
| } | ||
| if (body.outcome.blockingRegressionCount !== blockingCount(body.adverseDeltas)) fail("report.body.outcome.blockingRegressionCount: cross-field-invalid"); | ||
| unique(body.adverseDeltas.map((row) => `${row.kind}:${estimateKey(row)}`), "report.body.adverseDeltas"); |
There was a problem hiding this comment.
WARNING: Adverse deltas uniqueness check allows duplicate entries across different kinds
Keying the uniqueness set on ${row.kind}:${estimateKey(row)} permits two adverse rows for the exact same (endpoint, familyId) pair if one has kind: "adverse-interval" and the other has kind: "family-missing".
Because an estimate family at a given endpoint is either evaluated as an adverse interval regression or missing from the current release (not both), adverse rows should be unique on estimateKey(row) alone.
| unique(body.adverseDeltas.map((row) => `${row.kind}:${estimateKey(row)}`), "report.body.adverseDeltas"); | |
| unique(body.adverseDeltas.map(estimateKey), "report.body.adverseDeltas"); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (30 files)
Fix these issues in Kilo Cloud Reviewed by gemini-3.7-flash · Input: 519.1K · Output: 26.7K · Cached: 1.2M |
1 participant
Summary
Stack 6/6 of the release scorecard. Findings from three parallel reviews (
invariant-test-review,typescript-code-reviewer,reduce-complexity) and a finalponytail-review, applied as three commits.Shared parsing (
refactor):boolean/finiteNumber/text/textArray/nullable/countRecordmove intocontract-primitives; oneparseSystemVersionTuple(src/historian-eval/system-tuple.ts) serves the historian, metamorphic, and scorecard parsers and bounds each tuple field to the shape the runner writes. The paired-delta parser admits only the endpoint partition the estimator writes into each list and rejects duplicate estimate keys. Lane builders sort by code unit rather thanlocaleCompareso identical runs fingerprint identically on every host.Correctness (
fix):incomplete, no numbers); only a run that did not finish keeps its counts.present.(endpoint, family); an estimate that vanishes at one endpoint is a blockingfamily-missingrow.releaseFingerprintsis exempted by parent key.deriveOutcometo an allowed promotion at the tolerance boundary, mutate every cross-field guard of the report parser, spy onreadFileSyncto prove no lane read precedes freeze binding, and cover a failed gate, a non-comparable baseline, and every pre-registration disjunct.Complexity (
refactor): one factory for the three contract error classes, single owners for interval parsing and closed vocabularies,node:util.parseArgsin the script, dead exports removed.Unapplied review findings
comparison.ts:no-noise-flooradverse rows are non-blocking per plan R15. UndernoiseFloorSource: "none"every regression is therefore non-blocking. Kept as the plan specifies; worth a policy-owner decision.workingDirectory). Follow-up tasks below.createScorecardAdapterhas no production caller until a recomputers module exportsscorecardforscripts/prospective-holdout.ts.Follow-up beads tasks (epic
magic-context-x4l)Filed after merge review: gate probes for the four unproduced gates; historian hard-negative family beside
falseAuthoritativeMatches; build identity on incident-pool and dreamer reports; dreamer and retrieval scannable summary artifacts; release workflow collecting lane artifacts; tolerant baseline reader; remaining metric producers.Testing
bun run --cwd packages/e2e-tests typecheck,bun run --cwd packages/plugin typecheckbun run test:scorecard-unit(62),test:prospective-unit(125),test:paired-delta-unit(300),test:historian-eval-unit(301),test:metamorphic-unit(197),test:dreamer-eval-unit(165)bun test packages/plugin/scripts/retrieval-benchmark(239),bun test packages/e2e-tests/scripts/run-test-selection.test.tsStack
Depends on #182 and completes the stack.