From bcbe5251441332c5482e0af6b05e0676a6c9e17e Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 17:32:23 -0700 Subject: [PATCH] fix(review): velocity-aware exception in the unlinked-issue-match escalation for machine-cadence (miner) authors The repeat-escalation path was calibrated for human abuse cadence: a confirmed repeat by the same author within 90 days, correlated across every repo in the install, straight-closes on the theory that a human is unlikely to repeat the exact same "forgot to link the issue" mistake twice in that window. A systematic bug in a miner's own PR-body template could recur within hours rather than months, converting one tooling bug into cross-repo closure of otherwise- correct, properly-targeted work. Adds a velocity exception gated on CONFIRMED official-miner identity, not on speed alone: a repeat within the last hour from a verified Gittensor miner holds again (with a distinct message) instead of closing, while an unverified account repeating just as fast still escalates to close exactly as before -- fast-and-unverified is the MORE suspicious case, not less, so leniency only applies where an independent identity check backs it. Fixes #4512 --- src/db/repositories.ts | 14 ++ src/review/unlinked-issue-guardrail.ts | 64 +++++++-- test/unit/unlinked-issue-guardrail.test.ts | 144 +++++++++++++++++++++ 3 files changed, 212 insertions(+), 10 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 35960f6daf..68879f7dcf 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -2616,6 +2616,20 @@ export async function hasRecentAuditEventForOtherTarget(env: Env, actor: string, return rows.length > 0; } +/** Timestamp-returning variant of {@link hasRecentAuditEventForOtherTarget} (#4512): the newest matching + * row's `createdAt`, or `null` when there is none. Backs velocity-aware escalation logic that needs to know + * HOW RECENTLY a prior match happened, not just whether one exists within the window. */ +export async function mostRecentAuditEventForOtherTarget(env: Env, actor: string, eventType: string, currentTargetKey: string, sinceIso: string): Promise { + const db = getDb(env.DB); + const rows = await db + .select({ createdAt: auditEvents.createdAt }) + .from(auditEvents) + .where(and(eq(auditEvents.actor, actor), eq(auditEvents.eventType, eventType), not(eq(auditEvents.targetKey, currentTargetKey)), gte(auditEvents.createdAt, sinceIso))) + .orderBy(desc(auditEvents.createdAt)) + .limit(1); + return rows[0]?.createdAt ?? null; +} + /** Count-returning variant of {@link hasRecentAuditEvent}, additionally scoped to one `targetKey` (e.g. a single * `owner/repo#123` PR/issue) rather than the actor's activity across the whole repo. Backs the review-request * nagging cooldown (#2463): counting how many `@gittensory` pings a contributor has sent on ONE thread within diff --git a/src/review/unlinked-issue-guardrail.ts b/src/review/unlinked-issue-guardrail.ts index 6b8e17df06..7ca8a07176 100644 --- a/src/review/unlinked-issue-guardrail.ts +++ b/src/review/unlinked-issue-guardrail.ts @@ -13,7 +13,8 @@ // repo that hasn't opted in (the default) or a PR that already links an issue (the common case) pays // nothing beyond two boolean checks. -import { hasRecentAuditEventForOtherTarget, listOpenIssues, recordAuditEvent } from "../db/repositories"; +import { getFreshOfficialMinerDetection, mostRecentAuditEventForOtherTarget, listOpenIssues, recordAuditEvent, upsertOfficialMinerDetection } from "../db/repositories"; +import { fetchOfficialGittensorMiner } from "../gittensor/api"; import { findUnlinkedIssueCandidates, type CandidateOpenIssue } from "../signals/unlinked-issue-candidates"; import type { UnlinkedIssueGuardrailConfig } from "../types"; import { verifyUnlinkedIssueMatch } from "./unlinked-issue-match"; @@ -23,6 +24,36 @@ export const UNLINKED_ISSUE_MATCH_AUDIT_EVENT_TYPE = "github_app.unlinked_issue_ // Same recency convention as submitter-reputation.ts's REPUTATION_WINDOW_DAYS -- a match from a year ago // shouldn't silently escalate every fresh, unrelated match into an auto-close forever. const UNLINKED_ISSUE_MATCH_REPEAT_WINDOW_MS = 90 * 24 * 60 * 60 * 1000; +// #4512: a confirmed repeat inside this gap reads as "the same tooling bug firing again," not "a human +// deliberately farming the guardrail twice" -- no ordinary contributor realistically re-triggers the exact +// same unlinked-issue pattern this fast. Gated on CONFIRMED official-miner identity (below), not on speed +// alone, so this can't be used to launder genuine rapid-fire abuse from an unverified account: an unverified +// actor repeating this fast still escalates to close exactly as before. +const VELOCITY_EXCEPTION_MAX_GAP_MS = 60 * 60 * 1000; +// Mirrors processors.ts's own official-miner-detection cache TTLs (kept local -- importing them would create +// a circular dependency, since processors.ts is the one that imports FROM this module). +const OFFICIAL_MINER_DETECTION_TTL_MS = 5 * 60 * 1000; +const OFFICIAL_MINER_DETECTION_UNAVAILABLE_TTL_MS = 60 * 1000; + +/** Minimal cached miner-identity check, deliberately independent of processors.ts's getCachedOfficialMinerDetection + * (same cache table and TTLs, no audit-log side effect -- this call site doesn't need one). Fail-safe: any + * lookup failure resolves to "not a confirmed miner," never the reverse. */ +async function isConfirmedOfficialMiner(env: Env, login: string): Promise { + const cached = await getFreshOfficialMinerDetection(env, login).catch(() => null); + if (cached) return cached.status === "confirmed"; + // fetchOfficialGittensorMiner already converts every failure into a returned {status: "unavailable"} + // value rather than rejecting -- nothing to catch here. + const detection = await fetchOfficialGittensorMiner(login); + // A cache-write failure must never block the caller from using the freshly-fetched (just uncached) + // detection -- worst case, the next call re-fetches instead of hitting the cache. + const cacheable = await upsertOfficialMinerDetection( + env, + login, + detection, + detection.status === "unavailable" ? OFFICIAL_MINER_DETECTION_UNAVAILABLE_TTL_MS : OFFICIAL_MINER_DETECTION_TTL_MS, + ).catch(() => detection); + return cacheable.status === "confirmed"; +} export type UnlinkedIssueMatchDisposition = { kind: "hold"; reason: string; comment: string } | { kind: "close"; reason: string; comment: string }; @@ -43,15 +74,17 @@ export type ResolveUnlinkedIssueMatchDispositionInput = { prAuthorLogin: string | null | undefined; }; -/** Has this contributor triggered a confirmed unlinked-issue match on another PR (any repo) within the - * recency window? Fail-safe: a read error resolves to "no prior match" (never wrongly escalates on a DB hiccup). */ function unlinkedIssueMatchTargetKey(repoFullName: string, pullNumber: number): string { return `${repoFullName}#${pullNumber}`; } -async function hasPriorUnlinkedIssueMatch(env: Env, authorLogin: string, currentTargetKey: string): Promise { +/** Has this contributor triggered a confirmed unlinked-issue match on another PR (any repo) within the + * recency window, and if so when? Fail-safe: a read error resolves to "no prior match" (never wrongly + * escalates on a DB hiccup). Timestamp (not just a boolean) so the caller can apply the #4512 velocity + * exception. */ +async function priorUnlinkedIssueMatchTimestamp(env: Env, authorLogin: string, currentTargetKey: string): Promise { const sinceIso = new Date(Date.now() - UNLINKED_ISSUE_MATCH_REPEAT_WINDOW_MS).toISOString(); - return hasRecentAuditEventForOtherTarget(env, authorLogin, UNLINKED_ISSUE_MATCH_AUDIT_EVENT_TYPE, currentTargetKey, sinceIso).catch(() => false); + return mostRecentAuditEventForOtherTarget(env, authorLogin, UNLINKED_ISSUE_MATCH_AUDIT_EVENT_TYPE, currentTargetKey, sinceIso).catch(() => null); } /** Record THIS occurrence so a later PR from the same contributor can be recognized as a repeat. Fire-and- @@ -108,13 +141,24 @@ export async function resolveUnlinkedIssueMatchDisposition(env: Env, input: Reso }; } const currentTargetKey = unlinkedIssueMatchTargetKey(input.repoFullName, input.pullNumber); - const isRepeat = await hasPriorUnlinkedIssueMatch(env, authorLogin, currentTargetKey); + const priorMatchIso = await priorUnlinkedIssueMatchTimestamp(env, authorLogin, currentTargetKey); await recordUnlinkedIssueMatchOccurrence(env, input.repoFullName, input.pullNumber, authorLogin, candidate.issue.number); - if (isRepeat) { + if (priorMatchIso) { + const gapMs = Date.now() - new Date(priorMatchIso).getTime(); + // #4512 velocity exception: gated on CONFIRMED miner identity, not on speed alone -- an unverified + // account repeating this fast is the MORE suspicious case, not less, and still escalates to close. + const velocityExceptionApplies = gapMs >= 0 && gapMs < VELOCITY_EXCEPTION_MAX_GAP_MS && (await isConfirmedOfficialMiner(env, authorLogin).catch(() => false)); + if (!velocityExceptionApplies) { + return { + kind: "close", + reason: `this PR appears to directly solve open issue #${candidate.issue.number} without linking it${evidenceSuffix} — a repeat of the same unlinked-issue pattern already flagged on an earlier PR from this contributor`, + comment: `Closing: this PR doesn't link an issue, but its diff appears to directly solve #${candidate.issue.number} — the same unlinked-issue pattern already flagged on one of your earlier PRs. Please link the issue you're solving (e.g. \`Closes #N\`) going forward.`, + }; + } return { - kind: "close", - reason: `this PR appears to directly solve open issue #${candidate.issue.number} without linking it${evidenceSuffix} — a repeat of the same unlinked-issue pattern already flagged on an earlier PR from this contributor`, - comment: `Closing: this PR doesn't link an issue, but its diff appears to directly solve #${candidate.issue.number} — the same unlinked-issue pattern already flagged on one of your earlier PRs. Please link the issue you're solving (e.g. \`Closes #N\`) going forward.`, + kind: "hold", + reason: `this PR appears to directly solve open issue #${candidate.issue.number} without linking it${evidenceSuffix} — a repeat of the same unlinked-issue pattern flagged on an earlier PR from this contributor within the last hour, held rather than closed pending confirmation this is a genuine tooling issue rather than deliberate repeat abuse`, + comment: `This PR doesn't link an issue, but its diff appears to directly solve #${candidate.issue.number} — the same unlinked-issue pattern was flagged on one of your PRs within the last hour. Please link the issue you're solving (e.g. \`Closes #${candidate.issue.number}\`); repeated occurrences this close together will be reviewed manually rather than closed automatically.`, }; } return { diff --git a/test/unit/unlinked-issue-guardrail.test.ts b/test/unit/unlinked-issue-guardrail.test.ts index 22c3d4fcf6..0c1591d36c 100644 --- a/test/unit/unlinked-issue-guardrail.test.ts +++ b/test/unit/unlinked-issue-guardrail.test.ts @@ -209,6 +209,150 @@ describe("resolveUnlinkedIssueMatchDisposition", () => { expect(result?.kind).toBe("hold"); }); + describe("velocity exception for a CONFIRMED official miner (#4512)", () => { + function stubMinerFetch(githubUsername: string) { + return vi.fn(async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url === "https://api.gittensor.io/miners") return Response.json([{ githubUsername, githubId: "123", totalPrs: 2, totalMergedPrs: 2, isEligible: true, credibility: 1 }]); + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://api.gittensor.io/miners/123") return Response.json({}); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + return Response.json({}); + }); + } + + it("holds (does NOT close) a same-contributor repeat within the last hour when the author is a CONFIRMED official miner", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + vi.stubGlobal("fetch", stubMinerFetch("contributor-a")); + + const first = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(first?.kind).toBe("hold"); + + // Immediately repeated (well within the 1h velocity-exception window) -- confirmed miner, so this + // must hold (with a distinct "held pending confirmation" message), not close. + const second = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, pullNumber: 102, config: config() }); + expect(second?.kind).toBe("hold"); + expect(second?.reason).toContain("within the last hour"); + expect(second?.comment).toContain("reviewed manually"); + + vi.unstubAllGlobals(); + }); + + it("still escalates to a CLOSE for a CONFIRMED miner once the repeat gap exceeds the velocity-exception window", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + vi.stubGlobal("fetch", stubMinerFetch("contributor-a")); + + const first = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(first?.kind).toBe("hold"); + + // Backdate the recorded occurrence by 2 hours -- beyond the 1h velocity-exception window, so even a + // confirmed miner gets the ordinary escalation. + await env.DB.prepare("UPDATE audit_events SET created_at = ? WHERE actor = ?") + .bind(new Date(Date.now() - 2 * 60 * 60 * 1000).toISOString(), "contributor-a") + .run(); + + const second = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, pullNumber: 102, config: config() }); + expect(second?.kind).toBe("close"); + + vi.unstubAllGlobals(); + }); + + it("a THIRD match from the same confirmed miner hits the miner-detection cache instead of re-fetching", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + const fetchMock = stubMinerFetch("contributor-a"); + vi.stubGlobal("fetch", fetchMock); + + const first = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(first?.kind).toBe("hold"); + const second = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, pullNumber: 102, config: config() }); + expect(second?.kind).toBe("hold"); + const fetchCallsAfterSecond = fetchMock.mock.calls.length; + expect(fetchCallsAfterSecond).toBeGreaterThan(0); // the second call did fetch+cache the miner status + + const third = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, pullNumber: 103, config: config() }); + expect(third?.kind).toBe("hold"); + // The miner-detection cache (5m TTL) satisfies the third lookup -- no additional /miners* fetch. + expect(fetchMock.mock.calls.length).toBe(fetchCallsAfterSecond); + }); + + it("a miner-detection cache READ failure falls back to a fresh fetch rather than a false negative", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + vi.stubGlobal("fetch", stubMinerFetch("contributor-a")); + const realPrepare = env.DB.prepare.bind(env.DB); + env.DB.prepare = ((sql: string) => { + if (/SELECT.*FROM.*official_miner_detections/i.test(sql)) throw new Error("d1 down"); + return realPrepare(sql); + }) as typeof env.DB.prepare; + + const first = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(first?.kind).toBe("hold"); + const second = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, pullNumber: 102, config: config() }); + // The cache read is broken, but the fresh fetch still confirms the miner -> velocity exception still applies. + expect(second?.kind).toBe("hold"); + }); + + it("a miner-detection cache WRITE failure still uses the freshly-fetched confirmed status for this call", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + vi.stubGlobal("fetch", stubMinerFetch("contributor-a")); + const realPrepare = env.DB.prepare.bind(env.DB); + env.DB.prepare = ((sql: string) => { + if (/INSERT INTO.*official_miner_detections/i.test(sql)) throw new Error("d1 down"); + return realPrepare(sql); + }) as typeof env.DB.prepare; + + const first = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(first?.kind).toBe("hold"); + const second = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, pullNumber: 102, config: config() }); + expect(second?.kind).toBe("hold"); + }); + + it("does NOT apply the velocity exception when the Gittensor API itself is unavailable (fail-safe: uncertain identity never gets leniency)", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + // The /miners fetch itself fails outright -- fetchOfficialGittensorMiner converts this into + // {status: "unavailable"}, which must never be treated as "confirmed". + vi.stubGlobal("fetch", async () => { + throw new Error("network down"); + }); + + const first = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(first?.kind).toBe("hold"); + const second = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, pullNumber: 102, config: config() }); + expect(second?.kind).toBe("close"); + }); + + it("does NOT apply the velocity exception to an UNCONFIRMED (not_found) author repeating just as fast", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + // /miners returns an empty roster -- contributor-a resolves to "not_found", never "confirmed". + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + return Response.json({}); + }); + + const first = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(first?.kind).toBe("hold"); + + const second = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, pullNumber: 102, config: config() }); + expect(second?.kind).toBe("close"); + + vi.unstubAllGlobals(); + }); + }); + it("does not record an occurrence (and cannot escalate later) when the author login is only whitespace", async () => { const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); const env = createTestEnv({ AI: { run } as unknown as Ai });