From 1f9a1407288a43b285eb7083186310b89199105e Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Sun, 26 Jul 2026 23:17:49 +0900 Subject: [PATCH] fix(engine): dedupe provider signals per target in computeProviderTrackRecords The aggregation loop iterated the raw signals array, incrementing signals/decided/shared/consensus once per signal rather than once per distinct (provider, targetKey) pair. Because loadLiveProviderTrackRecords reads raw audit rows with no dedup, a provider re-reviewing the same PR (common after a new push) had its precision/agreement stats inflated proportionally to its revote count, while its consensus stance reflected only the last vote. Dedupe to one signal per (provider, targetKey) pair with latest-vote-wins -- reusing the same last-write semantics the stancesByTarget map already applies -- before the aggregation loop, so each distinct pair contributes exactly once. Adds a test asserting two same-provider votes on one target contribute once to signals/decided/consensus, on both the engine's own suite and the Codecov-graded vitest suite. Closes #8876 --- .../src/calibration/provider-track-record.ts | 9 +++++++- .../test/provider-track-record.test.ts | 20 ++++++++++++++++ .../unit/provider-track-record-engine.test.ts | 23 +++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/packages/loopover-engine/src/calibration/provider-track-record.ts b/packages/loopover-engine/src/calibration/provider-track-record.ts index 57e9e1a212..167d6c4704 100644 --- a/packages/loopover-engine/src/calibration/provider-track-record.ts +++ b/packages/loopover-engine/src/calibration/provider-track-record.ts @@ -100,9 +100,16 @@ export function computeProviderTrackRecords( stances.set(signal.provider, signal.vote === "fail"); } + // Dedupe to one signal per (provider, targetKey) pair, latest-vote-wins — matching stancesByTarget's + // last-write semantics above. loadLiveProviderTrackRecords reads raw audit rows with no dedup, so a provider + // re-reviewing the same PR (e.g. after a new push) would otherwise inflate its signals/decided/shared/consensus + // counters proportionally to its revote count while its stance reflects only the last vote (#8876). + const dedupedByProviderTarget = new Map(); + for (const signal of signals) dedupedByProviderTarget.set(`${signal.provider} ${signal.targetKey}`, signal); + const perRepo = new Map>(); // provider → repo → stats const overall = new Map(); - for (const signal of signals) { + for (const signal of dedupedByProviderTarget.values()) { let repos = perRepo.get(signal.provider); if (repos === undefined) { repos = new Map(); diff --git a/packages/loopover-engine/test/provider-track-record.test.ts b/packages/loopover-engine/test/provider-track-record.test.ts index 7a882032f8..bb20085fdb 100644 --- a/packages/loopover-engine/test/provider-track-record.test.ts +++ b/packages/loopover-engine/test/provider-track-record.test.ts @@ -51,3 +51,23 @@ test("null discipline: no fail votes -> null precision; no shared targets -> nul assert.equal(overall.splitRate, null); assert.equal(overall.agreementRate, 1); }); + +test("computeProviderTrackRecords counts a provider's repeated votes on one target once, latest-vote-wins (#8876)", () => { + const cases = [labeled("acme/widgets#1", "confirmed"), labeled("acme/widgets#2", "confirmed")]; + const signals = [ + signal("provider-a", "acme/widgets#1", "pass"), + signal("provider-a", "acme/widgets#1", "fail"), + signal("provider-b", "acme/widgets#1", "fail"), + signal("provider-a", "acme/widgets#2", "fail"), + ]; + const records = computeProviderTrackRecords(signals, cases); + const aOverall = records.find((r) => r.provider === "provider-a" && r.repoFullName === null)!; + // Without dedup this would be signals:3 (the stale pass on #1 counted); deduped it is 2 distinct targets, + // #1 resolving to the latest fail so precision stays 1. + assert.equal(aOverall.signals, 2); + assert.equal(aOverall.decided, 2); + assert.equal(aOverall.precision, 1); + assert.equal(aOverall.consensusRate, 1); + const bOverall = records.find((r) => r.provider === "provider-b" && r.repoFullName === null)!; + assert.equal(bOverall.signals, 1); +}); diff --git a/test/unit/provider-track-record-engine.test.ts b/test/unit/provider-track-record-engine.test.ts index 87b30498bf..7503f986ea 100644 --- a/test/unit/provider-track-record-engine.test.ts +++ b/test/unit/provider-track-record-engine.test.ts @@ -127,4 +127,27 @@ describe("computeProviderTrackRecords (#8228)", () => { it("returns an empty list for empty inputs", () => { expect(computeProviderTrackRecords([], [])).toEqual([]); }); + + it("counts a provider's repeated votes on one target exactly once, latest-vote-wins (#8876)", () => { + const cases = [labeled("acme/widgets#1", "confirmed"), labeled("acme/widgets#2", "confirmed")]; + const signals = [ + // provider-a re-reviews #1: an earlier pass then a later fail. Only the latest (fail) should count, once. + signal("provider-a", "acme/widgets#1", "pass"), + signal("provider-a", "acme/widgets#1", "fail"), + // provider-b reviews #1 once (fail) so #1 is a shared target with a consensus stance. + signal("provider-b", "acme/widgets#1", "fail"), + // provider-a also reviews #2 once, to prove distinct (provider, target) pairs still each count. + signal("provider-a", "acme/widgets#2", "fail"), + ]; + const records = computeProviderTrackRecords(signals, cases); + + const aOverall = records.find((r) => r.provider === "provider-a" && r.repoFullName === null)!; + // Without dedup this would be signals:3/decided:3 (the pass+fail on #1 both counted). Deduped: 2 distinct + // targets, and #1 resolves to the latest fail vote (so precision stays 1, not diluted by the stale pass). + expect(aOverall).toMatchObject({ signals: 2, decided: 2, confirmed: 2, precision: 1, agreementRate: 1 }); + const bOverall = records.find((r) => r.provider === "provider-b" && r.repoFullName === null)!; + // #1 is shared by a (latest fail) and b (fail): a genuine consensus, counted once for each provider. + expect(bOverall).toMatchObject({ signals: 1, decided: 1, consensusRate: 1 }); + expect(aOverall.consensusRate).toBe(1); + }); });