diff --git a/src/queue/processors.ts b/src/queue/processors.ts index ceaddf0ae2..4f476fb892 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -6329,7 +6329,7 @@ export async function resolveAutoReviewSkipForPullRequest( isFrozenForManualReview: boolean; forceAiReview?: boolean | undefined; repoFullName: string; - pr: { isDraft?: boolean | null; title: string; baseRef?: string | null; number: number }; + pr: { isDraft?: boolean | null; title: string; baseRef?: string | null; number: number; labels?: readonly string[] }; author: string | null; deliveryId: string; headSha: string | null | undefined; @@ -6346,6 +6346,7 @@ export async function resolveAutoReviewSkipForPullRequest( isDraft: args.pr.isDraft === true, author: args.author, title: args.pr.title, + labels: args.pr.labels ?? [], baseRef: args.pr.baseRef ?? null, reviewedCommitCount, }); @@ -8148,7 +8149,7 @@ async function maybePublishPrPublicSurface( isFrozenForManualReview, forceAiReview: webhook.forceAiReview, repoFullName, - pr: { number: pr.number, title: pr.title, baseRef: pr.baseRef ?? null, isDraft: pr.isDraft ?? null }, + pr: { number: pr.number, title: pr.title, baseRef: pr.baseRef ?? null, isDraft: pr.isDraft ?? null, labels: pr.labels }, author, deliveryId: webhook.deliveryId, headSha: advisory.headSha ?? null, diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 8144754bb4..933016a43c 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -414,6 +414,8 @@ export type AutoReviewConfig = { ignoreAuthors: string[]; /** `review.auto_review.ignore_title_keywords`: case-insensitive title substrings that skip AI review. Empty ⇒ no skip. (#2040) */ ignoreTitleKeywords: string[]; + /** `review.auto_review.skip_labels`: case-insensitive PR label names that skip AI review. Empty ⇒ no skip. (#2062) */ + skipLabels: string[]; /** `review.auto_review.base_branches`: base-ref globs whose PRs ARE reviewed; empty/unset ⇒ every base. (#2041) */ baseBranches: string[]; /** `review.auto_review.auto_pause_after_reviewed_commits`: after N published AI reviews on this PR, pause further @@ -425,6 +427,7 @@ export const EMPTY_AUTO_REVIEW_CONFIG: AutoReviewConfig = { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], + skipLabels: [], baseBranches: [], autoPauseAfterReviewedCommits: null, }; @@ -1768,6 +1771,7 @@ function autoReviewPresent(config: AutoReviewConfig): boolean { config.skipDrafts !== null || config.ignoreAuthors.length > 0 || config.ignoreTitleKeywords.length > 0 || + config.skipLabels.length > 0 || config.baseBranches.length > 0 || config.autoPauseAfterReviewedCommits !== null ); @@ -1785,6 +1789,7 @@ function parseAutoReviewConfig(value: JsonValue | undefined, warnings: string[]) skipDrafts: normalizeOptionalBoolean(record.skip_drafts, "review.auto_review.skip_drafts", warnings), ignoreAuthors: parseManifestGlobList(record.ignore_authors, "review.auto_review.ignore_authors", warnings), ignoreTitleKeywords: parseAutoReviewTitleKeywords(record.ignore_title_keywords, warnings), + skipLabels: parseAutoReviewSkipLabels(record.skip_labels, warnings), baseBranches: parseManifestGlobList(record.base_branches, "review.auto_review.base_branches", warnings), autoPauseAfterReviewedCommits: normalizeOptionalNonNegativeInt( record.auto_pause_after_reviewed_commits, @@ -1931,6 +1936,34 @@ function parseAutoReviewTitleKeywords(value: JsonValue | undefined, warnings: st return out; } +function parseAutoReviewSkipLabels(value: JsonValue | undefined, warnings: string[]): string[] { + if (value === undefined || value === null) return []; + if (!Array.isArray(value)) { + warnings.push(`Manifest "review.auto_review.skip_labels" must be a list of strings; ignoring it.`); + return []; + } + const seen = new Set(); + const out: string[] = []; + for (const [index, entry] of value.entries()) { + if (out.length >= MAX_PATH_INSTRUCTIONS) { + warnings.push(`Manifest "review.auto_review.skip_labels" is capped at ${MAX_PATH_INSTRUCTIONS} entries; dropping the rest.`); + break; + } + const raw = typeof entry === "string" ? entry.trim() : ""; + if (!raw) { + warnings.push(`Manifest "review.auto_review.skip_labels[${index}]" must be a non-empty string; ignoring it.`); + continue; + } + const safe = parsePublicSafeText(raw, `review.auto_review.skip_labels[${index}]`, warnings); + if (safe === null) continue; + const key = safe.toLowerCase(); + if (seen.has(key)) continue; + seen.add(key); + out.push(key); + } + return out; +} + /** Parse `review.pre_merge_checks` — an array of DETERMINISTIC pre-merge assertions. Each entry needs a non-empty * public-safe `name` and at least ONE assertion (`title_contains` / `description_contains` / `require_label`, * each public-safe); `when_paths` (optional) gates the check to PRs touching a matching glob; `enforce` (default @@ -2122,6 +2155,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.autoReview.skipDrafts !== null) autoReview.skip_drafts = review.autoReview.skipDrafts; if (review.autoReview.ignoreAuthors.length > 0) autoReview.ignore_authors = [...review.autoReview.ignoreAuthors]; if (review.autoReview.ignoreTitleKeywords.length > 0) autoReview.ignore_title_keywords = [...review.autoReview.ignoreTitleKeywords]; + if (review.autoReview.skipLabels.length > 0) autoReview.skip_labels = [...review.autoReview.skipLabels]; if (review.autoReview.baseBranches.length > 0) autoReview.base_branches = [...review.autoReview.baseBranches]; if (review.autoReview.autoPauseAfterReviewedCommits !== null) { autoReview.auto_pause_after_reviewed_commits = review.autoReview.autoPauseAfterReviewedCommits; @@ -2196,6 +2230,7 @@ export type AutoReviewEligibilityInput = { isDraft: boolean; author: string | null; title: string; + labels: readonly string[]; baseRef: string | null; reviewedCommitCount: number; }; @@ -2215,6 +2250,12 @@ export function evaluateAutoReviewSkipReason(config: AutoReviewConfig, input: Au return "review skipped (WIP title)"; } } + if (config.skipLabels.length > 0 && input.labels.length > 0) { + const prLabels = new Set(input.labels.map((label) => label.toLowerCase())); + if (config.skipLabels.some((label) => prLabels.has(label))) { + return "review skipped (label)"; + } + } if (config.baseBranches.length > 0) { const baseRef = input.baseRef?.trim() ?? ""; if (!baseRef || !config.baseBranches.some((glob) => matchesManifestPath(baseRef, glob))) { @@ -2235,6 +2276,7 @@ export function resolvePullRequestAutoReviewSkipReason(args: { isDraft: boolean; author: string | null; title: string; + labels?: readonly string[] | undefined; baseRef: string | null; reviewedCommitCount?: number | undefined; }): string | null { @@ -2243,6 +2285,7 @@ export function resolvePullRequestAutoReviewSkipReason(args: { isDraft: args.isDraft, author: args.author, title: args.title, + labels: args.labels ?? [], baseRef: args.baseRef, reviewedCommitCount: args.reviewedCommitCount ?? 0, }); diff --git a/test/unit/auto-review-wiring.test.ts b/test/unit/auto-review-wiring.test.ts index 07af65fae7..5a1cae88f6 100644 --- a/test/unit/auto-review-wiring.test.ts +++ b/test/unit/auto-review-wiring.test.ts @@ -24,6 +24,39 @@ describe("review.auto_review wiring (#1954)", () => { ).toBeNull(); }); + it("resolvePullRequestAutoReviewSkipReason: skips when a configured label is present", () => { + const manifest = parseFocusManifest({ review: { auto_review: { skip_labels: ["do-not-review"] } } }); + expect( + resolvePullRequestAutoReviewSkipReason({ + manifest, + isDraft: false, + author: "alice", + title: "feat: thing", + labels: ["Do-Not-Review"], + baseRef: "main", + }), + ).toBe("review skipped (label)"); + expect( + resolvePullRequestAutoReviewSkipReason({ + manifest, + isDraft: false, + author: "alice", + title: "feat: thing", + labels: ["feature"], + baseRef: "main", + }), + ).toBeNull(); + expect( + resolvePullRequestAutoReviewSkipReason({ + manifest, + isDraft: false, + author: "alice", + title: "feat: thing", + baseRef: "main", + }), + ).toBeNull(); + }); + it("resolvePullRequestAutoReviewSkipReason: matches the documented *[bot] author glob", () => { const manifest = parseFocusManifest({ review: { auto_review: { ignore_authors: ["*[bot]"] } } }); expect( @@ -108,7 +141,7 @@ describe("review.auto_review wiring (#1954)", () => { authorBlacklisted: false, isFrozenForManualReview: false, repoFullName: "acme/widgets", - pr: { number: 3, title: "WIP", baseRef: "main", isDraft: true }, + pr: { number: 3, title: "WIP", baseRef: "main", isDraft: true, labels: [] }, author: "alice", deliveryId: "d3", headSha: "sha3", @@ -116,6 +149,20 @@ describe("review.auto_review wiring (#1954)", () => { ).resolves.toEqual({ skipReason: "review skipped (draft)", reviewManifest: manifest }); expect(auditSpy).toHaveBeenCalled(); + const labelManifest = parseFocusManifest({ review: { auto_review: { skip_labels: ["do-not-review"] } } }); + loadSpy.mockResolvedValueOnce(labelManifest); + await expect( + resolveAutoReviewSkipForPullRequest({} as Env, { + authorBlacklisted: false, + isFrozenForManualReview: false, + repoFullName: "acme/widgets", + pr: { number: 6, title: "feat", baseRef: "main", isDraft: false, labels: ["Do-Not-Review"] }, + author: "alice", + deliveryId: "d6", + headSha: "sha6", + }), + ).resolves.toEqual({ skipReason: "review skipped (label)", reviewManifest: labelManifest }); + loadSpy.mockRejectedValueOnce(new Error("manifest unavailable")); await expect( resolveAutoReviewSkipForPullRequest({} as Env, { @@ -129,6 +176,19 @@ describe("review.auto_review wiring (#1954)", () => { }), ).resolves.toEqual({ skipReason: null, reviewManifest: null }); + loadSpy.mockResolvedValueOnce(labelManifest); + await expect( + resolveAutoReviewSkipForPullRequest({} as Env, { + authorBlacklisted: false, + isFrozenForManualReview: false, + repoFullName: "acme/widgets", + pr: { number: 7, title: "feat", baseRef: "main", isDraft: false }, + author: "alice", + deliveryId: "d7", + headSha: "sha7", + }), + ).resolves.toEqual({ skipReason: null, reviewManifest: labelManifest }); + loadSpy.mockRestore(); }); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index db3dfae653..51bf5cd76c 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -3080,6 +3080,7 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => { skip_drafts: true, ignore_authors: [" *[bot] ", "dependabot[bot]"], ignore_title_keywords: [" WIP ", "draft"], + skip_labels: [" do-not-review ", "WIP"], base_branches: ["main", "release/**"], }, }, @@ -3088,6 +3089,7 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => { skipDrafts: true, ignoreAuthors: ["*[bot]", "dependabot[bot]"], ignoreTitleKeywords: ["WIP", "draft"], + skipLabels: ["do-not-review", "wip"], baseBranches: ["main", "release/**"], autoPauseAfterReviewedCommits: null, }); @@ -3119,7 +3121,7 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => { it("evaluateAutoReviewSkipReason: byte-identical when unset; skips with deterministic reasons when configured", () => { const empty = { ...EMPTY_AUTO_REVIEW_CONFIG }; - const input = { isDraft: true, author: "dependabot[bot]", title: "WIP: bump deps", baseRef: "develop", reviewedCommitCount: 0 }; + const input = { isDraft: true, author: "dependabot[bot]", title: "WIP: bump deps", labels: [] as string[], baseRef: "develop", reviewedCommitCount: 0 }; expect(evaluateAutoReviewSkipReason(empty, input)).toBeNull(); expect(evaluateAutoReviewSkipReason({ ...empty, skipDrafts: true }, { ...input, isDraft: true })).toBe("review skipped (draft)"); expect(evaluateAutoReviewSkipReason({ ...empty, skipDrafts: true }, { ...input, isDraft: false })).toBeNull(); @@ -3130,6 +3132,11 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => { expect(evaluateAutoReviewSkipReason({ ...empty, ignoreAuthors: ["human"] }, input)).toBeNull(); expect(evaluateAutoReviewSkipReason({ ...empty, ignoreTitleKeywords: ["wip"] }, { ...input, title: "Fix WIP regression" })).toBe("review skipped (WIP title)"); expect(evaluateAutoReviewSkipReason({ ...empty, ignoreTitleKeywords: ["wip"] }, { ...input, title: "Fix regression" })).toBeNull(); + expect(evaluateAutoReviewSkipReason({ ...empty, skipLabels: ["do-not-review"] }, { ...input, labels: ["Do-Not-Review"] })).toBe("review skipped (label)"); + expect(evaluateAutoReviewSkipReason({ ...empty, skipLabels: ["wip", "hold"] }, { ...input, labels: ["hold"] })).toBe("review skipped (label)"); + expect(evaluateAutoReviewSkipReason({ ...empty, skipLabels: ["wip"] }, { ...input, labels: ["feature"] })).toBeNull(); + expect(evaluateAutoReviewSkipReason({ ...empty, skipLabels: ["wip"] }, { ...input, labels: [] })).toBeNull(); + expect(evaluateAutoReviewSkipReason({ ...empty, skipLabels: [] }, { ...input, labels: ["feature"] })).toBeNull(); expect(evaluateAutoReviewSkipReason({ ...empty, baseBranches: ["main"] }, { ...input, baseRef: "develop" })).toBe( "review skipped (base branch out of scope)", ); @@ -3178,10 +3185,32 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => { expect(reviewConfigToJson(authorsOnly.review)).toEqual({ auto_review: { ignore_authors: ["*[bot]"] } }); const keywordsOnly = parseFocusManifest({ review: { auto_review: { ignore_title_keywords: ["DRAFT"] } } }); expect(reviewConfigToJson(keywordsOnly.review)).toEqual({ auto_review: { ignore_title_keywords: ["DRAFT"] } }); + const labelsOnly = parseFocusManifest({ review: { auto_review: { skip_labels: ["do-not-review"] } } }); + expect(reviewConfigToJson(labelsOnly.review)).toEqual({ auto_review: { skip_labels: ["do-not-review"] } }); const basesOnly = parseFocusManifest({ review: { auto_review: { base_branches: ["main"] } } }); expect(reviewConfigToJson(basesOnly.review)).toEqual({ auto_review: { base_branches: ["main"] } }); }); + it("warns on invalid skip_labels list shapes, dedupes case-insensitively, and caps entries", () => { + const bad = parseFocusManifest({ review: { auto_review: { skip_labels: "wip" } } }); + expect(bad.review.autoReview.skipLabels).toEqual([]); + expect(bad.warnings.some((w) => /skip_labels.*must be a list/.test(w))).toBe(true); + const deduped = parseFocusManifest({ review: { auto_review: { skip_labels: ["WIP", "wip", ""] } } }); + expect(deduped.review.autoReview.skipLabels).toEqual(["wip"]); + expect(deduped.warnings.some((w) => /skip_labels\[2\]/.test(w))).toBe(true); + const nonString = parseFocusManifest({ review: { auto_review: { skip_labels: ["wip", 42] } } }); + expect(nonString.review.autoReview.skipLabels).toEqual(["wip"]); + expect(nonString.warnings.some((w) => /skip_labels\[1\]/.test(w))).toBe(true); + const unsafe = parseFocusManifest({ review: { auto_review: { skip_labels: ["wip", "reward payout"] } } }); + expect(unsafe.review.autoReview.skipLabels).toEqual(["wip"]); + expect(unsafe.warnings.some((w) => /skip_labels\[1\]/.test(w))).toBe(true); + const many = parseFocusManifest({ + review: { auto_review: { skip_labels: Array.from({ length: 60 }, (_, i) => `label${i}`) } }, + }); + expect(many.review.autoReview.skipLabels).toHaveLength(50); + expect(many.warnings.some((w) => /skip_labels.*capped/.test(w))).toBe(true); + }); + it("warns on invalid ignore_title_keywords list shapes and caps entries", () => { const bad = parseFocusManifest({ review: { auto_review: { ignore_title_keywords: "WIP" } } }); expect(bad.review.autoReview.ignoreTitleKeywords).toEqual([]); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index afac69578f..fee6041cfd 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -3244,6 +3244,52 @@ describe("queue processors", () => { expect(audit?.detail).toBe("review skipped (draft)"); }); + it("skips AI review when review.auto_review.skip_labels matches a PR label (#2062)", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }; } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await seedRegateChurnRepo(env); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { review: { auto_review: { skip_labels: ["do-not-review"] } } }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 78, + title: "Ready feature", + state: "open", + draft: false, + user: { login: "contributor" }, + head: { sha: "a78" }, + labels: [{ name: "Do-Not-Review" }], + body: "Closes #1", + } as never); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 78, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/pulls/78/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/78")) return Response.json({ number: 78, title: "Ready feature", state: "open", draft: false, user: { login: "contributor" }, head: { sha: "a78" }, labels: [{ name: "Do-Not-Review" }], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a78/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a78/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/78/comments")) return method === "POST" ? Response.json({ id: 78 }, { status: 201 }) : Response.json([]); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId: "auto-review-skip-label", repoFullName: "JSONbored/gittensory", prNumber: 78, installationId: 123 }), + ).resolves.toBeUndefined(); + expect(aiCalls).toBe(0); + const audit = await env.DB.prepare("select detail from audit_events where event_type = ? and target_key = ?") + .bind("github_app.ai_review_auto_review_skipped", "JSONbored/gittensory#78") + .first<{ detail: string }>(); + expect(audit?.detail).toBe("review skipped (label)"); + }); + it("runs AI review with cached manifest when auto_review eligibility passes (#1954)", async () => { let aiCalls = 0; const env = createTestEnv({ diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 3084f004c1..1420ceedeb 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1127,7 +1127,7 @@ describe("signal coverage edge cases", () => { collisions: buildCollisionReport(directRepo.fullName, [], [currentPr]), preflight: buildPreflightResult({ repoFullName: directRepo.fullName, title: "Fix isolated issue", body: "Fixes #99", linkedIssues: [99] }, directRepo, [], [currentPr]), settings: gateSettings, - review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [] }, linkedIssueSatisfaction: null }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [] }, linkedIssueSatisfaction: null }, aiReview: { notes: "The change is focused.\n\n**Nits (2)**\n- Add a test for the edge case.\n- Keep the validator helper scoped." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead