diff --git a/migrations/0139_review_targets_installation_submitter_idx.sql b/migrations/0139_review_targets_installation_submitter_idx.sql new file mode 100644 index 0000000000..2c092c857a --- /dev/null +++ b/migrations/0139_review_targets_installation_submitter_idx.sql @@ -0,0 +1,8 @@ +-- Supports the install-wide submitter-reputation read (#4513): a fleet identity spreading thin across many +-- repos in one self-hosted install never accumulates enough same-repo sample density for the per-repo +-- reputation signal (src/review/submitter-reputation.ts) to ever fire, since that signal is scoped to +-- `WHERE project = ? AND submitter = ?`. review_targets already carries installation_id (migrations/0050), +-- so a CONFIRMED official Gittensor miner can additionally be evaluated across every repo in the same +-- install via `WHERE installation_id = ? AND submitter = ?` -- this index makes that query as cheap as the +-- existing per-project one instead of a full-table scan. +CREATE INDEX IF NOT EXISTS idx_review_targets_installation_submitter_terminal ON review_targets (installation_id, submitter, terminal_at); diff --git a/src/gittensor/miner-detection-cache.ts b/src/gittensor/miner-detection-cache.ts new file mode 100644 index 0000000000..60ca216919 --- /dev/null +++ b/src/gittensor/miner-detection-cache.ts @@ -0,0 +1,30 @@ +// Minimal, cached "is this login a CONFIRMED official Gittensor miner" check (#4512/#4513), shared by call +// sites that need only a boolean identity check -- not the full audit-logged flow processors.ts's +// getCachedOfficialMinerDetection uses for PR-comment command authorization. Same cache table/TTLs, no +// audit-log side effect. Deliberately its own module (not exported from processors.ts) so review/-layer +// code (unlinked-issue-guardrail.ts, reputation-wire.ts) can import it without a circular dependency — +// processors.ts is the one that imports FROM those modules. + +import { getFreshOfficialMinerDetection, upsertOfficialMinerDetection } from "../db/repositories"; +import { fetchOfficialGittensorMiner } from "./api"; + +const OFFICIAL_MINER_DETECTION_TTL_MS = 5 * 60 * 1000; +const OFFICIAL_MINER_DETECTION_UNAVAILABLE_TTL_MS = 60 * 1000; + +/** Fail-safe: any lookup failure resolves to "not a confirmed miner," never the reverse. */ +export 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"; +} diff --git a/src/queue/processors.ts b/src/queue/processors.ts index e8737e9934..e440df437a 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -519,6 +519,7 @@ import { evaluateWithSurfaceLane } from "../review/content-lane-wire"; import { reviewThreadBlockerFinding } from "../review/review-thread-findings"; import { indexRepo, reindexChangedPaths } from "../review/rag-index"; import { + getEffectiveSubmitterReputation, isReputationEnabled, recordReputationOutcome, shouldSkipAiForReputation, @@ -569,7 +570,7 @@ import { import { neutralHoldReasonCode, nativeGateActionFromConclusion, recordNativeGateDecision } from "../review/parity-wire"; import { recordContributorGateDecision } from "../review/contributor-calibration"; import { recordPredictedGateCalibration } from "../review/predicted-gate-calibration-ledger"; -import { getSubmitterReputation, type SubmissionOutcome } from "../review/submitter-reputation"; +import type { SubmissionOutcome } from "../review/submitter-reputation"; import type { AdvisoryFinding, AiContentBlock, @@ -8498,7 +8499,7 @@ export async function runVisualVisionForAdvisory( ): Promise { if (args.mode === "paused" || args.routes.length === 0) return; try { - const visionReputation = await getSubmitterReputation(env, args.repoFullName, args.author ?? undefined); + const visionReputation = await getEffectiveSubmitterReputation(env, { repoFullName: args.repoFullName, submitter: args.author ?? undefined }); // BYOK resolution mirrors runAiReviewForAdvisory's own (re-resolved per-caller is this codebase's // established convention for this exact 3-line block, not an anti-pattern — see e.g. runAiSlopForAdvisory). const storedVisionKey = diff --git a/src/review/reputation-wire.ts b/src/review/reputation-wire.ts index 04461c1287..5db75d2fa2 100644 --- a/src/review/reputation-wire.ts +++ b/src/review/reputation-wire.ts @@ -13,11 +13,15 @@ // the AI-spend decision (private, server-side) and writes the private submitter_stats table. Fully fail-safe: // the ported module degrades to "neutral" / no-op on any DB error, so this never throws into the gate. +import { getRepository } from "../db/repositories"; +import { isConfirmedOfficialMiner } from "../gittensor/miner-detection-cache"; import { getSubmitterCadence, getSubmitterReputation, + getSubmitterReputationAcrossInstall, isMachinePacedCadence, recordSubmissionOutcome, + type ReputationConfig, type SubmissionOutcome, type SubmitterStats, } from "./submitter-reputation"; @@ -56,9 +60,40 @@ export function shouldDowngradeToDeterministic(stats: SubmitterStats): boolean { } /** - * Flag-gated, fail-safe: read the submitter's INTERNAL reputation and report whether the AI-spend gate should - * 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 + * Resolve the EFFECTIVE reputation signal for a submitter (#4513): the per-repo signal from + * {@link getSubmitterReputation}, additionally widened to an install-wide view for a CONFIRMED official + * Gittensor miner — but ONLY when the per-repo signal alone doesn't already justify caution, so an ordinary + * (non-miner) submitter or one already flagged per-repo pays no extra lookup. Closes a real blind spot: a + * fleet identity spreading thin across many repos in one install never accumulates same-repo sample density, + * so the per-repo-only signal stays permanently "neutral" for it even while it burns full AI-review spend on + * every submission. Fail-safe throughout: an identity-check or install-wide-read failure just keeps the + * per-repo result, never throws, never upgrades a signal the per-repo read didn't already produce. + */ +export async function getEffectiveSubmitterReputation( + env: Env, + args: { repoFullName: string; submitter: string | null | undefined }, + cfg?: ReputationConfig, +): Promise { + const perRepo = await getSubmitterReputation(env, args.repoFullName, args.submitter ?? undefined, cfg); + if (shouldDowngradeToDeterministic(perRepo)) return perRepo; + const submitter = args.submitter?.trim(); + if (!submitter) return perRepo; + /* v8 ignore next -- isConfirmedOfficialMiner already catches every internal failure point itself and never rejects; this guards only a future implementation change. */ + const isMiner = await isConfirmedOfficialMiner(env, submitter).catch(() => false); + if (!isMiner) return perRepo; + const repo = await getRepository(env, args.repoFullName).catch(() => null); + if (!repo?.installationId) return perRepo; + // getSubmitterReputationAcrossInstall already degrades to neutral internally on any read failure (mirrors + // getSubmitterReputation) -- nothing to catch here. + const acrossInstall = await getSubmitterReputationAcrossInstall(env, repo.installationId, submitter, cfg); + return shouldDowngradeToDeterministic(acrossInstall) ? acrossInstall : perRepo; +} + +/** + * Flag-gated, fail-safe: read the submitter's INTERNAL reputation (install-wide-aware for a confirmed miner, + * see {@link getEffectiveSubmitterReputation}) and report whether the AI-spend gate should 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 @@ -72,7 +107,10 @@ export async function shouldSkipAiForReputation( args: { project: string; submitter: string | null | undefined }, ): Promise { if (!isReputationEnabled(env)) return false; - const stats = await getSubmitterReputation(env, args.project, args.submitter ?? undefined); + // Combines both extensions to the base per-repo signal: install-wide widening for a confirmed miner + // (#4513, getEffectiveSubmitterReputation) first, then the cadence check (#4514) as an independent + // second signal -- neither subsumes the other, so both must run, not just whichever merged more recently. + const stats = await getEffectiveSubmitterReputation(env, { repoFullName: args.project, submitter: args.submitter }); 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 04b9c5f19e..9744297ace 100644 --- a/src/review/submitter-reputation.ts +++ b/src/review/submitter-reputation.ts @@ -308,3 +308,42 @@ export async function getSubmitterReputation(env: Env, project: string, submitte const decided = agg.merged + agg.closed; return { ...agg, closeRate: decided > 0 ? agg.closed / decided : 0, signal }; } + +/** Install-wide sibling of {@link getSubmitterReputation} (#4513): the SAME quality-weighted, recency-windowed + * signal derivation, but aggregated across EVERY repo `review_targets` has recorded for this installation_id + * (migrations/0050), not just one project. Closes a real blind spot: a fleet identity spreading thin across + * many repos in one self-hosted install never accumulates same-repo sample density for the per-project + * signal to ever leave "neutral," even while it burns full paid AI-review spend on every submission. Callers + * should reserve this for a CONFIRMED official Gittensor miner identity (this function does not itself check + * that) -- an ordinary contributor's reputation stays intentionally scoped per-repo. The all-time + * submitter_stats aggregate (submissions/merged/closed/manual, /stats-view only, not the signal) is NOT + * widened here: that table is keyed (project, submitter) with no installation_id column, and only the + * SIGNAL — not the display counts — gates the AI-spend decision. Fail-safe: any read error degrades to + * "neutral", identical to the per-project function. */ +export async function getSubmitterReputationAcrossInstall( + env: Env, + installationId: number, + submitter: string | undefined, + cfg: ReputationConfig = DEFAULT_REPUTATION_CONFIG, +): Promise { + const neutral: SubmitterStats = { submissions: 0, merged: 0, closed: 0, manual: 0, closeRate: 0, signal: "neutral" }; + if (!submitter) return neutral; + let signal: ReputationSignal = "neutral"; + try { + const result = await storage(env) + .prepare( + `SELECT status, json_extract(decision_json, '$.reasonCode') AS reasonCode + FROM review_targets + WHERE installation_id = ? AND submitter = ? AND terminal_at IS NOT NULL AND terminal_at >= datetime('now', ?) + ORDER BY terminal_at DESC LIMIT ?`, + ) + .bind(installationId, submitter, `-${cfg.windowDays} days`, REPUTATION_WINDOW_ROW_CAP) + .all<{ status: string; reasonCode: string | null }>(); + /* v8 ignore next -- D1's .all() always populates results; the fallback only protects a driver anomaly. */ + const rows = result?.results ?? []; + signal = signalFromCounts(countOutcomes(rows), cfg); + } catch { + signal = "neutral"; // fail-safe — never throw into the gate. + } + return { ...neutral, signal }; +} diff --git a/test/unit/miner-detection-cache.test.ts b/test/unit/miner-detection-cache.test.ts new file mode 100644 index 0000000000..cd5b7487e6 --- /dev/null +++ b/test/unit/miner-detection-cache.test.ts @@ -0,0 +1,97 @@ +import { describe, expect, it, vi } from "vitest"; +import { isConfirmedOfficialMiner } from "../../src/gittensor/miner-detection-cache"; +import { createTestEnv } from "../helpers/d1"; + +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({}); + }); +} + +describe("isConfirmedOfficialMiner (#4513, shared with #4512's unlinked-issue-guardrail)", () => { + it("resolves true for a login present in the /miners roster", async () => { + const env = createTestEnv(); + vi.stubGlobal("fetch", stubMinerFetch("farmer99")); + try { + expect(await isConfirmedOfficialMiner(env, "farmer99")).toBe(true); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("resolves false for a login absent from an empty /miners roster (not_found)", async () => { + const env = createTestEnv(); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + return Response.json({}); + }); + try { + expect(await isConfirmedOfficialMiner(env, "farmer99")).toBe(false); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("resolves false (fail-safe) when the Gittensor API itself is unavailable", async () => { + const env = createTestEnv(); + vi.stubGlobal("fetch", async () => { + throw new Error("network down"); + }); + try { + expect(await isConfirmedOfficialMiner(env, "farmer99")).toBe(false); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("a second call within the TTL hits the cache instead of re-fetching", async () => { + const env = createTestEnv(); + const fetchMock = stubMinerFetch("farmer99"); + vi.stubGlobal("fetch", fetchMock); + try { + expect(await isConfirmedOfficialMiner(env, "farmer99")).toBe(true); + const callsAfterFirst = fetchMock.mock.calls.length; + expect(callsAfterFirst).toBeGreaterThan(0); + expect(await isConfirmedOfficialMiner(env, "farmer99")).toBe(true); + expect(fetchMock.mock.calls.length).toBe(callsAfterFirst); // cache hit -- no additional fetch + } finally { + vi.unstubAllGlobals(); + } + }); + + it("a cache READ failure falls back to a fresh fetch rather than a false negative", async () => { + const env = createTestEnv(); + vi.stubGlobal("fetch", stubMinerFetch("farmer99")); + 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; + try { + expect(await isConfirmedOfficialMiner(env, "farmer99")).toBe(true); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("a cache WRITE failure still uses the freshly-fetched status for this call", async () => { + const env = createTestEnv(); + vi.stubGlobal("fetch", stubMinerFetch("farmer99")); + 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; + try { + expect(await isConfirmedOfficialMiner(env, "farmer99")).toBe(true); + } finally { + vi.unstubAllGlobals(); + } + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 3b3cdbd153..7228241d93 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -3993,19 +3993,25 @@ describe("queue processors", () => { GITTENSORY_REVIEW_REPUTATION: "true", }); await seedRegateChurnRepo(env); - await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 62, title: "Reputation PR", state: "open", user: { login: "contributor" }, head: { sha: "a62" }, labels: [], body: "Closes #1" }); + // Deliberately NOT "contributor" -- several other tests earlier in this file (e.g. line ~3732) stub the + // Gittensor miners endpoint to confirm "contributor" as an official miner, and that caches a "confirmed" + // official_miner_detections row (5-min TTL) in this file's shared D1 instance. A submitter this test's own + // fetch stub never confirms must still resolve as NOT a miner, or #4513's install-wide widening adds a 4th + // reputation-scan prepare and this invariant's count goes stale for reasons unrelated to what it's testing. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 62, title: "Reputation PR", state: "open", user: { login: "reputation-single-read-user" }, head: { sha: "a62" }, labels: [], body: "Closes #1" }); await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 62, status: "complete", reviewsSyncedAt: new Date().toISOString() }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { const url = input.toString(); const method = init?.method ?? "GET"; if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); if (url.includes("/pulls/62/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); - if (url.endsWith("/pulls/62")) return Response.json({ number: 62, title: "Reputation PR", state: "open", user: { login: "contributor" }, head: { sha: "a62" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.endsWith("/pulls/62")) return Response.json({ number: 62, title: "Reputation PR", state: "open", user: { login: "reputation-single-read-user" }, head: { sha: "a62" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); if (url.includes("/commits/a62/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); if (url.includes("/commits/a62/status")) return Response.json({ state: "success", statuses: [] }); if (url.includes("/issues/62/comments")) return method === "POST" ? Response.json({ id: 62 }, { status: 201 }) : Response.json([]); if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + if (url.includes("/miners")) return Response.json([]); return Response.json({}); }); diff --git a/test/unit/reputation-wiring.test.ts b/test/unit/reputation-wiring.test.ts index 323c40b074..40ce114508 100644 --- a/test/unit/reputation-wiring.test.ts +++ b/test/unit/reputation-wiring.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from "vitest"; import { reputationOutcomeFromTerminalState, runAiReviewForAdvisory, shouldStartAiReviewForAdvisory } from "../../src/queue/processors"; import { + getEffectiveSubmitterReputation, isReputationEnabled, recordReputationOutcome, shouldDowngradeToDeterministic, @@ -12,6 +13,30 @@ import { evaluateGateCheck } from "../../src/rules/advisory"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import type { Advisory, RepositorySettings } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; +import { upsertRepositoryFromGitHub } from "../../src/db/repositories"; + +// Seeds one terminal review_targets row -- the raw table getSubmitterReputation(AcrossInstall) reads from +// (not part of the Drizzle schema; migrations/0050 is the source of truth for these columns). +async function seedReviewTarget( + env: Env, + args: { project: string; repo: string; number: number; installationId: number; submitter: string; status: string; reasonCode?: string | null }, +) { + await env.DB.prepare( + `INSERT INTO review_targets (id, project, kind, repo, number, installation_id, submitter, status, decision_json, terminal_at) + VALUES (?, ?, 'pull_request', ?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP)`, + ) + .bind( + `${args.project}:pull_request:${args.repo}#${args.number}`, + args.project, + args.repo, + args.number, + args.installationId, + args.submitter, + args.status, + args.reasonCode === undefined ? null : JSON.stringify({ reasonCode: args.reasonCode }), + ) + .run(); +} // A submitter who FLOODED the project with submissions but landed almost none — the burst anti-abuse pattern. async function seedSubmitter( @@ -298,6 +323,151 @@ describe("shouldSkipAiForReputation (helper)", () => { }); }); +describe("getEffectiveSubmitterReputation (#4513, install-wide for a confirmed miner)", () => { + function stubMinerFetch(githubUsername: string) { + return 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("widens to the install-wide signal for a CONFIRMED miner when the per-repo signal alone stays neutral", async () => { + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "org/repo-a", owner: { login: "org" } }, 999); + // Only 1 sample on repo-a itself -- per-repo signal stays "neutral" (below minSample). + await seedReviewTarget(env, { project: "org/repo-a", repo: "org/repo-a", number: 1, installationId: 999, submitter: "farmer99", status: "closed", reasonCode: "dual_review_declined" }); + // But spread across OTHER repos in the SAME install, farmer99 has a clear serial-decline pattern. + for (let i = 0; i < 7; i++) { + await seedReviewTarget(env, { project: `org/repo-${i}`, repo: `org/repo-${i}`, number: i + 10, installationId: 999, submitter: "farmer99", status: "closed", reasonCode: "dual_review_declined" }); + } + vi.stubGlobal("fetch", stubMinerFetch("farmer99")); + try { + const rep = await getEffectiveSubmitterReputation(env, { repoFullName: "org/repo-a", submitter: "farmer99" }); + expect(rep.signal).toBe("low"); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("does NOT widen for an UNCONFIRMED submitter with the identical cross-repo pattern -- stays per-repo neutral", async () => { + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "org/repo-a", owner: { login: "org" } }, 999); + await seedReviewTarget(env, { project: "org/repo-a", repo: "org/repo-a", number: 1, installationId: 999, submitter: "farmer99", status: "closed", reasonCode: "dual_review_declined" }); + for (let i = 0; i < 7; i++) { + await seedReviewTarget(env, { project: `org/repo-${i}`, repo: `org/repo-${i}`, number: i + 10, installationId: 999, submitter: "farmer99", status: "closed", reasonCode: "dual_review_declined" }); + } + // /miners returns an empty roster -- farmer99 resolves as "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({}); + }); + try { + const rep = await getEffectiveSubmitterReputation(env, { repoFullName: "org/repo-a", submitter: "farmer99" }); + expect(rep.signal).toBe("neutral"); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("skips the miner-identity lookup entirely when the per-repo signal already justifies downgrading", async () => { + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "org/repo-a", owner: { login: "org" } }, 999); + // A clear per-repo serial-decline pattern on its own -- already "low" without any cross-repo help. + for (let i = 0; i < 7; i++) { + await seedReviewTarget(env, { project: "org/repo-a", repo: "org/repo-a", number: i, installationId: 999, submitter: "farmer99", status: "closed", reasonCode: "dual_review_declined" }); + } + const fetchMock = vi.fn(stubMinerFetch("farmer99")); + vi.stubGlobal("fetch", fetchMock); + try { + const rep = await getEffectiveSubmitterReputation(env, { repoFullName: "org/repo-a", submitter: "farmer99" }); + expect(rep.signal).toBe("low"); + // The miner-identity lookup (and therefore the cross-repo query) never ran -- unnecessary once the + // per-repo signal alone already justifies caution. + expect(fetchMock).not.toHaveBeenCalled(); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("a CONFIRMED miner whose cross-repo history is ALSO clean falls back to the (neutral) per-repo result", async () => { + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "org/repo-a", owner: { login: "org" } }, 999); + // Only 1 sample per-repo AND only 1 sample install-wide -- neither crosses minSample, so both the + // per-repo AND the cross-repo reads land on "neutral"; the ternary's FALSE branch (acrossInstall doesn't + // justify downgrading) must return perRepo, not silently substitute the (also-neutral) acrossInstall. + await seedReviewTarget(env, { project: "org/repo-a", repo: "org/repo-a", number: 1, installationId: 999, submitter: "farmer99", status: "merged", reasonCode: "dual_review_approved" }); + vi.stubGlobal("fetch", stubMinerFetch("farmer99")); + try { + const rep = await getEffectiveSubmitterReputation(env, { repoFullName: "org/repo-a", submitter: "farmer99" }); + expect(rep.signal).toBe("neutral"); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("fails safe to the per-repo result when the repository lookup itself throws", async () => { + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "org/repo-a", owner: { login: "org" } }, 999); + await seedReviewTarget(env, { project: "org/repo-a", repo: "org/repo-a", number: 1, installationId: 999, submitter: "farmer99", status: "merged", reasonCode: "dual_review_approved" }); + vi.stubGlobal("fetch", stubMinerFetch("farmer99")); + const realPrepare = env.DB.prepare.bind(env.DB); + env.DB.prepare = ((sql: string) => { + if (/FROM\s+["`]?repositories["`]?/i.test(sql)) throw new Error("d1 down"); + return realPrepare(sql); + }) as typeof env.DB.prepare; + try { + const rep = await getEffectiveSubmitterReputation(env, { repoFullName: "org/repo-a", submitter: "farmer99" }); + expect(rep.signal).toBe("neutral"); // per-repo result (also neutral here), never throws + } finally { + vi.unstubAllGlobals(); + } + }); + + it("returns the per-repo result unchanged when there is no submitter at all", async () => { + const env = createTestEnv(); + const rep = await getEffectiveSubmitterReputation(env, { repoFullName: "org/repo-a", submitter: undefined }); + expect(rep.signal).toBe("neutral"); + }); + + it("fails safe to the (already-neutral) per-repo result, without throwing, when the repo has no resolvable installationId", async () => { + const env = createTestEnv(); + // repo-a is never registered -> getRepository resolves to null -> nothing to widen with, even though + // farmer99 IS a confirmed miner and has a real cross-repo pattern recorded under installation 999. + await seedReviewTarget(env, { project: "org/repo-a", repo: "org/repo-a", number: 1, installationId: 999, submitter: "farmer99", status: "closed", reasonCode: "dual_review_declined" }); + vi.stubGlobal("fetch", stubMinerFetch("farmer99")); + try { + const rep = await getEffectiveSubmitterReputation(env, { repoFullName: "org/repo-a", submitter: "farmer99" }); + // Only 1 sample, below minSample -> the per-repo signal itself is neutral, and with no installationId to + // widen with the function must return exactly that (not throw, not fabricate a signal). + expect(rep.signal).toBe("neutral"); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("fails safe to the per-repo result, without throwing, when the getRepository read itself errors", async () => { + const env = createTestEnv(); + await seedReviewTarget(env, { project: "org/repo-a", repo: "org/repo-a", number: 1, installationId: 999, submitter: "farmer99", status: "closed", reasonCode: "dual_review_declined" }); + vi.stubGlobal("fetch", stubMinerFetch("farmer99")); + const realPrepare = env.DB.prepare.bind(env.DB); + env.DB.prepare = ((sql: string) => { + if (/FROM.*"?repositories"?/i.test(sql)) throw new Error("d1 down"); + return realPrepare(sql); + }) as typeof env.DB.prepare; + try { + const rep = await getEffectiveSubmitterReputation(env, { repoFullName: "org/repo-a", submitter: "farmer99" }); + expect(rep.signal).toBe("neutral"); + } finally { + vi.unstubAllGlobals(); + } + }); +}); + describe("processGitHubWebhook records the reputation outcome on a terminal PR (flag-ON call site)", () => { it("FLAG-ON: a closed+merged PR webhook records a 'merged' outcome for the submitter", async () => { const { processJob } = await import("../../src/queue/processors"); diff --git a/test/unit/submitter-reputation.test.ts b/test/unit/submitter-reputation.test.ts index 6a18a6a8bc..fe7985f4de 100644 --- a/test/unit/submitter-reputation.test.ts +++ b/test/unit/submitter-reputation.test.ts @@ -6,6 +6,7 @@ import { DEFAULT_REPUTATION_CONFIG, getSubmitterCadence, getSubmitterReputation, + getSubmitterReputationAcrossInstall, isMachinePacedCadence, recordSubmissionOutcome, REPUTATION_WINDOW_DAYS, @@ -374,6 +375,67 @@ describe("getSubmitterCadence (D1, fail-safe) (#4514)", () => { }); }); +describe("getSubmitterReputationAcrossInstall (#4513)", () => { + function makeInstallEnv(opts: { windowRows: Row[]; boundInstallationId?: number[]; throwOnAll?: boolean }): Env { + return { + DB: { + prepare: () => ({ + bind: (installationId: number, _submitter: string, ..._rest: unknown[]) => { + opts.boundInstallationId?.push(installationId); + return { + all: async () => { + if (opts.throwOnAll) throw new Error("D1 boom"); + return { results: opts.windowRows.map((r) => ({ status: r.status, reasonCode: r.reasonCode })) }; + }, + }; + }, + }), + }, + } as unknown as Env; + } + + it("returns neutral with no submitter (early return, no DB touch)", async () => { + const rep = await getSubmitterReputationAcrossInstall({} as Env, 123, undefined); + expect(rep).toEqual({ submissions: 0, merged: 0, closed: 0, manual: 0, closeRate: 0, signal: "neutral" }); + }); + + it("derives the SAME quality-weighted signal as the per-project function, from installation-scoped rows", async () => { + // 8 recent submissions across (implicitly) many repos in the install, almost all genuine declines -> low. + const env = makeInstallEnv({ windowRows: rows(["merged", "dual_review_approved", 1], ["closed", "dual_review_declined", 7]) }); + const rep = await getSubmitterReputationAcrossInstall(env, 123, "farmer99"); + expect(rep.signal).toBe("low"); + }); + + it("binds the installation_id (not a project string) as the scoping parameter", async () => { + const bound: number[] = []; + const env = makeInstallEnv({ windowRows: [], boundInstallationId: bound }); + await getSubmitterReputationAcrossInstall(env, 456, "farmer99"); + expect(bound).toEqual([456]); + }); + + it("does NOT widen the all-time submitter_stats aggregate -- submissions/merged/closed/manual stay zero (signal-only)", async () => { + const env = makeInstallEnv({ windowRows: rows(["merged", "dual_review_approved", 6]) }); + const rep = await getSubmitterReputationAcrossInstall(env, 123, "farmer99"); + expect(rep.submissions).toBe(0); + expect(rep.merged).toBe(0); + expect(rep.closeRate).toBe(0); + }); + + it("fail-safe: degrades to neutral when the install-wide query throws, never throws into the caller", async () => { + const env = makeInstallEnv({ windowRows: [], throwOnAll: true }); + const rep = await getSubmitterReputationAcrossInstall(env, 123, "farmer99"); + expect(rep.signal).toBe("neutral"); + }); + + it("respects a custom windowDays/minSample config the same way the per-project function does", async () => { + const cfg: ReputationConfig = { ...DEFAULT_REPUTATION_CONFIG, minSample: 100 }; + const env = makeInstallEnv({ windowRows: rows(["closed", "dual_review_declined", 8]) }); + const rep = await getSubmitterReputationAcrossInstall(env, 123, "farmer99", cfg); + // Only 8 samples, well under the raised minSample of 100 -> neutral regardless of how bad they look. + expect(rep.signal).toBe("neutral"); + }); +}); + // 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 { diff --git a/test/unit/visual-vision-wiring.test.ts b/test/unit/visual-vision-wiring.test.ts index fb9d7133cd..9acb156981 100644 --- a/test/unit/visual-vision-wiring.test.ts +++ b/test/unit/visual-vision-wiring.test.ts @@ -54,6 +54,20 @@ function stubShotsAndProvider(providerResponseText: string | null) { })); } +/** #4513: getEffectiveSubmitterReputation now checks confirmed-official-miner identity (a fetch to + * api.gittensor.io/miners) whenever a submitter's PER-REPO reputation signal is "neutral" -- i.e. in every + * scenario below that doesn't already mock getSubmitterReputation to a non-neutral signal. That identity + * check is unrelated to whether this function goes on to spend on a vision call, so a bare `vi.fn()` + * asserting NO fetch at all is no longer accurate; this resolves the miner check to "not a miner" (an empty + * roster) so the rest of each test's decline/self-host logic runs exactly as before. */ +function stubMinerCheckOnly() { + return vi.fn(async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + return new Response("not found", { status: 404 }); + }); +} + describe("runVisualVisionForAdvisory", () => { it("no-ops on an empty route list -- never touches D1 or the network", async () => { const env = byokEnv(); @@ -113,7 +127,7 @@ describe("runVisualVisionForAdvisory", () => { it("declines when no route crossed the pixel-diff threshold (no_confirmed_regression) -- never resolves BYOK", async () => { const env = byokEnv(); await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-vision-key", model: null }); - const fetchMock = vi.fn(); + const fetchMock = stubMinerCheckOnly(); vi.stubGlobal("fetch", fetchMock); const adv = findingsHolder(); await runVisualVisionForAdvisory(env, { @@ -127,7 +141,9 @@ describe("runVisualVisionForAdvisory", () => { routes: [route({ path: "/app", beforeUrl: "https://x/gittensory/shot?key=b", afterUrl: "https://x/gittensory/shot?key=a" })], }); expect(adv.findings).toEqual([]); - expect(fetchMock).not.toHaveBeenCalled(); + // The only network activity is the (unrelated) confirmed-official-miner identity check -- never a + // real BYOK/vision-spend call. + expect(fetchMock.mock.calls.map((c) => String(c[0]))).toEqual(["https://api.gittensor.io/miners"]); }); it("declines for a low-reputation submitter even with a confirmed regression and BYOK configured", async () => { @@ -162,7 +178,7 @@ describe("runVisualVisionForAdvisory", () => { it("declines when BYOK is not configured (aiReviewByok off) even with a confirmed regression", async () => { const env = byokEnv(); - const fetchMock = vi.fn(); + const fetchMock = stubMinerCheckOnly(); vi.stubGlobal("fetch", fetchMock); const adv = findingsHolder(); await runVisualVisionForAdvisory(env, { @@ -176,13 +192,13 @@ describe("runVisualVisionForAdvisory", () => { routes: [route({ path: "/app", diffUrl: "https://x/gittensory/shot?key=diff", beforeUrl: "https://x/gittensory/shot?key=b", afterUrl: "https://x/gittensory/shot?key=a" })], }); expect(adv.findings).toEqual([]); - expect(fetchMock).not.toHaveBeenCalled(); + expect(fetchMock.mock.calls.map((c) => String(c[0]))).toEqual(["https://api.gittensor.io/miners"]); }); it("declines when the submitter is not a confirmed contributor, even with BYOK configured", async () => { const env = byokEnv(); await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-vision-key", model: null }); - const fetchMock = vi.fn(); + const fetchMock = stubMinerCheckOnly(); vi.stubGlobal("fetch", fetchMock); const adv = findingsHolder(); await runVisualVisionForAdvisory(env, { @@ -196,13 +212,13 @@ describe("runVisualVisionForAdvisory", () => { routes: [route({ path: "/app", diffUrl: "https://x/gittensory/shot?key=diff", beforeUrl: "https://x/gittensory/shot?key=b", afterUrl: "https://x/gittensory/shot?key=a" })], }); expect(adv.findings).toEqual([]); - expect(fetchMock).not.toHaveBeenCalled(); + expect(fetchMock.mock.calls.map((c) => String(c[0]))).toEqual(["https://api.gittensor.io/miners"]); }); it("skips BYOK (declines, falls back to nothing) when the declared provider doesn't match the stored key", async () => { const env = byokEnv(); await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-vision-key", model: null }); - const fetchMock = vi.fn(); + const fetchMock = stubMinerCheckOnly(); vi.stubGlobal("fetch", fetchMock); const adv = findingsHolder(); await runVisualVisionForAdvisory(env, { @@ -216,7 +232,7 @@ describe("runVisualVisionForAdvisory", () => { routes: [route({ path: "/app", diffUrl: "https://x/gittensory/shot?key=diff", beforeUrl: "https://x/gittensory/shot?key=b", afterUrl: "https://x/gittensory/shot?key=a" })], }); expect(adv.findings).toEqual([]); - expect(fetchMock).not.toHaveBeenCalled(); + expect(fetchMock.mock.calls.map((c) => String(c[0]))).toEqual(["https://api.gittensor.io/miners"]); }); it("calls the BYOK vision provider with before+after images and publishes a returned finding (desktop route)", async () => { @@ -402,7 +418,7 @@ describe("runVisualVisionForAdvisory", () => { const env = byokEnv(); await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-vision-key", model: null }); vi.spyOn(repositories, "getDecryptedRepositoryAiKey").mockRejectedValueOnce(new Error("D1 unavailable")); - const fetchMock = vi.fn(); + const fetchMock = stubMinerCheckOnly(); vi.stubGlobal("fetch", fetchMock); const adv = findingsHolder(); await expect( @@ -418,7 +434,7 @@ describe("runVisualVisionForAdvisory", () => { }), ).resolves.toBeUndefined(); expect(adv.findings).toEqual([]); - expect(fetchMock).not.toHaveBeenCalled(); + expect(fetchMock.mock.calls.map((c) => String(c[0]))).toEqual(["https://api.gittensor.io/miners"]); }); }); @@ -475,7 +491,7 @@ describe("runVisualVisionForAdvisory: self-host local vision provider (#4335)", const runMock = vi.fn(async () => ({ response: findingsResponse([{ path: "/app", body: "should not run" }]) })); const env = byokEnv(); (env as unknown as { AI_VISION: unknown }).AI_VISION = { run: runMock }; - const fetchMock = vi.fn(); + const fetchMock = stubMinerCheckOnly(); vi.stubGlobal("fetch", fetchMock); const adv = findingsHolder(); await runVisualVisionForAdvisory(env, { @@ -489,7 +505,7 @@ describe("runVisualVisionForAdvisory: self-host local vision provider (#4335)", routes: selfHostVisionRoutes(), }); expect(runMock).not.toHaveBeenCalled(); - expect(fetchMock).not.toHaveBeenCalled(); + expect(fetchMock.mock.calls.map((c) => String(c[0]))).toEqual(["https://api.gittensor.io/miners"]); expect(adv.findings).toEqual([]); }); @@ -592,7 +608,7 @@ describe("runVisualVisionForAdvisory: self-host local vision provider (#4335)", it("still declines entirely when NEITHER BYOK nor env.AI_VISION is configured", async () => { const env = byokEnv(); - const fetchMock = vi.fn(); + const fetchMock = stubMinerCheckOnly(); vi.stubGlobal("fetch", fetchMock); const adv = findingsHolder(); await runVisualVisionForAdvisory(env, { @@ -606,6 +622,6 @@ describe("runVisualVisionForAdvisory: self-host local vision provider (#4335)", routes: selfHostVisionRoutes(), }); expect(adv.findings).toEqual([]); - expect(fetchMock).not.toHaveBeenCalled(); + expect(fetchMock.mock.calls.map((c) => String(c[0]))).toEqual(["https://api.gittensor.io/miners"]); }); });