diff --git a/src/db/repositories.ts b/src/db/repositories.ts index b22e6cc95f..16093dce7f 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -2636,6 +2636,21 @@ export async function mostRecentAuditEventForOtherTarget(env: Env, actor: string return rows[0]?.createdAt ?? null; } +/** Count-returning, cross-repo variant (#4515): how many recent events of this type has this actor generated, + * across EVERY target (repo/PR), within the recency window? Unlike {@link countRecentAuditEventsForActorAndTarget} + * (scoped to one target thread), this counts an actor's ACTIVITY VOLUME irrespective of which PR/repo each + * event landed on -- backs a per-actor rate ceiling on an expensive operation (e.g. a paid AI call) that a + * single-target-scoped counter would never catch for an actor spreading attempts across many repos. */ +export async function countRecentAuditEventsForActor(env: Env, actor: string, eventType: string, sinceIso: string): Promise { + const db = getDb(env.DB); + const [row] = await db + .select({ count: sql`count(*)` }) + .from(auditEvents) + .where(and(eq(auditEvents.actor, actor), eq(auditEvents.eventType, eventType), gte(auditEvents.createdAt, sinceIso))); + /* v8 ignore next -- count(*) always returns exactly one row; the empty-array guard only satisfies the destructure type. */ + return row?.count ?? 0; +} + /** 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 7ca8a07176..aabd127cd2 100644 --- a/src/review/unlinked-issue-guardrail.ts +++ b/src/review/unlinked-issue-guardrail.ts @@ -13,11 +13,21 @@ // 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 { getFreshOfficialMinerDetection, mostRecentAuditEventForOtherTarget, listOpenIssues, recordAuditEvent, upsertOfficialMinerDetection } from "../db/repositories"; +import { + countRecentAuditEventsForActor, + getFreshOfficialMinerDetection, + mostRecentAuditEventForOtherTarget, + listOpenIssues, + recordAiUsageEvent, + recordAuditEvent, + sumAiEstimatedNeuronsSince, + upsertOfficialMinerDetection, +} from "../db/repositories"; import { fetchOfficialGittensorMiner } from "../gittensor/api"; -import { findUnlinkedIssueCandidates, type CandidateOpenIssue } from "../signals/unlinked-issue-candidates"; +import { BEST_REVIEW_MODELS, RELIABLE_FALLBACK_MODELS, clampNumber, estimateNeurons, utcDayStartIso } from "../services/ai-review"; +import { findUnlinkedIssueCandidates, MAX_CANDIDATES, type CandidateOpenIssue } from "../signals/unlinked-issue-candidates"; import type { UnlinkedIssueGuardrailConfig } from "../types"; -import { verifyUnlinkedIssueMatch } from "./unlinked-issue-match"; +import { DIFF_CHAR_BUDGET, MAX_TOKENS, verifyUnlinkedIssueMatch } from "./unlinked-issue-match"; /** Shared with any future reader that wants to correlate these holds/closes across repos for one contributor. */ export const UNLINKED_ISSUE_MATCH_AUDIT_EVENT_TYPE = "github_app.unlinked_issue_match_hold"; @@ -35,6 +45,81 @@ const VELOCITY_EXCEPTION_MAX_GAP_MS = 60 * 60 * 1000; const OFFICIAL_MINER_DETECTION_TTL_MS = 5 * 60 * 1000; const OFFICIAL_MINER_DETECTION_UNAVAILABLE_TTL_MS = 60 * 1000; +// #4515: every candidate below costs one real (if small) AI call, so two cost-control gates run ahead of the +// loop -- a per-actor RATE ceiling, and a check against the shared daily neuron budget every other free-tier +// AI feature draws from (sumAiEstimatedNeuronsSince/AI_DAILY_NEURON_BUDGET, mirroring ai-slop.ts's own +// pre-call budget check). Both are cost controls, not correctness gates: on any read failure they fail +// toward "proceed as if this layer didn't exist" (full verification runs), never toward silently disabling +// the guardrail they sit in front of by skipping straight to `undefined`. +export const UNLINKED_ISSUE_VERIFY_ATTEMPT_AUDIT_EVENT_TYPE = "github_app.unlinked_issue_verify_attempt"; +// Generous by design: a legitimate contributor never approaches this in an hour even opening several PRs +// back-to-back. Sized to catch a scripted/abusive burst hammering the AI verifier, not ordinary human cadence. +const VERIFY_RATE_CEILING_MAX_ATTEMPTS = 15; +const VERIFY_RATE_CEILING_WINDOW_MS = 60 * 60 * 1000; +// Flat overhead for the parts of the verifier's prompt that aren't the (already-bounded) diff -- the system +// prompt, PR title/body, and candidate issue title/body. None of these are cheaply boundable per candidate +// ahead of time, so this deliberately over-, never under-, estimates: a budget check must never undercount. +const VERIFY_PROMPT_OVERHEAD_CHAR_ESTIMATE = 2_000; +// verifyUnlinkedIssueMatch tries a primary model and, ONLY on a thrown error, a fallback -- two calls is the +// real worst case per candidate, not the common case, but this budget check must size for the worst case. +const VERIFY_MAX_MODEL_ATTEMPTS_PER_CANDIDATE = 2; +// Review feedback on #4551: the budget check above reads sumAiEstimatedNeuronsSince, but without a writer +// this feature's own real AI spend never contributed to that counter -- a free rider that respects every +// OTHER feature's usage but never counts its own, so the true aggregate spend could silently exceed +// AI_DAILY_NEURON_BUDGET by however much this feature actually used. recordUnlinkedIssueVerifyUsage (below) +// closes that gap by recording into the SAME shared ai_usage_events table this check reads from. +const UNLINKED_ISSUE_VERIFY_USAGE_FEATURE = "unlinked_issue_verify"; + +/** Has this actor already run the AI verifier at or beyond the rate ceiling in the last window, across every + * repo/PR? Fail-safe: a read error resolves to "not rate-limited," so the pre-#4515 unconditional- + * verification behavior takes over rather than a DB hiccup silently disabling this guardrail. */ +async function isOverUnlinkedIssueVerifyRateCeiling(env: Env, authorLogin: string): Promise { + const sinceIso = new Date(Date.now() - VERIFY_RATE_CEILING_WINDOW_MS).toISOString(); + const count = await countRecentAuditEventsForActor(env, authorLogin, UNLINKED_ISSUE_VERIFY_ATTEMPT_AUDIT_EVENT_TYPE, sinceIso).catch(() => 0); + return count >= VERIFY_RATE_CEILING_MAX_ATTEMPTS; +} + +/** Would verifying `candidateCount` candidates (each up to {@link VERIFY_MAX_MODEL_ATTEMPTS_PER_CANDIDATE} + * model calls) risk exceeding the shared daily AI neuron budget -- the same counter/env var every other + * free-tier AI feature draws from? `candidateCount` is defensively re-clamped to {@link MAX_CANDIDATES}: the + * caller already bounds it there, but this estimate must never balloon even if that invariant ever slips. + * Fail-safe for the same reason as the rate ceiling above: a read error resolves to "budget available." */ +async function isUnlinkedIssueVerifyBudgetExceeded(env: Env, candidateCount: number): Promise { + const worstCaseCandidateCount = Math.min(candidateCount, MAX_CANDIDATES); + const estimatedNeurons = estimateNeurons( + DIFF_CHAR_BUDGET + VERIFY_PROMPT_OVERHEAD_CHAR_ESTIMATE, + MAX_TOKENS, + worstCaseCandidateCount * VERIFY_MAX_MODEL_ATTEMPTS_PER_CANDIDATE, + ); + // Resolved IDENTICALLY to ai-slop.ts's own pre-call check -- both features sum into the same + // sumAiEstimatedNeuronsSince counter, so a divergent default/ceiling here would under- or over-count + // against the one real shared budget. + const rawNeuronBudget = Number(env.AI_DAILY_NEURON_BUDGET); + const budget = clampNumber(env.AI_DAILY_NEURON_BUDGET && Number.isFinite(rawNeuronBudget) ? rawNeuronBudget : 10_000_000, 0, 10_000_000); + const used = await sumAiEstimatedNeuronsSince(env, utcDayStartIso()).catch(() => 0); + const remainingBudget = Math.max(0, budget - used); + return estimatedNeurons > remainingBudget; +} + +/** Record ONE candidate's actual AI spend into the shared `ai_usage_events` ledger -- the SAME table + * {@link isUnlinkedIssueVerifyBudgetExceeded} sums from, so this feature's own usage counts against the + * budget it itself enforces on others (see the module-level comment on {@link UNLINKED_ISSUE_VERIFY_USAGE_FEATURE}). + * Records the same worst-case per-candidate estimate the budget check itself uses -- a deliberate over-count + * (verifyUnlinkedIssueMatch's common case is ONE model call, not the two this sizes for), not a precise + * post-hoc token read, mirroring ai-slop.ts's own pre-computed-estimate recording convention. Best-effort: a + * write failure is swallowed (telemetry must never block the gate). */ +async function recordUnlinkedIssueVerifyUsage(env: Env, repoFullName: string, pullNumber: number): Promise { + const estimatedNeurons = estimateNeurons(DIFF_CHAR_BUDGET + VERIFY_PROMPT_OVERHEAD_CHAR_ESTIMATE, MAX_TOKENS, VERIFY_MAX_MODEL_ATTEMPTS_PER_CANDIDATE); + await recordAiUsageEvent(env, { + feature: UNLINKED_ISSUE_VERIFY_USAGE_FEATURE, + route: "github_app.unlinked_issue_verify", + model: [BEST_REVIEW_MODELS[0], RELIABLE_FALLBACK_MODELS[0]].join("+"), + status: "ok", + estimatedNeurons, + detail: `unlinked-issue-match verification for ${repoFullName}#${pullNumber}`, + }).catch(() => undefined); +} + /** 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. */ @@ -101,6 +186,19 @@ async function recordUnlinkedIssueMatchOccurrence(env: Env, repoFullName: string }).catch(() => undefined); } +/** Record that the AI verifier actually ran against one candidate, so {@link isOverUnlinkedIssueVerifyRateCeiling} + * accumulates this actor's volume correctly across every repo/PR they touch, not just this thread. Fire-and- + * forget, same rationale as {@link recordUnlinkedIssueMatchOccurrence}: a write failure must never block the gate. */ +async function recordUnlinkedIssueVerifyAttempt(env: Env, repoFullName: string, pullNumber: number, authorLogin: string): Promise { + await recordAuditEvent(env, { + eventType: UNLINKED_ISSUE_VERIFY_ATTEMPT_AUDIT_EVENT_TYPE, + actor: authorLogin, + targetKey: unlinkedIssueMatchTargetKey(repoFullName, pullNumber), + outcome: "completed", + detail: "unlinked-issue-match AI verifier invoked", + }).catch(() => undefined); +} + /** * Resolve the unlinked-issue-match disposition for one PR, or `undefined` when nothing should hold or close * it. Checks candidates in the pre-filter's ranked order and acts on the FIRST one that clears @@ -124,7 +222,15 @@ export async function resolveUnlinkedIssueMatchDisposition(env: Env, input: Reso }); if (candidates.length === 0) return undefined; const authorLogin = input.prAuthorLogin?.trim() || null; + // #4515: cost-control gates ahead of the AI loop below. An unidentifiable author can't be rate-limited + // individually (nothing to key the ceiling on), so only the shared budget check applies to them. + if (authorLogin && (await isOverUnlinkedIssueVerifyRateCeiling(env, authorLogin))) return undefined; + if (await isUnlinkedIssueVerifyBudgetExceeded(env, candidates.length)) return undefined; for (const candidate of candidates) { + if (authorLogin) await recordUnlinkedIssueVerifyAttempt(env, input.repoFullName, input.pullNumber, authorLogin); + // Record spend regardless of authorLogin -- an AI call happens either way; only the PER-ACTOR rate + // ceiling above needs a known actor, this shared-budget accounting does not. + await recordUnlinkedIssueVerifyUsage(env, input.repoFullName, input.pullNumber); const verdict = await verifyUnlinkedIssueMatch(env, { prTitle: input.prTitle, prBody: input.prBody, diff --git a/src/review/unlinked-issue-match.ts b/src/review/unlinked-issue-match.ts index 86254394bb..9d5f03efdc 100644 --- a/src/review/unlinked-issue-match.ts +++ b/src/review/unlinked-issue-match.ts @@ -20,9 +20,11 @@ export type UnlinkedIssueMatchVerdict = { const NO_MATCH: UnlinkedIssueMatchVerdict = { matched: false, confidence: 0, evidence: "" }; -const MAX_TOKENS = 400; +// Exported so the guardrail orchestrator (unlinked-issue-guardrail.ts, #4515) can size its own worst-case +// per-PR AI-spend estimate off the same numbers, rather than a second, driftable copy of them. +export const MAX_TOKENS = 400; // This check only needs enough diff to judge scope overlap, not the full multi-file review budget. -const DIFF_CHAR_BUDGET = 6_000; +export const DIFF_CHAR_BUDGET = 6_000; type AiRunner = { run: (model: string, options: unknown, extra?: unknown) => Promise }; diff --git a/src/signals/unlinked-issue-candidates.ts b/src/signals/unlinked-issue-candidates.ts index 491a7a4fd7..b78b330369 100644 --- a/src/signals/unlinked-issue-candidates.ts +++ b/src/signals/unlinked-issue-candidates.ts @@ -27,8 +27,10 @@ export type FindUnlinkedIssueCandidatesInput = { }; // Bound the AI-verifier fan-out per PR: even a repo with hundreds of open issues only ever sends its -// top-scoring handful for a real (paid/self-host-compute) AI call. -const MAX_CANDIDATES = 3; +// top-scoring handful for a real (paid/self-host-compute) AI call. Exported so the guardrail orchestrator +// (unlinked-issue-guardrail.ts, #4515) can size its own worst-case per-PR AI-spend estimate off the same +// number, rather than a second, driftable copy of this constant. +export const MAX_CANDIDATES = 3; // A path/basename mention is a much stronger signal than shared vocabulary — worth several tokens' score, // and (deliberately) enough on its own to qualify a candidate even with zero token overlap (an issue that // names the exact file this PR touches is worth checking regardless of shared wording). diff --git a/test/unit/unlinked-issue-guardrail.test.ts b/test/unit/unlinked-issue-guardrail.test.ts index 0c1591d36c..c2b967f650 100644 --- a/test/unit/unlinked-issue-guardrail.test.ts +++ b/test/unit/unlinked-issue-guardrail.test.ts @@ -1,7 +1,11 @@ import { describe, expect, it, vi } from "vitest"; import { createTestEnv } from "../helpers/d1"; -import { upsertIssueFromGitHub, hasRecentAuditEvent } from "../../src/db/repositories"; -import { resolveUnlinkedIssueMatchDisposition, UNLINKED_ISSUE_MATCH_AUDIT_EVENT_TYPE } from "../../src/review/unlinked-issue-guardrail"; +import { countRecentAuditEventsForActor, recordAuditEvent, sumAiEstimatedNeuronsSince, upsertIssueFromGitHub, hasRecentAuditEvent } from "../../src/db/repositories"; +import { + resolveUnlinkedIssueMatchDisposition, + UNLINKED_ISSUE_MATCH_AUDIT_EVENT_TYPE, + UNLINKED_ISSUE_VERIFY_ATTEMPT_AUDIT_EVENT_TYPE, +} from "../../src/review/unlinked-issue-guardrail"; import type { UnlinkedIssueGuardrailConfig } from "../../src/types"; function config(overrides: Partial = {}): UnlinkedIssueGuardrailConfig { @@ -364,4 +368,149 @@ describe("resolveUnlinkedIssueMatchDisposition", () => { expect(second?.kind).toBe("hold"); }); }); + + describe("cost-control gates ahead of the AI verifier (#4515)", () => { + async function seedVerifyAttempts(env: Awaited>, actor: string, count: number) { + for (let i = 0; i < count; i++) { + await recordAuditEvent(env, { + eventType: UNLINKED_ISSUE_VERIFY_ATTEMPT_AUDIT_EVENT_TYPE, + actor, + targetKey: `owner/other-repo#${900 + i}`, + outcome: "completed", + detail: "seed", + }); + } + } + + it("skips AI verification entirely once the per-actor rate ceiling is already met, even on a brand new PR", 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"); + await seedVerifyAttempts(env, "contributor-a", 15); + + const result = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(result).toBeUndefined(); + expect(run).not.toHaveBeenCalled(); + }); + + it("still verifies normally just below the rate ceiling (boundary check)", 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"); + await seedVerifyAttempts(env, "contributor-a", 14); + + const result = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(result?.kind).toBe("hold"); + expect(run).toHaveBeenCalled(); + }); + + it("does not count a DIFFERENT actor's verify attempts toward this actor's ceiling", 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"); + await seedVerifyAttempts(env, "someone-else", 20); + + const result = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(result?.kind).toBe("hold"); + expect(run).toHaveBeenCalled(); + }); + + it("records a verify-attempt audit event for the candidate it checks, so the ceiling accumulates across separate PRs", 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"); + + await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + + expect(await countRecentAuditEventsForActor(env, "contributor-a", UNLINKED_ISSUE_VERIFY_ATTEMPT_AUDIT_EVENT_TYPE, "2000-01-01T00:00:00.000Z")).toBe(1); + }); + + it("records this candidate's actual spend into the SAME shared ai_usage_events counter the budget check reads (review feedback on #4551)", 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 usedBefore = await sumAiEstimatedNeuronsSince(env, "2000-01-01T00:00:00.000Z"); + expect(usedBefore).toBe(0); + + await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + + const usedAfter = await sumAiEstimatedNeuronsSince(env, "2000-01-01T00:00:00.000Z"); + expect(usedAfter).toBeGreaterThan(0); + }); + + it("records spend even when the author login is unknown (unlike the rate ceiling, this does not need a known actor)", 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"); + + await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, prAuthorLogin: null, config: config() }); + + expect(await sumAiEstimatedNeuronsSince(env, "2000-01-01T00:00:00.000Z")).toBeGreaterThan(0); + }); + + it("swallows a usage-recording write failure without affecting the verification result", 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 realPrepare = env.DB.prepare.bind(env.DB); + env.DB.prepare = ((sql: string) => { + if (/INSERT INTO.*ai_usage_events/i.test(sql)) throw new Error("d1 down"); + return realPrepare(sql); + }) as typeof env.DB.prepare; + + const result = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(result?.kind).toBe("hold"); + }); + + it("fails open (verification still runs) when the rate-ceiling read itself errors", 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 realPrepare = env.DB.prepare.bind(env.DB); + env.DB.prepare = ((sql: string) => { + if (/SELECT.*FROM.*audit_events/i.test(sql)) throw new Error("d1 down"); + return realPrepare(sql); + }) as typeof env.DB.prepare; + + const result = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(result?.kind).toBe("hold"); + expect(run).toHaveBeenCalled(); + }); + + it("skips AI verification entirely when the shared daily neuron budget is exhausted", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai, AI_DAILY_NEURON_BUDGET: "1" }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + + const result = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(result).toBeUndefined(); + expect(run).not.toHaveBeenCalled(); + }); + + it("defaults the shared neuron budget HIGH (10M) when AI_DAILY_NEURON_BUDGET is unset — does not spuriously block verification", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai, AI_DAILY_NEURON_BUDGET: "" }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + + const result = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(result?.kind).toBe("hold"); + expect(run).toHaveBeenCalled(); + }); + + it("fails open (verification still runs) when the shared-budget read itself errors", 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 realPrepare = env.DB.prepare.bind(env.DB); + env.DB.prepare = ((sql: string) => { + if (/SELECT.*FROM.*ai_usage_events/i.test(sql)) throw new Error("d1 down"); + return realPrepare(sql); + }) as typeof env.DB.prepare; + + const result = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(result?.kind).toBe("hold"); + expect(run).toHaveBeenCalled(); + }); + }); });