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
13 changes: 11 additions & 2 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ import {
type ContributorProfile,
} from "../signals/engine";
import { isDuplicateClusterWinnerByClaim } from "../signals/duplicate-winner";
import { buildUnifiedReviewDiff } from "../review/review-diff";
import { buildUnifiedReviewDiff, totalAddedLineCount } from "../review/review-diff";
import { estimateReviewEffort } from "../review/review-effort";
import { buildUnifiedCommentBody } from "../review/unified-comment-bridge";
import { randomUUID } from "node:crypto";
Expand Down Expand Up @@ -6334,6 +6334,8 @@ export async function resolveAutoReviewSkipForPullRequest(
deliveryId: string;
headSha: string | null | undefined;
changedPaths?: readonly string[] | undefined;
addedLineCount?: number | undefined;
changedFileCount?: number | undefined;
},
): Promise<{ skipReason: string | null; reviewManifest: FocusManifest | null }> {
if (args.authorBlacklisted || args.isFrozenForManualReview) {
Expand All @@ -6349,6 +6351,8 @@ export async function resolveAutoReviewSkipForPullRequest(
title: args.pr.title,
labels: args.pr.labels ?? [],
changedPaths: args.changedPaths ?? [],
addedLineCount: args.addedLineCount ?? 0,
changedFileCount: args.changedFileCount ?? 0,
baseRef: args.pr.baseRef ?? null,
reviewedCommitCount,
});
Expand Down Expand Up @@ -8143,7 +8147,10 @@ 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);
const autoReviewFiles = await getReviewFiles();
const autoReviewChangedPaths = autoReviewFiles.map((file) => file.path);
const autoReviewAddedLineCount = totalAddedLineCount(autoReviewFiles);
const autoReviewChangedFileCount = autoReviewFiles.length;
({
skipReason: autoReviewSkipReason,
reviewManifest: reviewManifestForAutoReview,
Expand All @@ -8157,6 +8164,8 @@ async function maybePublishPrPublicSurface(
deliveryId: webhook.deliveryId,
headSha: advisory.headSha ?? null,
changedPaths: autoReviewChangedPaths,
addedLineCount: autoReviewAddedLineCount,
changedFileCount: autoReviewChangedFileCount,
}));
// 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
13 changes: 13 additions & 0 deletions src/review/review-diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,19 @@ export function addedLineCount(patch: string | undefined): number {
return n;
}

/** Sum added-line counts across a PR file list — used by auto-review size-cap eligibility (#2065). */
export function totalAddedLineCount(
files: readonly { patch?: string | null | undefined; payload?: { patch?: unknown } | null | undefined }[],
): number {
let total = 0;
for (const file of files) {
const patch =
file.patch ?? (typeof file.payload?.patch === "string" ? file.payload.patch : undefined);
total += addedLineCount(patch);
}
return total;
}

/** Split a unified patch into hunks (each starting at an `@@` header); any preamble stays as hunk 0. */
function splitHunks(patch: string): string[] {
const hunks: string[] = [];
Expand Down
34 changes: 34 additions & 0 deletions src/signals/focus-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,10 @@ export type AutoReviewConfig = {
/** `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.max_added_lines`: skip AI review when total added lines exceed this cap. 0 (default) ⇒ no cap. (#2065) */
maxAddedLines: number;
/** `review.auto_review.max_files`: skip AI review when changed-file count exceeds this cap. 0 (default) ⇒ no cap. (#2065) */
maxFiles: number;
/** `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 @@ -433,6 +437,8 @@ export const EMPTY_AUTO_REVIEW_CONFIG: AutoReviewConfig = {
ignoreTitleKeywords: [],
skipLabels: [],
skipDocsOnly: null,
maxAddedLines: 0,
maxFiles: 0,
baseBranches: [],
autoPauseAfterReviewedCommits: null,
};
Expand Down Expand Up @@ -804,6 +810,16 @@ function normalizeOptionalNonNegativeInt(value: JsonValue | undefined, field: st
return value;
}

/** Parse auto-review size caps where 0 means disabled (byte-identical default). (#2065) */
function normalizeAutoReviewSizeCap(value: JsonValue | undefined, field: string, warnings: string[]): number {
if (value === undefined || value === null) return 0;
if (typeof value !== "number" || !Number.isFinite(value) || !Number.isInteger(value) || value < 0) {
warnings.push(`Manifest field "${field}" must be a non-negative integer; ignoring it.`);
return 0;
}
return value;
}

/** Normalize an optional confidence threshold in [0,1] (#7) — a fractional value (NOT a 0-100 score), so it is
* clamped into range WITHOUT rounding. Absent/null ⇒ null (the resolver leaves the gate's 0.93 default in place);
* a non-finite/non-number value is ignored with a warning. */
Expand Down Expand Up @@ -1778,6 +1794,8 @@ function autoReviewPresent(config: AutoReviewConfig): boolean {
config.ignoreTitleKeywords.length > 0 ||
config.skipLabels.length > 0 ||
config.skipDocsOnly !== null ||
config.maxAddedLines > 0 ||
config.maxFiles > 0 ||
config.baseBranches.length > 0 ||
config.autoPauseAfterReviewedCommits !== null
);
Expand All @@ -1797,6 +1815,8 @@ function parseAutoReviewConfig(value: JsonValue | undefined, warnings: string[])
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),
maxAddedLines: normalizeAutoReviewSizeCap(record.max_added_lines, "review.auto_review.max_added_lines", warnings),
maxFiles: normalizeAutoReviewSizeCap(record.max_files, "review.auto_review.max_files", warnings),
baseBranches: parseManifestGlobList(record.base_branches, "review.auto_review.base_branches", warnings),
autoPauseAfterReviewedCommits: normalizeOptionalNonNegativeInt(
record.auto_pause_after_reviewed_commits,
Expand Down Expand Up @@ -2164,6 +2184,8 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue
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.maxAddedLines > 0) autoReview.max_added_lines = review.autoReview.maxAddedLines;
if (review.autoReview.maxFiles > 0) autoReview.max_files = review.autoReview.maxFiles;
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 @@ -2240,6 +2262,8 @@ export type AutoReviewEligibilityInput = {
title: string;
labels: readonly string[];
changedPaths: readonly string[];
addedLineCount: number;
changedFileCount: number;
baseRef: string | null;
reviewedCommitCount: number;
};
Expand Down Expand Up @@ -2270,6 +2294,12 @@ export function evaluateAutoReviewSkipReason(config: AutoReviewConfig, input: Au
return "review skipped (docs only)";
}
}
if (config.maxAddedLines > 0 && input.addedLineCount > config.maxAddedLines) {
return "review skipped (too large)";
}
if (config.maxFiles > 0 && input.changedFileCount > config.maxFiles) {
return "review skipped (too large)";
}
if (config.baseBranches.length > 0) {
const baseRef = input.baseRef?.trim() ?? "";
if (!baseRef || !config.baseBranches.some((glob) => matchesManifestPath(baseRef, glob))) {
Expand All @@ -2292,6 +2322,8 @@ export function resolvePullRequestAutoReviewSkipReason(args: {
title: string;
labels?: readonly string[] | undefined;
changedPaths?: readonly string[] | undefined;
addedLineCount?: number | undefined;
changedFileCount?: number | undefined;
baseRef: string | null;
reviewedCommitCount?: number | undefined;
}): string | null {
Expand All @@ -2302,6 +2334,8 @@ export function resolvePullRequestAutoReviewSkipReason(args: {
title: args.title,
labels: args.labels ?? [],
changedPaths: args.changedPaths ?? [],
addedLineCount: args.addedLineCount ?? 0,
changedFileCount: args.changedFileCount ?? 0,
baseRef: args.baseRef,
reviewedCommitCount: args.reviewedCommitCount ?? 0,
});
Expand Down
12 changes: 12 additions & 0 deletions test/unit/auto-review-config-matrix.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ describe("review.auto_review parse ↔ reviewConfigToJson round-trip (#2071)", (
{ 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: "max_added_lines", autoReview: { max_added_lines: 500 } },
{ name: "max_files", autoReview: { max_files: 25 } },
{ 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 @@ -82,6 +84,8 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => {
title: "WIP: bump deps",
labels: [],
changedPaths: [],
addedLineCount: 0,
changedFileCount: 0,
baseRef: "develop",
reviewedCommitCount: 5,
};
Expand Down Expand Up @@ -125,6 +129,12 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => {
input: { ...allTriggers, isDraft: false, author: "alice", title: "docs: readme", changedPaths: ["README.md"] },
reason: "review skipped (docs only)",
},
{
name: "too large when added-line cap exceeded and earlier filters are off",
config: { ...allConfigured, skipDrafts: false, ignoreAuthors: [], ignoreTitleKeywords: [], skipDocsOnly: false, maxAddedLines: 10 },
input: { ...allTriggers, isDraft: false, author: "alice", title: "feat", baseRef: "main", addedLineCount: 11 },
reason: "review skipped (too large)",
},
{
name: "base branch when earlier filters are off",
config: { ...allConfigured, skipDrafts: false, ignoreAuthors: [], ignoreTitleKeywords: [] },
Expand All @@ -149,6 +159,8 @@ describe("evaluateAutoReviewSkipReason predicate precedence (#2071)", () => {
title: "feat: add widget",
labels: [],
changedPaths: [],
addedLineCount: 0,
changedFileCount: 0,
baseRef: "main",
reviewedCommitCount: 0,
},
Expand Down
76 changes: 76 additions & 0 deletions test/unit/auto-review-wiring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,65 @@ describe("review.auto_review wiring (#1954)", () => {
).toBeNull();
});

it("resolvePullRequestAutoReviewSkipReason: skips oversized PRs when size caps are configured", () => {
const linesManifest = parseFocusManifest({ review: { auto_review: { max_added_lines: 10 } } });
expect(
resolvePullRequestAutoReviewSkipReason({
manifest: linesManifest,
isDraft: false,
author: "alice",
title: "feat: big change",
addedLineCount: 11,
changedFileCount: 1,
baseRef: "main",
}),
).toBe("review skipped (too large)");
expect(
resolvePullRequestAutoReviewSkipReason({
manifest: linesManifest,
isDraft: false,
author: "alice",
title: "feat: big change",
addedLineCount: 10,
changedFileCount: 1,
baseRef: "main",
}),
).toBeNull();

const filesManifest = parseFocusManifest({ review: { auto_review: { max_files: 2 } } });
expect(
resolvePullRequestAutoReviewSkipReason({
manifest: filesManifest,
isDraft: false,
author: "alice",
title: "feat: wide change",
addedLineCount: 1,
changedFileCount: 3,
baseRef: "main",
}),
).toBe("review skipped (too large)");
expect(
resolvePullRequestAutoReviewSkipReason({
manifest: filesManifest,
isDraft: false,
author: "alice",
title: "feat: wide change",
addedLineCount: 1,
changedFileCount: 2,
baseRef: "main",
}),
).toBeNull();
expect(
resolvePullRequestAutoReviewSkipReason({
manifest: filesManifest,
isDraft: false,
author: "alice",
title: "feat: wide change",
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 @@ -221,6 +280,23 @@ describe("review.auto_review wiring (#1954)", () => {
}),
).resolves.toEqual({ skipReason: "review skipped (docs only)", reviewManifest: docsManifest });

const sizeManifest = parseFocusManifest({ review: { auto_review: { max_added_lines: 1 } } });
loadSpy.mockResolvedValueOnce(sizeManifest);
await expect(
resolveAutoReviewSkipForPullRequest({} as Env, {
authorBlacklisted: false,
isFrozenForManualReview: false,
repoFullName: "acme/widgets",
pr: { number: 9, title: "feat", baseRef: "main", isDraft: false, labels: [] },
author: "alice",
deliveryId: "d9",
headSha: "sha9",
changedPaths: ["src/a.ts"],
addedLineCount: 2,
changedFileCount: 1,
}),
).resolves.toEqual({ skipReason: "review skipped (too large)", reviewManifest: sizeManifest });

loadSpy.mockRejectedValueOnce(new Error("manifest unavailable"));
await expect(
resolveAutoReviewSkipForPullRequest({} as Env, {
Expand Down
27 changes: 26 additions & 1 deletion test/unit/focus-manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3091,6 +3091,8 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => {
ignoreTitleKeywords: ["WIP", "draft"],
skipLabels: ["do-not-review", "wip"],
skipDocsOnly: null,
maxAddedLines: 0,
maxFiles: 0,
baseBranches: ["main", "release/**"],
autoPauseAfterReviewedCommits: null,
});
Expand Down Expand Up @@ -3122,7 +3124,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[], changedPaths: [] as string[], baseRef: "develop", reviewedCommitCount: 0 };
const input = { isDraft: true, author: "dependabot[bot]", title: "WIP: bump deps", labels: [] as string[], changedPaths: [] as string[], addedLineCount: 0, changedFileCount: 0, 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 @@ -3142,6 +3144,12 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => {
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, maxAddedLines: 10 }, { ...input, addedLineCount: 11 })).toBe("review skipped (too large)");
expect(evaluateAutoReviewSkipReason({ ...empty, maxAddedLines: 10 }, { ...input, addedLineCount: 10 })).toBeNull();
expect(evaluateAutoReviewSkipReason({ ...empty, maxAddedLines: 0 }, { ...input, addedLineCount: 999 })).toBeNull();
expect(evaluateAutoReviewSkipReason({ ...empty, maxFiles: 3 }, { ...input, changedFileCount: 4 })).toBe("review skipped (too large)");
expect(evaluateAutoReviewSkipReason({ ...empty, maxFiles: 3 }, { ...input, changedFileCount: 3 })).toBeNull();
expect(evaluateAutoReviewSkipReason({ ...empty, maxFiles: 0 }, { ...input, changedFileCount: 99 })).toBeNull();
expect(evaluateAutoReviewSkipReason({ ...empty, baseBranches: ["main"] }, { ...input, baseRef: "develop" })).toBe(
"review skipped (base branch out of scope)",
);
Expand Down Expand Up @@ -3194,10 +3202,27 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => {
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 linesCap = parseFocusManifest({ review: { auto_review: { max_added_lines: 500 } } });
expect(reviewConfigToJson(linesCap.review)).toEqual({ auto_review: { max_added_lines: 500 } });
const filesCap = parseFocusManifest({ review: { auto_review: { max_files: 25 } } });
expect(reviewConfigToJson(filesCap.review)).toEqual({ auto_review: { max_files: 25 } });
const basesOnly = parseFocusManifest({ review: { auto_review: { base_branches: ["main"] } } });
expect(reviewConfigToJson(basesOnly.review)).toEqual({ auto_review: { base_branches: ["main"] } });
});

it("warns on invalid max_added_lines and max_files values", () => {
const badLines = parseFocusManifest({ review: { auto_review: { max_added_lines: -1 } } });
expect(badLines.review.autoReview.maxAddedLines).toBe(0);
expect(badLines.warnings.some((w) => /max_added_lines.*non-negative integer/.test(w))).toBe(true);
const badFiles = parseFocusManifest({ review: { auto_review: { max_files: "many" } } });
expect(badFiles.review.autoReview.maxFiles).toBe(0);
expect(badFiles.warnings.some((w) => /max_files.*non-negative integer/.test(w))).toBe(true);
const explicitZero = parseFocusManifest({ review: { auto_review: { max_added_lines: 0, max_files: 0 } } });
expect(explicitZero.review.autoReview.maxAddedLines).toBe(0);
expect(explicitZero.review.autoReview.maxFiles).toBe(0);
expect(reviewConfigToJson(explicitZero.review)).toBeNull();
});

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();
Expand Down
Loading
Loading