diff --git a/.env.example b/.env.example index 68b959a90b..4c07f51a74 100644 --- a/.env.example +++ b/.env.example @@ -828,6 +828,10 @@ REDIS_URL=redis://redis:6379 # REQUIRED for the self-host review # ORB_COLLECTOR_URL=https://api.loopover.ai/v1/orb/ingest # loopover's hosted collector (default; override for your own) # ORB_COLLECTOR_TOKEN= # bearer credential for a private/self-hosted collector (ORB_COLLECTOR_URL # # above). Unset when using loopover's own hosted collector. +# ORB_COLLECTOR_INSTANCE_SECRET= # per-instance credential returned ONCE by an operator's +# # POST /v1/internal/orb/instances/register call (#9121). +# # Required for the published risk-control guarantee to be +# # accepted from this instance; unset if not opted into that. # # Token broker (optional): get GitHub tokens from the central Orb (you installed the Orb App) instead of running # your own GitHub App. Set the enrollment secret the operator issued for your install; unset = use your own App key. diff --git a/apps/loopover-ui/src/lib/selfhost-env-reference.ts b/apps/loopover-ui/src/lib/selfhost-env-reference.ts index d2455fabdf..432b7741d6 100644 --- a/apps/loopover-ui/src/lib/selfhost-env-reference.ts +++ b/apps/loopover-ui/src/lib/selfhost-env-reference.ts @@ -401,6 +401,10 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [ name: "ORB_BROKER_URL", firstReference: "src/server.ts", }, + { + name: "ORB_COLLECTOR_INSTANCE_SECRET", + firstReference: "src/selfhost/orb-collector.ts", + }, { name: "ORB_COLLECTOR_TOKEN", firstReference: "src/selfhost/orb-collector.ts", @@ -741,6 +745,7 @@ export const SELFHOST_ENV_REFERENCE_MARKDOWN = [ "| `ORB_ANONYMIZE` | `src/selfhost/orb-collector.ts` |", "| `ORB_APP_ID` | `src/selfhost/orb-collector.ts` |", "| `ORB_BROKER_URL` | `src/server.ts` |", + "| `ORB_COLLECTOR_INSTANCE_SECRET` | `src/selfhost/orb-collector.ts` |", "| `ORB_COLLECTOR_TOKEN` | `src/selfhost/orb-collector.ts` |", "| `ORB_COLLECTOR_URL` | `src/selfhost/orb-collector.ts` |", "| `ORB_ENROLLMENT_SECRET` | `src/selfhost/orb-collector.ts` |", diff --git a/migrations/0187_orb_instance_credentials.sql b/migrations/0187_orb_instance_credentials.sql new file mode 100644 index 0000000000..da4e9f4e35 --- /dev/null +++ b/migrations/0187_orb_instance_credentials.sql @@ -0,0 +1,20 @@ +-- #9121: the "registered instance" trust gate on the published risk-control guarantee authenticated +-- nothing -- instance_id was a plain body-supplied field, checked only against the shared FLEET-WIDE +-- ORB_INGEST_TOKEN every exporter holds. Any holder of that token could present ANY registered instance's +-- id and write (or, via an absent arm, DELETE) that instance's published guarantee. This adds a per-instance +-- credential the collector mints at registration time (returned once, in plaintext, then only its hash is +-- kept) and requires it for every risk-control write. Ordinary outcome/health ingest is UNCHANGED -- it +-- stays open by design (see 0061's own comment); only the risk-control write path now checks this. +ALTER TABLE orb_instances ADD COLUMN ingest_secret_hash TEXT; + +-- Per-instance risk-control arms, replacing the two global `system_flags` cells +-- (`riskcontrol:fleet:close` / `riskcontrol:fleet:merge`) that let ANY registered instance overwrite the +-- SAME fleet-wide row. Scoped per instance so one compromised or miscalibrated peer can only ever affect +-- its own row; the public read (loadFleetGuarantee) aggregates across registered instances at query time. +CREATE TABLE IF NOT EXISTS orb_risk_control_arms ( + instance_id TEXT NOT NULL, + arm TEXT NOT NULL, + payload_json TEXT NOT NULL, + updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, + PRIMARY KEY (instance_id, arm) +); diff --git a/migrations/0188_pull_request_issue_author_github_id.sql b/migrations/0188_pull_request_issue_author_github_id.sql new file mode 100644 index 0000000000..137ff5f322 --- /dev/null +++ b/migrations/0188_pull_request_issue_author_github_id.sql @@ -0,0 +1,8 @@ +-- #9125: every contributor-scoped anti-abuse control (the blacklist, open-item caps, moderation tally, +-- review-nag ping counter) keyed on the mutable GitHub login -- the immutable numeric user id was captured +-- on other paths (auth/security.ts, orb/oauth.ts) but discarded at the webhook content-ingest boundary. A +-- banned contributor could clear every one of these controls at once by renaming their account. These +-- columns let the ingest upsert persist the id alongside the login so identity-keyed controls can match on +-- id-when-present, surviving a rename. +ALTER TABLE pull_requests ADD COLUMN author_github_id INTEGER; +ALTER TABLE issues ADD COLUMN author_github_id INTEGER; diff --git a/migrations/0189_submitter_outcome_log.sql b/migrations/0189_submitter_outcome_log.sql new file mode 100644 index 0000000000..09eb2a3eaf --- /dev/null +++ b/migrations/0189_submitter_outcome_log.sql @@ -0,0 +1,18 @@ +-- #9131: submitter_stats.submissions counted webhook PASSES, not submissions -- recordSubmissionOutcome had +-- no per-PR idempotency key, so re-gating the SAME PR (a body edit, a push, or a third party's review +-- comment on a rival's held PR) incremented the counter again every time. This log is the idempotency key: +-- one row per (project, submitter, pull_number, outcome) ever actually counted. recordSubmissionOutcome +-- INSERT OR IGNOREs here first and only bumps submitter_stats when the insert actually created a new row -- +-- so N re-gates of one PR yield exactly one recorded outcome, and a still-open "manual" hold is recorded (at +-- most) once per PR rather than once per re-gate. recorded_at also gives the burst-detection signal a real +-- WINDOW to decay against, instead of reading the all-time submitter_stats aggregate forever. +CREATE TABLE IF NOT EXISTS submitter_outcome_log ( + project TEXT NOT NULL, + submitter TEXT NOT NULL, + pull_number INTEGER NOT NULL, + outcome TEXT NOT NULL, + recorded_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, + PRIMARY KEY (project, submitter, pull_number, outcome) +); + +CREATE INDEX IF NOT EXISTS submitter_outcome_log_window_idx ON submitter_outcome_log(project, submitter, recorded_at); diff --git a/packages/loopover-engine/src/settings/contributor-blacklist.ts b/packages/loopover-engine/src/settings/contributor-blacklist.ts index d4c88828d1..8e6142d992 100644 --- a/packages/loopover-engine/src/settings/contributor-blacklist.ts +++ b/packages/loopover-engine/src/settings/contributor-blacklist.ts @@ -51,6 +51,9 @@ export function normalizeContributorBlacklist(input: unknown): { entries: Contri if (seen.has(key)) continue; // first occurrence wins seen.add(key); const entry: ContributorBlacklistEntry = { login }; + // #9125: an optional immutable id, rename-proofing this entry. Dropped (not widened to 0/negative) if + // malformed, same discipline as every other field here. + if (typeof record.githubId === "number" && Number.isInteger(record.githubId) && record.githubId > 0) entry.githubId = record.githubId; if (typeof record.reason === "string" && record.reason.trim().length > 0) entry.reason = record.reason.trim().slice(0, MAX_REASON_CHARS); if (Array.isArray(record.evidence)) { const evidence = record.evidence.filter((ref): ref is string => typeof ref === "string" && ref.trim().length > 0).map((ref) => ref.trim().slice(0, MAX_EVIDENCE_CHARS)).slice(0, MAX_EVIDENCE); @@ -62,17 +65,33 @@ export function normalizeContributorBlacklist(input: unknown): { entries: Contri return { entries, warnings }; } -/** The blacklist entry matching `login` (case-insensitive), or null. Tolerates an absent list (treated as empty) - * so callers can pass the optional `settings.contributorBlacklist` directly. */ -export function findBlacklistEntry(login: string | null | undefined, entries: ContributorBlacklistEntry[] | undefined): ContributorBlacklistEntry | null { - if (!login) return null; - const key = login.toLowerCase(); - return (entries ?? []).find((entry) => entry.login.toLowerCase() === key) ?? null; +/** + * The blacklist entry matching `login` OR `githubId` (case-insensitive login; exact id), or null. Tolerates + * an absent list (treated as empty) so callers can pass the optional `settings.contributorBlacklist` + * directly. + * + * #9125: matches id-WHEN-PRESENT union login, so a banned contributor cannot clear the block by renaming -- + * GitHub carries the account (and its immutable id) across a rename, so an entry that has captured the id + * still matches under the new login even though `entry.login` itself is now stale. `githubId` is optional + * on BOTH sides (the entry and the call): omit it and this behaves exactly as the login-only match always + * did, so existing entries and callers that haven't threaded an id through yet keep working unchanged. + */ +export function findBlacklistEntry( + login: string | null | undefined, + entries: ContributorBlacklistEntry[] | undefined, + githubId?: number | null | undefined, +): ContributorBlacklistEntry | null { + const key = login ? login.toLowerCase() : null; + return ( + (entries ?? []).find( + (entry) => (typeof githubId === "number" && entry.githubId === githubId) || (key !== null && entry.login.toLowerCase() === key), + ) ?? null + ); } -/** True iff `login` is on the resolved blacklist. */ -export function isAuthorBlacklisted(login: string | null | undefined, entries: ContributorBlacklistEntry[] | undefined): boolean { - return findBlacklistEntry(login, entries) !== null; +/** True iff `login` (or `githubId`) is on the resolved blacklist. */ +export function isAuthorBlacklisted(login: string | null | undefined, entries: ContributorBlacklistEntry[] | undefined, githubId?: number | null | undefined): boolean { + return findBlacklistEntry(login, entries, githubId) !== null; } /** Union multiple blacklist sources (e.g. the shared/global list + the per-repo list) by case-insensitive login. diff --git a/packages/loopover-engine/src/types/manifest-deps-types.ts b/packages/loopover-engine/src/types/manifest-deps-types.ts index 6cb99c9adf..2a38ec0c72 100644 --- a/packages/loopover-engine/src/types/manifest-deps-types.ts +++ b/packages/loopover-engine/src/types/manifest-deps-types.ts @@ -126,6 +126,10 @@ export type AdvisoryAiRoutingConfig = { export type ContributorBlacklistEntry = { login: string; + /** #9125: the login's IMMUTABLE numeric GitHub user id, when the operator has it (e.g. copied from an + * audit event or the API). Optional so existing login-only entries keep working; when present, matching + * is id-when-present UNION login, so a renamed-then-back account still can't shed a ban by renaming. */ + githubId?: number | undefined; /** Why the account is blocked. Free-text maintainer metadata; not published in automated close comments. */ reason?: string | undefined; /** PR/issue URLs (or other maintainer refs) evidencing the block. */ diff --git a/scripts/check-schema-drift.ts b/scripts/check-schema-drift.ts index 3c82f3bc09..075b225305 100644 --- a/scripts/check-schema-drift.ts +++ b/scripts/check-schema-drift.ts @@ -56,6 +56,7 @@ export const RAW_SQL_ONLY_TABLES: Set = new Set([ "orb_pr_outcomes", "orb_relay_failures", "orb_reuse_counters", + "orb_risk_control_arms", "orb_signals", "orb_webhook_events", "override_audit", @@ -66,6 +67,7 @@ export const RAW_SQL_ONLY_TABLES: Set = new Set([ "review_targets", "submission_drafts", "submission_user_tokens", + "submitter_outcome_log", "submitter_stats", "system_flags", "tunables_overrides", diff --git a/src/api/routes.ts b/src/api/routes.ts index 1522a2fd4d..680860d9f7 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -19,9 +19,11 @@ import { buildClearedBrowserSessionCookie, buildClearedGitHubOAuthStateCookie, buildGitHubOAuthStateCookie, + createOpaqueToken, extractBearerToken, extractBrowserSessionToken, extractCookieValue, + hashToken, isAuthorizedGitHubSessionLogin, isMcpReadRepoAllowed, isMcpReadUnscoped, @@ -1667,7 +1669,7 @@ export function createApp() { listInstallationHealth(c.env), listLatestGitHubRateLimitObservations(c.env, 20), ]); - const scope = identity.kind === "session" && !summary.roles.includes("operator") ? await loadControlPanelAccessScope(c.env, identity.actor) : null; + const scope = identity.kind === "session" && !summary.roles.includes("operator") ? await loadControlPanelAccessScope(c.env, identity.actor, identity.session?.githubUserId) : null; const scopedRepoNames = new Set(scope?.repositoryFullNames.map((repo) => repo.toLowerCase()) ?? []); const scopedInstallationIds = new Set(scope?.installationIds ?? []); const scopedAccountLogins = new Set(scope?.accountLogins.map((login) => login.toLowerCase()) ?? []); @@ -2269,7 +2271,7 @@ export function createApp() { ]); // Tenant-scoped identically to /v1/app/maintainer-dashboard (#7659) -- a non-operator session must // only ever see their own repositories/installations/rate-limit telemetry, never the full fleet. - const scope = identity.kind === "session" && !summary.roles.includes("operator") ? await loadControlPanelAccessScope(c.env, identity.actor) : null; + const scope = identity.kind === "session" && !summary.roles.includes("operator") ? await loadControlPanelAccessScope(c.env, identity.actor, identity.session?.githubUserId) : null; const scopedRepoNames = new Set(scope?.repositoryFullNames.map((repo) => repo.toLowerCase()) ?? []); const scopedInstallationIds = new Set(scope?.installationIds ?? []); const scopedAccountLogins = new Set(scope?.accountLogins.map((accountLogin) => accountLogin.toLowerCase()) ?? []); @@ -4431,8 +4433,11 @@ export function createApp() { const body = await readOrbIngestBody(c.req.raw, c.req.header("content-length")); if (body === null) return c.json({ error: "payload_too_large" }, 413); if (!body) return c.json({ error: "invalid_request" }, 400); - const result = await handleOrbIngest(body, c.env.DB); - if ("error" in result) return c.json(result, 400); + // #9121: the per-instance credential proving THIS sender is the registered instance it claims to be in + // the body — distinct from the shared fleet-wide bearer token checked above, which proves only "some + // fleet member". Absent for an unregistered instance or one that hasn't (re-)registered since #9121. + const result = await handleOrbIngest(body, c.env.DB, c.req.header("x-orb-instance-secret")); + if ("error" in result) return c.json(result, result.error === "instance_unauthenticated" ? 403 : 400); return c.json(result, 200); }); @@ -4518,20 +4523,29 @@ export function createApp() { // Opt an instance into (or out of) fleet calibration. Body: { instanceId, registered? } (registered // defaults true). Upserts so an operator can register an instance that has ingested but isn't recorded yet. + // + // #9121: registering ALSO mints a fresh per-instance ingest credential — the only way "registered" can + // mean anything on the risk-control write path is if the identity it trusts is proven by a secret only + // the real instance holds, not merely claimed in the request body. Returned ONCE, in plaintext, here; + // only its hash is ever persisted, so the operator must copy it into the instance's config now (as + // ORB_COLLECTOR_INSTANCE_SECRET) — a repeat register call rotates it, invalidating the previous value. app.post("/v1/internal/orb/instances/register", async (c) => { const payload = (await c.req.json().catch(() => null)) as { instanceId?: unknown; registered?: unknown } | null; const instanceId = typeof payload?.instanceId === "string" ? payload.instanceId : ""; if (!instanceId) return c.json({ error: "instanceId required" }, 400); const registered = payload?.registered === false ? 0 : 1; + const instanceSecret = registered === 1 ? createOpaqueToken("orbis") : null; + const instanceSecretHash = instanceSecret ? await hashToken(instanceSecret) : null; await c.env.DB .prepare( - `INSERT INTO orb_instances (instance_id, registered, registered_at) VALUES (?, ?, CURRENT_TIMESTAMP) + `INSERT INTO orb_instances (instance_id, registered, registered_at, ingest_secret_hash) VALUES (?, ?, CURRENT_TIMESTAMP, ?) ON CONFLICT(instance_id) DO UPDATE SET registered = excluded.registered, - registered_at = CASE WHEN excluded.registered = 1 THEN CURRENT_TIMESTAMP ELSE NULL END`, + registered_at = CASE WHEN excluded.registered = 1 THEN CURRENT_TIMESTAMP ELSE NULL END, + ingest_secret_hash = CASE WHEN excluded.registered = 1 THEN excluded.ingest_secret_hash ELSE orb_instances.ingest_secret_hash END`, ) - .bind(instanceId, registered) + .bind(instanceId, registered, instanceSecretHash) .run(); - return c.json({ instanceId, registered: registered === 1 }); + return c.json({ instanceId, registered: registered === 1, ...(instanceSecret ? { instanceSecret } : {}) }); }); // Central Orb GitHub App installation registry — the onboarding gate. Every installation the Orb App webhook @@ -5400,12 +5414,12 @@ function authRedirectWithError(env: Env, reason: string): string { } async function buildSessionResponse(env: Env, identity: Extract) { - const roleSummary = await loadControlPanelRoleSummary(env, identity.actor); + const roleSummary = await loadControlPanelRoleSummary(env, identity.actor, identity.session?.githubUserId); return { status: "authenticated", login: identity.session.login, - githubId: identity.session.githubUserId ?? null, - github_id: identity.session.githubUserId ?? null, + githubId: identity.session?.githubUserId ?? null, + github_id: identity.session?.githubUserId ?? null, roles: roleSummary.roles, roleSummary, confirmedMiner: roleSummary.confirmedMiner, @@ -6255,7 +6269,7 @@ type ProtectedRouteContext = { // handler; it only decides whether a session may REACH a path — the per-route guards above enforce the // actual identity/repo scope. A path added here MUST be scoped by a per-route guard in its handler. function canSessionAccessPath(env: Env, identity: Extract, path: string): boolean { - if (isAuthorizedGitHubSessionLogin(env, identity.actor)) return true; + if (isAuthorizedGitHubSessionLogin(env, identity.actor, identity.session?.githubUserId)) return true; if (path.startsWith("/v1/app/")) return true; if (isIssueQualityPath(path)) return true; if (isRepoSettingsPath(path)) return true; @@ -6414,7 +6428,7 @@ async function authenticateRequestIdentity(c: ProtectedRouteContext): Promise allowedRoles.includes(role)) ? null : c.json({ error: "insufficient_role" }, 403); } @@ -6449,7 +6463,7 @@ async function resolveAppInstallationScope( } const scope = identity.kind === "session" && !summary.roles.includes("operator") - ? await loadControlPanelAccessScope(c.env, identity.actor) + ? await loadControlPanelAccessScope(c.env, identity.actor, identity.session?.githubUserId) : null; return { identity, scope }; } @@ -6520,8 +6534,8 @@ async function requireCommandPreviewRepoAccess( async function requireDiscoveryAccessForApi(c: ProtectedRouteContext, identity: AuthIdentity): Promise { if (identity.kind === "session") { - if (isAuthorizedGitHubSessionLogin(c.env, identity.actor)) return null; - const scope = await loadControlPanelAccessScope(c.env, identity.actor); + if (isAuthorizedGitHubSessionLogin(c.env, identity.actor, identity.session?.githubUserId)) return null; + const scope = await loadControlPanelAccessScope(c.env, identity.actor, identity.session?.githubUserId); if (scope.operator) return null; return c.json({ error: "forbidden", reason: "cross_repo_search_requires_discovery_access" }, 403); } @@ -6532,7 +6546,7 @@ async function requireDiscoveryAccessForApi(c: ProtectedRouteContext, identity: } async function canApiAccessRepo(env: Env, identity: AuthIdentity, repoFullName: string): Promise { - if (identity.kind === "session") return canLoginAccessRepo(env, identity.actor, repoFullName); + if (identity.kind === "session") return canLoginAccessRepo(env, identity.actor, repoFullName, identity.session?.githubUserId); if (identity.kind === "static" && identity.actor === "mcp") { return isMcpReadRepoAllowed(env.MCP_READ_REPO_ALLOWLIST, repoFullName); } @@ -6554,9 +6568,9 @@ async function requireSessionRepoAccess( repoFullName: string, repo: RepositoryRecord | null, ): Promise { - const summary = await loadControlPanelRoleSummary(c.env, identity.actor); + const summary = await loadControlPanelRoleSummary(c.env, identity.actor, identity.session?.githubUserId); if (summary.roles.includes("operator")) return null; - const scope = await loadControlPanelAccessScope(c.env, identity.actor); + const scope = await loadControlPanelAccessScope(c.env, identity.actor, identity.session?.githubUserId); const requestedRepo = repoFullName.toLowerCase(); const scopedRepoNames = new Set(scope.repositoryFullNames.map((name) => name.toLowerCase())); if (scopedRepoNames.has(requestedRepo)) return null; @@ -6592,7 +6606,7 @@ async function requireRepoWriteAccess(c: ProtectedRouteContext, fullName: string const gate = await requireRepoMaintainer(c, fullName); if (gate instanceof Response) return gate; if (gate.identity?.kind !== "session") return gate; // server-to-server token: no per-repo push check - const summary = await loadControlPanelRoleSummary(c.env, gate.identity.actor); + const summary = await loadControlPanelRoleSummary(c.env, gate.identity.actor, gate.identity.session?.githubUserId); if (summary.roles.includes("operator")) return gate; // operators manage any repo const repo = await getRepository(c.env, fullName); const installationId = repo?.installationId ?? null; @@ -6618,7 +6632,7 @@ async function skippedPrAuditRepoScope( requestedRepo: string | undefined, ): Promise { if (identity.kind !== "session" || roles.includes("operator")) return requestedRepo ? [requestedRepo] : undefined; - const scope = await loadControlPanelAccessScope(c.env, identity.actor); + const scope = await loadControlPanelAccessScope(c.env, identity.actor, identity.session?.githubUserId); const scopedRepoNames = new Set(scope.repositoryFullNames.map((name) => name.toLowerCase())); if (requestedRepo) { return scopedRepoNames.has(requestedRepo.toLowerCase()) ? [requestedRepo] : c.json({ error: "forbidden_repo" }, 403); diff --git a/src/auth/security.ts b/src/auth/security.ts index 78f6939a6e..d1fc0f059c 100644 --- a/src/auth/security.ts +++ b/src/auth/security.ts @@ -135,12 +135,43 @@ export async function authenticateSessionToken(env: Env, token: string | undefin return { kind: "session", actor: session.login, session }; } -export function isAuthorizedGitHubSessionLogin(env: Env, login: string): boolean { +/** + * #9126: fleet-operator trust by LOGIN ALONE is a released-handle takeover risk — GitHub lets a login be + * renamed or an account be deleted, after which the bare handle can be re-registered by anyone. Configuring + * `ADMIN_GITHUB_IDS` binds trust to the session's IMMUTABLE `githubUserId` instead, mirroring the one place + * in the codebase that already got this right ({@link isPerTenantAdmin}'s live-permission path, and + * `verifyInstallationAdmin` in src/orb/oauth.ts, which binds to the immutable `account_id`). When an id + * allowlist is configured it is authoritative — a matching LOGIN with a non-matching (or absent) id is + * DENIED, since that is exactly the released-handle-takeover shape this closes. Falls back to login-only + * when `ADMIN_GITHUB_IDS` is unset, so an existing self-host deployment that hasn't configured it keeps + * working byte-identically. + */ +export function isAuthorizedGitHubSessionLogin(env: Env, login: string, githubUserId?: number | null | undefined): boolean { + const allowedIds = parseGitHubIdList(env.ADMIN_GITHUB_IDS); + if (allowedIds.size > 0) return typeof githubUserId === "number" && allowedIds.has(githubUserId); const allowedLogins = parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS); if (allowedLogins.size === 0) return false; return allowedLogins.has(login.toLowerCase()); } +/** Parse a numeric GitHub-user-id allowlist env (`ADMIN_GITHUB_IDS`) into a Set. Same whitespace-OR-comma + * convention as {@link parseGitHubLoginList}; a non-numeric or non-finite entry is dropped rather than + * widening the match. */ +export function parseGitHubIdList(value: string | undefined): Set { + return new Set( + (value ?? "") + .split(/[\s,]+/) + .map((entry) => entry.trim()) + // `Number("")` is 0, not NaN -- an unset/blank env var must parse to an EMPTY set (no id "0"), or an + // env with no ADMIN_GITHUB_IDS configured at all would wrongly treat id 0 as configured and deny every + // real operator whose id is never 0. Drop blanks before the numeric parse, mirroring + // parseGitHubLoginList's `.filter(Boolean)` after trim. + .filter(Boolean) + .map((entry) => Number(entry)) + .filter((id) => Number.isFinite(id) && Number.isInteger(id)), + ); +} + /** #4889 hosted per-repo admin mode. When ON, the global ADMIN_GITHUB_LOGINS allowlist stops granting * fleet-wide maintainer trust at the review/queue exemption sites — each consults the live per-repo GitHub * permission instead ({@link isPerTenantAdmin}). OFF (the default) keeps self-host's existing diff --git a/src/db/repositories.ts b/src/db/repositories.ts index d300e380de..812382fc69 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -482,6 +482,7 @@ export async function upsertPullRequestFromGitHub( title: resolvedTitle, state: resolvedState, authorLogin: pr.user?.login, + authorGithubId: pr.user?.id, authorAssociation: resolvedAuthorAssociation, headSha: resolvedHeadSha, headRef: resolvedHeadRef, @@ -511,6 +512,7 @@ export async function upsertPullRequestFromGitHub( title: resolvedTitle, state: resolvedState, authorLogin: pr.user?.login, + authorGithubId: pr.user?.id, authorAssociation: resolvedAuthorAssociation, headSha: resolvedHeadSha, headRef: resolvedHeadRef, @@ -614,6 +616,7 @@ export async function upsertIssueFromGitHub(env: Env, repoFullName: string, issu title: issue.title, state: resolvedState, authorLogin: issue.user?.login, + authorGithubId: issue.user?.id, authorAssociation: issue.author_association, htmlUrl: issue.html_url, labelsJson: resolvedLabelsJson, @@ -629,6 +632,7 @@ export async function upsertIssueFromGitHub(env: Env, repoFullName: string, issu title: issue.title, state: resolvedState, authorLogin: issue.user?.login, + authorGithubId: issue.user?.id, authorAssociation: issue.author_association, htmlUrl: issue.html_url, labelsJson: resolvedLabelsJson, @@ -4752,12 +4756,25 @@ export async function listOtherOpenPullRequests(env: Env, fullName: string, numb return rows.map(toPullRequestRecordFromRow); } -export async function listOtherOpenPullRequestsForAuthor(env: Env, fullName: string, number: number, authorLogin: string): Promise { - const db = getDb(env.DB); +// #9125: `authorGithubId` is optional and ADDITIVE -- when the caller has it, a sibling PR matches on the +// immutable id OR the (renameable) login, so a contributor who renamed between two PRs still gets counted +// against their own cap. Omit it and this behaves exactly as the login-only match always did. +export async function listOtherOpenPullRequestsForAuthor( + env: Env, + fullName: string, + number: number, + authorLogin: string, + authorGithubId?: number | null, +): Promise { + const db = getDb(env.DB); + const authorMatch = + typeof authorGithubId === "number" + ? or(sql`lower(${pullRequests.authorLogin}) = lower(${authorLogin})`, eq(pullRequests.authorGithubId, authorGithubId)) + : sql`lower(${pullRequests.authorLogin}) = lower(${authorLogin})`; const rows = await db .select() .from(pullRequests) - .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.state, "open"), not(eq(pullRequests.number, number)), sql`lower(${pullRequests.authorLogin}) = lower(${authorLogin})`)) + .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.state, "open"), not(eq(pullRequests.number, number)), authorMatch)) // Keep the per-webhook live-verification and sibling-wake work budget fixed. The cap path only needs the // lowest-numbered siblings to preserve the "oldest PRs win" rule, and wake coalescing can discover later // over-cap siblings from their own deliveries without letting one delivery fan out across an unbounded set. @@ -6915,6 +6932,7 @@ function toPullRequestRecord(repoFullName: string, pr: GitHubPullRequestPayload) title: pr.title, state: pr.state, authorLogin: pr.user?.login, + authorGithubId: pr.user?.id, authorAssociation: pr.author_association, headSha: pr.head?.sha, headRef: pr.head?.ref, @@ -6952,6 +6970,7 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull title: row.title, state: row.state, authorLogin: row.authorLogin, + authorGithubId: row.authorGithubId, authorAssociation: row.authorAssociation, headSha: row.headSha, headRef: row.headRef, @@ -7040,6 +7059,7 @@ function toIssueRecord(repoFullName: string, issue: GitHubIssuePayload): IssueRe title: issue.title, state: issue.state, authorLogin: issue.user?.login, + authorGithubId: issue.user?.id, authorAssociation: issue.author_association, htmlUrl: issue.html_url, body: issue.body, @@ -7114,6 +7134,7 @@ function toIssueRecordFromRow(row: typeof issues.$inferSelect): IssueRecord { title: row.title, state: row.state, authorLogin: row.authorLogin, + authorGithubId: row.authorGithubId, authorAssociation: row.authorAssociation, htmlUrl: row.htmlUrl, body: payload.body, diff --git a/src/db/schema.ts b/src/db/schema.ts index 6ff68aa72f..c79a155791 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -343,6 +343,10 @@ export const pullRequests = sqliteTable( title: text("title").notNull(), state: text("state").notNull(), authorLogin: text("author_login"), + // #9125: the login's IMMUTABLE numeric id, from the webhook payload's `user.id` -- a login can be + // renamed or the account deleted-and-re-registered by anyone; every contributor-scoped control (the + // blacklist, open-item caps, moderation tally) must be able to key on this instead to survive a rename. + authorGithubId: integer("author_github_id"), authorAssociation: text("author_association"), headSha: text("head_sha"), headRef: text("head_ref"), @@ -554,6 +558,8 @@ export const issues = sqliteTable( title: text("title").notNull(), state: text("state").notNull(), authorLogin: text("author_login"), + // #9125: mirrors pull_requests.author_github_id -- the immutable id behind the (renameable) login. + authorGithubId: integer("author_github_id"), authorAssociation: text("author_association"), htmlUrl: text("html_url"), labelsJson: text("labels_json").notNull().default("[]"), diff --git a/src/env.d.ts b/src/env.d.ts index ea6ee7ef44..be7dd9189c 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -138,6 +138,11 @@ declare global { onMerge?: import("./services/ai-review").OnMerge | undefined; }; ADMIN_GITHUB_LOGINS?: string; + /** #9126: numeric GitHub user-id allowlist binding fleet-operator trust to the session's IMMUTABLE id + * rather than the renameable/re-registerable ADMIN_GITHUB_LOGINS login. When set and non-empty it is + * AUTHORITATIVE — see isAuthorizedGitHubSessionLogin (src/auth/security.ts). Unset falls back to the + * login-only allowlist, byte-identical to before this existed. */ + ADMIN_GITHUB_IDS?: string; /** Install-wide contributor open-item cap (#2562, anti-abuse): the max PRs+issues a single non-owner/ * admin/bot contributor may have open ACROSS EVERY repo this install gates, combined. Purely an * install-scoped aggregate over this same database (no cross-instance networking) -- catches an actor diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 09006f4974..1939307af2 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -3375,7 +3375,7 @@ export class LoopoverMcp { } return; } - const summary = await loadControlPanelRoleSummary(this.env, this.identity.actor); + const summary = await loadControlPanelRoleSummary(this.env, this.identity.actor, this.identity.session?.githubUserId); if (!summary.roles.some((role) => role === "maintainer" || role === "owner" || role === "operator")) { throw new Error("Forbidden: maintainer, owner, or operator role is required for focus-manifest (insufficient_role)."); } @@ -3452,13 +3452,13 @@ export class LoopoverMcp { // check this function could add — a static mcp caller can only ever reach here already fully trusted. (#2455) private async requireWatchableRepo(login: string, repoFullName: string): Promise { if (this.identity.kind !== "session") return; - if (await canWatchRepo(this.env, login, repoFullName)) return; + if (await canWatchRepo(this.env, login, repoFullName, this.identity.session?.githubUserId)) return; throw new Error("Forbidden: session cannot watch this repository."); } private loadSessionAccessScope(): Promise { if (this.identity.kind !== "session") throw new Error("Session access scope is only available for session identities."); - this.accessScopePromise ??= loadControlPanelAccessScope(this.env, this.identity.actor); + this.accessScopePromise ??= loadControlPanelAccessScope(this.env, this.identity.actor, this.identity.session?.githubUserId); return this.accessScopePromise; } @@ -3944,7 +3944,7 @@ export class LoopoverMcp { /** Cross-repo search requires unscoped MCP read (wildcard allowlist) or operator/session authority. */ private async requireDiscoveryAccess(): Promise { if (this.identity.kind === "session") { - if (isAuthorizedGitHubSessionLogin(this.env, this.identity.actor)) return; + if (isAuthorizedGitHubSessionLogin(this.env, this.identity.actor, this.identity.session?.githubUserId)) return; const scope = await this.loadSessionAccessScope(); if (scope.operator) return; throw new Error("Forbidden: cross-repo opportunity search requires operator or unscoped MCP read access."); @@ -4142,7 +4142,7 @@ export class LoopoverMcp { } private async canAccessRepo(fullName: string): Promise { - if (this.identity.kind === "session") return canLoginAccessRepo(this.env, this.identity.actor, fullName); + if (this.identity.kind === "session") return canLoginAccessRepo(this.env, this.identity.actor, fullName, this.identity.session?.githubUserId); // The static `mcp` identity is a shared, end-user-obtainable CLI credential — scope it to the operator's // MCP_READ_REPO_ALLOWLIST instead of trusting it for every installed repo, mirroring requireRepoManageAccess's // MCP_ACTUATION_REPO_ALLOWLIST scoping for writes. api/internal static identities remain trusted (operator-only @@ -4234,7 +4234,7 @@ export class LoopoverMcp { // unscoped MCP_READ_REPO_ALLOWLIST wildcard, matching requireOperatorAccess/requireDiscoveryAccess above. private async requireSkippedPrAuditAccess(requestedRepo: string | undefined): Promise { if (this.identity.kind === "session") { - const [summary, scope] = await Promise.all([loadControlPanelRoleSummary(this.env, this.identity.actor), this.loadSessionAccessScope()]); + const [summary, scope] = await Promise.all([loadControlPanelRoleSummary(this.env, this.identity.actor, this.identity.session?.githubUserId), this.loadSessionAccessScope()]); if (!summary.roles.some((role) => role === "maintainer" || role === "owner" || role === "operator")) { throw new Error("Forbidden: maintainer, owner, or operator role is required for the skipped-PR audit."); } @@ -5532,7 +5532,7 @@ function redactSensitiveForMcp(value: unknown): unknown { async function authenticateMcpRequest(c: AppContext): Promise { const identity = await authenticatePrivateToken(c.env, extractBearerToken(c.req.header("authorization"))); if (!identity || identity.kind !== "session") return identity; - const summary = await loadControlPanelRoleSummary(c.env, identity.actor); + const summary = await loadControlPanelRoleSummary(c.env, identity.actor, identity.session?.githubUserId); return summary.roles.length > 0 ? identity : null; } diff --git a/src/orb/ingest.ts b/src/orb/ingest.ts index d0570da415..005d936fc0 100644 --- a/src/orb/ingest.ts +++ b/src/orb/ingest.ts @@ -2,6 +2,7 @@ // Accepts anonymized, reversal-aware outcome batches from self-hosted instances (exportOrbBatch). // No raw repo names, owner identifiers, commit SHAs, or PR content — only HMAC-anonymized hashes + // aggregate calibration metadata (verdict, outcome, reversal, bucketed reason, cycle time). +import { hashToken } from "../auth/security"; const MAX_BATCH = 500; const MAX_INSTANCE_ID_CHARS = 64; @@ -103,7 +104,7 @@ function clampCycleMs(value: unknown): number | null { return Math.round(value); } -export async function handleOrbIngest(body: string, db: D1Database): Promise { +export async function handleOrbIngest(body: string, db: D1Database, presentedInstanceSecret?: string): Promise { let payload: unknown; try { payload = JSON.parse(body); @@ -208,25 +209,44 @@ export async function handleOrbIngest(body: string, db: D1Database): Promise(); - if (registeredRow?.registered === 1) { + const instanceRow = await db + .prepare("SELECT registered, ingest_secret_hash FROM orb_instances WHERE instance_id = ?") + .bind(instance_id) + .first<{ registered: number; ingest_secret_hash: string | null }>(); + if (instanceRow?.registered === 1) { + // A registered instance's identity must be PROVEN by its own credential, not merely claimed in the + // body — any holder of the shared fleet-wide bearer token could otherwise present ANY registered + // instance_id. An instance registered before this credential existed (or not yet re-registered to + // mint one) has no hash to check against, so the write is refused rather than silently trusted. + const presentedHash = presentedInstanceSecret ? await hashToken(presentedInstanceSecret) : null; + const authenticated = Boolean(instanceRow.ingest_secret_hash) && presentedHash === instanceRow.ingest_secret_hash; + if (!authenticated) return { error: "instance_unauthenticated" }; for (const arm of ["close", "merge"]) { + // An ABSENT key is "no change" (this ingest tick had nothing new to say about the arm) — NEVER a + // retraction. Only an EXPLICIT `null` retracts, so a truncated, partial, or older-schema payload + // can't silently delete a live guarantee (#9121). const value = (riskControl as Record)[arm]; - if (value !== undefined && value !== null && typeof value === "object") { + if (value === undefined) continue; + if (value === null) { + await db.prepare(`DELETE FROM orb_risk_control_arms WHERE instance_id = ? AND arm = ?`).bind(instance_id, arm).run(); + } else if (typeof value === "object" && !Array.isArray(value)) { await db - .prepare(`INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES (?, ?, CURRENT_TIMESTAMP)`) - .bind(`riskcontrol:fleet:${arm}`, JSON.stringify(value).slice(0, 2000)) + .prepare( + `INSERT INTO orb_risk_control_arms (instance_id, arm, payload_json, updated_at) VALUES (?, ?, ?, CURRENT_TIMESTAMP) + ON CONFLICT(instance_id, arm) DO UPDATE SET payload_json = excluded.payload_json, updated_at = CURRENT_TIMESTAMP`, + ) + .bind(instance_id, arm, JSON.stringify(value).slice(0, 2000)) .run(); - } else { - // The sender no longer publishes this arm — retract the fleet copy too (stale guarantees lie). - await db.prepare(`DELETE FROM system_flags WHERE key = ?`).bind(`riskcontrol:fleet:${arm}`).run(); } } } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 418a68d2b9..0913fb3162 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2734,7 +2734,7 @@ async function resolvePerRepoContributorCapMatch( ? Math.max(1, Math.ceil(settings.contributorOpenPrCap / 2)) : settings.contributorOpenPrCap; if (typeof contributorOpenPrCap === "number" && pr.authorLogin && !isAutoCloseExempt(pr.authorLogin, settings.autoCloseExemptLogins)) { - const otherAuthorOpenPullRequests = await listOtherOpenPullRequestsForAuthor(env, repoFullName, pr.number, pr.authorLogin); + const otherAuthorOpenPullRequests = await listOtherOpenPullRequestsForAuthor(env, repoFullName, pr.number, pr.authorLogin, pr.authorGithubId); const confirmedOpen = new Set(); await mapWithConcurrency(otherAuthorOpenPullRequests, CONTRIBUTOR_CAP_LIVE_CHECK_CONCURRENCY, async (other) => { const liveState = await fetchLivePullRequestState(env, repoFullName, other.number, token, admissionKey).catch(() => undefined); @@ -3196,6 +3196,7 @@ async function runAgentMaintenancePlanAndExecute( const blacklistEntry = findBlacklistEntry( pr.authorLogin, settings.contributorBlacklist, + pr.authorGithubId, ); // Screenshot-table gate (#2006): a DETERMINISTIC check (no AI) that an in-scope (label/path-matched) @@ -5863,8 +5864,15 @@ async function maybeCloseIssueOverContributorCap( const otherOpenIssues = await listOpenIssues(env, repoFullName); const authorLoginLower = authorLogin.toLowerCase(); + const authorGithubId = issue.authorGithubId; + // #9125: id-when-present union login -- a sibling issue matches on the immutable id even if it (or this + // issue) was opened under a login that has since been renamed, so the cap can't be cleared by renaming. const otherAuthorIssueNumbers = otherOpenIssues - .filter((other) => (other.authorLogin ?? "").toLowerCase() === authorLoginLower && other.number !== issue.number) + .filter( + (other) => + other.number !== issue.number && + ((typeof authorGithubId === "number" && other.authorGithubId === authorGithubId) || (other.authorLogin ?? "").toLowerCase() === authorLoginLower), + ) .map((other) => other.number); // Live-verify each OTHER counted sibling before trusting it toward the cap (#2479 gate finding): the stored @@ -6973,17 +6981,24 @@ async function handlePullRequestWebhookEvent( // outcome is derived ONLY from the PR's realized terminal state + the gate verdict (no PR content); // nothing is ever surfaced publicly. Flag-OFF (default) is an immediate no-op (nothing recorded), so the // path is byte-identical. Best-effort: a record failure must never affect the gate or the public surface. - const reputationOutcome = (await convergedFeatureActive( - env, - repoFullName, - "reputation", - )) + // + // #9131: a review/review-comment/review-thread event re-gates the SAME PR without the submitter + // submitting anything -- reusing PR_TYPE_LABEL_IRRELEVANT_EVENT_NAMES (the same three review-family + // event names, already enumerated above for an unrelated purpose) rather than a second copy of the + // list. Any third party's comment must never be able to drive a rival's reputation counter. + const reputationOutcome = (!PR_TYPE_LABEL_IRRELEVANT_EVENT_NAMES.has(eventName) && + (await convergedFeatureActive( + env, + repoFullName, + "reputation", + ))) ? reputationOutcomeFromTerminalState(pr, payload.pull_request, gate) : undefined; if (reputationOutcome) { await recordReputationOutcome(env, { project: repoFullName, submitter: pr.authorLogin ?? null, + pullNumber: pr.number, outcome: reputationOutcome, }).catch((error) => { /* v8 ignore next -- best-effort: a reputation-record failure is logged, never surfaced to the gate. */ @@ -10145,6 +10160,7 @@ async function maybePublishPrPublicSurface( const authorBlacklisted = isAuthorBlacklisted( author, settings.contributorBlacklist, + pr.authorGithubId, ); // #regate-churn (maintainer-gated freeze): once a PR is held for manual review -- the manual-review label is // already on it from a PRIOR pass -- a repeat CONTRIBUTOR push must not buy a fresh, real AI review. That is diff --git a/src/review/public-stats.ts b/src/review/public-stats.ts index 77a1fcf307..d5eecdd289 100644 --- a/src/review/public-stats.ts +++ b/src/review/public-stats.ts @@ -468,12 +468,24 @@ export async function getPublicStats( // for an empty fleet. const fleetAccuracyPct = fleet.fleet.decisionAccuracy === null ? null : Math.round(fleet.fleet.decisionAccuracy * 1000) / 10; - // #8835: live per-arm guarantees, published by a REGISTERED instance's risk-control calibration and - // stored by ingest under riskcontrol:fleet:. Fail-open null — a flags blip hides the guarantee - // rather than fabricating or freezing one. + // #8835/#9121: live per-arm guarantees, published by REGISTERED (and, since #9121, credential-authenticated + // — see src/orb/ingest.ts) instances' risk-control calibrations, stored per-instance in + // orb_risk_control_arms. Was a single system_flags cell any registered instance could overwrite (or + // delete, via an absent arm); now scoped per instance and aggregated HERE at read time, preferring the + // largest sample size (nAtLambda) — more data is a more trustworthy estimate, so one low-n or stale peer + // can no longer eclipse a well-calibrated one just by writing last. Fail-open null — a flags blip hides + // the guarantee rather than fabricating or freezing one. const readGuarantee = async (arm: string): Promise<{ alpha: number; lambda: number; coveragePct: number; n: number } | null> => { try { - const row = await env.DB.prepare("SELECT value FROM system_flags WHERE key = ?").bind(`riskcontrol:fleet:${arm}`).first<{ value: string }>(); + const row = await env.DB.prepare( + `SELECT o.payload_json AS value FROM orb_risk_control_arms o + JOIN orb_instances i ON i.instance_id = o.instance_id AND i.registered = 1 + WHERE o.arm = ? + ORDER BY CAST(json_extract(o.payload_json, '$.nAtLambda') AS REAL) DESC, o.updated_at DESC + LIMIT 1`, + ) + .bind(arm) + .first<{ value: string }>(); if (!row?.value) return null; const parsed = JSON.parse(row.value) as { alpha?: unknown; lambda?: unknown; coverageAtLambda?: unknown; nAtLambda?: unknown }; if (typeof parsed.alpha !== "number" || typeof parsed.lambda !== "number" || typeof parsed.coverageAtLambda !== "number" || typeof parsed.nAtLambda !== "number") return null; diff --git a/src/review/reputation-wire.ts b/src/review/reputation-wire.ts index d1b714b93a..ed48387fb9 100644 --- a/src/review/reputation-wire.ts +++ b/src/review/reputation-wire.ts @@ -138,8 +138,8 @@ export async function shouldSkipAiForReputation( */ export async function recordReputationOutcome( env: Env, - args: { project: string; submitter: string | null | undefined; outcome: SubmissionOutcome }, + args: { project: string; submitter: string | null | undefined; pullNumber: number; outcome: SubmissionOutcome }, ): Promise { if (!isReputationEnabled(env)) return; - await recordSubmissionOutcome(env, args.project, args.submitter ?? undefined, args.outcome); + await recordSubmissionOutcome(env, args.project, args.submitter ?? undefined, args.pullNumber, args.outcome); } diff --git a/src/review/submitter-reputation.ts b/src/review/submitter-reputation.ts index 7997def8b6..b81b46b291 100644 --- a/src/review/submitter-reputation.ts +++ b/src/review/submitter-reputation.ts @@ -260,12 +260,26 @@ export function countOutcomes(rows: Array<{ status: string; reasonCode: string | return c; } -/** Record a terminal outcome for a submitter (internal; fail-safe no-op on any error). Keeps submitter_stats - * current for the operator /stats view — it is NO LONGER the source of the signal (review_targets is). */ -export async function recordSubmissionOutcome(env: Env, project: string, submitter: string | undefined, outcome: SubmissionOutcome): Promise { +/** + * Record a terminal outcome for a submitter (internal; fail-safe no-op on any error). Keeps submitter_stats + * current for the operator /stats view — it is NO LONGER the source of the signal (review_targets is). + * + * #9131: idempotent per (project, submitter, pullNumber, outcome) via submitter_outcome_log. The prior + * shape counted webhook PASSES, not submissions — every re-gate of the SAME PR (a body edit, a push, or a + * third party's review comment on a rival's held PR) bumped the counter again, with no idempotency key at + * all. `INSERT OR IGNORE` into the log first; the submitter_stats increment only runs when that insert + * actually created a row, so N re-gates of one PR (or N adversarial comments on it) yield exactly one + * counted outcome — including "manual", which used to accrue once per re-gate of a still-open, held PR. + */ +export async function recordSubmissionOutcome(env: Env, project: string, submitter: string | undefined, pullNumber: number, outcome: SubmissionOutcome): Promise { if (!submitter) return; const col = outcome === "merged" ? "merged" : outcome === "closed" ? "closed" : "manual"; try { + const logged = await storage(env) + .prepare(`INSERT OR IGNORE INTO submitter_outcome_log (project, submitter, pull_number, outcome) VALUES (?, ?, ?, ?)`) + .bind(project, submitter, pullNumber, outcome) + .run(); + if (logged.meta.changes === 0) return; // already counted this exact (project, submitter, PR, outcome) await storage(env) .prepare( `INSERT INTO submitter_stats (project, submitter, submissions, ${col}, last_seen) VALUES (?, ?, 1, 1, CURRENT_TIMESTAMP) @@ -284,15 +298,30 @@ export async function recordSubmissionOutcome(env: Env, project: string, submitt export async function getSubmitterReputation(env: Env, project: string, 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; - // The all-time aggregate counts (for /stats only — NOT the signal). Best-effort: a failure here still lets the - // signal derive (and vice-versa); either failing degrades to neutral defaults, never throws. + // #9131: WINDOWED (not all-time) counts, read from the idempotent submitter_outcome_log rather than + // submitter_stats -- so a burst state DECAYS as old outcomes age out of the window, instead of a serial + // false-positive from months ago permanently gating this submitter with no recovery but a merge. The + // all-time submitter_stats table still exists and is still maintained (recordSubmissionOutcome), but only + // for the separate /stats operator view (src/review/contributor-trust-profile.ts reads it directly) -- + // nothing that feeds a gate decision should read an undecaying aggregate. Best-effort: a failure here + // still lets the signal derive (and vice-versa); either failing degrades to neutral defaults, never throws. let agg = { submissions: 0, merged: 0, closed: 0, manual: 0 }; try { const row = await storage(env) - .prepare("SELECT submissions, merged, closed, manual FROM submitter_stats WHERE project = ? AND submitter = ?") - .bind(project, submitter) - .first<{ submissions: number; merged: number; closed: number; manual: number }>(); - if (row) agg = { submissions: row.submissions, merged: row.merged, closed: row.closed, manual: row.manual }; + .prepare( + `SELECT COUNT(*) AS submissions, + SUM(CASE WHEN outcome = 'merged' THEN 1 ELSE 0 END) AS merged, + SUM(CASE WHEN outcome = 'closed' THEN 1 ELSE 0 END) AS closed, + SUM(CASE WHEN outcome = 'manual' THEN 1 ELSE 0 END) AS manual + FROM submitter_outcome_log + WHERE project = ? AND submitter = ? AND recorded_at >= datetime('now', ?)`, + ) + .bind(project, submitter, `-${cfg.windowDays} days`) + .first<{ submissions: number; merged: number | null; closed: number | null; manual: number | null }>(); + // A submitter with zero rows in the window is a real, common case (SUM over an empty set is NULL, COUNT + // is 0) -- ?? 0 on each SUM column, not just a truthy-row check, or a genuinely-neutral submitter would + // read `merged: null` and corrupt closeRate's arithmetic below. + if (row) agg = { submissions: row.submissions, merged: row.merged ?? 0, closed: row.closed ?? 0, manual: row.manual ?? 0 }; } catch { // keep neutral aggregate defaults } diff --git a/src/selfhost/orb-collector.ts b/src/selfhost/orb-collector.ts index a7807702b0..14fe42fef4 100644 --- a/src/selfhost/orb-collector.ts +++ b/src/selfhost/orb-collector.ts @@ -11,6 +11,9 @@ // ORB_AIR_GAP=true — air-gapped/offline deployments only: compute locally, never send // ORB_ANONYMIZE=true — HMAC-hash repo/PR before export (default: true) // ORB_COLLECTOR_TOKEN= — bearer credential for the hosted collector +// ORB_COLLECTOR_INSTANCE_SECRET= — #9121: per-instance credential returned ONCE by +// POST /v1/internal/orb/instances/register; required for the risk_control (published guarantee) field +// to be accepted. Unset is fine for an instance not opted into fleet calibration. // // No diffs, no code, no comments, no logins, no commit SHAs — only verdict + outcome + reversal + a bucketed // reason category + cycle time, with repo/PR identifiers HMAC'd by a key the collector never holds (so it @@ -285,6 +288,11 @@ export async function exportOrbBatch(db: D1Database, batchSize = 200, fetchFn: t const body = JSON.stringify(payload); const signature = createHmac("sha256", secret).update(body).digest("hex"); const collectorToken = process.env.ORB_COLLECTOR_TOKEN; + // #9121: the per-instance credential the collector issued when an operator registered this instance for + // fleet calibration — distinct from collectorToken (shared fleet-wide, proves only "some fleet member") + // and from the anonymization secret above (privacy, never shared with the collector). Only meaningful + // once the risk_control field is actually populated; an unset value is fine for an unregistered instance. + const instanceSecret = process.env.ORB_COLLECTOR_INSTANCE_SECRET; try { const res = await fetchFn(collectorUrl, { @@ -292,6 +300,7 @@ export async function exportOrbBatch(db: D1Database, batchSize = 200, fetchFn: t headers: { "content-type": "application/json", "x-orb-signature": `sha256=${signature}`, + ...(instanceSecret ? { "x-orb-instance-secret": instanceSecret } : {}), "x-orb-instance": instance, ...(collectorToken ? { authorization: `Bearer ${collectorToken}` } : {}), }, diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index f04a0a3bfb..f787409ff5 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -123,7 +123,7 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // at any point while it sits waiting. `settings` was fetched fresh at the top of this function, so this // mirrors the exact same pure check the planner uses (processors.ts), just re-run against CURRENT effective config. if (pending.actionClass === "close" && pending.params.closeKind === "blacklist" && pr) { - const stillBlacklisted = findBlacklistEntry(pr.authorLogin, settings.contributorBlacklist) !== null; + const stillBlacklisted = findBlacklistEntry(pr.authorLogin, settings.contributorBlacklist, pr.authorGithubId) !== null; if (!stillBlacklisted) { await setPendingAgentActionStatus(env, pending.id, { status: "rejected", decidedBy: input.decidedBy }); await recordAuditEvent(env, { diff --git a/src/services/control-panel-roles.ts b/src/services/control-panel-roles.ts index 1398c8a050..f21a9e0148 100644 --- a/src/services/control-panel-roles.ts +++ b/src/services/control-panel-roles.ts @@ -21,21 +21,28 @@ export type ControlPanelAccessScope = { accountLogins: string[]; }; -export async function loadControlPanelAccessScope(env: Env, login: string): Promise { +// #9126: every function below takes an OPTIONAL `githubUserId` alongside `login` -- when the caller has a +// live session it should always thread `identity.session.githubUserId` through so isAuthorizedGitHubSessionLogin +// can bind operator trust to the immutable id (see that function's own doc comment). Passing `undefined` is +// safe and behavior-preserving whenever no live session exists (e.g. a stored watcher-list login with no +// session attached) OR no ADMIN_GITHUB_IDS allowlist is configured; it only stops granting the login-only +// shortcut once an operator has explicitly opted into id-binding without that particular call site also +// threading the id. +export async function loadControlPanelAccessScope(env: Env, login: string, githubUserId?: number | null): Promise { const [repositories, installations, pullRequests] = await Promise.all([listRepositories(env), listInstallations(env), listAllPullRequests(env)]); return buildControlPanelAccessScope({ login, generatedAt: nowIso(), confirmedMiner: false, - operator: isAuthorizedGitHubSessionLogin(env, login), + operator: isAuthorizedGitHubSessionLogin(env, login, githubUserId), repositories, installations, pullRequests, }); } -export async function canLoginAccessRepo(env: Env, login: string, fullName: string): Promise { - const [scope, repo] = await Promise.all([loadControlPanelAccessScope(env, login), getRepository(env, fullName)]); +export async function canLoginAccessRepo(env: Env, login: string, fullName: string, githubUserId?: number | null): Promise { + const [scope, repo] = await Promise.all([loadControlPanelAccessScope(env, login, githubUserId), getRepository(env, fullName)]); if (scope.operator) return true; const requestedRepo = fullName.toLowerCase(); if (scope.repositoryFullNames.some((name) => name.toLowerCase() === requestedRepo)) return true; @@ -46,14 +53,14 @@ export async function canLoginAccessRepo(env: Env, login: string, fullName: stri // PUBLIC gittensor-tracked repos they don't own or maintain, so a tracked public repo is watchable by any // contributor. A PRIVATE repo is gated to maintainer/owner/operator scope so its issues never fan out to a // non-collaborator. An untracked repo (unknown visibility) is treated as not watchable (fail-closed). -export async function canWatchRepo(env: Env, login: string, fullName: string): Promise { +export async function canWatchRepo(env: Env, login: string, fullName: string, githubUserId?: number | null): Promise { const repo = await getRepository(env, fullName); if (!repo) return false; if (!repo.isPrivate) return true; - return canLoginAccessRepo(env, login, fullName); + return canLoginAccessRepo(env, login, fullName, githubUserId); } -export async function loadControlPanelRoleSummary(env: Env, login: string): Promise { +export async function loadControlPanelRoleSummary(env: Env, login: string, githubUserId?: number | null): Promise { const [miner, repositories, installations, pullRequests] = await Promise.all([ getFreshOfficialMinerDetection(env, login).catch(() => null), listRepositories(env), @@ -64,7 +71,7 @@ export async function loadControlPanelRoleSummary(env: Env, login: string): Prom login, generatedAt: nowIso(), confirmedMiner: miner?.status === "confirmed", - operator: isAuthorizedGitHubSessionLogin(env, login), + operator: isAuthorizedGitHubSessionLogin(env, login, githubUserId), repositories, installations, pullRequests, diff --git a/src/types.ts b/src/types.ts index 77ccfea46c..581f693b42 100644 --- a/src/types.ts +++ b/src/types.ts @@ -420,10 +420,7 @@ export type GitHubPullRequestPayload = { created_at?: string | null; updated_at?: string | null; closed_at?: string | null; - user?: { - login?: string; - type?: string; - }; + user?: GitHubWebhookUserPayload; author_association?: string; head?: { sha?: string; @@ -452,9 +449,7 @@ export type GitHubIssuePayload = { created_at?: string | null; updated_at?: string | null; closed_at?: string | null; - user?: { - login?: string; - }; + user?: GitHubWebhookUserPayload; author_association?: string; labels?: Array<{ name?: string }>; body?: string | null; @@ -641,6 +636,10 @@ export type PullRequestRecord = { title: string; state: string; authorLogin?: string | null | undefined; + /** #9125: the author's IMMUTABLE numeric GitHub user id, from the webhook payload's `user.id`. Identity- + * keyed contributor controls (blacklist, caps) should match on this when present, since `authorLogin` can + * be changed by the account holder at will. */ + authorGithubId?: number | null | undefined; authorAssociation?: string | null | undefined; headSha?: string | null | undefined; headRef?: string | null | undefined; @@ -748,6 +747,8 @@ export type IssueRecord = { title: string; state: string; authorLogin?: string | null | undefined; + /** #9125: mirrors PullRequestRecord.authorGithubId -- the immutable id behind the (renameable) login. */ + authorGithubId?: number | null | undefined; authorAssociation?: string | null | undefined; htmlUrl?: string | null | undefined; body?: string | null | undefined; @@ -1665,6 +1666,10 @@ export type AdvisoryAiRoutingConfig = { * analysis. Metadata can come from private configuration and must not be echoed to public surfaces. */ export type ContributorBlacklistEntry = { login: string; + /** #9125: the login's IMMUTABLE numeric GitHub user id, when the operator has it (e.g. copied from an + * audit event or the API). Optional so existing login-only entries keep working; when present, matching + * is id-when-present UNION login, so a renamed-then-back account still can't shed a ban by renaming. */ + githubId?: number | undefined; /** Why the account is blocked. Free-text maintainer metadata; not published in automated close comments. */ reason?: string | undefined; /** PR/issue URLs (or other maintainer refs) evidencing the block. */ diff --git a/test/integration/orb-ingest.test.ts b/test/integration/orb-ingest.test.ts index 51ca7ed449..6262c1234d 100644 --- a/test/integration/orb-ingest.test.ts +++ b/test/integration/orb-ingest.test.ts @@ -430,6 +430,7 @@ describe("Orb instance registry routes (/v1/internal/orb/instances)", () => { describe("GET /v1/internal/fleet/analytics route", () => { const app = createApp(); + const auth = { authorization: "Bearer dev-internal-token" }; it("returns the fleet report, honoring ?days (bearer-gated)", async () => { const res = await app.request("/v1/internal/fleet/analytics?days=30", { headers: { authorization: "Bearer dev-internal-token" } }, createTestEnv()); @@ -447,22 +448,48 @@ describe("GET /v1/internal/fleet/analytics route", () => { expect(res.status).toBe(401); }); - it("stores risk_control calibrations ONLY for REGISTERED senders and retracts absent arms (#8835)", async () => { - const db = new TestD1Database() as unknown as D1Database; - const flag = async () => (await (db as unknown as TestD1Database).prepare("SELECT value FROM system_flags WHERE key='riskcontrol:fleet:close'").first<{ value: string }>())?.value; - const send = (risk_control: unknown) => - handleOrbIngest(JSON.stringify({ instance_id: "inst1", events: [{ repo_hash: "rh", pr_hash: `g${Math.random()}`, outcome: "merged" }], risk_control }), db); + it("stores risk_control calibrations ONLY for a REGISTERED, CREDENTIAL-AUTHENTICATED sender; an absent arm is a no-op and only an explicit null retracts (#8835/#9121)", async () => { + const env = createTestEnv(); + const db = env.DB as unknown as D1Database; + const flag = async () => + ( + await (db as unknown as TestD1Database) + .prepare("SELECT payload_json FROM orb_risk_control_arms WHERE instance_id='inst1' AND arm='close'") + .first<{ payload_json: string }>() + )?.payload_json; + const send = (risk_control: unknown, instanceSecret?: string) => + handleOrbIngest(JSON.stringify({ instance_id: "inst1", events: [{ repo_hash: "rh", pr_hash: `g${Math.random()}`, outcome: "merged" }], risk_control }), db, instanceSecret); // Unregistered sender: the strongest homepage claim must not be plantable via open ingest. await send({ close: { alpha: 0.015, lambda: 0.94, coverageAtLambda: 0.8, nAtLambda: 200 } }); expect(await flag()).toBeUndefined(); - await (db as unknown as TestD1Database).prepare("UPDATE orb_instances SET registered = 1 WHERE instance_id = 'inst1'").run(); - await send({ close: { alpha: 0.015, lambda: 0.94, coverageAtLambda: 0.8, nAtLambda: 200 } }); + // #9121: registering now mints a per-instance credential -- the real, HTTP registration route is used + // here (not a raw SQL UPDATE) so this test exercises the actual credential-issuance path. + const reg = await app.request("/v1/internal/orb/instances/register", { method: "POST", headers: auth, body: JSON.stringify({ instanceId: "inst1" }) }, env); + const { instanceSecret } = (await reg.json()) as { instanceSecret: string }; + expect(instanceSecret).toMatch(/^orbis_[0-9a-f]{64}$/); + + // Registered but WITHOUT the credential: the claim is refused, not silently accepted. + const rejected = await send({ close: { alpha: 0.015, lambda: 0.94, coverageAtLambda: 0.8, nAtLambda: 200 } }); + expect(rejected).toEqual({ error: "instance_unauthenticated" }); + expect(await flag()).toBeUndefined(); + + // Registered AND credential-authenticated: the claim is accepted. + await send({ close: { alpha: 0.015, lambda: 0.94, coverageAtLambda: 0.8, nAtLambda: 200 } }, instanceSecret); + expect(JSON.parse((await flag())!)).toMatchObject({ lambda: 0.94 }); + + // An ABSENT arm is "no change", never an implicit retraction (#9121) -- the stored value survives. + await send({}, instanceSecret); expect(JSON.parse((await flag())!)).toMatchObject({ lambda: 0.94 }); - // The sender stops publishing the arm → the fleet copy retracts (a stale guarantee lies). - await send({}); + // Only an EXPLICIT null retracts. + await send({ close: null }, instanceSecret); + expect(await flag()).toBeUndefined(); + + // A WRONG credential is also refused, not silently accepted. + const wrongSecret = await send({ close: { alpha: 0.015, lambda: 0.5, coverageAtLambda: 0.8, nAtLambda: 200 } }, "orbis_wrongwrongwrong"); + expect(wrongSecret).toEqual({ error: "instance_unauthenticated" }); expect(await flag()).toBeUndefined(); }); }); diff --git a/test/unit/access-boundary.test.ts b/test/unit/access-boundary.test.ts index a1629b2c31..06a9239e12 100644 --- a/test/unit/access-boundary.test.ts +++ b/test/unit/access-boundary.test.ts @@ -142,6 +142,21 @@ describe("access boundary: per-repo maintainer data is repo-scoped", () => { expect((await app.request(SETTINGS_B, { headers: { cookie: `loopover_session=${token}` } }, env)).status).toBe(200); }); + it("#9126: with ADMIN_GITHUB_IDS configured, a RELEASED-AND-RE-REGISTERED operator login is denied end-to-end (the id no longer matches)", async () => { + const { app, env } = await setup({ ADMIN_GITHUB_LOGINS: "ops-admin", ADMIN_GITHUB_IDS: "9" }); + // Someone else now holds the "ops-admin" handle, with a DIFFERENT immutable id. + const { token } = await createSessionForGitHubUser(env, { login: "ops-admin", id: 12345 }); + const res = await app.request(SETTINGS_B, { headers: { cookie: `loopover_session=${token}` } }, env); + expect(res.status).toBe(403); + }); + + it("#9126: with ADMIN_GITHUB_IDS configured, the ORIGINAL operator's id still bypasses per-repo scope even under a renamed login", async () => { + const { app, env } = await setup({ ADMIN_GITHUB_LOGINS: "ops-admin", ADMIN_GITHUB_IDS: "9" }); + const { token } = await createSessionForGitHubUser(env, { login: "ops-admin-renamed", id: 9 }); + const res = await app.request(SETTINGS_B, { headers: { cookie: `loopover_session=${token}` } }, env); + expect(res.status).toBe(200); + }); + it("a server-to-server token reads settings without per-repo session scope", async () => { const { app, env } = await setup(); const res = await app.request(SETTINGS_A, { headers: { authorization: `Bearer ${env.LOOPOVER_API_TOKEN}` } }, env); diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index 479d3b07b7..f05a69b010 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -16,6 +16,19 @@ function fileRecord(over: Partial & { path: string }): Pu return { repoFullName: "acme/widgets", pullNumber: 3, status: "modified", additions: 1, deletions: 0, changes: 1, payload: {}, ...over }; } +// #9131: the reputation burst check (submissions >= 8 && merged < 1) now reads its counts from the +// WINDOWED submitter_outcome_log, not the all-time submitter_stats row -- seed both so this file's existing +// burst-simulation tests still exercise the same shape (submitter_stats is also still written by +// recordSubmissionOutcome and kept here for parity, even though nothing under test reads it directly). +async function seedBurstSubmitter(env: Env, project: string, submitter: string): Promise { + await env.DB.prepare("INSERT INTO submitter_stats (project, submitter, submissions, merged, closed, manual, last_seen) VALUES (?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP)") + .bind(project, submitter, 8, 0, 8, 0) + .run(); + for (let pullNumber = 1; pullNumber <= 8; pullNumber++) { + await env.DB.prepare("INSERT INTO submitter_outcome_log (project, submitter, pull_number, outcome) VALUES (?, ?, ?, 'closed')").bind(project, submitter, pullNumber).run(); + } +} + describe("buildAiReviewDiff", () => { it("includes patches and headers, lists a patch-less file, and truncates oversized diffs (source-first)", () => { const diff = buildAiReviewDiff([ @@ -177,13 +190,13 @@ describe("shouldStartAiReviewForAdvisory", () => { it("does not start when the reputation gate downgrades the PR to deterministic-only", async () => { const env = createTestEnv({ AI: { run: vi.fn() } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", LOOPOVER_REVIEW_REPUTATION: "true", LOOPOVER_REVIEW_REPOS: "acme/widgets" }); - await env.DB.prepare("INSERT INTO submitter_stats (project, submitter, submissions, merged, closed, manual, last_seen) VALUES (?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP)").bind("acme/widgets", "alice", 8, 0, 8, 0).run(); + await seedBurstSubmitter(env, "acme/widgets", "alice"); await expect(shouldStartAiReviewForAdvisory(env, base)).resolves.toBe(false); }); it("honors aiReviewAllAuthors as an explicit self-host review requirement even when reputation would skip", async () => { const env = createTestEnv({ AI: { run: vi.fn() } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", LOOPOVER_REVIEW_REPUTATION: "true", LOOPOVER_REVIEW_REPOS: "acme/widgets" }); - await env.DB.prepare("INSERT INTO submitter_stats (project, submitter, submissions, merged, closed, manual, last_seen) VALUES (?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP)").bind("acme/widgets", "alice", 8, 0, 8, 0).run(); + await seedBurstSubmitter(env, "acme/widgets", "alice"); await expect( shouldStartAiReviewForAdvisory(env, { ...base, @@ -198,7 +211,7 @@ describe("shouldStartAiReviewForAdvisory", () => { // reported. it("#9008: forceAiReview bypasses ONLY the reputation skip, not the hard entry gates", async () => { const env = createTestEnv({ AI: { run: vi.fn() } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", LOOPOVER_REVIEW_REPUTATION: "true", LOOPOVER_REVIEW_REPOS: "acme/widgets" }); - await env.DB.prepare("INSERT INTO submitter_stats (project, submitter, submissions, merged, closed, manual, last_seen) VALUES (?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP)").bind("acme/widgets", "alice", 8, 0, 8, 0).run(); + await seedBurstSubmitter(env, "acme/widgets", "alice"); // Without force, reputation still skips (pinned above) -- WITH force, the same low-reputation author is // reviewed. await expect(shouldStartAiReviewForAdvisory(env, { ...base, forceAiReview: true })).resolves.toBe(true); diff --git a/test/unit/auth.test.ts b/test/unit/auth.test.ts index aeabba60aa..3b28996b68 100644 --- a/test/unit/auth.test.ts +++ b/test/unit/auth.test.ts @@ -102,6 +102,46 @@ describe("private-beta auth and rate limiting", () => { expect(extractCookieValue("loopover_session=%E0%A4%A", "loopover_session")).toBeUndefined(); }); + describe("isAuthorizedGitHubSessionLogin: id-binding (#9126, a released GitHub handle must not grant operator access)", () => { + it("no ADMIN_GITHUB_IDS configured: falls back to login-only, byte-identical to before #9126", () => { + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "jsonbored" }); + expect(isAuthorizedGitHubSessionLogin(env, "jsonbored")).toBe(true); + expect(isAuthorizedGitHubSessionLogin(env, "jsonbored", 12345)).toBe(true); // an id doesn't matter here + expect(isAuthorizedGitHubSessionLogin(env, "stranger")).toBe(false); + }); + + it("ADMIN_GITHUB_IDS configured: a matching LOGIN with a NON-matching id is DENIED -- the released-handle takeover this closes", () => { + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "jsonbored", ADMIN_GITHUB_IDS: "555" }); + // The configured login, but presented with an id that doesn't match (e.g. the handle was released and + // re-registered by a stranger) -- must be denied even though the login string matches. + expect(isAuthorizedGitHubSessionLogin(env, "jsonbored", 999)).toBe(false); + expect(isAuthorizedGitHubSessionLogin(env, "jsonbored")).toBe(false); // no id presented at all + }); + + it("ADMIN_GITHUB_IDS configured: the matching id authorizes regardless of login", () => { + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "jsonbored", ADMIN_GITHUB_IDS: "555" }); + expect(isAuthorizedGitHubSessionLogin(env, "jsonbored", 555)).toBe(true); + // Even under a DIFFERENT login (e.g. jsonbored renamed) -- the id is what's authoritative once configured. + expect(isAuthorizedGitHubSessionLogin(env, "renamed-handle", 555)).toBe(true); + }); + + it("ADMIN_GITHUB_IDS configured but blank/whitespace-only parses to an EMPTY set, not id 0 -- falls back to login-only", () => { + // Number("") is 0, not NaN -- parseGitHubIdList must not let an unset/blank env accidentally "configure" + // id 0, which would deny every real operator (whose id is never 0) via the id-only branch. + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "jsonbored", ADMIN_GITHUB_IDS: " " }); + expect(isAuthorizedGitHubSessionLogin(env, "jsonbored")).toBe(true); + }); + + it("parses a whitespace-or-comma id list, dropping non-numeric/non-integer entries", () => { + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "", ADMIN_GITHUB_IDS: "111, 222 333,not-a-number 4.5" }); + expect(isAuthorizedGitHubSessionLogin(env, "anyone", 111)).toBe(true); + expect(isAuthorizedGitHubSessionLogin(env, "anyone", 222)).toBe(true); + expect(isAuthorizedGitHubSessionLogin(env, "anyone", 333)).toBe(true); + expect(isAuthorizedGitHubSessionLogin(env, "anyone", 4.5)).toBe(false); // dropped: not an integer + expect(isAuthorizedGitHubSessionLogin(env, "anyone", 999)).toBe(false); + }); + }); + it("enforces burst limits inside the Durable Object bucket", async () => { const state = memoryDurableObjectState(); const limiter = new RateLimiter(state as unknown as DurableObjectState, createTestEnv()); diff --git a/test/unit/contributor-blacklist-engine.test.ts b/test/unit/contributor-blacklist-engine.test.ts index 17da65fc4b..9dd516f6c1 100644 --- a/test/unit/contributor-blacklist-engine.test.ts +++ b/test/unit/contributor-blacklist-engine.test.ts @@ -70,6 +70,18 @@ describe("normalizeContributorBlacklist (#1425) [engine]", () => { expect(entries[2]?.reason?.length).toBe(200); expect(entries[2]?.evidence).toHaveLength(10); }); + + it("accepts a valid githubId and drops a malformed one (#9125)", () => { + const { entries } = normalizeContributorBlacklist([ + { login: "a", githubId: 12345 }, + { login: "b", githubId: -1 }, + { login: "c", githubId: 0 }, + { login: "d", githubId: 1.5 }, + { login: "e", githubId: "12345" }, + { login: "f" }, + ]); + expect(entries.map((e) => e.githubId)).toEqual([12345, undefined, undefined, undefined, undefined, undefined]); + }); }); describe("findBlacklistEntry / isAuthorBlacklisted [engine]", () => { @@ -93,6 +105,32 @@ describe("findBlacklistEntry / isAuthorBlacklisted [engine]", () => { expect(findBlacklistEntry("anyone", undefined)).toBeNull(); expect(isAuthorBlacklisted("anyone", undefined)).toBe(false); }); + + describe("id-when-present union login (#9125: a banned contributor cannot clear the block by renaming)", () => { + const idList: ContributorBlacklistEntry[] = [{ login: "spammer99", githubId: 555, reason: "farming" }]; + + it("a NEW login for the same immutable id still matches (the rename this issue closes)", () => { + expect(findBlacklistEntry("spammer99x", idList, 555)?.login).toBe("spammer99"); + expect(isAuthorBlacklisted("spammer99x", idList, 555)).toBe(true); + }); + + it("the same login with a NON-matching id still matches on login (union, not AND)", () => { + expect(isAuthorBlacklisted("spammer99", idList, 999)).toBe(true); + }); + + it("a different login and a different id is a genuine non-match", () => { + expect(isAuthorBlacklisted("innocent", idList, 999)).toBe(false); + }); + + it("omitting githubId at the call site falls back to login-only, byte-identical to before #9125", () => { + expect(isAuthorBlacklisted("spammer99", idList)).toBe(true); + expect(isAuthorBlacklisted("spammer99x", idList)).toBe(false); + }); + + it("an entry with no githubId of its own is still matched by login regardless of the caller's id", () => { + expect(isAuthorBlacklisted("Mona", list, 42)).toBe(true); + }); + }); }); describe("mergeContributorBlacklists (global ∪ per-repo) [engine]", () => { diff --git a/test/unit/contributor-blacklist.test.ts b/test/unit/contributor-blacklist.test.ts index bfdd07a706..18cf5cc462 100644 --- a/test/unit/contributor-blacklist.test.ts +++ b/test/unit/contributor-blacklist.test.ts @@ -118,6 +118,18 @@ describe("normalizeContributorBlacklist (#1425)", () => { expect(entries[2]?.reason?.length).toBe(200); // reason capped expect(entries[2]?.evidence).toHaveLength(10); // evidence capped }); + + it("accepts a valid githubId and drops a malformed one (#9125)", () => { + const { entries } = normalizeContributorBlacklist([ + { login: "a", githubId: 12345 }, + { login: "b", githubId: -1 }, + { login: "c", githubId: 0 }, + { login: "d", githubId: 1.5 }, + { login: "e", githubId: "12345" }, + { login: "f" }, + ]); + expect(entries.map((e) => e.githubId)).toEqual([12345, undefined, undefined, undefined, undefined, undefined]); + }); }); describe("findBlacklistEntry / isAuthorBlacklisted", () => { @@ -141,6 +153,32 @@ describe("findBlacklistEntry / isAuthorBlacklisted", () => { expect(findBlacklistEntry("anyone", undefined)).toBeNull(); expect(isAuthorBlacklisted("anyone", undefined)).toBe(false); }); + + describe("id-when-present union login (#9125: a banned contributor cannot clear the block by renaming)", () => { + const idList: ContributorBlacklistEntry[] = [{ login: "spammer99", githubId: 555, reason: "farming" }]; + + it("a NEW login for the same immutable id still matches (the rename this issue closes)", () => { + expect(findBlacklistEntry("spammer99x", idList, 555)?.login).toBe("spammer99"); + expect(isAuthorBlacklisted("spammer99x", idList, 555)).toBe(true); + }); + + it("the same login with a NON-matching id still matches on login (union, not AND)", () => { + expect(isAuthorBlacklisted("spammer99", idList, 999)).toBe(true); + }); + + it("a different login and a different id is a genuine non-match", () => { + expect(isAuthorBlacklisted("innocent", idList, 999)).toBe(false); + }); + + it("omitting githubId at the call site falls back to login-only, byte-identical to before #9125", () => { + expect(isAuthorBlacklisted("spammer99", idList)).toBe(true); + expect(isAuthorBlacklisted("spammer99x", idList)).toBe(false); + }); + + it("an entry with no githubId of its own is still matched by login regardless of the caller's id", () => { + expect(isAuthorBlacklisted("Mona", list, 42)).toBe(true); + }); + }); }); describe("mergeContributorBlacklists (global ∪ per-repo)", () => { diff --git a/test/unit/public-stats.test.ts b/test/unit/public-stats.test.ts index d04f97f366..d1fde747a1 100644 --- a/test/unit/public-stats.test.ts +++ b/test/unit/public-stats.test.ts @@ -831,19 +831,43 @@ describe("getPublicStats — live aggregate over the review ledger", () => { }); }); -describe("fleetAccuracy.guaranteed (#8835)", () => { - it("publishes a live per-arm guarantee from the fleet flags; malformed or absent flags read null (fail-open)", async () => { +describe("fleetAccuracy.guaranteed (#8835/#9121)", () => { + it("publishes a live per-arm guarantee from a REGISTERED instance's orb_risk_control_arms row; malformed, unregistered, or absent rows read null (fail-open)", async () => { const env = createTestEnv({ LOOPOVER_PUBLIC_STATS_REPOS: "" }); - await env.DB.prepare(`INSERT INTO system_flags (key, value) VALUES ('riskcontrol:fleet:close', ?)`) + await env.DB.prepare(`INSERT INTO orb_instances (instance_id, registered) VALUES ('inst-a', 1)`).run(); + await env.DB.prepare(`INSERT INTO orb_risk_control_arms (instance_id, arm, payload_json) VALUES ('inst-a', 'close', ?)`) .bind(JSON.stringify({ alpha: 0.015, lambda: 0.94, coverageAtLambda: 0.82, nAtLambda: 240 })) .run(); - await env.DB.prepare(`INSERT INTO system_flags (key, value) VALUES ('riskcontrol:fleet:merge', '{broken')`).run(); + await env.DB.prepare(`INSERT INTO orb_risk_control_arms (instance_id, arm, payload_json) VALUES ('inst-a', 'merge', '{broken')`).run(); const out = await getPublicStats(env, NOW); expect(out.fleetAccuracy.guaranteed.close).toEqual({ alpha: 0.015, lambda: 0.94, coveragePct: 82, n: 240 }); expect(out.fleetAccuracy.guaranteed.merge).toBeNull(); - // A structurally-wrong flag (missing fields) also reads null rather than publishing garbage. - await env.DB.prepare(`UPDATE system_flags SET value = '{"alpha":"high"}' WHERE key = 'riskcontrol:fleet:close'`).run(); + // A structurally-wrong row (missing fields) also reads null rather than publishing garbage. + await env.DB.prepare(`UPDATE orb_risk_control_arms SET payload_json = '{"alpha":"high"}' WHERE instance_id = 'inst-a' AND arm = 'close'`).run(); const again = await getPublicStats(env, NOW); expect(again.fleetAccuracy.guaranteed.close).toBeNull(); }); + + it("#9121: an UNREGISTERED instance's row never publishes (the same open-ingest-can't-plant-a-guarantee invariant, now enforced at read time too)", async () => { + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS_REPOS: "" }); + await env.DB.prepare(`INSERT INTO orb_instances (instance_id, registered) VALUES ('inst-b', 0)`).run(); + await env.DB.prepare(`INSERT INTO orb_risk_control_arms (instance_id, arm, payload_json) VALUES ('inst-b', 'close', ?)`) + .bind(JSON.stringify({ alpha: 0.015, lambda: 0.94, coverageAtLambda: 0.82, nAtLambda: 240 })) + .run(); + const out = await getPublicStats(env, NOW); + expect(out.fleetAccuracy.guaranteed.close).toBeNull(); + }); + + it("#9121: aggregates across multiple registered instances, preferring the larger sample size (nAtLambda)", async () => { + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS_REPOS: "" }); + await env.DB.prepare(`INSERT INTO orb_instances (instance_id, registered) VALUES ('small-n', 1), ('big-n', 1)`).run(); + await env.DB.prepare(`INSERT INTO orb_risk_control_arms (instance_id, arm, payload_json) VALUES ('small-n', 'close', ?)`) + .bind(JSON.stringify({ alpha: 0.015, lambda: 0.9, coverageAtLambda: 0.7, nAtLambda: 30 })) + .run(); + await env.DB.prepare(`INSERT INTO orb_risk_control_arms (instance_id, arm, payload_json) VALUES ('big-n', 'close', ?)`) + .bind(JSON.stringify({ alpha: 0.015, lambda: 0.97, coverageAtLambda: 0.9, nAtLambda: 5000 })) + .run(); + const out = await getPublicStats(env, NOW); + expect(out.fleetAccuracy.guaranteed.close).toEqual({ alpha: 0.015, lambda: 0.97, coveragePct: 90, n: 5000 }); + }); }); diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index e1683172b2..aed167b889 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -46,6 +46,7 @@ import { upsertOfficialMinerDetection, upsertPullRequestFile, upsertPullRequestFromGitHub, + listOtherOpenPullRequestsForAuthor, upsertIssueWatchSubscription, upsertRepositoryAiKey, upsertRepositorySettings, @@ -2895,6 +2896,26 @@ describe("queue processors", () => { expect(ledgerRows?.n).toBeGreaterThanOrEqual(1); }); + it("#9125: listOtherOpenPullRequestsForAuthor matches on the immutable authorGithubId when present, surviving a rename that would otherwise clear the cap", async () => { + const env = createTestEnv(); + // Two PRs opened under the OLD login, one under the NEW (post-rename) login -- all the SAME immutable id. + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 1, title: "old-login PR 1", state: "open", user: { login: "farmer99", id: 555 }, head: { sha: "s1" }, labels: [], body: "" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 2, title: "old-login PR 2", state: "open", user: { login: "farmer99", id: 555 }, head: { sha: "s2" }, labels: [], body: "" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "new-login PR", state: "open", user: { login: "farmer99x", id: 555 }, head: { sha: "s3" }, labels: [], body: "" }); + // A genuinely unrelated PR, different id and login entirely -- must never be counted. + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 4, title: "unrelated", state: "open", user: { login: "someone-else", id: 999 }, head: { sha: "s4" }, labels: [], body: "" }); + + // Without threading the id (login-only, pre-#9125 behavior): the renamed PR (#3) is invisible to a query + // scoped to the OLD login -- exactly the gap that let a rename clear the cap. + const loginOnly = await listOtherOpenPullRequestsForAuthor(env, "owner/repo", 1, "farmer99"); + expect(loginOnly.map((pr) => pr.number).sort()).toEqual([2]); + + // With the id threaded through: PR #2 (same old login) AND PR #3 (renamed, same id) both count; the + // unrelated PR #4 never does. + const withId = await listOtherOpenPullRequestsForAuthor(env, "owner/repo", 1, "farmer99", 555); + expect(withId.map((pr) => pr.number).sort()).toEqual([2, 3]); + }); + it("pre-merge contributor-cap re-check (#7284-fix): a contributor well UNDER a configured cap merges normally through the full pipeline", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertInstallation(env, { @@ -5226,6 +5247,51 @@ describe("queue processors", () => { expect(closeAudit?.n).toBeGreaterThanOrEqual(1); }); + it("#9125: contributor open-ISSUE cap survives a rename -- two sibling issues opened under the OLD login still count toward the cap on a 3rd issue opened under the renamed login, via the shared immutable id", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", issues: "write" }, events: ["issues"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + // Two sibling issues opened under the OLD login, both carrying the immutable id 555. + await upsertIssueFromGitHub(env, "JSONbored/gittensory", { number: 60, title: "Farmer issue one", state: "open", user: { login: "farmer99", id: 555 }, labels: [], body: "x" }); + await upsertIssueFromGitHub(env, "JSONbored/gittensory", { number: 61, title: "Farmer issue two", state: "open", user: { login: "farmer99", id: 555 }, labels: [], body: "y" }); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + autonomy: { close: "auto", label: "auto" }, + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { contributorCapLabel: "spam-cap", contributorOpenIssueCap: 2 } }, "repo_file"); + const seen = { closed: false, labels: [] as string[], comments: [] as string[] }; + 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: "installation-token" }); + if ((url.endsWith("/issues/60") || url.endsWith("/issues/61")) && method === "GET") return Response.json({ state: "open" }); + if (url.endsWith("/issues/62") && method === "PATCH") { seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; return Response.json({ state: "closed" }); } + if (url.includes("/issues/62/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/62/labels") && method === "POST") { seen.labels.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[])); return Response.json([]); } + if (url.includes("/issues/62/comments") && method === "POST") { seen.comments.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); return Response.json({ id: 1 }, { status: 201 }); } + return Response.json({}); + }); + + // The 3rd issue is opened under a NEW login -- same immutable id -- as if the contributor renamed + // between opening #61 and #62. Before #9125 this cleared the cap (the login-only match saw a stranger). + await processJob(env, { + type: "github-webhook", + deliveryId: "contributor-issue-cap-rename", + eventName: "issues", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 62, title: "Farmer's 3rd issue, post-rename", state: "open", user: { login: "farmer99x", id: 555 }, labels: [], body: "x" }, + }, + }); + + expect(seen.closed).toBe(true); + expect(seen.labels).toContain("spam-cap"); + }); + it("contributor open-ISSUE cap (#2270): bounds the sibling live-check fan-out instead of firing one request per open issue at once (#2766 parity)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertInstallation(env, { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index fde44f13b5..fc9ca62808 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -4990,12 +4990,14 @@ describe("queue processors", () => { await processJob(env, { type: "agent-regate-pr", deliveryId: "reputation-single-read", repoFullName: "JSONbored/gittensory", prNumber: 62, installationId: 123 }); // Before #4507, the outer caller-scope computation AND runAiReviewForAdvisory's own internal check each // independently scanned review_targets for this submitter — 2 full sets (6 prepares), not 1 (3). + // #9131: the windowed aggregate (submissions/merged/closed/manual, the burst check's data source) now + // reads submitter_outcome_log instead of submitter_stats — same one-read invariant, new table name. const reputationPrepares = spy.mock.calls .slice(before) .map(([sql]) => String(sql)) - .filter((sql) => sql.includes("submitter_stats") || sql.includes("terminal_at IS NOT NULL") || sql.includes("created_at >= datetime")); + .filter((sql) => sql.includes("submitter_outcome_log") || sql.includes("terminal_at IS NOT NULL") || sql.includes("created_at >= datetime")); spy.mockRestore(); - expect(reputationPrepares).toHaveLength(3); // submitter_stats + review_targets quality scan + cadence scan, ONCE + expect(reputationPrepares).toHaveLength(3); // submitter_outcome_log agg + review_targets quality scan + cadence scan, ONCE }); it("INVARIANT (#4446): a real agent-regate-pr pass with AI review persists a non-negative reviewDurationMs onto the publish audit event", async () => { diff --git a/test/unit/reputation-wiring.test.ts b/test/unit/reputation-wiring.test.ts index e049dd13b3..4036001471 100644 --- a/test/unit/reputation-wiring.test.ts +++ b/test/unit/reputation-wiring.test.ts @@ -39,6 +39,13 @@ async function seedReviewTarget( } // A submitter who FLOODED the project with submissions but landed almost none — the burst anti-abuse pattern. +// #9131: getSubmitterReputation's aggregate (submissions/merged/closed/manual — what the burst check reads) +// is now WINDOWED from submitter_outcome_log, not the all-time submitter_stats row, so seeding this +// submitter's history for a burst-detection test must populate that log with one row per DISTINCT +// (synthetic) pull_number — a global counter keeps every seeded row's key unique even across multiple +// seedSubmitter calls in the same test. submitter_stats is also written so the /stats-facing all-time +// aggregate stays internally consistent with whatever was "seeded", even though nothing under test reads it. +let seedPullNumberCounter = 900_000; async function seedSubmitter( env: Env, args: { project: string; submitter: string; submissions: number; merged: number; closed: number; manual: number }, @@ -48,6 +55,20 @@ async function seedSubmitter( ) .bind(args.project, args.submitter, args.submissions, args.merged, args.closed, args.manual) .run(); + const rows: Array<"merged" | "closed" | "manual"> = [ + ...Array(args.merged).fill("merged" as const), + ...Array(args.closed).fill("closed" as const), + ...Array(args.manual).fill("manual" as const), + ]; + // `submissions` may exceed merged+closed+manual (a caller simulating raw webhook-pass noise pre-#9131); + // pad the remainder with "manual" rows so the windowed submissions COUNT(*) still matches what was asked. + while (rows.length < args.submissions) rows.push("manual"); + for (const outcome of rows) { + seedPullNumberCounter += 1; + await env.DB.prepare("INSERT INTO submitter_outcome_log (project, submitter, pull_number, outcome) VALUES (?, ?, ?, ?)") + .bind(args.project, args.submitter, seedPullNumberCounter, outcome) + .run(); + } } function aiEnv(over: Partial = {}) { @@ -718,12 +739,56 @@ describe("processGitHubWebhook records the reputation outcome on a terminal PR ( const row = await env.DB.prepare("SELECT COUNT(*) AS n FROM submitter_stats").first<{ n: number }>(); expect(row?.n).toBe(0); }); + + it("#9131: a pull_request_review webhook on a rival's held PR NEVER drives the reputation counter, even though the gate re-runs and would otherwise flag 'manual'", async () => { + const { processJob } = await import("../../src/queue/processors"); + const { upsertRepositorySettings } = await import("../../src/db/repositories"); + const env = createTestEnv({ LOOPOVER_REVIEW_REPUTATION: "true" }); + // A held PR: gate ON with a blocking rule, so re-gating this same PR would route to "manual" if reached. + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory" }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { publicSurface: "off", commentMode: "off", checkRunMode: "off" } }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + return new Response("not found", { status: 404 }); + }); + const pull_request = { + number: 4246, + title: "Rival's held PR", + state: "open" as const, + merged_at: null, + user: { login: "rival-author" }, + head: { sha: "riva1sha" }, + labels: [], + body: "no linked issue at all", // no "Fixes #N" → the linked-issue rule blocks → gate routes to manual + }; + try { + // A THIRD PARTY's review comment re-gates the same PR -- must never be read as a submission by + // "rival-author". + await processJob(env, { + type: "github-webhook", + deliveryId: "rep-review-comment-no-count", + eventName: "pull_request_review_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: true, owner: { login: "JSONbored" } }, + pull_request, + }, + }); + } finally { + vi.unstubAllGlobals(); + } + const row = await env.DB.prepare("SELECT COUNT(*) AS n FROM submitter_stats WHERE submitter = 'rival-author'").first<{ n: number }>(); + expect(row?.n).toBe(0); + }); }); describe("recordReputationOutcome + the 0046 submitter_stats migration", () => { it("FLAG-OFF (default): records NOTHING — the table stays empty", async () => { const env = createTestEnv({ LOOPOVER_REVIEW_REPUTATION: "false" }); - await recordReputationOutcome(env, { project: "acme/widgets", submitter: "alice", outcome: "closed" }); + await recordReputationOutcome(env, { project: "acme/widgets", submitter: "alice", pullNumber: 1, outcome: "closed" }); // The migration applied (the table exists and is queryable) but nothing was written. const row = await env.DB.prepare("SELECT COUNT(*) AS n FROM submitter_stats").first<{ n: number }>(); expect(row?.n).toBe(0); @@ -731,8 +796,8 @@ describe("recordReputationOutcome + the 0046 submitter_stats migration", () => { it("FLAG-ON: records the outcome and a round-trip read reflects the counts (migration applied)", async () => { const env = createTestEnv({ LOOPOVER_REVIEW_REPUTATION: "true" }); - await recordReputationOutcome(env, { project: "acme/widgets", submitter: "alice", outcome: "merged" }); - await recordReputationOutcome(env, { project: "acme/widgets", submitter: "alice", outcome: "closed" }); + await recordReputationOutcome(env, { project: "acme/widgets", submitter: "alice", pullNumber: 1, outcome: "merged" }); + await recordReputationOutcome(env, { project: "acme/widgets", submitter: "alice", pullNumber: 2, outcome: "closed" }); const stats = await getSubmitterReputation(env, "acme/widgets", "alice"); expect(stats.submissions).toBe(2); expect(stats.merged).toBe(1); @@ -748,14 +813,17 @@ describe("recordReputationOutcome + the 0046 submitter_stats migration", () => { preparedSql = sql; return { bind: vi.fn(() => ({ - run: vi.fn(async () => ({})), + // #9131: the FIRST prepare (the idempotency-log INSERT OR IGNORE) must report a real change or + // recordSubmissionOutcome short-circuits before ever reaching the submitter_stats upsert this + // test asserts on -- preparedSql ends up holding whichever prepare ran LAST. + run: vi.fn(async () => ({ meta: { changes: 1 } })), })), }; }), }, } as unknown as Env; - await recordSubmissionOutcome(env, "acme/widgets", "alice", "merged"); + await recordSubmissionOutcome(env, "acme/widgets", "alice", 1, "merged"); expect(preparedSql).toContain("submissions = submitter_stats.submissions + 1"); expect(preparedSql).toContain("merged = submitter_stats.merged + 1"); diff --git a/test/unit/routes-watches.test.ts b/test/unit/routes-watches.test.ts index 43a4898c9e..27b19430e6 100644 --- a/test/unit/routes-watches.test.ts +++ b/test/unit/routes-watches.test.ts @@ -3,7 +3,7 @@ import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; import { describe, expect, it } from "vitest"; import { createApp } from "../../src/api/routes"; import { LoopoverMcp } from "../../src/mcp/server"; -import { upsertIssueWatchSubscription, upsertRepositoryFromGitHub } from "../../src/db/repositories"; +import { upsertInstallation, upsertIssueWatchSubscription, upsertRepositoryFromGitHub } from "../../src/db/repositories"; import { createTestEnv } from "../helpers/d1"; // #6746: GET/POST/DELETE /v1/contributors/:login/watches — the REST mirror of the loopover_watch_issues MCP tool. @@ -26,6 +26,20 @@ async function seedPublicRepo(env: ReturnType, fullName: s await upsertRepositoryFromGitHub(env, { name: name!, full_name: fullName, private: false, owner: { login: owner! } }, 555); } +async function seedPrivateRepo(env: ReturnType, fullName: string): Promise { + const [owner, name] = fullName.split("/"); + await upsertRepositoryFromGitHub(env, { name: name!, full_name: fullName, private: true, owner: { login: owner! } }, 555); +} + +// Mirrors access-boundary.test.ts's seedOwnedRepo: an installed (not merely upserted) repo, so +// buildControlPanelAccessScope's ownedInstalledRepos/accountLogins actually grant the owning login access. +async function seedInstalledPrivateRepo(env: ReturnType, owner: string, name: string, installationId: number): Promise { + await upsertInstallation(env, { + installation: { id: installationId, account: { login: owner, id: installationId, type: "User" }, repository_selection: "selected", permissions: { metadata: "read" }, events: ["repository"] }, + }); + await upsertRepositoryFromGitHub(env, { name, full_name: `${owner}/${name}`, private: true, owner: { login: owner } }, installationId); +} + describe("GET /v1/contributors/:login/watches (#6746)", () => { it("returns the contributor's watch subscriptions", async () => { const app = createApp(); @@ -106,6 +120,32 @@ describe("POST /v1/contributors/:login/watches (#6746)", () => { await expect(response.json()).resolves.toEqual({ error: "forbidden_repo" }); }); + it("403s a PRIVATE repo the login has no maintainer/owner/operator scope over", async () => { + const app = createApp(); + const env = createTestEnv(); + await seedPrivateRepo(env, "acme/secrets"); + const response = await app.request( + "/v1/contributors/miner1/watches", + { method: "POST", headers: jsonHeaders(env), body: JSON.stringify({ repoFullName: "acme/secrets" }) }, + env, + ); + expect(response.status).toBe(403); + await expect(response.json()).resolves.toEqual({ error: "forbidden_repo" }); + }); + + it("watches a PRIVATE repo the login owns via an installed account (accountLogins match grants canLoginAccessRepo)", async () => { + const app = createApp(); + const env = createTestEnv(); + await seedInstalledPrivateRepo(env, "acme", "secrets", 555); + const response = await app.request( + "/v1/contributors/acme/watches", + { method: "POST", headers: jsonHeaders(env), body: JSON.stringify({ repoFullName: "acme/secrets" }) }, + env, + ); + expect(response.status).toBe(200); + await expect(response.json()).resolves.toMatchObject({ changed: "watching acme/secrets" }); + }); + it("rejects a malformed or non-JSON body with 400", async () => { const app = createApp(); const env = createTestEnv(); diff --git a/test/unit/submitter-reputation.test.ts b/test/unit/submitter-reputation.test.ts index c5f28b8d2d..35198bf27c 100644 --- a/test/unit/submitter-reputation.test.ts +++ b/test/unit/submitter-reputation.test.ts @@ -14,6 +14,7 @@ import { type ReputationConfig, signalFromCounts, } from "../../src/review/submitter-reputation"; +import { createTestEnv, TestD1Database } from "../helpers/d1"; // NOTE: this is the SELF-CONTAINED native port of reviewbot's submitter-reputation test. The reviewbot // original also exercised applyNonContentGate / decideNonContentGate (the gate wiring + owner exemption); @@ -207,40 +208,73 @@ describe("recordSubmissionOutcome / getSubmitterReputation (D1, fail-safe)", () expect(rep.signal).toBe("neutral"); }); it("recordSubmissionOutcome never throws (no submitter / no DB / DB ok)", async () => { - await expect(recordSubmissionOutcome({} as Env, "p", undefined, "merged")).resolves.toBeUndefined(); - await expect(recordSubmissionOutcome({ DB: { prepare: () => ({ bind: () => ({ run: async () => undefined }) }) } } as unknown as Env, "p", "u", "closed")).resolves.toBeUndefined(); + await expect(recordSubmissionOutcome({} as Env, "p", undefined, 1, "merged")).resolves.toBeUndefined(); + await expect( + recordSubmissionOutcome( + { DB: { prepare: () => ({ bind: () => ({ run: async () => ({ meta: { changes: 1 } }) }) }) } } as unknown as Env, + "p", + "u", + 1, + "closed", + ), + ).resolves.toBeUndefined(); }); it("recordSubmissionOutcome binds the right column per outcome (merged / closed / manual ternary)", async () => { // Capture the prepared SQL so we can assert the `${col}` interpolation picks the correct column for each // outcome — exercises both ternary arms of `col` (merged → "merged", closed → "closed", manual → "manual"). + // #9131: the FIRST prepare is now the idempotency-log INSERT OR IGNORE; the SECOND (only reached when that + // insert reports a real change) is the submitter_stats upsert this test cares about. const seen: string[] = []; const mkEnv = () => ({ DB: { prepare: (sql: string) => { seen.push(sql); - return { bind: () => ({ run: async () => undefined }) }; + return { bind: () => ({ run: async () => ({ meta: { changes: 1 } }) }) }; }, }, }) as unknown as Env; - await recordSubmissionOutcome(mkEnv(), "p", "u", "merged"); - expect(seen[0]).toContain(", merged, last_seen)"); - expect(seen[0]).toContain("submissions = submitter_stats.submissions + 1"); - expect(seen[0]).toContain("merged = submitter_stats.merged + 1"); + await recordSubmissionOutcome(mkEnv(), "p", "u", 1, "merged"); + expect(seen[0]).toContain("submitter_outcome_log"); + expect(seen[1]).toContain(", merged, last_seen)"); + expect(seen[1]).toContain("submissions = submitter_stats.submissions + 1"); + expect(seen[1]).toContain("merged = submitter_stats.merged + 1"); seen.length = 0; - await recordSubmissionOutcome(mkEnv(), "p", "u", "closed"); - expect(seen[0]).toContain(", closed, last_seen)"); - expect(seen[0]).toContain("submissions = submitter_stats.submissions + 1"); - expect(seen[0]).toContain("closed = submitter_stats.closed + 1"); + await recordSubmissionOutcome(mkEnv(), "p", "u", 2, "closed"); + expect(seen[1]).toContain(", closed, last_seen)"); + expect(seen[1]).toContain("submissions = submitter_stats.submissions + 1"); + expect(seen[1]).toContain("closed = submitter_stats.closed + 1"); seen.length = 0; - await recordSubmissionOutcome(mkEnv(), "p", "u", "manual"); - expect(seen[0]).toContain(", manual, last_seen)"); - expect(seen[0]).toContain("submissions = submitter_stats.submissions + 1"); - expect(seen[0]).toContain("manual = submitter_stats.manual + 1"); + await recordSubmissionOutcome(mkEnv(), "p", "u", 3, "manual"); + expect(seen[1]).toContain(", manual, last_seen)"); + expect(seen[1]).toContain("submissions = submitter_stats.submissions + 1"); + expect(seen[1]).toContain("manual = submitter_stats.manual + 1"); + }); + + it("recordSubmissionOutcome is idempotent per (project, submitter, pullNumber, outcome) — N re-gates of one PR count once (#9131)", async () => { + const db = new TestD1Database() as unknown as D1Database; + const env = { DB: db } as unknown as Env; + const statRow = async () => + (await (db as unknown as TestD1Database).prepare("SELECT submissions, manual FROM submitter_stats WHERE project=? AND submitter=?").bind("p", "u").first<{ submissions: number; manual: number }>())!; + + // Five re-gates of the SAME still-open PR (the self-inflicted #9131 shape: a body edit or push re-gating + // a held PR) all record "manual" — must count exactly once, not five times. + for (let i = 0; i < 5; i++) await recordSubmissionOutcome(env, "p", "u", 42, "manual"); + expect((await statRow()).submissions).toBe(1); + expect((await statRow()).manual).toBe(1); + + // A DIFFERENT pull_number for the same submitter is a genuinely new submission and counts again. + await recordSubmissionOutcome(env, "p", "u", 43, "manual"); + expect((await statRow()).submissions).toBe(2); + expect((await statRow()).manual).toBe(2); + + // A different OUTCOME on the same PR (e.g. eventually merged) is also a new, distinct key. + await recordSubmissionOutcome(env, "p", "u", 42, "merged"); + expect((await statRow()).submissions).toBe(3); }); it("recordSubmissionOutcome swallows a DB error fail-safe (logs, never throws)", async () => { @@ -256,7 +290,7 @@ describe("recordSubmissionOutcome / getSubmitterReputation (D1, fail-safe)", () }), }, } as unknown as Env; - await expect(recordSubmissionOutcome(env, "p", "u", "merged")).resolves.toBeUndefined(); + await expect(recordSubmissionOutcome(env, "p", "u", 1, "merged")).resolves.toBeUndefined(); }); it("getSubmitterReputation → neutral with no submitter (early return guard)", async () => {