From f3dce33a11f8d969659a39fc8bddc6b21bc809dd Mon Sep 17 00:00:00 2001 From: bohdansolovie Date: Mon, 6 Jul 2026 06:12:08 +0200 Subject: [PATCH] feat(review): skip auto-review of docs-only PRs via review.auto_review.skip_docs_only Add a manifest knob that skips AI review when every changed file classifies as docs, thread changed paths through the queue processor, and cover parse, eligibility, wiring, and integration paths for codecov. Co-authored-by: Cursor --- src/queue/processors.ts | 4 ++ src/signals/focus-manifest.ts | 16 ++++++ test/unit/auto-review-config-matrix.test.ts | 13 +++++ test/unit/auto-review-wiring.test.ts | 58 +++++++++++++++++++++ test/unit/focus-manifest.test.ts | 18 ++++++- test/unit/queue.test.ts | 49 +++++++++++++++++ test/unit/signals-coverage.test.ts | 2 +- 7 files changed, 158 insertions(+), 2 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 4f476fb892..b630b62717 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -6333,6 +6333,7 @@ export async function resolveAutoReviewSkipForPullRequest( author: string | null; deliveryId: string; headSha: string | null | undefined; + changedPaths?: readonly string[] | undefined; }, ): Promise<{ skipReason: string | null; reviewManifest: FocusManifest | null }> { if (args.authorBlacklisted || args.isFrozenForManualReview) { @@ -6347,6 +6348,7 @@ export async function resolveAutoReviewSkipForPullRequest( author: args.author, title: args.pr.title, labels: args.pr.labels ?? [], + changedPaths: args.changedPaths ?? [], baseRef: args.pr.baseRef ?? null, reviewedCommitCount, }); @@ -8141,6 +8143,7 @@ async function maybePublishPrPublicSurface( pr.labels.some((label) => label.toLowerCase() === manualReviewLabel.toLowerCase()); let reviewManifestForAutoReview: FocusManifest | null = null; let autoReviewSkipReason: string | null = null; + const autoReviewChangedPaths = (await getReviewFiles()).map((file) => file.path); ({ skipReason: autoReviewSkipReason, reviewManifest: reviewManifestForAutoReview, @@ -8153,6 +8156,7 @@ async function maybePublishPrPublicSurface( author, deliveryId: webhook.deliveryId, headSha: advisory.headSha ?? null, + changedPaths: autoReviewChangedPaths, })); // review.changed_files_summary (#1957) + review.effort_score (#1955): both deterministic, no-AI — resolve // them here, UNCONDITIONALLY, rather than inside the aiReviewWillRun-gated closure below. These sections diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 933016a43c..57e0b444b0 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -12,6 +12,7 @@ import { normalizeModerationLabel, normalizeModerationRules } from "../settings/ import { REES_ANALYZER_NAME_SET, type ReesAnalyzerName } from "../review/enrichment-analyzer-names"; import { hasUnsafeWildcardCount } from "./change-guardrail"; import { PUBLIC_LOCAL_PATH_INLINE } from "./redaction"; +import { classifyChangedFile } from "./path-matchers"; import { isSafeHttpUrl } from "../review/content-lane/safe-url"; export type FocusManifestSource = "repo_file" | "api_record" | "none"; @@ -416,6 +417,9 @@ export type AutoReviewConfig = { 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.skip_docs_only`: when true, PRs whose every changed file classifies as docs skip AI review. + * null (default) ⇒ docs PRs reviewed as today. Empty changed-file list ⇒ NOT docs-only (fail-safe eligible). (#2063) */ + skipDocsOnly: boolean | null; /** `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 @@ -428,6 +432,7 @@ export const EMPTY_AUTO_REVIEW_CONFIG: AutoReviewConfig = { ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], + skipDocsOnly: null, baseBranches: [], autoPauseAfterReviewedCommits: null, }; @@ -1772,6 +1777,7 @@ function autoReviewPresent(config: AutoReviewConfig): boolean { config.ignoreAuthors.length > 0 || config.ignoreTitleKeywords.length > 0 || config.skipLabels.length > 0 || + config.skipDocsOnly !== null || config.baseBranches.length > 0 || config.autoPauseAfterReviewedCommits !== null ); @@ -1790,6 +1796,7 @@ function parseAutoReviewConfig(value: JsonValue | undefined, warnings: string[]) ignoreAuthors: parseManifestGlobList(record.ignore_authors, "review.auto_review.ignore_authors", warnings), ignoreTitleKeywords: parseAutoReviewTitleKeywords(record.ignore_title_keywords, warnings), skipLabels: parseAutoReviewSkipLabels(record.skip_labels, warnings), + skipDocsOnly: normalizeOptionalBoolean(record.skip_docs_only, "review.auto_review.skip_docs_only", warnings), baseBranches: parseManifestGlobList(record.base_branches, "review.auto_review.base_branches", warnings), autoPauseAfterReviewedCommits: normalizeOptionalNonNegativeInt( record.auto_pause_after_reviewed_commits, @@ -2156,6 +2163,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue 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.skipDocsOnly !== null) autoReview.skip_docs_only = review.autoReview.skipDocsOnly; 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; @@ -2231,6 +2239,7 @@ export type AutoReviewEligibilityInput = { author: string | null; title: string; labels: readonly string[]; + changedPaths: readonly string[]; baseRef: string | null; reviewedCommitCount: number; }; @@ -2256,6 +2265,11 @@ export function evaluateAutoReviewSkipReason(config: AutoReviewConfig, input: Au return "review skipped (label)"; } } + if (config.skipDocsOnly === true && input.changedPaths.length > 0) { + if (input.changedPaths.every((path) => classifyChangedFile(path) === "docs")) { + return "review skipped (docs only)"; + } + } if (config.baseBranches.length > 0) { const baseRef = input.baseRef?.trim() ?? ""; if (!baseRef || !config.baseBranches.some((glob) => matchesManifestPath(baseRef, glob))) { @@ -2277,6 +2291,7 @@ export function resolvePullRequestAutoReviewSkipReason(args: { author: string | null; title: string; labels?: readonly string[] | undefined; + changedPaths?: readonly string[] | undefined; baseRef: string | null; reviewedCommitCount?: number | undefined; }): string | null { @@ -2286,6 +2301,7 @@ export function resolvePullRequestAutoReviewSkipReason(args: { author: args.author, title: args.title, labels: args.labels ?? [], + changedPaths: args.changedPaths ?? [], baseRef: args.baseRef, reviewedCommitCount: args.reviewedCommitCount ?? 0, }); diff --git a/test/unit/auto-review-config-matrix.test.ts b/test/unit/auto-review-config-matrix.test.ts index 41865b0cdd..54faa72f43 100644 --- a/test/unit/auto-review-config-matrix.test.ts +++ b/test/unit/auto-review-config-matrix.test.ts @@ -22,6 +22,8 @@ describe("review.auto_review parse ↔ reviewConfigToJson round-trip (#2071)", ( { name: "skip_drafts: false", autoReview: { skip_drafts: false } }, { name: "ignore_authors", autoReview: { ignore_authors: ["*[bot]", "dependabot[bot]"] } }, { name: "ignore_title_keywords", autoReview: { ignore_title_keywords: ["WIP", "draft"] } }, + { name: "skip_docs_only: true", autoReview: { skip_docs_only: true } }, + { name: "skip_docs_only: false", autoReview: { skip_docs_only: false } }, { name: "base_branches", autoReview: { base_branches: ["main", "release/**"] } }, { name: "auto_pause_after_reviewed_commits", autoReview: { auto_pause_after_reviewed_commits: 3 } }, { @@ -78,11 +80,14 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => { isDraft: true, author: "dependabot[bot]", title: "WIP: bump deps", + labels: [], + changedPaths: [], baseRef: "develop", reviewedCommitCount: 5, }; const allConfigured: AutoReviewConfig = { + ...EMPTY_AUTO_REVIEW_CONFIG, skipDrafts: true, ignoreAuthors: ["*[bot]"], ignoreTitleKeywords: ["wip"], @@ -114,6 +119,12 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => { input: { ...allTriggers, isDraft: false, author: "alice" }, reason: "review skipped (WIP title)", }, + { + name: "docs only when earlier filters are off", + config: { ...allConfigured, skipDrafts: false, ignoreAuthors: [], ignoreTitleKeywords: [], skipDocsOnly: true }, + input: { ...allTriggers, isDraft: false, author: "alice", title: "docs: readme", changedPaths: ["README.md"] }, + reason: "review skipped (docs only)", + }, { name: "base branch when earlier filters are off", config: { ...allConfigured, skipDrafts: false, ignoreAuthors: [], ignoreTitleKeywords: [] }, @@ -136,6 +147,8 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => { isDraft: false, author: "alice", title: "feat: add widget", + labels: [], + changedPaths: [], baseRef: "main", reviewedCommitCount: 0, }, diff --git a/test/unit/auto-review-wiring.test.ts b/test/unit/auto-review-wiring.test.ts index 5a1cae88f6..39c16736bd 100644 --- a/test/unit/auto-review-wiring.test.ts +++ b/test/unit/auto-review-wiring.test.ts @@ -57,6 +57,49 @@ describe("review.auto_review wiring (#1954)", () => { ).toBeNull(); }); + it("resolvePullRequestAutoReviewSkipReason: skips docs-only PRs when configured", () => { + const manifest = parseFocusManifest({ review: { auto_review: { skip_docs_only: true } } }); + expect( + resolvePullRequestAutoReviewSkipReason({ + manifest, + isDraft: false, + author: "alice", + title: "docs: update guide", + changedPaths: ["README.md", "docs/guide.md"], + baseRef: "main", + }), + ).toBe("review skipped (docs only)"); + expect( + resolvePullRequestAutoReviewSkipReason({ + manifest, + isDraft: false, + author: "alice", + title: "docs: update guide", + changedPaths: ["README.md", "src/a.ts"], + baseRef: "main", + }), + ).toBeNull(); + expect( + resolvePullRequestAutoReviewSkipReason({ + manifest, + isDraft: false, + author: "alice", + title: "docs: update guide", + changedPaths: [], + baseRef: "main", + }), + ).toBeNull(); + expect( + resolvePullRequestAutoReviewSkipReason({ + manifest, + isDraft: false, + author: "alice", + title: "docs: update guide", + baseRef: "main", + }), + ).toBeNull(); + }); + it("resolvePullRequestAutoReviewSkipReason: matches the documented *[bot] author glob", () => { const manifest = parseFocusManifest({ review: { auto_review: { ignore_authors: ["*[bot]"] } } }); expect( @@ -163,6 +206,21 @@ describe("review.auto_review wiring (#1954)", () => { }), ).resolves.toEqual({ skipReason: "review skipped (label)", reviewManifest: labelManifest }); + const docsManifest = parseFocusManifest({ review: { auto_review: { skip_docs_only: true } } }); + loadSpy.mockResolvedValueOnce(docsManifest); + await expect( + resolveAutoReviewSkipForPullRequest({} as Env, { + authorBlacklisted: false, + isFrozenForManualReview: false, + repoFullName: "acme/widgets", + pr: { number: 8, title: "docs", baseRef: "main", isDraft: false, labels: [] }, + author: "alice", + deliveryId: "d8", + headSha: "sha8", + changedPaths: ["README.md"], + }), + ).resolves.toEqual({ skipReason: "review skipped (docs only)", reviewManifest: docsManifest }); + loadSpy.mockRejectedValueOnce(new Error("manifest unavailable")); await expect( resolveAutoReviewSkipForPullRequest({} as Env, { diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 51bf5cd76c..928daadc46 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -3090,6 +3090,7 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => { ignoreAuthors: ["*[bot]", "dependabot[bot]"], ignoreTitleKeywords: ["WIP", "draft"], skipLabels: ["do-not-review", "wip"], + skipDocsOnly: null, baseBranches: ["main", "release/**"], autoPauseAfterReviewedCommits: null, }); @@ -3121,7 +3122,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", labels: [] as string[], baseRef: "develop", reviewedCommitCount: 0 }; + const input = { isDraft: true, author: "dependabot[bot]", title: "WIP: bump deps", labels: [] as string[], changedPaths: [] 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(); @@ -3137,6 +3138,10 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => { 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, skipDocsOnly: true }, { ...input, changedPaths: ["README.md", "docs/guide.md"] })).toBe("review skipped (docs only)"); + expect(evaluateAutoReviewSkipReason({ ...empty, skipDocsOnly: true }, { ...input, changedPaths: ["README.md", "src/a.ts"] })).toBeNull(); + expect(evaluateAutoReviewSkipReason({ ...empty, skipDocsOnly: true }, { ...input, changedPaths: [] })).toBeNull(); + expect(evaluateAutoReviewSkipReason({ ...empty, skipDocsOnly: false }, { ...input, changedPaths: ["README.md"] })).toBeNull(); expect(evaluateAutoReviewSkipReason({ ...empty, baseBranches: ["main"] }, { ...input, baseRef: "develop" })).toBe( "review skipped (base branch out of scope)", ); @@ -3187,10 +3192,21 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => { 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 docsOnly = parseFocusManifest({ review: { auto_review: { skip_docs_only: true } } }); + expect(reviewConfigToJson(docsOnly.review)).toEqual({ auto_review: { skip_docs_only: true } }); const basesOnly = parseFocusManifest({ review: { auto_review: { base_branches: ["main"] } } }); expect(reviewConfigToJson(basesOnly.review)).toEqual({ auto_review: { base_branches: ["main"] } }); }); + it("warns on invalid skip_docs_only values and round-trips explicit false", () => { + const bad = parseFocusManifest({ review: { auto_review: { skip_docs_only: "yes" } } }); + expect(bad.review.autoReview.skipDocsOnly).toBeNull(); + expect(bad.warnings.some((w) => /skip_docs_only.*boolean/.test(w))).toBe(true); + const explicitOff = parseFocusManifest({ review: { auto_review: { skip_docs_only: false } } }); + expect(explicitOff.review.autoReview.skipDocsOnly).toBe(false); + expect(parseFocusManifest({ review: reviewConfigToJson(explicitOff.review) }).review.autoReview.skipDocsOnly).toBe(false); + }); + 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([]); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index fee6041cfd..688ec6740c 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -3290,6 +3290,55 @@ describe("queue processors", () => { expect(audit?.detail).toBe("review skipped (label)"); }); + it("skips AI review for docs-only PRs when review.auto_review.skip_docs_only is enabled (#2063)", 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_docs_only: true } } }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 79, + title: "docs: update readme", + state: "open", + draft: false, + user: { login: "contributor" }, + head: { sha: "a79" }, + labels: [], + body: "Closes #1", + } as never); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 79, 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/79/files")) return Response.json([ + { filename: "README.md", status: "modified", additions: 2, deletions: 0, changes: 2, patch: "@@\n+docs" }, + { filename: "docs/guide.md", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+more" }, + ]); + if (url.endsWith("/pulls/79")) return Response.json({ number: 79, title: "docs: update readme", state: "open", draft: false, user: { login: "contributor" }, head: { sha: "a79" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a79/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a79/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/79/comments")) return method === "POST" ? Response.json({ id: 79 }, { 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-docs-only", repoFullName: "JSONbored/gittensory", prNumber: 79, 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#79") + .first<{ detail: string }>(); + expect(audit?.detail).toBe("review skipped (docs only)"); + }); + 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 1420ceedeb..b3efb40960 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: [], 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 }, + 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: [], skipDocsOnly: null, 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