diff --git a/src/queue/processors.ts b/src/queue/processors.ts index b44982356c..a2fb196796 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -6343,6 +6343,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.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, @@ -6350,7 +6356,7 @@ export async function resolveAutoReviewSkipForPullRequest( author: args.author, title: args.pr.title, labels: args.pr.labels ?? [], - changedPaths: args.changedPaths ?? [], + changedPaths, addedLineCount: args.addedLineCount ?? 0, changedFileCount: args.changedFileCount ?? 0, baseRef: args.pr.baseRef ?? null, @@ -7373,9 +7379,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( @@ -7387,12 +7400,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. */ 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 0b4b757832..d443db0503 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -12,8 +12,8 @@ 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"; +import { isDocsOnlyChangedPaths } from "../review/changed-files-classify"; export type FocusManifestSource = "repo_file" | "api_record" | "none"; export type FocusManifestLinkedIssuePolicy = "required" | "preferred" | "optional"; @@ -2289,10 +2289,8 @@ 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.skipDocsOnly === true && isDocsOnlyChangedPaths(input.changedPaths)) { + return "review skipped (docs only)"; } if (config.maxAddedLines > 0 && input.addedLineCount > config.maxAddedLines) { return "review skipped (too large)"; diff --git a/test/unit/auto-review-config-matrix.test.ts b/test/unit/auto-review-config-matrix.test.ts index 5166491c88..9741c794b8 100644 --- a/test/unit/auto-review-config-matrix.test.ts +++ b/test/unit/auto-review-config-matrix.test.ts @@ -83,11 +83,11 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => { author: "dependabot[bot]", title: "WIP: bump deps", labels: [], - changedPaths: [], - addedLineCount: 0, - changedFileCount: 0, baseRef: "develop", reviewedCommitCount: 5, + changedPaths: ["docs/guide.md"], + addedLineCount: 0, + changedFileCount: 0, }; const allConfigured: AutoReviewConfig = { @@ -95,6 +95,8 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => { skipDrafts: true, ignoreAuthors: ["*[bot]"], ignoreTitleKeywords: ["wip"], + skipLabels: ["hold"], + skipDocsOnly: true, baseBranches: ["main"], autoPauseAfterReviewedCommits: 1, }; @@ -137,8 +139,8 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => { }, { name: "base branch when earlier filters are off", - config: { ...allConfigured, skipDrafts: false, ignoreAuthors: [], ignoreTitleKeywords: [] }, - input: { ...allTriggers, isDraft: false, author: "alice", title: "chore: bump" }, + config: { ...allConfigured, skipDrafts: false, ignoreAuthors: [], ignoreTitleKeywords: [], skipDocsOnly: false }, + input: { ...allTriggers, isDraft: false, author: "alice", title: "chore: bump", changedPaths: ["src/app.ts"] }, reason: "review skipped (base branch out of scope)", }, { @@ -158,11 +160,11 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => { author: "alice", title: "feat: add widget", labels: [], + baseRef: "main", + reviewedCommitCount: 0, changedPaths: [], addedLineCount: 0, changedFileCount: 0, - baseRef: "main", - reviewedCommitCount: 0, }, reason: null, }, diff --git a/test/unit/auto-review-wiring.test.ts b/test/unit/auto-review-wiring.test.ts index 621e06cde1..0912ebbc4e 100644 --- a/test/unit/auto-review-wiring.test.ts +++ b/test/unit/auto-review-wiring.test.ts @@ -280,6 +280,24 @@ describe("review.auto_review wiring (#1954)", () => { }), ).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(); + const sizeManifest = parseFocusManifest({ review: { auto_review: { max_added_lines: 1 } } }); loadSpy.mockResolvedValueOnce(sizeManifest); await expect( 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 7aab87f9e4..e9f0921ad0 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -3140,7 +3140,9 @@ 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", "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(); @@ -3252,6 +3254,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 43dc6979ba..b510c623fd 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -3312,6 +3312,8 @@ describe("queue processors", () => { 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"; @@ -3333,10 +3335,10 @@ describe("queue processors", () => { 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") + 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(audit?.detail).toBe("review skipped (docs only)"); + expect(visibilitySkip?.detail).toBe("docs_only"); }); it("skips AI review when review.auto_review.max_added_lines is exceeded (#2065)", async () => { @@ -13514,6 +13516,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", () => {