Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string | null> {
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
Expand Down
64 changes: 54 additions & 10 deletions src/review/unlinked-issue-guardrail.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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<boolean> {
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 };

Expand All @@ -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<boolean> {
/** 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<string | null> {
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-
Expand Down Expand Up @@ -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 {
Expand Down
144 changes: 144 additions & 0 deletions test/unit/unlinked-issue-guardrail.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down