Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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,
});
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down
16 changes: 16 additions & 0 deletions src/signals/focus-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand All @@ -428,6 +432,7 @@ export const EMPTY_AUTO_REVIEW_CONFIG: AutoReviewConfig = {
ignoreAuthors: [],
ignoreTitleKeywords: [],
skipLabels: [],
skipDocsOnly: null,
baseBranches: [],
autoPauseAfterReviewedCommits: null,
};
Expand Down Expand Up @@ -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
);
Expand All @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -2231,6 +2239,7 @@ export type AutoReviewEligibilityInput = {
author: string | null;
title: string;
labels: readonly string[];
changedPaths: readonly string[];
baseRef: string | null;
reviewedCommitCount: number;
};
Expand All @@ -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))) {
Expand All @@ -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 {
Expand All @@ -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,
});
Expand Down
13 changes: 13 additions & 0 deletions test/unit/auto-review-config-matrix.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 } },
{
Expand Down Expand Up @@ -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"],
Expand Down Expand Up @@ -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: [] },
Expand All @@ -136,6 +147,8 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => {
isDraft: false,
author: "alice",
title: "feat: add widget",
labels: [],
changedPaths: [],
baseRef: "main",
reviewedCommitCount: 0,
},
Expand Down
58 changes: 58 additions & 0 deletions test/unit/auto-review-wiring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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, {
Expand Down
18 changes: 17 additions & 1 deletion test/unit/focus-manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand Down Expand Up @@ -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();
Expand All @@ -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)",
);
Expand Down Expand Up @@ -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([]);
Expand Down
49 changes: 49 additions & 0 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
Loading
Loading