From ec733033b9d6c0fd2629b3a9b61dcd578b21b911 Mon Sep 17 00:00:00 2001 From: jony376 Date: Sun, 5 Jul 2026 21:23:19 -0700 Subject: [PATCH] feat(review): skip auto-review of docs-only PRs via review.auto_review.skip_docs_only Add manifest parsing and eligibility checks so PRs whose changed files are all docs can bypass the review path when the knob is enabled. Co-authored-by: Cursor --- src/queue/processors.ts | 24 +++- src/review/changed-files-classify.ts | 7 ++ src/review/review-eligibility.ts | 37 ++++-- src/signals/focus-manifest.ts | 13 ++ test/unit/auto-review-config-matrix.test.ts | 6 + test/unit/auto-review-wiring.test.ts | 32 +++++ test/unit/changed-files-classify.test.ts | 12 +- test/unit/focus-manifest.test.ts | 21 +++- test/unit/queue.test.ts | 126 ++++++++++++++++++++ test/unit/review-eligibility.test.ts | 45 +++++++ test/unit/signals-coverage.test.ts | 2 +- 11 files changed, 307 insertions(+), 18 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 4f476fb892..f915ce7781 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; labels?: readonly string[] }; + pr: { isDraft?: boolean | null; title: string; baseRef?: string | null; number: number; labels?: readonly string[]; changedPaths?: readonly string[] }; author: string | null; deliveryId: string; headSha: string | null | undefined; @@ -6340,6 +6340,12 @@ export async function resolveAutoReviewSkipForPullRequest( } const reviewManifest = await loadRepoFocusManifest(env, args.repoFullName).catch(() => null); const reviewedCommitCount = await countPublishedAiReviewHeads(env, args.repoFullName, args.pr.number).catch(() => 0); + const autoReviewConfig = resolveReviewAutoReviewConfig(reviewManifest); + let changedPaths = args.pr.changedPaths ?? []; + if (autoReviewConfig.skipDocsOnly === true && changedPaths.length === 0) { + const storedFiles = await listPullRequestFiles(env, args.repoFullName, args.pr.number).catch(() => []); + changedPaths = storedFiles.map((file) => file.path); + } const skipReason = resolvePullRequestAutoReviewSkipReason({ forceAiReview: args.forceAiReview, manifest: reviewManifest, @@ -6349,6 +6355,7 @@ export async function resolveAutoReviewSkipForPullRequest( labels: args.pr.labels ?? [], baseRef: args.pr.baseRef ?? null, reviewedCommitCount, + changedPaths, }); if (skipReason) { await auditPullRequestAutoReviewSkip(env, { @@ -7367,9 +7374,16 @@ async function maybePublishPrPublicSurface( return undefined; const reviewManifest = await loadRepoFocusManifest(env, repoFullName).catch(() => null); const autoReviewConfig = resolveReviewAutoReviewConfig(reviewManifest); + let changedPathsForEligibility = pr.changedFiles ?? []; + if (autoReviewConfig.skipDocsOnly === true && changedPathsForEligibility.length === 0) { + const storedFiles = await listPullRequestFiles(env, repoFullName, pr.number).catch(() => []); + changedPathsForEligibility = storedFiles.map((file) => file.path); + } const reviewEligibility = decideReviewEligibility({ authorLogin: author, ignoreAuthors: autoReviewConfig.ignoreAuthors, + skipDocsOnly: autoReviewConfig.skipDocsOnly, + changedPaths: changedPathsForEligibility, }); if (!reviewEligibility.eligible) { await auditPrVisibilitySkip( @@ -7381,12 +7395,16 @@ async function maybePublishPrPublicSurface( webhook.deliveryId, ); if (gateEnabled) { + const skippedGateSummary = + reviewEligibility.skipReason === "docs_only" + ? "Review skipped: docs-only PR." + : "Review skipped: ignored author."; const gateCheckResult = await createOrUpdateSkippedGateCheckRun( env, installationId, repoFullName, advisory, - "Review skipped: ignored author.", + skippedGateSummary, mode, ); /* v8 ignore next -- permission-missing audit behavior mirrors the existing skipped-check path above. */ @@ -8149,7 +8167,7 @@ async function maybePublishPrPublicSurface( isFrozenForManualReview, forceAiReview: webhook.forceAiReview, repoFullName, - pr: { number: pr.number, title: pr.title, baseRef: pr.baseRef ?? null, isDraft: pr.isDraft ?? null, labels: pr.labels }, + pr: { number: pr.number, title: pr.title, baseRef: pr.baseRef ?? null, isDraft: pr.isDraft ?? null, labels: pr.labels, changedPaths: pr.changedFiles ?? [] }, author, deliveryId: webhook.deliveryId, headSha: advisory.headSha ?? null, diff --git a/src/review/changed-files-classify.ts b/src/review/changed-files-classify.ts index 94875d5e65..603b1347b3 100644 --- a/src/review/changed-files-classify.ts +++ b/src/review/changed-files-classify.ts @@ -30,3 +30,10 @@ export function classifyChangedFile(path: string): ReviewFileClass { if (isConfigFile(path)) return "config"; return "source"; } + +/** True when every non-empty changed path classifies as docs; empty/blank lists are fail-safe eligible. (#2063) */ +export function isDocsOnlyChangedPaths(paths: readonly string[]): boolean { + const normalized = paths.map((path) => (path ?? "").trim()).filter(Boolean); + if (normalized.length === 0) return false; + return normalized.every((path) => classifyChangedFile(path) === "docs"); +} diff --git a/src/review/review-eligibility.ts b/src/review/review-eligibility.ts index cf38e100a7..0befc6dc29 100644 --- a/src/review/review-eligibility.ts +++ b/src/review/review-eligibility.ts @@ -1,10 +1,13 @@ +import { isDocsOnlyChangedPaths } from "./changed-files-classify"; import { matchesManifestPath } from "../signals/focus-manifest"; -export type ReviewEligibilitySkipReason = "ignored_author"; +export type ReviewEligibilitySkipReason = "ignored_author" | "docs_only"; export type ReviewEligibilityInput = { authorLogin?: string | null | undefined; ignoreAuthors?: readonly string[] | null | undefined; + skipDocsOnly?: boolean | null | undefined; + changedPaths?: readonly string[] | null | undefined; }; export type ReviewEligibilityDecision = @@ -29,26 +32,36 @@ function normalizeAuthorLogin(login: string | null | undefined): string { return (login ?? "").trim(); } +export { isDocsOnlyChangedPaths } from "./changed-files-classify"; + /** * Decide whether the auto-review pipeline should spend/reply for this PR author. This is intentionally narrower * than the gate decision: ignored authors only suppress review/public output, never create a blocker. */ export function decideReviewEligibility(input: ReviewEligibilityInput): ReviewEligibilityDecision { const author = normalizeAuthorLogin(input.authorLogin); - if (!author) return REVIEW_ELIGIBLE; - - for (const pattern of input.ignoreAuthors ?? []) { - const trimmed = pattern.trim(); - if (!trimmed) continue; - if (matchesManifestPath(author, trimmed)) { - return { - eligible: false, - skipReason: "ignored_author", - matchedPattern: trimmed, - }; + if (author) { + for (const pattern of input.ignoreAuthors ?? []) { + const trimmed = pattern.trim(); + if (!trimmed) continue; + if (matchesManifestPath(author, trimmed)) { + return { + eligible: false, + skipReason: "ignored_author", + matchedPattern: trimmed, + }; + } } } + if (input.skipDocsOnly === true && isDocsOnlyChangedPaths(input.changedPaths ?? [])) { + return { + eligible: false, + skipReason: "docs_only", + matchedPattern: "docs-only", + }; + } + return REVIEW_ELIGIBLE; } diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 933016a43c..32883e1d81 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -13,6 +13,7 @@ import { REES_ANALYZER_NAME_SET, type ReesAnalyzerName } from "../review/enrichm import { hasUnsafeWildcardCount } from "./change-guardrail"; import { PUBLIC_LOCAL_PATH_INLINE } from "./redaction"; import { isSafeHttpUrl } from "../review/content-lane/safe-url"; +import { isDocsOnlyChangedPaths } from "../review/changed-files-classify"; export type FocusManifestSource = "repo_file" | "api_record" | "none"; export type FocusManifestLinkedIssuePolicy = "required" | "preferred" | "optional"; @@ -416,6 +417,8 @@ 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, docs-only PRs skip AI review. null (default) ⇒ reviewed as today. (#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 +431,7 @@ export const EMPTY_AUTO_REVIEW_CONFIG: AutoReviewConfig = { ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], + skipDocsOnly: null, baseBranches: [], autoPauseAfterReviewedCommits: null, }; @@ -1772,6 +1776,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 +1795,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 +2162,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; @@ -2233,6 +2240,7 @@ export type AutoReviewEligibilityInput = { labels: readonly string[]; baseRef: string | null; reviewedCommitCount: number; + changedPaths: readonly string[]; }; /** Evaluate `review.auto_review` eligibility. Returns a quiet skip reason string, or null when AI review should proceed. (#1954) */ @@ -2256,6 +2264,9 @@ export function evaluateAutoReviewSkipReason(config: AutoReviewConfig, input: Au return "review skipped (label)"; } } + if (config.skipDocsOnly === true && isDocsOnlyChangedPaths(input.changedPaths)) { + return "review skipped (docs only)"; + } if (config.baseBranches.length > 0) { const baseRef = input.baseRef?.trim() ?? ""; if (!baseRef || !config.baseBranches.some((glob) => matchesManifestPath(baseRef, glob))) { @@ -2279,6 +2290,7 @@ export function resolvePullRequestAutoReviewSkipReason(args: { labels?: readonly string[] | undefined; baseRef: string | null; reviewedCommitCount?: number | undefined; + changedPaths?: readonly string[] | undefined; }): string | null { if (args.forceAiReview === true) return null; return evaluateAutoReviewSkipReason(resolveAutoReviewConfig(args.manifest), { @@ -2288,6 +2300,7 @@ export function resolvePullRequestAutoReviewSkipReason(args: { labels: args.labels ?? [], baseRef: args.baseRef, reviewedCommitCount: args.reviewedCommitCount ?? 0, + changedPaths: args.changedPaths ?? [], }); } diff --git a/test/unit/auto-review-config-matrix.test.ts b/test/unit/auto-review-config-matrix.test.ts index 41865b0cdd..697e22a6a2 100644 --- a/test/unit/auto-review-config-matrix.test.ts +++ b/test/unit/auto-review-config-matrix.test.ts @@ -78,14 +78,18 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => { isDraft: true, author: "dependabot[bot]", title: "WIP: bump deps", + labels: [], baseRef: "develop", reviewedCommitCount: 5, + changedPaths: ["docs/guide.md"], }; const allConfigured: AutoReviewConfig = { skipDrafts: true, ignoreAuthors: ["*[bot]"], ignoreTitleKeywords: ["wip"], + skipLabels: ["hold"], + skipDocsOnly: true, baseBranches: ["main"], autoPauseAfterReviewedCommits: 1, }; @@ -136,8 +140,10 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => { isDraft: false, author: "alice", title: "feat: add widget", + labels: [], baseRef: "main", reviewedCommitCount: 0, + changedPaths: [], }, reason: null, }, diff --git a/test/unit/auto-review-wiring.test.ts b/test/unit/auto-review-wiring.test.ts index 5a1cae88f6..953c47ea9b 100644 --- a/test/unit/auto-review-wiring.test.ts +++ b/test/unit/auto-review-wiring.test.ts @@ -163,6 +163,38 @@ 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: 9, title: "docs", baseRef: "main", isDraft: false, labels: [], changedPaths: ["docs/guide.md"] }, + author: "alice", + deliveryId: "d9", + headSha: "sha9", + }), + ).resolves.toEqual({ skipReason: "review skipped (docs only)", reviewManifest: docsManifest }); + + const filesSpy = vi.spyOn(repositoriesModule, "listPullRequestFiles").mockResolvedValue([ + { path: "docs/guide.md" } as Awaited>[number], + ]); + loadSpy.mockResolvedValueOnce(docsManifest); + await expect( + resolveAutoReviewSkipForPullRequest({} as Env, { + authorBlacklisted: false, + isFrozenForManualReview: false, + repoFullName: "acme/widgets", + pr: { number: 10, title: "docs", baseRef: "main", isDraft: false, labels: [] }, + author: "alice", + deliveryId: "d10", + headSha: "sha10", + }), + ).resolves.toEqual({ skipReason: "review skipped (docs only)", reviewManifest: docsManifest }); + expect(filesSpy).toHaveBeenCalledWith(expect.anything(), "acme/widgets", 10); + filesSpy.mockRestore(); + loadSpy.mockRejectedValueOnce(new Error("manifest unavailable")); await expect( resolveAutoReviewSkipForPullRequest({} as Env, { diff --git a/test/unit/changed-files-classify.test.ts b/test/unit/changed-files-classify.test.ts index fedaf30607..60d175d0c0 100644 --- a/test/unit/changed-files-classify.test.ts +++ b/test/unit/changed-files-classify.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { classifyChangedFile } from "../../src/review/changed-files-classify"; +import { classifyChangedFile, isDocsOnlyChangedPaths } from "../../src/review/changed-files-classify"; import { isConfigFile, isDocsFile, isGeneratedFile, isLockfile, isMinifiedFile, isVendoredFile } from "../../src/signals/path-matchers"; import { isTestFile } from "../../src/signals/local-branch"; @@ -44,3 +44,13 @@ describe("classifyChangedFile (#2143)", () => { expect(classifyChangedFile("assets/logo.bin")).toBe("source"); }); }); + +describe("isDocsOnlyChangedPaths (#2063)", () => { + it("returns true only when every non-empty path is docs", () => { + expect(isDocsOnlyChangedPaths(["docs/guide.md", "README.md"])).toBe(true); + expect(isDocsOnlyChangedPaths(["docs/guide.md", "src/app.ts"])).toBe(false); + expect(isDocsOnlyChangedPaths([])).toBe(false); + expect(isDocsOnlyChangedPaths(["", " "])).toBe(false); + expect(isDocsOnlyChangedPaths(["README.md", ""])).toBe(true); + }); +}); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 51bf5cd76c..f8098e0ec3 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[], baseRef: "develop", reviewedCommitCount: 0, changedPaths: [] as string[] }; 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,12 @@ 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: ["docs/guide.md", "README.md"] })).toBe( + "review skipped (docs only)", + ); + expect(evaluateAutoReviewSkipReason({ ...empty, skipDocsOnly: true }, { ...input, changedPaths: ["docs/guide.md", "src/app.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)", ); @@ -3211,6 +3218,18 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => { expect(many.warnings.some((w) => /skip_labels.*capped/.test(w))).toBe(true); }); + it("parses skip_docs_only and round-trips through reviewConfigToJson (#2063)", () => { + const on = parseFocusManifest({ review: { auto_review: { skip_docs_only: true } } }); + expect(on.review.autoReview.skipDocsOnly).toBe(true); + expect(reviewConfigToJson(on.review)).toEqual({ auto_review: { skip_docs_only: true } }); + const off = parseFocusManifest({ review: { auto_review: { skip_docs_only: false } } }); + expect(off.review.autoReview.skipDocsOnly).toBe(false); + expect(parseFocusManifest({ review: reviewConfigToJson(off.review) }).review.autoReview.skipDocsOnly).toBe(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); + }); + 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 fee6041cfd..5f0f877af0 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -3290,6 +3290,57 @@ describe("queue processors", () => { expect(audit?.detail).toBe("review skipped (label)"); }); + it("skips AI review when review.auto_review.skip_docs_only matches an all-docs diff (#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 only", + 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() }); + await upsertPullRequestFile(env, { repoFullName: "JSONbored/gittensory", pullNumber: 79, path: "docs/guide.md", status: "modified", additions: 1, deletions: 0, changes: 1, payload: {} }); + await upsertPullRequestFile(env, { repoFullName: "JSONbored/gittensory", pullNumber: 79, path: "README.md", status: "modified", additions: 1, deletions: 0, changes: 1, payload: {} }); + 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: "docs/guide.md", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+docs" }, + { filename: "README.md", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+readme" }, + ]); + if (url.endsWith("/pulls/79")) return Response.json({ number: 79, title: "Docs only", 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 visibilitySkip = await env.DB.prepare("select detail from audit_events where event_type = ? and target_key = ?") + .bind("github_app.pr_visibility_skipped", "JSONbored/gittensory#79") + .first<{ detail: string }>(); + expect(visibilitySkip?.detail).toBe("docs_only"); + }); + it("runs AI review with cached manifest when auto_review eligibility passes (#1954)", async () => { let aiCalls = 0; const env = createTestEnv({ @@ -13417,6 +13468,81 @@ describe("queue processors", () => { expect(JSON.parse(visibilitySkip?.metadata_json ?? "{}")).toMatchObject({ deliveryId: "ignored-author-skip" }); }); + it("publishes a skipped review check for docs-only PRs when skip_docs_only is enabled (#2063)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "all_prs", + publicSurface: "comment_only", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + linkedIssueGateMode: "block", + }); + const calls = { skippedChecks: 0, comments: 0, minerList: 0 }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") { + calls.minerList += 1; + return Response.json([]); + } + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/commits/docsonly123/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/issues/59/comments")) { + calls.comments += 1; + return Response.json([]); + } + if (url.includes("/check-runs") && method === "POST") { + const body = JSON.parse(String(init?.body ?? "{}")) as { status?: string; conclusion?: string; output?: { title?: string; summary?: string } }; + expect(body).toMatchObject({ + status: "completed", + conclusion: "skipped", + output: { + title: "Gittensory Orb Review Agent skipped", + summary: "Review skipped: docs-only PR.", + }, + }); + calls.skippedChecks += 1; + return Response.json({ id: 932 }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { + gate: { linkedIssue: "block" }, + review: { auto_review: { skip_docs_only: true } }, + }); + await upsertPullRequestFile(env, { repoFullName: "JSONbored/gittensory", pullNumber: 59, path: "docs/guide.md", status: "modified", additions: 1, deletions: 0, changes: 1, payload: {} }); + await processJob(env, { + type: "github-webhook", + deliveryId: "docs-only-skip", + eventName: "pull_request", + 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" } }, + pull_request: { number: 59, title: "Docs update", state: "open", user: { login: "alice" }, head: { sha: "docsonly123" }, labels: [], body: "No issue link." }, + }, + }); + + expect(calls).toEqual({ skippedChecks: 1, comments: 0, minerList: 0 }); + const visibilitySkip = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ? and target_key = ?") + .bind("github_app.pr_visibility_skipped", "JSONbored/gittensory#59") + .first<{ detail: string; metadata_json: string }>(); + expect(visibilitySkip?.detail).toBe("docs_only"); + expect(JSON.parse(visibilitySkip?.metadata_json ?? "{}")).toMatchObject({ deliveryId: "docs-only-skip" }); + }); + it("audits ignored authors without a skipped check when review checks are disabled", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await persistRegistrySnapshot( diff --git a/test/unit/review-eligibility.test.ts b/test/unit/review-eligibility.test.ts index 0627d25437..c162448132 100644 --- a/test/unit/review-eligibility.test.ts +++ b/test/unit/review-eligibility.test.ts @@ -104,6 +104,51 @@ describe("decideReviewEligibility", () => { matchedPattern: null, }); }); + + it("skips docs-only PRs when skip_docs_only is enabled (#2063)", () => { + expect( + decideReviewEligibility({ + authorLogin: "alice", + skipDocsOnly: true, + changedPaths: ["docs/guide.md", "README.md"], + }), + ).toEqual({ + eligible: false, + skipReason: "docs_only", + matchedPattern: "docs-only", + }); + expect( + decideReviewEligibility({ + authorLogin: "alice", + skipDocsOnly: true, + changedPaths: ["docs/guide.md", "src/app.ts"], + }), + ).toEqual({ + eligible: true, + skipReason: null, + matchedPattern: null, + }); + expect(decideReviewEligibility({ skipDocsOnly: true, changedPaths: [] })).toEqual({ + eligible: true, + skipReason: null, + matchedPattern: null, + }); + expect(decideReviewEligibility({ skipDocsOnly: true, changedPaths: null })).toEqual({ + eligible: true, + skipReason: null, + matchedPattern: null, + }); + expect(decideReviewEligibility({ skipDocsOnly: false, changedPaths: ["README.md"] })).toEqual({ + eligible: true, + skipReason: null, + matchedPattern: null, + }); + expect(decideReviewEligibility({ skipDocsOnly: null, changedPaths: ["README.md"] })).toEqual({ + eligible: true, + skipReason: null, + matchedPattern: null, + }); + }); }); describe("review eligibility glob matrix", () => { 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