diff --git a/src/queue/review-evasion.ts b/src/queue/review-evasion.ts index 54f5895c45..a3a1ba3d85 100644 --- a/src/queue/review-evasion.ts +++ b/src/queue/review-evasion.ts @@ -290,6 +290,8 @@ async function closeDraftDodgeAttemptIfBlocked( draftDodgeAuthorLogin.length > 0 && // #4889: per-repo admin mode swaps the global-allowlist grant for the live per-repo permission. (await isPerTenantAdmin(env, installationId, repoFullName, draftDodgeAuthorLogin)); + if (isProtectedAutomationAuthor(pr.authorLogin, env)) return; + if (isAutoCloseExempt(pr.authorLogin, settings.autoCloseExemptLogins)) return; if ( block && block.headSha === pr.headSha && diff --git a/src/selfhost/metrics.ts b/src/selfhost/metrics.ts index 60feae4909..d135305d51 100644 --- a/src/selfhost/metrics.ts +++ b/src/selfhost/metrics.ts @@ -168,6 +168,7 @@ export const DEFAULT_METRIC_META: readonly (readonly [string, MetricMeta])[] = [ ["loopover_active_review_reconciliation_terminalized_total", { help: "Orphaned active_review_tracking rows terminalized after a live GitHub check confirmed the PR is closed, by repo.", type: "counter" }], ["loopover_open_pr_reconciliation_missing_total", { help: "Open PRs found missing from local tracking during reconciliation, by repo.", type: "counter" }], ["loopover_orb_relay_malformed_events_total", { help: "Orb relay batch entries dropped for missing/mistyped required fields (deliveryId/eventName/rawBody).", type: "counter" }], + ["loopover_orb_relay_multiple_live_enrollments_total", { help: "Forwarded orb events where more than one LIVE enrollment existed for the installation (a blue/green swap, or a secret rotated but not yet revoked) -- the winner is still elected deterministically, but the overlap is no longer silent (#9150).", type: "counter" }], ["loopover_orb_relay_register_total", { help: "Orb relay registration attempts, by mode and result (registered/recovered/failed).", type: "counter" }], ["loopover_pr_outcomes_total", { help: "Recorded PR gate outcomes, by decision.", type: "counter" }], ["loopover_close_audit_holdouts_total", { help: "Would-auto-close PRs diverted to human adjudication by the close-audit holdout (#8831).", type: "counter" }], @@ -181,6 +182,7 @@ export const DEFAULT_METRIC_META: readonly (readonly [string, MetricMeta])[] = [ ["loopover_rees_enrich_request_duration_seconds", { help: "REES /v1/enrich call duration in seconds, for calls that were actually attempted (excludes the auth-rejected circuit-breaker skip).", type: "histogram" }], ["loopover_metrics_sampler_errors_total", { help: "Scrape-time gauge sampler failures, by metric name -- a failing sampler previously emitted no series at all, silently. Any occurrence means that metric's value is currently invisible to Prometheus this scrape (see the sentinel gauges' own -1-on-failure convention).", type: "counter" }], ["loopover_review_source_fresh", { help: "1 when a review/ops/reputation source table has a row inside its own consumer's window, 0 when stale -- labeled by table and window_days. review_targets was silently orphaned by the 2026-06-22 convergence cutover for months before anyone noticed; this makes the next such orphaning loud instead.", type: "gauge" }], + ["loopover_private_manifest_warnings_total", { help: "Private-manifest layer warnings (a malformed shared/global/repo config layer dropped during load), counted one per warning rather than one per load -- a sustained run means a mount is repeatedly serving truncated or invalid config (#9065).", type: "counter" }], ]; const metricMeta = new Map(DEFAULT_METRIC_META); diff --git a/test/unit/queue-lifecycle-guards.test.ts b/test/unit/queue-lifecycle-guards.test.ts index 7606aa71b4..51b61643f3 100644 --- a/test/unit/queue-lifecycle-guards.test.ts +++ b/test/unit/queue-lifecycle-guards.test.ts @@ -1140,6 +1140,53 @@ describe("converted_to_draft gate-close (draft-dodge prevention)", () => { expect(JSON.parse(audit?.metadata_json ?? "{}").explanationPosted).toBe(true); }); + it("REGRESSION (#9294): honors settings.autoCloseExemptLogins -- the shared allowlist its five sibling review-evasion guards already use", async () => { + const calls: Array<{ url: string; method: string; body?: unknown }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method, body: init?.body ? JSON.parse(String(init.body)) : undefined }); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.endsWith("/issues/42/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + if (url.endsWith("/pulls/42") && method === "PATCH") return Response.json({ state: "closed" }); + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupRepo(env); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { reviewCheckMode: "required", autoCloseExemptLogins: ["contributor"] } }); + await recordGateBlockOutcome(env, { repoFullName: "JSONbored/gittensory", pullNumber: 42, headSha: "abc123", blockerCodes: ["missing_linked_issue"] }); + + await processJob(env, { type: "github-webhook", deliveryId: "draft-dodge-exempt", eventName: "pull_request", payload: draftPayload("contributor") }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + const audit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.draft_dodge_closed").first<{ n: number }>(); + expect(audit?.n).toBe(0); + }); + + it("REGRESSION (#9294): honors protected-automation-author exemption -- the same allowlist its five sibling review-evasion guards already use", async () => { + const calls: Array<{ url: string; method: string; body?: unknown }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method, body: init?.body ? JSON.parse(String(init.body)) : undefined }); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.endsWith("/issues/42/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + if (url.endsWith("/pulls/42") && method === "PATCH") return Response.json({ state: "closed" }); + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupRepo(env); + await recordGateBlockOutcome(env, { repoFullName: "JSONbored/gittensory", pullNumber: 42, headSha: "abc123", blockerCodes: ["missing_linked_issue"] }); + + await processJob(env, { type: "github-webhook", deliveryId: "draft-dodge-protected-bot", eventName: "pull_request", payload: draftPayload("dependabot[bot]") }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + const audit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.draft_dodge_closed").first<{ n: number }>(); + expect(audit?.n).toBe(0); + }); + it("#8801: a FAILED close records outcome 'error' with the failure named — never a false 'completed' (the #2260 contract)", async () => { const calls: Array<{ url: string; method: string }> = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { diff --git a/test/unit/review-evasion-per-repo-admin.test.ts b/test/unit/review-evasion-per-repo-admin.test.ts index 837bd40996..71c859efed 100644 --- a/test/unit/review-evasion-per-repo-admin.test.ts +++ b/test/unit/review-evasion-per-repo-admin.test.ts @@ -4,6 +4,7 @@ import { maybeCloseReviewEvasionSelfClose, maybeRecloseDisallowedReopen, } from "../../src/queue/review-evasion"; +import { recordGateBlockOutcome } from "../../src/db/repositories"; import { createTestEnv } from "../helpers/d1"; import type { GitHubWebhookPayload, PullRequestRecord, RepositorySettings } from "../../src/types"; import { generatePrivateKeyPem } from "../helpers/github-app-key"; @@ -137,3 +138,29 @@ describe("maybeCloseDraftDodgeAttempt fleet-operator exemption (#4889)", () => { expect(urls).toEqual([]); }); }); + +describe("maybeCloseDraftDodgeAttempt shared exemption allowlists (#9294)", () => { + it("returns early without closing when the author is on settings.autoCloseExemptLogins and the head is gate-blocked", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + const urls = stubGitHub((url) => { + if (url.endsWith("/pulls/7") || url.endsWith("/issues/7/comments")) return Response.json({}); + return undefined; + }); + await recordGateBlockOutcome(env, { repoFullName: "owner/repo", pullNumber: 7, headSha: "abc123", blockerCodes: ["missing_linked_issue"] }); + const settings = { reviewEvasionProtection: "on", autoCloseExemptLogins: ["trusted-bot"], autonomy: { close: "auto" } } as unknown as RepositorySettings; + await maybeCloseDraftDodgeAttempt(env, "d1", 123, "owner/repo", pr({ authorLogin: "trusted-bot", headSha: "abc123" }), settings); + expect(urls.some((url) => url.includes("/pulls/7"))).toBe(false); + }); + + it("returns early without closing when the author is a protected automation bot and the head is gate-blocked", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + const urls = stubGitHub((url) => { + if (url.endsWith("/pulls/7") || url.endsWith("/issues/7/comments")) return Response.json({}); + return undefined; + }); + await recordGateBlockOutcome(env, { repoFullName: "owner/repo", pullNumber: 7, headSha: "abc123", blockerCodes: ["missing_linked_issue"] }); + const settings = { reviewEvasionProtection: "on", autoCloseExemptLogins: [], autonomy: { close: "auto" } } as unknown as RepositorySettings; + await maybeCloseDraftDodgeAttempt(env, "d1", 123, "owner/repo", pr({ authorLogin: "github-actions[bot]", headSha: "abc123" }), settings); + expect(urls.some((url) => url.includes("/pulls/7"))).toBe(false); + }); +}); diff --git a/test/unit/salvageability.test.ts b/test/unit/salvageability.test.ts index 2f4c8c843f..d217fb8c3f 100644 --- a/test/unit/salvageability.test.ts +++ b/test/unit/salvageability.test.ts @@ -81,11 +81,21 @@ describe("resolveAiReviewSalvageableHold", () => { expect(hold!.comment).toContain("HELD with guidance"); }); - it("defaults: no configured floor uses the gate default, and a confidence-less blocker counts as certainty (at/above floor)", () => { + // #9085 (landed via #9237): an absent confidence used to degrade to 1.0 -- maximum certainty -- so "the + // model said nothing" silently cleared even the default floor here. It now degrades to + // CONFIDENCE_WHEN_UNSTATED (0.5), which is sub-floor against the 0.93 gate default, so the low-confidence + // hold owns that case and this resolver stands down. #9085 renamed both sibling assertions in + // rules.test.ts but missed this third consumption site, leaving it asserting the pre-change semantics. + // Both calls are load-bearing for coverage: the first is the ONLY case in this file that exercises the + // `aiReviewCloseConfidence ?? DEFAULT_AI_REVIEW_CLOSE_CONFIDENCE` default arm (every other case passes an + // explicit floor), and the second is the only one exercising the `confidence ?? CONFIDENCE_WHEN_UNSTATED` + // nullish arm. + it("defaults: no configured floor uses the gate default (0.93), and a confidence-less blocker is sub-floor", () => { + expect(resolveAiReviewSalvageableHold(aiEval(0.99), { aiReviewSalvageabilityMinScore: 60 }, salv)).toBeDefined(); + const noConfidence = aiEval(0.99); delete (noConfidence.blockers[0] as { confidence?: number }).confidence; - const hold = resolveAiReviewSalvageableHold(noConfidence, { aiReviewSalvageabilityMinScore: 60 }, salv); - expect(hold).toBeDefined(); + expect(resolveAiReviewSalvageableHold(noConfidence, { aiReviewSalvageabilityMinScore: 60 }, salv)).toBeUndefined(); }); it("knob unset (the default) or no score: never changes a disposition", () => { diff --git a/test/unit/selfhost-pg-retention.test.ts b/test/unit/selfhost-pg-retention.test.ts index 96c9742d70..eab1b84fa9 100644 --- a/test/unit/selfhost-pg-retention.test.ts +++ b/test/unit/selfhost-pg-retention.test.ts @@ -32,10 +32,15 @@ function makeRetentionPgPool(remaining: Record = {}): MockPgPool return { rows: [{ n: remaining[table] ?? 0 }], rowCount: 1 }; } - const deleteMatch = /^DELETE FROM (\w+) WHERE ctid IN \(SELECT ctid FROM \1 WHERE .*? LIMIT (\d+)\)$/i.exec(q); + // #9083 (via #9237) changed the emitted shape: retention now deletes by PRIMARY KEY (`id`, `delivery_id`, + // ...) with an explicit `ORDER BY `, falling back to the physical row key only for the + // composite-key tables that have no single-column PK -- an index-backed range delete replacing a + // full-scan-per-batch semi-join. Group 2 captures whichever key column is in play, so BOTH paths (mapped + // PK and the `ctid` fallback) stay exercised by this mock rather than one silently ceasing to match. + const deleteMatch = /^DELETE FROM (\w+) WHERE (\w+) IN \(SELECT \2 FROM \1 WHERE .*? ORDER BY \w+ LIMIT (\d+)\)$/i.exec(q); if (deleteMatch) { const table = deleteMatch[1] as string; - const limit = Number(deleteMatch[2]); + const limit = Number(deleteMatch[3]); const have = remaining[table] ?? 0; const changes = Math.min(have, limit); remaining[table] = have - changes; diff --git a/test/unit/worker-entry-boundary.test.ts b/test/unit/worker-entry-boundary.test.ts index 3f0b8bab59..5f6ec43459 100644 --- a/test/unit/worker-entry-boundary.test.ts +++ b/test/unit/worker-entry-boundary.test.ts @@ -72,14 +72,34 @@ describe("worker entry boundary", () => { expect(forbidden, `worker entry must not reach agent-only modules: ${forbidden.join(", ")}`).toEqual([]); }); - it("does not reference pixelmatch, pngjs, visual-diff, gifenc, or sharp in worker-reachable source", () => { + // Scans MODULE SPECIFIERS, not raw file text. What this guards is the Worker BUNDLE: a Node-only dep can + // only get bundled by being imported (statically or dynamically), so parsing the same specifiers + // collectReachableSources already walks catches every real inclusion path. Grepping whole-file content + // instead produced a false positive the moment ordinary English prose contained one of these words -- + // #9230 added the user-facing string "route(s) crossed the visual-diff threshold" to + // src/review/visual/visual-findings.ts (a file worker-reachable since #4120, importing none of these deps), + // and the raw-content regex failed a green tree over a sentence. Bending correct user-facing copy to dodge + // a test regex would have been the wrong repair; narrowing the check to what it actually means is the right + // one. + it("does not import pixelmatch, pngjs, visual-diff, gifenc, or sharp from worker-reachable source", () => { const hits = collectReachableSources(WORKER_ENTRY) .map((file) => { - const content = readFileSync(file, "utf8"); - return FORBIDDEN_IDENTIFIERS.test(content) ? relativeToRoot(file) : null; + const offending = parseImportSpecifiers(file).filter((specifier) => FORBIDDEN_IDENTIFIERS.test(specifier)); + return offending.length > 0 ? `${relativeToRoot(file)} (${offending.join(", ")})` : null; }) .filter((entry): entry is string => entry !== null); - expect(hits, `worker-reachable files must not mention Node-only visual diff/GIF/image deps: ${hits.join(", ")}`).toEqual([]); + expect(hits, `worker-reachable files must not import Node-only visual diff/GIF/image deps: ${hits.join(", ")}`).toEqual([]); + }); + + // Proves the specifier-scoped check above is still DISCRIMINATING, not vacuously passing: the same regex + // must still flag a real dependency import, and must still ignore the same word in prose. Without this, a + // future edit that broke the matching entirely would look identical to a clean tree. + it("the forbidden-identifier check still flags a real import specifier and still ignores prose", () => { + expect(["sharp", "pixelmatch", "gifenc", "pngjs", "@foo/visual-diff"].every((specifier) => FORBIDDEN_IDENTIFIERS.test(specifier))).toBe(true); + expect(["./capture", "../../types", "node:fs", "hono"].some((specifier) => FORBIDDEN_IDENTIFIERS.test(specifier))).toBe(false); + // The exact #9230 prose that broke the old whole-file scan is not a module specifier, so it is correctly + // invisible to a specifier-scoped check -- while the bare dep name it contains still is not. + expect(parseImportSpecifiers(join(srcRoot, "review/visual/visual-findings.ts")).some((s) => FORBIDDEN_IDENTIFIERS.test(s))).toBe(false); }); it("does not reference visual diff or GIF modules in the published MCP bin bundle", () => {