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
5 changes: 3 additions & 2 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
pr: { isDraft?: boolean | null; title: string; baseRef?: string | null; number: number; labels?: readonly string[] };
author: string | null;
deliveryId: string;
headSha: string | null | undefined;
Expand All @@ -6346,6 +6346,7 @@ export async function resolveAutoReviewSkipForPullRequest(
isDraft: args.pr.isDraft === true,
author: args.author,
title: args.pr.title,
labels: args.pr.labels ?? [],
baseRef: args.pr.baseRef ?? null,
reviewedCommitCount,
});
Expand Down Expand Up @@ -8148,7 +8149,7 @@ async function maybePublishPrPublicSurface(
isFrozenForManualReview,
forceAiReview: webhook.forceAiReview,
repoFullName,
pr: { number: pr.number, title: pr.title, baseRef: pr.baseRef ?? null, isDraft: pr.isDraft ?? null },
pr: { number: pr.number, title: pr.title, baseRef: pr.baseRef ?? null, isDraft: pr.isDraft ?? null, labels: pr.labels },
author,
deliveryId: webhook.deliveryId,
headSha: advisory.headSha ?? null,
Expand Down
43 changes: 43 additions & 0 deletions src/signals/focus-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,8 @@ export type AutoReviewConfig = {
ignoreAuthors: string[];
/** `review.auto_review.ignore_title_keywords`: case-insensitive title substrings that skip AI review. Empty ⇒ no skip. (#2040) */
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.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 @@ -425,6 +427,7 @@ export const EMPTY_AUTO_REVIEW_CONFIG: AutoReviewConfig = {
skipDrafts: null,
ignoreAuthors: [],
ignoreTitleKeywords: [],
skipLabels: [],
baseBranches: [],
autoPauseAfterReviewedCommits: null,
};
Expand Down Expand Up @@ -1768,6 +1771,7 @@ function autoReviewPresent(config: AutoReviewConfig): boolean {
config.skipDrafts !== null ||
config.ignoreAuthors.length > 0 ||
config.ignoreTitleKeywords.length > 0 ||
config.skipLabels.length > 0 ||
config.baseBranches.length > 0 ||
config.autoPauseAfterReviewedCommits !== null
);
Expand All @@ -1785,6 +1789,7 @@ function parseAutoReviewConfig(value: JsonValue | undefined, warnings: string[])
skipDrafts: normalizeOptionalBoolean(record.skip_drafts, "review.auto_review.skip_drafts", warnings),
ignoreAuthors: parseManifestGlobList(record.ignore_authors, "review.auto_review.ignore_authors", warnings),
ignoreTitleKeywords: parseAutoReviewTitleKeywords(record.ignore_title_keywords, warnings),
skipLabels: parseAutoReviewSkipLabels(record.skip_labels, warnings),
baseBranches: parseManifestGlobList(record.base_branches, "review.auto_review.base_branches", warnings),
autoPauseAfterReviewedCommits: normalizeOptionalNonNegativeInt(
record.auto_pause_after_reviewed_commits,
Expand Down Expand Up @@ -1931,6 +1936,34 @@ function parseAutoReviewTitleKeywords(value: JsonValue | undefined, warnings: st
return out;
}

function parseAutoReviewSkipLabels(value: JsonValue | undefined, warnings: string[]): string[] {
if (value === undefined || value === null) return [];
if (!Array.isArray(value)) {
warnings.push(`Manifest "review.auto_review.skip_labels" must be a list of strings; ignoring it.`);
return [];
}
const seen = new Set<string>();
const out: string[] = [];
for (const [index, entry] of value.entries()) {
if (out.length >= MAX_PATH_INSTRUCTIONS) {
warnings.push(`Manifest "review.auto_review.skip_labels" is capped at ${MAX_PATH_INSTRUCTIONS} entries; dropping the rest.`);
break;
}
const raw = typeof entry === "string" ? entry.trim() : "";
if (!raw) {
warnings.push(`Manifest "review.auto_review.skip_labels[${index}]" must be a non-empty string; ignoring it.`);
continue;
}
const safe = parsePublicSafeText(raw, `review.auto_review.skip_labels[${index}]`, warnings);
if (safe === null) continue;
const key = safe.toLowerCase();
if (seen.has(key)) continue;
seen.add(key);
out.push(key);
}
return out;
}

/** Parse `review.pre_merge_checks` — an array of DETERMINISTIC pre-merge assertions. Each entry needs a non-empty
* public-safe `name` and at least ONE assertion (`title_contains` / `description_contains` / `require_label`,
* each public-safe); `when_paths` (optional) gates the check to PRs touching a matching glob; `enforce` (default
Expand Down Expand Up @@ -2122,6 +2155,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue
if (review.autoReview.skipDrafts !== null) autoReview.skip_drafts = review.autoReview.skipDrafts;
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.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 @@ -2196,6 +2230,7 @@ export type AutoReviewEligibilityInput = {
isDraft: boolean;
author: string | null;
title: string;
labels: readonly string[];
baseRef: string | null;
reviewedCommitCount: number;
};
Expand All @@ -2215,6 +2250,12 @@ export function evaluateAutoReviewSkipReason(config: AutoReviewConfig, input: Au
return "review skipped (WIP title)";
}
}
if (config.skipLabels.length > 0 && input.labels.length > 0) {
const prLabels = new Set(input.labels.map((label) => label.toLowerCase()));
if (config.skipLabels.some((label) => prLabels.has(label))) {
return "review skipped (label)";
}
}
if (config.baseBranches.length > 0) {
const baseRef = input.baseRef?.trim() ?? "";
if (!baseRef || !config.baseBranches.some((glob) => matchesManifestPath(baseRef, glob))) {
Expand All @@ -2235,6 +2276,7 @@ export function resolvePullRequestAutoReviewSkipReason(args: {
isDraft: boolean;
author: string | null;
title: string;
labels?: readonly string[] | undefined;
baseRef: string | null;
reviewedCommitCount?: number | undefined;
}): string | null {
Expand All @@ -2243,6 +2285,7 @@ export function resolvePullRequestAutoReviewSkipReason(args: {
isDraft: args.isDraft,
author: args.author,
title: args.title,
labels: args.labels ?? [],
baseRef: args.baseRef,
reviewedCommitCount: args.reviewedCommitCount ?? 0,
});
Expand Down
62 changes: 61 additions & 1 deletion test/unit/auto-review-wiring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,39 @@ describe("review.auto_review wiring (#1954)", () => {
).toBeNull();
});

it("resolvePullRequestAutoReviewSkipReason: skips when a configured label is present", () => {
const manifest = parseFocusManifest({ review: { auto_review: { skip_labels: ["do-not-review"] } } });
expect(
resolvePullRequestAutoReviewSkipReason({
manifest,
isDraft: false,
author: "alice",
title: "feat: thing",
labels: ["Do-Not-Review"],
baseRef: "main",
}),
).toBe("review skipped (label)");
expect(
resolvePullRequestAutoReviewSkipReason({
manifest,
isDraft: false,
author: "alice",
title: "feat: thing",
labels: ["feature"],
baseRef: "main",
}),
).toBeNull();
expect(
resolvePullRequestAutoReviewSkipReason({
manifest,
isDraft: false,
author: "alice",
title: "feat: thing",
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 @@ -108,14 +141,28 @@ describe("review.auto_review wiring (#1954)", () => {
authorBlacklisted: false,
isFrozenForManualReview: false,
repoFullName: "acme/widgets",
pr: { number: 3, title: "WIP", baseRef: "main", isDraft: true },
pr: { number: 3, title: "WIP", baseRef: "main", isDraft: true, labels: [] },
author: "alice",
deliveryId: "d3",
headSha: "sha3",
}),
).resolves.toEqual({ skipReason: "review skipped (draft)", reviewManifest: manifest });
expect(auditSpy).toHaveBeenCalled();

const labelManifest = parseFocusManifest({ review: { auto_review: { skip_labels: ["do-not-review"] } } });
loadSpy.mockResolvedValueOnce(labelManifest);
await expect(
resolveAutoReviewSkipForPullRequest({} as Env, {
authorBlacklisted: false,
isFrozenForManualReview: false,
repoFullName: "acme/widgets",
pr: { number: 6, title: "feat", baseRef: "main", isDraft: false, labels: ["Do-Not-Review"] },
author: "alice",
deliveryId: "d6",
headSha: "sha6",
}),
).resolves.toEqual({ skipReason: "review skipped (label)", reviewManifest: labelManifest });

loadSpy.mockRejectedValueOnce(new Error("manifest unavailable"));
await expect(
resolveAutoReviewSkipForPullRequest({} as Env, {
Expand All @@ -129,6 +176,19 @@ describe("review.auto_review wiring (#1954)", () => {
}),
).resolves.toEqual({ skipReason: null, reviewManifest: null });

loadSpy.mockResolvedValueOnce(labelManifest);
await expect(
resolveAutoReviewSkipForPullRequest({} as Env, {
authorBlacklisted: false,
isFrozenForManualReview: false,
repoFullName: "acme/widgets",
pr: { number: 7, title: "feat", baseRef: "main", isDraft: false },
author: "alice",
deliveryId: "d7",
headSha: "sha7",
}),
).resolves.toEqual({ skipReason: null, reviewManifest: labelManifest });

loadSpy.mockRestore();
});

Expand Down
31 changes: 30 additions & 1 deletion test/unit/focus-manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3080,6 +3080,7 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => {
skip_drafts: true,
ignore_authors: [" *[bot] ", "dependabot[bot]"],
ignore_title_keywords: [" WIP ", "draft"],
skip_labels: [" do-not-review ", "WIP"],
base_branches: ["main", "release/**"],
},
},
Expand All @@ -3088,6 +3089,7 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => {
skipDrafts: true,
ignoreAuthors: ["*[bot]", "dependabot[bot]"],
ignoreTitleKeywords: ["WIP", "draft"],
skipLabels: ["do-not-review", "wip"],
baseBranches: ["main", "release/**"],
autoPauseAfterReviewedCommits: null,
});
Expand Down Expand Up @@ -3119,7 +3121,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", baseRef: "develop", reviewedCommitCount: 0 };
const input = { isDraft: true, author: "dependabot[bot]", title: "WIP: bump deps", labels: [] 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 @@ -3130,6 +3132,11 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => {
expect(evaluateAutoReviewSkipReason({ ...empty, ignoreAuthors: ["human"] }, input)).toBeNull();
expect(evaluateAutoReviewSkipReason({ ...empty, ignoreTitleKeywords: ["wip"] }, { ...input, title: "Fix WIP regression" })).toBe("review skipped (WIP title)");
expect(evaluateAutoReviewSkipReason({ ...empty, ignoreTitleKeywords: ["wip"] }, { ...input, title: "Fix regression" })).toBeNull();
expect(evaluateAutoReviewSkipReason({ ...empty, skipLabels: ["do-not-review"] }, { ...input, labels: ["Do-Not-Review"] })).toBe("review skipped (label)");
expect(evaluateAutoReviewSkipReason({ ...empty, skipLabels: ["wip", "hold"] }, { ...input, labels: ["hold"] })).toBe("review skipped (label)");
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, baseBranches: ["main"] }, { ...input, baseRef: "develop" })).toBe(
"review skipped (base branch out of scope)",
);
Expand Down Expand Up @@ -3178,10 +3185,32 @@ describe("review.auto_review (#1954 / #2038–#2041)", () => {
expect(reviewConfigToJson(authorsOnly.review)).toEqual({ auto_review: { ignore_authors: ["*[bot]"] } });
const keywordsOnly = parseFocusManifest({ review: { auto_review: { ignore_title_keywords: ["DRAFT"] } } });
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 basesOnly = parseFocusManifest({ review: { auto_review: { base_branches: ["main"] } } });
expect(reviewConfigToJson(basesOnly.review)).toEqual({ auto_review: { base_branches: ["main"] } });
});

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([]);
expect(bad.warnings.some((w) => /skip_labels.*must be a list/.test(w))).toBe(true);
const deduped = parseFocusManifest({ review: { auto_review: { skip_labels: ["WIP", "wip", ""] } } });
expect(deduped.review.autoReview.skipLabels).toEqual(["wip"]);
expect(deduped.warnings.some((w) => /skip_labels\[2\]/.test(w))).toBe(true);
const nonString = parseFocusManifest({ review: { auto_review: { skip_labels: ["wip", 42] } } });
expect(nonString.review.autoReview.skipLabels).toEqual(["wip"]);
expect(nonString.warnings.some((w) => /skip_labels\[1\]/.test(w))).toBe(true);
const unsafe = parseFocusManifest({ review: { auto_review: { skip_labels: ["wip", "reward payout"] } } });
expect(unsafe.review.autoReview.skipLabels).toEqual(["wip"]);
expect(unsafe.warnings.some((w) => /skip_labels\[1\]/.test(w))).toBe(true);
const many = parseFocusManifest({
review: { auto_review: { skip_labels: Array.from({ length: 60 }, (_, i) => `label${i}`) } },
});
expect(many.review.autoReview.skipLabels).toHaveLength(50);
expect(many.warnings.some((w) => /skip_labels.*capped/.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([]);
Expand Down
46 changes: 46 additions & 0 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3244,6 +3244,52 @@ describe("queue processors", () => {
expect(audit?.detail).toBe("review skipped (draft)");
});

it("skips AI review when review.auto_review.skip_labels matches a PR label (#2062)", 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_labels: ["do-not-review"] } } });
await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", {
number: 78,
title: "Ready feature",
state: "open",
draft: false,
user: { login: "contributor" },
head: { sha: "a78" },
labels: [{ name: "Do-Not-Review" }],
body: "Closes #1",
} as never);
await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 78, 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/78/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]);
if (url.endsWith("/pulls/78")) return Response.json({ number: 78, title: "Ready feature", state: "open", draft: false, user: { login: "contributor" }, head: { sha: "a78" }, labels: [{ name: "Do-Not-Review" }], body: "Closes #1", mergeable_state: "clean" });
if (url.includes("/commits/a78/check-runs")) return Response.json({ total_count: 0, check_runs: [] });
if (url.includes("/commits/a78/status")) return Response.json({ state: "success", statuses: [] });
if (url.includes("/issues/78/comments")) return method === "POST" ? Response.json({ id: 78 }, { 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-label", repoFullName: "JSONbored/gittensory", prNumber: 78, 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#78")
.first<{ detail: string }>();
expect(audit?.detail).toBe("review skipped (label)");
});

it("runs AI review with cached manifest when auto_review eligibility passes (#1954)", async () => {
let aiCalls = 0;
const env = createTestEnv({
Expand Down
Loading
Loading