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