diff --git a/migrations/0134_review_targets_cadence_idx.sql b/migrations/0134_review_targets_cadence_idx.sql new file mode 100644 index 0000000000..4b3b0941c2 --- /dev/null +++ b/migrations/0134_review_targets_cadence_idx.sql @@ -0,0 +1,5 @@ +-- Supports the submission-cadence signal (#4514): getSubmitterCadence queries ALL review_targets rows (not +-- just terminal ones -- a fresh burst of still-open submissions is exactly what this needs to catch) filtered +-- by (project, submitter, created_at). This shape isn't covered by the existing review_targets indexes +-- (migrations/0050), which are keyed on status/verdict/terminal_at, not created_at alongside submitter. +CREATE INDEX IF NOT EXISTS idx_review_targets_project_submitter_created ON review_targets (project, submitter, created_at); diff --git a/src/review/reputation-wire.ts b/src/review/reputation-wire.ts index bb8ec25a07..04461c1287 100644 --- a/src/review/reputation-wire.ts +++ b/src/review/reputation-wire.ts @@ -14,7 +14,9 @@ // the ported module degrades to "neutral" / no-op on any DB error, so this never throws into the gate. import { + getSubmitterCadence, getSubmitterReputation, + isMachinePacedCadence, recordSubmissionOutcome, type SubmissionOutcome, type SubmitterStats, @@ -58,6 +60,12 @@ export function shouldDowngradeToDeterministic(stats: SubmitterStats): boolean { * downgrade to a deterministic-only review. When the flag is OFF this returns false IMMEDIATELY — no DB read — * so the AI-spend gate is byte-identical to today. `project` namespaces the per-(project, submitter) rows * (gittensory uses the repo full name). NEVER throws: the ported module already degrades to neutral on error. + * + * Also checks submission CADENCE (#4514): every quality-based signal above only tells you whether a + * submitter's outcomes were good or bad, never how FAST they arrived -- a fast, well-formed, strategically + * low-value submitter clears every quality bar while still being invisible to those signals. A cadence read + * this tight, sustained across this many consecutive submissions, is not a pattern any human contributor + * plausibly sustains, independent of whether the submissions themselves look fine. */ export async function shouldSkipAiForReputation( env: Env, @@ -65,7 +73,9 @@ export async function shouldSkipAiForReputation( ): Promise { if (!isReputationEnabled(env)) return false; const stats = await getSubmitterReputation(env, args.project, args.submitter ?? undefined); - return shouldDowngradeToDeterministic(stats); + if (shouldDowngradeToDeterministic(stats)) return true; + const cadence = await getSubmitterCadence(env, args.project, args.submitter ?? undefined); + return isMachinePacedCadence(cadence); } /** diff --git a/src/review/submitter-reputation.ts b/src/review/submitter-reputation.ts index 72558c8615..04b9c5f19e 100644 --- a/src/review/submitter-reputation.ts +++ b/src/review/submitter-reputation.ts @@ -70,6 +70,58 @@ export const REPUTATION_WINDOW_DAYS = 90; // Hard ceiling on rows pulled for one submitter's window so a pathological history can't blow the query up. const REPUTATION_WINDOW_ROW_CAP = 500; +// ── Submission-cadence signal (#4514). Every signal above is QUALITY-based (was the outcome good or bad) -- +// none of them have a TIMING dimension, so a fast, well-formed, strategically-low-value submitter is +// invisible to the one dimension (superhuman pace) that would otherwise be a strong tell. This is queried +// from ALL review_targets rows (not just terminal ones, unlike the quality signal above) -- a fresh burst of +// still-open submissions is exactly the case this needs to catch, and by the time they become terminal the +// (paid) AI review has already run on each one. ── +const CADENCE_WINDOW_HOURS = 24; +// Need at least this many recent submissions before judging pace at all -- a lone fast submission (a real +// contributor who happened to open two PRs close together) is not a pattern. +const CADENCE_MIN_SAMPLE = 5; +// A human contributor, even a fast one, does not sustain a sub-10-minute median gap between distinct PR +// submissions across many consecutive attempts -- reading, writing, and testing each change takes real time. +const CADENCE_MAX_MEDIAN_GAP_MS = 10 * 60 * 1000; + +export type SubmissionCadence = { count: number; medianGapMs: number | null }; + +/** Pure: the median gap (ms) between consecutive submissions, given their created_at timestamps in any order. + * `medianGapMs` is `null` when there are fewer than 2 samples (no gap to measure). */ +export function computeSubmissionCadence(createdAtIsoTimestamps: readonly string[]): SubmissionCadence { + const sorted = [...createdAtIsoTimestamps].map((t) => new Date(t).getTime()).sort((a, b) => a - b); + if (sorted.length < 2) return { count: sorted.length, medianGapMs: null }; + const gaps: number[] = []; + for (let i = 1; i < sorted.length; i++) gaps.push(sorted[i]! - sorted[i - 1]!); + gaps.sort((a, b) => a - b); + const mid = Math.floor(gaps.length / 2); + const medianGapMs = gaps.length % 2 === 0 ? (gaps[mid - 1]! + gaps[mid]!) / 2 : gaps[mid]!; + return { count: sorted.length, medianGapMs }; +} + +/** Pure: does this cadence read as machine-paced? Needs both a real sample size AND a gap tighter than any + * human contributor plausibly sustains across that many consecutive attempts. */ +export function isMachinePacedCadence(cadence: SubmissionCadence): boolean { + return cadence.count >= CADENCE_MIN_SAMPLE && cadence.medianGapMs !== null && cadence.medianGapMs < CADENCE_MAX_MEDIAN_GAP_MS; +} + +/** Per-repo submission cadence for one submitter over the last {@link CADENCE_WINDOW_HOURS}. Fail-safe: + * any read error degrades to `{ count: 0, medianGapMs: null }` (never machine-paced), identical in spirit to + * {@link getSubmitterReputation}'s fail-safe-to-neutral. */ +export async function getSubmitterCadence(env: Env, project: string, submitter: string | undefined): Promise { + if (!submitter) return { count: 0, medianGapMs: null }; + try { + const result = await storage(env) + .prepare(`SELECT created_at AS createdAt FROM review_targets WHERE project = ? AND submitter = ? AND created_at >= datetime('now', ?) ORDER BY created_at DESC LIMIT ?`) + .bind(project, submitter, `-${CADENCE_WINDOW_HOURS} hours`, REPUTATION_WINDOW_ROW_CAP) + .all<{ createdAt: string }>(); + const createdAts = (result?.results ?? []).map((r) => r.createdAt); + return computeSubmissionCadence(createdAts); + } catch { + return { count: 0, medianGapMs: null }; + } +} + // ── reasonCode → quality bucket (#reputation-redesign). Buckets reflect the LIVE D1 reasonCode taxonomy. ── // SUCCESS: a genuine reviewer/merge approval. // QUALITY_FAIL: a genuine RECENT reviewer reject (real quality signal). diff --git a/test/unit/reputation-wiring.test.ts b/test/unit/reputation-wiring.test.ts index 9120da5be0..e41b76e754 100644 --- a/test/unit/reputation-wiring.test.ts +++ b/test/unit/reputation-wiring.test.ts @@ -149,6 +149,56 @@ describe("shouldSkipAiForReputation (helper)", () => { expect(await shouldSkipAiForReputation(env, { project: "acme/widgets", submitter: "burster" })).toBe(true); expect(await shouldSkipAiForReputation(env, { project: "acme/widgets", submitter: "newcomer" })).toBe(false); }); + + it("FLAG-ON: false for a null submitter (the ?? undefined coalesce on both the quality and cadence reads)", async () => { + const env = createTestEnv({ GITTENSORY_REVIEW_REPUTATION: "true" }); + expect(await shouldSkipAiForReputation(env, { project: "acme/widgets", submitter: null })).toBe(false); + }); + + describe("submission-cadence signal (#4514)", () => { + async function seedReviewTarget(env: Env, args: { number: number; submitter: string; createdAt: string }) { + await env.DB.prepare( + `INSERT INTO review_targets (id, project, kind, repo, number, submitter, status, decision_json, terminal_at, created_at) + VALUES (?, 'acme/widgets', 'pull_request', 'acme/widgets', ?, ?, 'merged', ?, ?, ?)`, + ) + .bind(`acme/widgets:pull_request:acme/widgets#${args.number}`, args.number, args.submitter, JSON.stringify({ reasonCode: "dual_review_approved" }), args.createdAt, args.createdAt) + .run(); + } + + it("FLAG-ON: true for a machine-paced submitter even though every submission itself looks fine (quality-neutral)", async () => { + const env = createTestEnv({ GITTENSORY_REVIEW_REPUTATION: "true" }); + // Anchored to now (minus a couple hours of headroom) -- the cadence query only looks back 24h, so a + // fixed past date would fall outside the window and vacuously read as "0 samples, not machine-paced". + const t0 = Date.now() - 2 * 60 * 60_000; + for (let i = 0; i < 5; i++) { + // All merged/approved -- the QUALITY signal alone stays neutral/trusted; only cadence should trip this. + await seedReviewTarget(env, { number: i, submitter: "speedster", createdAt: new Date(t0 + i * 5 * 60_000).toISOString() }); + } + expect(await shouldSkipAiForReputation(env, { project: "acme/widgets", submitter: "speedster" })).toBe(true); + }); + + it("FLAG-ON: false for the same number of submissions spread naturally over hours (comfortably human pace)", async () => { + const env = createTestEnv({ GITTENSORY_REVIEW_REPUTATION: "true" }); + const t0 = Date.now() - 20 * 60 * 60_000; + for (let i = 0; i < 5; i++) { + await seedReviewTarget(env, { number: i + 100, submitter: "steady", createdAt: new Date(t0 + i * 3 * 60 * 60_000).toISOString() }); + } + expect(await shouldSkipAiForReputation(env, { project: "acme/widgets", submitter: "steady" })).toBe(false); + }); + + it("FLAG-ON: skips the (extra) cadence read once the quality/burst signal already justifies downgrading", async () => { + const env = createTestEnv({ GITTENSORY_REVIEW_REPUTATION: "true" }); + await seedSubmitter(env, { project: "acme/widgets", submitter: "burster", submissions: 12, merged: 0, closed: 12, manual: 0 }); + const spy = vi.spyOn(env.DB, "prepare"); + const before = spy.mock.calls.length; + expect(await shouldSkipAiForReputation(env, { project: "acme/widgets", submitter: "burster" })).toBe(true); + // Exactly the calls the quality/burst read itself makes (submitter_stats + review_targets window) -- + // no additional prepare() for a cadence query once the burst check alone already returned true. + const afterQualityOnlyCallCount = spy.mock.calls.length - before; + spy.mockRestore(); + expect(afterQualityOnlyCallCount).toBe(2); + }); + }); }); describe("processGitHubWebhook records the reputation outcome on a terminal PR (flag-ON call site)", () => { diff --git a/test/unit/submitter-reputation.test.ts b/test/unit/submitter-reputation.test.ts index 80b72aa8f0..6a18a6a8bc 100644 --- a/test/unit/submitter-reputation.test.ts +++ b/test/unit/submitter-reputation.test.ts @@ -1,9 +1,12 @@ import { describe, expect, it } from "vitest"; import { classifyOutcome, + computeSubmissionCadence, countOutcomes, DEFAULT_REPUTATION_CONFIG, + getSubmitterCadence, getSubmitterReputation, + isMachinePacedCadence, recordSubmissionOutcome, REPUTATION_WINDOW_DAYS, type ReputationConfig, @@ -273,6 +276,104 @@ describe("recordSubmissionOutcome / getSubmitterReputation (D1, fail-safe)", () }); }); +describe("computeSubmissionCadence (pure) (#4514)", () => { + it("returns count and null medianGapMs for 0 or 1 samples (nothing to measure a gap between)", () => { + expect(computeSubmissionCadence([])).toEqual({ count: 0, medianGapMs: null }); + expect(computeSubmissionCadence(["2026-01-01T00:00:00.000Z"])).toEqual({ count: 1, medianGapMs: null }); + }); + + it("computes the median gap between consecutive submissions, order-independent", () => { + // Gaps: 10min, 20min, 30min -> sorted [10,20,30] -> median 20min. + const t0 = new Date("2026-01-01T00:00:00.000Z").getTime(); + const timestamps = [t0, t0 + 10 * 60_000, t0 + 30 * 60_000, t0 + 60 * 60_000].map((ms) => new Date(ms).toISOString()); + // Shuffle the input order -- the function must sort internally, not assume caller ordering. + const shuffled = [timestamps[2]!, timestamps[0]!, timestamps[3]!, timestamps[1]!]; + expect(computeSubmissionCadence(shuffled)).toEqual({ count: 4, medianGapMs: 20 * 60_000 }); + }); + + it("averages the two middle gaps for an even number of gaps", () => { + // 3 timestamps -> 2 gaps: 10min, 30min -> even count -> average = 20min. + const t0 = new Date("2026-01-01T00:00:00.000Z").getTime(); + const timestamps = [t0, t0 + 10 * 60_000, t0 + 40 * 60_000].map((ms) => new Date(ms).toISOString()); + expect(computeSubmissionCadence(timestamps)).toEqual({ count: 3, medianGapMs: 20 * 60_000 }); + }); +}); + +describe("isMachinePacedCadence (pure) (#4514)", () => { + it("requires BOTH the minimum sample size AND a sub-threshold median gap", () => { + // Below minSample (5) -- fast, but not enough samples to call it a pattern. + expect(isMachinePacedCadence({ count: 4, medianGapMs: 60_000 })).toBe(false); + // Enough samples, but the gap is comfortably human (well over 10min). + expect(isMachinePacedCadence({ count: 10, medianGapMs: 60 * 60_000 })).toBe(false); + // No gap at all to measure (count < 2 internally, or explicitly null). + expect(isMachinePacedCadence({ count: 8, medianGapMs: null })).toBe(false); + // Enough samples AND a tight gap -- machine-paced. + expect(isMachinePacedCadence({ count: 5, medianGapMs: 5 * 60_000 })).toBe(true); + expect(isMachinePacedCadence({ count: 20, medianGapMs: 60_000 })).toBe(true); + }); + + it("is a boundary at exactly the configured thresholds", () => { + // Exactly at minSample, exactly under the max gap -- still counts. + expect(isMachinePacedCadence({ count: 5, medianGapMs: 10 * 60_000 - 1 })).toBe(true); + // Exactly AT the max gap -- not strictly under, so not machine-paced. + expect(isMachinePacedCadence({ count: 5, medianGapMs: 10 * 60_000 })).toBe(false); + }); +}); + +describe("getSubmitterCadence (D1, fail-safe) (#4514)", () => { + function makeCadenceEnv(createdAts: string[]): Env { + return { + DB: { + prepare: () => ({ + bind: () => ({ + all: async () => ({ results: createdAts.map((createdAt) => ({ createdAt })) }), + }), + }), + }, + } as unknown as Env; + } + + it("returns count 0 / null with no submitter (early return, no DB touch)", async () => { + expect(await getSubmitterCadence({} as Env, "p", undefined)).toEqual({ count: 0, medianGapMs: null }); + }); + + it("derives cadence from the queried created_at timestamps", async () => { + const t0 = new Date("2026-01-01T00:00:00.000Z").getTime(); + const env = makeCadenceEnv([t0, t0 + 5 * 60_000, t0 + 10 * 60_000, t0 + 15 * 60_000, t0 + 20 * 60_000].map((ms) => new Date(ms).toISOString())); + const cadence = await getSubmitterCadence(env, "p", "farmer99"); + expect(cadence).toEqual({ count: 5, medianGapMs: 5 * 60_000 }); + expect(isMachinePacedCadence(cadence)).toBe(true); + }); + + it("fail-safe: degrades to count 0 / null when the query throws, never throws into the caller", async () => { + const env = { + DB: { + prepare: () => ({ + bind: () => ({ + all: async () => { + throw new Error("D1 boom"); + }, + }), + }), + }, + } as unknown as Env; + expect(await getSubmitterCadence(env, "p", "farmer99")).toEqual({ count: 0, medianGapMs: null }); + }); + + it("fail-safe: degrades to count 0 / null when the query returns a malformed result (?? [] fallback)", async () => { + const env = { + DB: { + prepare: () => ({ + bind: () => ({ + all: async () => undefined, + }), + }), + }, + } as unknown as Env; + expect(await getSubmitterCadence(env, "p", "farmer99")).toEqual({ count: 0, medianGapMs: null }); + }); +}); + // A minimal D1 stub: the first query (.first) returns submitter_stats; the window query (.all) returns the // review_targets rows. Both come off the same prepared-statement stub (the two call sites use .first vs .all). function makeEnv(opts: { statRow: { submissions: number; merged: number; closed: number; manual: number } | null; windowRows: Row[] }): Env {