From 6779a314a1b1914ae81f68f69bd24333ae0d8ac8 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Thu, 30 Jul 2026 02:39:27 +0800 Subject: [PATCH] fix(engine): guard benchmark ground-truth against invalid window and duplicate roster benchmarkHorizonEnd threw a bare RangeError for an unparseable frozenAt or a non-finite horizonDays (new Date(NaN).toISOString()), and deriveBenchmarkGroundTruth silently emitted two truths for a duplicated workUnitId, inflating workUnits, unresolved, and unresolvedRate's denominator. Add negated-compound guards throwing invalid_frozen_at / invalid_horizon_days in benchmarkHorizonEnd (called before any other work), and reject a duplicated roster entry with invalid_duplicate_work_unit_id rather than de-duplicating a corrupted snapshot. Production formulas, thresholds, and valid-input output are unchanged. Closes #9644 --- .../src/calibration/benchmark-ground-truth.ts | 40 ++++++++++++++- .../test/benchmark-ground-truth.test.ts | 51 +++++++++++++++++++ 2 files changed, 89 insertions(+), 2 deletions(-) diff --git a/packages/loopover-engine/src/calibration/benchmark-ground-truth.ts b/packages/loopover-engine/src/calibration/benchmark-ground-truth.ts index 1a1a39566..d5a21ae83 100644 --- a/packages/loopover-engine/src/calibration/benchmark-ground-truth.ts +++ b/packages/loopover-engine/src/calibration/benchmark-ground-truth.ts @@ -26,6 +26,21 @@ // later event — including a later reversal — cannot retroactively change a settled label. That is what // makes a leaderboard reproducible instead of drifting under its own scores. // +// Three inputs this module REJECTS rather than mislabels, each a caller/snapshot-builder bug that must fail +// loudly instead of scoring corrupted numbers — the same throw-for-caller-bug discipline `splitBacktestCorpus` +// uses for an out-of-range fraction: +// +// INVALID `frozenAt`. An unparseable `frozenAt` throws `invalid_frozen_at` — checked in `benchmarkHorizonEnd` +// before any label is derived, so a bad window fails closed instead of producing an `Invalid Date` horizon +// whose every subsequent comparison silently drops the event. +// +// INVALID `horizonDays`. A non-finite `horizonDays` throws `invalid_horizon_days`, in the same guard, for the +// same reason — a `NaN` window is a caller bug, not an empty benchmark. +// +// DUPLICATE ROSTER ENTRY. A `workUnitIds` roster containing the same id twice throws +// `invalid_duplicate_work_unit_id` — silently de-duplicating would hide a corrupted snapshot (#9259) while +// double-counting the unit in `workUnits`, in `unresolved`, and in every rate divided by them. +// // Pure core (this module); the IO wrapper that reads events from D1 lives engine-side of the harness, the // same split every sibling calibration module uses. @@ -99,7 +114,16 @@ const MS_PER_DAY = 24 * 60 * 60 * 1000; /** The inclusive horizon end for a snapshot. Exported so the harness, the scorer, and any third-party * verifier compute the SAME instant rather than three slightly different ones. */ export function benchmarkHorizonEnd(frozenAt: string, horizonDays: number): string { - return new Date(Date.parse(frozenAt) + horizonDays * MS_PER_DAY).toISOString(); + const frozenAtMs = Date.parse(frozenAt); + // Negated-compound guards so a NaN also fails closed, rather than flowing into `new Date(NaN).toISOString()` + // (a bare `RangeError`) — a named reason names the offending field the way the sibling validators do. + if (!Number.isFinite(frozenAtMs)) { + throw new Error(`invalid_frozen_at: ${frozenAt}`); + } + if (!Number.isFinite(horizonDays)) { + throw new Error(`invalid_horizon_days: ${horizonDays}`); + } + return new Date(frozenAtMs + horizonDays * MS_PER_DAY).toISOString(); } function withinHorizon(occurredAt: string, frozenAtMs: number, horizonEndMs: number): boolean { @@ -125,10 +149,22 @@ export function deriveBenchmarkGroundTruth(input: { events: readonly RealizedMaintainerEvent[]; reversals?: readonly RealizedReversalEvent[] | undefined; }): BenchmarkGroundTruthSet { - const frozenAtMs = Date.parse(input.frozenAt); + // Reject an invalid window BEFORE any other work, so no partial result is ever computed for a bad horizon. const horizonEnd = benchmarkHorizonEnd(input.frozenAt, input.horizonDays); + const frozenAtMs = Date.parse(input.frozenAt); const horizonEndMs = Date.parse(horizonEnd); + // A duplicated roster entry is a snapshot-builder bug (#9259): emitting two truths for one id would + // double-count it in `workUnits`, in `unresolved`, and in `unresolvedRate`'s denominator. Reject it loudly + // rather than de-duplicating, which would let the corrupted snapshot score as if it were clean. + const seenWorkUnitIds = new Set(); + for (const workUnitId of input.workUnitIds) { + if (seenWorkUnitIds.has(workUnitId)) { + throw new Error(`invalid_duplicate_work_unit_id: ${workUnitId}`); + } + seenWorkUnitIds.add(workUnitId); + } + // The SETTLED state is the LAST qualifying action in the window, not the first: a maintainer who labels, // then requests changes, then merges has settled on the merge. Ties on identical timestamps keep the // earlier-listed event, so a caller's stable input order yields a stable label. diff --git a/packages/loopover-engine/test/benchmark-ground-truth.test.ts b/packages/loopover-engine/test/benchmark-ground-truth.test.ts index ccbef3c90..9c17f4fb7 100644 --- a/packages/loopover-engine/test/benchmark-ground-truth.test.ts +++ b/packages/loopover-engine/test/benchmark-ground-truth.test.ts @@ -193,6 +193,57 @@ test("an empty work-unit roster reports a NULL unresolved rate, never 0 (which w assert.deepEqual(scoreableGroundTruths(set), []); }); +test("REGRESSION: benchmarkHorizonEnd throws a NAMED error for an unparseable frozenAt or non-finite horizonDays", () => { + // Fails closed as `invalid_frozen_at` rather than the bare RangeError `new Date(NaN).toISOString()` throws. + assert.throws(() => benchmarkHorizonEnd("not a date", 14), /invalid_frozen_at: not a date/); + assert.throws(() => benchmarkHorizonEnd("", 14), /invalid_frozen_at/); + // A non-finite horizonDays is a caller bug, not an empty benchmark — NaN and Infinity both fail closed. + assert.throws(() => benchmarkHorizonEnd(FROZEN, Number.NaN), /invalid_horizon_days: NaN/); + assert.throws(() => benchmarkHorizonEnd(FROZEN, Number.POSITIVE_INFINITY), /invalid_horizon_days/); +}); + +test("REGRESSION: the window guard fires from deriveBenchmarkGroundTruth before any partial result is computed", () => { + assert.throws( + () => deriveBenchmarkGroundTruth({ ...BASE, frozenAt: "not a date", workUnitIds: ["o/r#1"], events: [] }), + /invalid_frozen_at: not a date/, + ); + assert.throws( + () => deriveBenchmarkGroundTruth({ ...BASE, horizonDays: Number.NaN, workUnitIds: ["o/r#1"], events: [] }), + /invalid_horizon_days: NaN/, + ); +}); + +test("REGRESSION: a duplicated workUnitId is rejected, never silently de-duplicated into a corrupt denominator", () => { + assert.throws( + () => deriveBenchmarkGroundTruth({ ...BASE, workUnitIds: ["o/r#1", "o/r#2", "o/r#1"], events: [] }), + /invalid_duplicate_work_unit_id: o\/r#1/, + ); +}); + +test("the guards are ADDITIVE: a valid input still yields the exact same ground-truth set", () => { + const set = deriveBenchmarkGroundTruth({ + ...BASE, + workUnitIds: ["o/r#1", "o/r#2"], + events: [ + { workUnitId: "o/r#1", action: "merge", occurredAt: at(3) }, + { workUnitId: "o/r#2", action: "close", occurredAt: at(6), reasonClass: "spam" }, + ], + reversals: [{ workUnitId: "o/r#1", kind: "reversal_reverted", occurredAt: at(8) }], + }); + assert.deepEqual(set, { + schemaVersion: 1, + snapshotRef: BASE.snapshotRef, + horizonDays: 14, + frozenAt: FROZEN, + horizonEnd: "2026-07-15T00:00:00.000Z", + truths: [ + { workUnitId: "o/r#1", outcome: "settled", action: "merge", settledAt: at(3), reversal: { kind: "reversal_reverted", occurredAt: at(8) } }, + { workUnitId: "o/r#2", outcome: "settled", action: "close", reasonClass: "spam", settledAt: at(6), reversal: null }, + ], + coverage: { workUnits: 2, scoreable: 2, unresolved: 0, unresolvedRate: 0 }, + }); +}); + test("close/label parameters are dropped when the settled action is not the one they belong to", () => { // A malformed upstream event carrying a reasonClass on a merge must not leak it into the label. const set = deriveBenchmarkGroundTruth({