feat(e2e): assemble the scorecard report, holdout adapter, and operator script - #182
feat(e2e): assemble the scorecard report, holdout adapter, and operator script#182ahrav wants to merge 1 commit into
Conversation
Make scorecard generation reproducible from one evidence bundle so the release reviewer and the prospective-holdout validator reach the same decision from the same bytes. Promotion allowance is computed inside the builder from gate rows, lane statuses, required slots, and blocking regressions; no caller supplies it, and the builder validates its own output through the wire parser before returning it. The prospective-holdout seam is filled by an adapter whose result fingerprint pins every input that shaped the outcome, so substituting a lane report or a paired fact changes the recorded fingerprint. The operator command refuses without writing when the policy is not frozen or the report fails the privacy scan, and otherwise exits by the report's own outcome. The README states that the first release-grade run exits 2 until the four unproduced gate probes exist.
|
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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| export function scorecardExitCode(report: ScorecardReport): ScorecardExitCode { | ||
| if (report.body.outcome.hardGateFailures.length > 0) return 2; | ||
| return report.body.outcome.promotionAllowed && report.body.evidence.baseline.status !== "schema-mismatch" ? 0 : 1; |
There was a problem hiding this comment.
report.body.outcome.promotionAllowed (computed by deriveOutcome in report-contract.ts) never accounts for baseline comparability — it's derived purely from gates, lanes, families, and adverse deltas. scorecardExitCode bolts a baseline.status !== "schema-mismatch" check on top here, but that check lives only in this function, not in the outcome itself.
Concretely: if the policy pins a baselineScorecardReportFingerprint but the baseline file is corrupt/unreadable, loadBaseline returns status: "schema-mismatch". compareWithBaseline then takes the baseline.status !== "present" branch, returning adverseDeltas: [] (so blockingRegressionCount stays 0), and with all gates passed and lanes present, deriveOutcome computes promotionAllowed: true.
run-scorecard.ts still exits 1 correctly because of the special case here, but createScorecardAdapter.evaluate() (line 104 below) reads report.body.outcome.promotionAllowed directly and has no equivalent baseline-comparability guard — so the prospective-holdout pipeline could reach decision: "promote" off a report whose baseline was never actually comparable. This schema-mismatch baseline path also isn't covered by any test in report.test.ts.
Worth considering: fold the baseline-comparability check into deriveOutcome itself (e.g. pass baseline status in) so promotionAllowed is correct everywhere it's read, rather than re-derived ad hoc per caller.
| if (policyFingerprint !== bundle.policyFingerprint) throw new ScorecardContractError(["adapter: policy-fingerprint-mismatch"]); | ||
| const pairedDelta = laneEvidence(bundle, "paired-delta"); | ||
| const pairedFacts = pairedFactsFingerprint(holdout.pairs); | ||
| if (pairedDelta.report === null || pairedDelta.report.body.analysis.pairedFactsFingerprint !== pairedFacts) { |
There was a problem hiding this comment.
evaluate() throws an uncaught ScorecardContractError("adapter: evidence-pairs-mismatch") whenever the paired-delta lane report is simply null — which conflates two very different situations: genuine tampering (facts don't match a present paired-delta report) vs. the paired-delta artifact being legitimately absent/incomplete (a normal LaneEvidence status the rest of the bundle already models gracefully via laneEvidence/mandatoryEvidenceComplete).
buildProspectiveReport (prospective-holdout/report.ts:133) calls input.scorecard.evaluate(...) with no try/catch, so once this adapter is wired into the real release flow, a missing paired-delta artifact crashes prospective report generation entirely instead of yielding the insufficient-evidence/hold decision the rest of the pipeline is designed to express for exactly this case.
Only report.test.ts exercises the throw, and only for the true-mismatch case (PAIRED_FACTS.slice(1) against a present paired-delta report) — there's no test for pairedDelta.report === null (lane missing/incomplete) reaching this adapter.
Might be worth distinguishing: if pairedDelta.report === null, return an outcome reflecting the report's own (already-correct) mandatoryEvidenceComplete: false rather than throwing, and reserve the throw for when a report is present but its pairedFactsFingerprint doesn't match.
Review summaryReviewed the scorecard report/adapter/CLI stack (
No security issues found beyond the existing privacy-scan gate, which looks correctly wired ( |
![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: 34cc3a237c
ℹ️ 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".
| evidence: { lanes, baseline: { status: bundle.baseline.status, reportFingerprint: bundle.baseline.reportFingerprint } }, | ||
| outcome, |
There was a problem hiding this comment.
When the policy names a baseline but --baseline is omitted, unreadable, or fingerprint-mismatched, loadBaseline returns schema-mismatch, yet deriveOutcome does not receive that status and can still set promotionAllowed to true. The CLI happens to return 1, but createScorecardAdapter forwards this outcome directly, so the prospective report can promote once the gates pass despite having no comparable baseline. Make baseline comparability part of the outcome or mandatory-evidence calculation rather than only the CLI exit-code check.
Useful? React with 👍 / 👎.
| evaluate(holdout, policyFingerprint): ScorecardOutcome { | ||
| if (policyFingerprint !== bundle.policyFingerprint) throw new ScorecardContractError(["adapter: policy-fingerprint-mismatch"]); | ||
| const pairedDelta = laneEvidence(bundle, "paired-delta"); | ||
| const pairedFacts = pairedFactsFingerprint(holdout.pairs); | ||
| if (pairedDelta.report === null || pairedDelta.report.body.analysis.pairedFactsFingerprint !== pairedFacts) { |
There was a problem hiding this comment.
When the holdout estimator comes from a different analysis of the same pairs and policy—for example, one with different bootstrap results—the paired-facts check passes while holdout.estimator is ignored. The prospective report then combines direction and evidence sufficiency from one analysis with a scorecard promotion decision derived from another, and recomputation with the same mismatched adapters will reproduce rather than detect it. Verify the supplied estimator result fingerprint against the paired-delta analysis used by this scorecard.
Useful? React with 👍 / 👎.
| ```sh | ||
| gh run download <run-id> --name <artifact> --dir artifacts/ | ||
| ``` |
There was a problem hiding this comment.
Following this command does not produce the filenames or shapes consumed by loadEvidenceBundle: gh run download --help states that a selected artifact's contents are extracted without renaming, while the checked workflows upload names such as paired-delta-${mode}-report.json (paired-delta-eval.yml:126-160) and historian-eval-report.json (historian-eval.yml:118-127); the dreamer artifact is nested per-run JSON files (dreamer-eval.yml:67-77), and retrieval contains three reports (retrieval-benchmark.yml:72-99). Therefore the documented collect-then-score flow reports lanes as missing or malformed. Add a collector/normalizer, or document the required renames, dreamer array assembly, and retrieval report selection.
Useful? React with 👍 / 👎.
| export function runScorecard(args: ScorecardCliArgs, log: (line: string) => void = console.log): ScorecardExitCode { | ||
| try { | ||
| const bundle = loadEvidenceBundle(args.sources); |
There was a problem hiding this comment.
When an operator reruns the command with the same --out path and evidence loading or privacy validation now fails, the catch path returns 2 without touching the previous report. That leaves an older, potentially promotion-allowed scorecard at the documented output path, where later archival or review can mistake it for the failed run's result. Remove or otherwise invalidate the destination before the first fallible validation step, as the incident runner does for the same stale-artifact hazard.
Useful? React with 👍 / 👎.
| export function createScorecardAdapter(input: { bundle: ScorecardEvidenceBundle; report: ScorecardReport }): ScorecardAdapter { | ||
| const { bundle, report } = input; |
There was a problem hiding this comment.
When callers pair a current evidence bundle with a valid report built from another bundle under the same policy, the factory accepts both independently. Evaluation then validates the policy and paired facts against the current bundle but copies gate failures, promotion state, and the report fingerprint from the unrelated report, producing a hybrid result that deterministic recomputation with the same adapter will accept. Rebuild the report inside the factory or verify its fingerprint and target/evidence projections against bundle before exposing the adapter.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| adverseDeltas: comparison.adverseDeltas, | ||
| maxToleratedRegressions: bundle.policy.maxToleratedRegressions, | ||
| }); | ||
| const limitations = [ |
There was a problem hiding this comment.
WARNING: Deduplicate reason codes in limitations array to prevent contract validation failure
buildScorecardReport constructs limitations by concatenating reason codes from bundle.limitations, comparison.limitations, bundle.baseline.diagnostics, and conditional reason codes. When parseScorecardReport validates the report body, idArray(value.limitations, ...) calls unique(), which throws a contract error (report.body.limitations: duplicate) if any reason code is present in more than one source. Deduplicating the array (e.g. [...new Set([...])]) before constructing the body ensures the report adheres to contract uniqueness invariants.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const value = argv[index + 1]; | ||
| if (!KNOWN_FLAGS.includes(flag)) throw new Error(`unknown argument: ${flag}\n${USAGE}`); | ||
| if (value === undefined || value.startsWith("--")) throw new Error(`${flag} requires a value\n${USAGE}`); | ||
| if (values.has(flag)) throw new Error(`${flag} given twice`); |
There was a problem hiding this comment.
WARNING: Short flags like -h are not detected as missing parameter values
Line 30 checks value.startsWith("--") to detect missing flag values. If the command is invoked with a parameter followed immediately by a short flag (such as run-scorecard.ts --freeze -h), "-h" is not recognized as a missing value because it does not start with "--". Consequently, "-h" is accepted as the value for --freeze and the help request is ignored. Checking value.startsWith("-") properly flags both short and long options passed as argument values.
| if (values.has(flag)) throw new Error(`${flag} given twice`); | |
| if (value === undefined || value.startsWith("-")) throw new Error(`${flag} requires a value\n${USAGE}`); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const hex = "a".repeat(64); | ||
| const base = ["--freeze", "freeze", "--freeze-fingerprint", hex, "--artifacts", "artifacts", "--out", "out.json"]; | ||
|
|
||
| it("resolves paths, defaults the policy locations to the e2e root, and accepts an optional baseline", () => { |
There was a problem hiding this comment.
SUGGESTION: Incomplete CLI flag and path assertions in parseArgs test
The parseArgs test suite does not assert args.sources.artifactsDir, args.out, or the default resolution for args.sources.policies.analysisPath. In addition, custom path overrides for --policies and --paired-delta-policy, as well as missing-flag validation for --artifacts and --out, are currently not exercised.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| : { ...row, status: "passed", observedCount: 0, evidenceFingerprint: H1, sourceLane: "incident", diagnostic: null }); | ||
| body.outcome.hardGateFailures = []; | ||
| body.limitations = body.limitations.filter((code) => code !== "hard-gates-unobserved"); | ||
| body.outcome.promotionAllowed = body.outcome.mandatoryEvidenceComplete && body.outcome.blockingRegressionCount <= bundle.policy.maxToleratedRegressions; |
There was a problem hiding this comment.
WARNING: Test helper allGatesPassed bypasses deriveOutcome calculation
allGatesPassed directly re-implements and assigns body.outcome.promotionAllowed = body.outcome.mandatoryEvidenceComplete && body.outcome.blockingRegressionCount <= bundle.policy.maxToleratedRegressions on the cloned report. This means test cases calling allGatesPassed assert against the helper's manual assignment rather than testing deriveOutcome's computation of promotionAllowed when safety gates pass.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Reviewed by gemini-3.7-flash · Input: 376.6K · Output: 22.5K · Cached: 779.8K |
1 participant
Summary
Stack 5/6 of the release scorecard.
buildScorecardReport(bundle)emits the twelve fixed sections in order (target, five families, safety gates, regret, adverse deltas, limitations, evidence, outcome).promotionAllowedandmandatoryEvidenceCompleteare derived inside from gate rows, lane statuses, required slots, and blocking count; no caller supplies them and no scalar aggregate exists. The builder validates its own output throughparseScorecardReport.scorecardExitCode: 2 = any gate non-passing; 1 = evidence incomplete, non-comparable baseline, or blocking regressions over tolerance; 0 = promotion allowed.publishScorecardReport: refuses (scorecard: privacy-rejected) when the report fails the shared privacy scan, else canonical 2-space JSON viawriteJsonAtomically.createScorecardAdapterfills the prospective-holdoutScorecardAdapterseam: rejects a foreign policy fingerprint or paired facts the paired-delta report did not analyze; itsresultFingerprintpins policy, freeze manifest, every lane report, paired facts, baseline, and the scorecard report.scripts/run-scorecard.ts:--freeze --freeze-fingerprint --artifacts --out [--policies] [--paired-delta-policy] [--baseline]. Refusals exit 2 with no report.### Scorecard: pre-registration, gate/lane statuses, reason codes, exit codes,gh run download→ score → review steps, and the fail-closed expectation for the first release run.Deviation from the plan: the prospective-holdout unit tests keep their controllable stub adapter; the real-adapter end-to-end case (
hard-gate-failed, recompute to the samescorecardResultFingerprint, substitution changes it) lives insrc/scorecard/report.test.ts.Testing
bun run test:scorecard-unit(54 passed at this head) incl. AE1, AE2, AE8, AE9 and the release-shape script run: exit 2, fournot-observedgates, onepassedinjection gate.bun run test:prospective-unit(125 passed)Stack
Depends on #181. Next: #183.