Skip to content

fix(ai-review): fence untrusted PR content, stop shifting inline anchors, and stop judging a fix you cannot see #9145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 27, 2026
Merged

fix(ai-review): fence untrusted PR content, stop shifting inline anchors, and stop judging a fix you cannot see #9145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
55 changes: 54 additions & 1 deletion src/queue/processors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ import {
} from "../review/inline-comments";
import { evaluateClaCheck } from "../review/cla-check";
import { evaluatePreMergeChecks } from "../review/pre-merge-checks";
import { secretLeakFinding } from "../review/safety";
import { reviewInputHasPromptInjection, secretLeakFinding } from "../review/safety";
import { lockfileTamperRiskFinding } from "../review/lockfile-tamper";
import {
buildIssuePlanComment,
Expand Down Expand Up @@ -7497,6 +7497,46 @@ export function maybeAddRequiredAutoReviewSkipHold(
return true;
}

/**
* #9035 — a DETECTED prompt-injection attempt holds the PR for a human.
*
* Until now the defense was defang alone, which by design never touched the verdict: a caught attacker got a
* completely normal roll, and if a paraphrase slipped past the regex, nothing else stood between the attacker
* and the reviewer. That is the wrong shape for a signal this strong. Manipulation text aimed at the reviewer
* is not an ordinary code-quality observation — it is evidence of intent, and the one thing it must not buy is
* an automated decision.
*
* Held, never closed. The detector is a regex over a repository whose own subject matter is AI review, so a
* false positive on a legitimate PR discussing prompt handling is entirely possible, and a hold costs that
* contributor a wait while a close would cost them their PR. Fires only where the repo requires blocking AI
* review, matching its two sibling holds exactly.
*
* PURE (mutates the advisory it is given, like its siblings); the caller owns the detection input.
*/
export function maybeAddPromptInjectionHold(
env: Env,
args: {
settings: RepositorySettings;
advisory: Pick<Awaited<ReturnType<typeof buildPullRequestAdvisory>>, "headSha" | "findings">;
repoFullName: string;
author: string | null;
confirmedContributor: boolean;
skipAiReview?: boolean | undefined;
injectionDetected: boolean;
},
): boolean {
if (!args.injectionDetected || !shouldRequirePublicAiReviewForAdvisory(env, args)) return false;
args.advisory.findings.push({
code: "ai_review_inconclusive",
severity: "warning",
title: "Reviewer-manipulation text detected in this pull request",
detail:
"This pull request's title, description, or diff contains text addressed at an automated reviewer (for example instructions to ignore prior rules or to approve the change). That content is treated as data and was redacted before review, but the attempt itself means this pull request is held for a person rather than decided automatically.",
action: "A maintainer should review this pull request manually and confirm the content is legitimate.",
});
return true;
}

/**
* #9015 — the REPUTATION skip's fail-closed hold, the exact sibling of the contributor-controlled skip
* above. A reputation downgrade (low signal, or the submissions>=8/merged<1 burst) suppresses AI review
Expand Down Expand Up @@ -10121,6 +10161,19 @@ async function maybePublishPrPublicSurface(
skipAiReview: webhook.skipAiReview,
reputationSkipped: preComputedReputationSkip === true,
});
// #9035: a caught reviewer-manipulation attempt is evidence of intent, not a code-quality observation, and
// must not buy an automated decision. Same fail-closed shape as the two holds above.
maybeAddPromptInjectionHold(env, {
settings,
advisory,
repoFullName,
author,
confirmedContributor,
skipAiReview: webhook.skipAiReview,
// Title and body only: those are the author-controlled fields available at this point, and they are
// the ones an attacker actually writes prose into. The diff is fenced and defanged on its own path.
injectionDetected: reviewInputHasPromptInjection({ title: pr.title, body: pr.body }),
});
// #one-shot-review-cadence: only even attempts the lookup when the review would otherwise be eligible to
// run fresh this pass (mirrors how the frozen/paused branches below are similarly mutually exclusive) --
// a PR that's blacklisted/frozen/already-skipped for another reason never shows AI content at all today,
Expand Down
263 changes: 143 additions & 120 deletions src/review/inline-comments-select.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,120 +1,143 @@
/** Pure inline-comment selection with optional per-category caps (#2159). */

import { classifyFindingCategory, type FindingCategory } from "./finding-category-classify";
import { shouldShowInlineFinding } from "./finding-severity-filter";
import type { InlineFinding } from "../services/ai-review";
import type { ReviewFindingSeverity } from "../signals/focus-manifest";
import type { PullRequestFileRecord } from "../types";

export const DEFAULT_MAX_INLINE_COMMENTS = 10;

/** PURE: the set of NEW-file (RIGHT-side) line numbers a unified-diff patch makes commentable. */
export function rightSideLinesFromPatch(patch: string): Set<number> {
const lines = new Set<number>();
let right = 0;
for (const raw of patch.split("\n")) {
const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw);
if (header?.[1]) {
right = Number.parseInt(header[1], 10);
continue;
}
if (right === 0) continue;
const marker = raw[0];
if (marker === undefined || marker === "-" || marker === "\\") continue;
lines.add(right);
right += 1;
}
return lines;
}

/** Higher-priority categories survive per-category and total caps first (#2159). */
const INLINE_COMMENT_CATEGORY_PRIORITY: Record<FindingCategory, number> = {
security: 0,
correctness: 1,
performance: 2,
maintainability: 3,
tests: 4,
style: 5,
};

export function inlineFindingCategory(finding: InlineFinding): FindingCategory {
return finding.category ?? classifyFindingCategory(finding);
}

/** Lower rank sorts earlier. Blockers always beat nits; ties break on category priority. */
export function compareInlineFindingPriority(left: InlineFinding, right: InlineFinding): number {
const leftSeverity = left.severity === "blocker" ? 0 : 1;
const rightSeverity = right.severity === "blocker" ? 0 : 1;
if (leftSeverity !== rightSeverity) return leftSeverity - rightSeverity;
const leftCategory = INLINE_COMMENT_CATEGORY_PRIORITY[inlineFindingCategory(left)];
const rightCategory = INLINE_COMMENT_CATEGORY_PRIORITY[inlineFindingCategory(right)];
return leftCategory - rightCategory;
}

export type InlineCommentSelectOptions = {
suggestionsEnabled?: boolean | undefined;
categoriesEnabled?: boolean | undefined;
minFindingSeverity?: ReviewFindingSeverity | null | undefined;
/** When unset, preserve first-seen order with only the total cap (#2159 default-off). */
perCategoryCap?: number | null | undefined;
maxComments?: number | undefined;
};

type AnchoredInlineFinding = { finding: InlineFinding; index: number };

function anchorableInlineFindings(
findings: InlineFinding[],
files: Pick<PullRequestFileRecord, "path" | "payload">[],
minFindingSeverity: ReviewFindingSeverity | null | undefined,
): AnchoredInlineFinding[] {
const rightLinesByPath = new Map<string, Set<number>>();
for (const file of files) {
const patch = typeof file.payload?.patch === "string" ? file.payload.patch : "";
if (patch) rightLinesByPath.set(file.path, rightSideLinesFromPatch(patch));
}
const out: AnchoredInlineFinding[] = [];
const seen = new Set<string>();
for (let index = 0; index < findings.length; index++) {
const finding = findings[index]!;
if (!shouldShowInlineFinding(finding.severity, minFindingSeverity)) continue;
const validLines = rightLinesByPath.get(finding.path);
if (!validLines || !validLines.has(finding.line)) continue;
const key = `${finding.path}:${finding.line}`;
if (seen.has(key)) continue;
seen.add(key);
out.push({ finding, index });
}
return out;
}

/** Select anchorable inline findings, optionally applying a per-category sub-cap before the total cap. */
export function selectAnchoredInlineFindings(
findings: InlineFinding[],
files: Pick<PullRequestFileRecord, "path" | "payload">[],
options: InlineCommentSelectOptions,
): InlineFinding[] {
const anchored = anchorableInlineFindings(findings, files, options.minFindingSeverity);
const maxComments = options.maxComments ?? DEFAULT_MAX_INLINE_COMMENTS;
const perCategoryCap = options.perCategoryCap;
const ordered =
perCategoryCap == null
? anchored
: [...anchored].sort((left, right) => {
const byPriority = compareInlineFindingPriority(left.finding, right.finding);
if (byPriority !== 0) return byPriority;
return left.index - right.index;
});
const perCategoryCounts = new Map<FindingCategory, number>();
const out: InlineFinding[] = [];
for (const { finding } of ordered) {
if (out.length >= maxComments) break;
if (perCategoryCap != null) {
const category = inlineFindingCategory(finding);
const count = perCategoryCounts.get(category) ?? 0;
if (count >= perCategoryCap) continue;
perCategoryCounts.set(category, count + 1);
}
out.push(finding);
}
return out;
}
/** Pure inline-comment selection with optional per-category caps (#2159). */

import { classifyFindingCategory, type FindingCategory } from "./finding-category-classify";
import { addedLinesByPath } from "./inline-suggestion-anchor";
import { shouldShowInlineFinding } from "./finding-severity-filter";
import type { InlineFinding } from "../services/ai-review";
import type { ReviewFindingSeverity } from "../signals/focus-manifest";
import type { PullRequestFileRecord } from "../types";

export const DEFAULT_MAX_INLINE_COMMENTS = 10;

/** PURE: the set of NEW-file (RIGHT-side) line numbers a unified-diff patch makes commentable. */
export function rightSideLinesFromPatch(patch: string): Set<number> {
const lines = new Set<number>();
let right = 0;
// A patch ending in "\n" splits to a trailing empty element that is a split artifact, not a diff line. It is
// dropped here rather than inside the loop so that a remaining empty element genuinely means "a context line
// whose leading space was stripped" (#9076) and can be counted as one.
const rawLines = patch.split("\n");
if (rawLines.length > 0 && rawLines[rawLines.length - 1] === "") rawLines.pop();
for (const raw of rawLines) {
const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw);
if (header?.[1]) {
right = Number.parseInt(header[1], 10);
continue;
}
if (right === 0) continue;
const marker = raw[0];
if (marker === "-" || marker === "\\") continue;
// #9076: an EMPTY patch line (marker `undefined`) is a context line whose single space was stripped, not a
// line that does not exist. Skipping it without advancing `right` desynchronized every subsequent line
// number in the file, so findings anchored after it pointed somewhere else entirely. Git emits `" "` for a
// blank context line so real GitHub payloads should not hit this, but nothing enforced that and the
// failure was silent — counting it as the context line it is costs nothing and removes the whole class.
lines.add(right);
right += 1;
}
return lines;
}

/** Higher-priority categories survive per-category and total caps first (#2159). */
const INLINE_COMMENT_CATEGORY_PRIORITY: Record<FindingCategory, number> = {
security: 0,
correctness: 1,
performance: 2,
maintainability: 3,
tests: 4,
style: 5,
};

export function inlineFindingCategory(finding: InlineFinding): FindingCategory {
return finding.category ?? classifyFindingCategory(finding);
}

/** Lower rank sorts earlier. Blockers always beat nits; ties break on category priority. */
export function compareInlineFindingPriority(left: InlineFinding, right: InlineFinding): number {
const leftSeverity = left.severity === "blocker" ? 0 : 1;
const rightSeverity = right.severity === "blocker" ? 0 : 1;
if (leftSeverity !== rightSeverity) return leftSeverity - rightSeverity;
const leftCategory = INLINE_COMMENT_CATEGORY_PRIORITY[inlineFindingCategory(left)];
const rightCategory = INLINE_COMMENT_CATEGORY_PRIORITY[inlineFindingCategory(right)];
return leftCategory - rightCategory;
}

export type InlineCommentSelectOptions = {
suggestionsEnabled?: boolean | undefined;
categoriesEnabled?: boolean | undefined;
minFindingSeverity?: ReviewFindingSeverity | null | undefined;
/** When unset, preserve first-seen order with only the total cap (#2159 default-off). */
perCategoryCap?: number | null | undefined;
maxComments?: number | undefined;
};

type AnchoredInlineFinding = { finding: InlineFinding; index: number };

function anchorableInlineFindings(
findings: InlineFinding[],
files: Pick<PullRequestFileRecord, "path" | "payload">[],
minFindingSeverity: ReviewFindingSeverity | null | undefined,
): AnchoredInlineFinding[] {
const rightLinesByPath = new Map<string, Set<number>>();
for (const file of files) {
const patch = typeof file.payload?.patch === "string" ? file.payload.patch : "";
if (patch) rightLinesByPath.set(file.path, rightSideLinesFromPatch(patch));
}
// #9076: BLOCKERS must land on an ADDED line, not merely a commentable one. rightSideLinesFromPatch admits
// every RIGHT-side line — added AND unchanged context — while the reviewer prompt explicitly asks for "an
// ADDED (`+`) line" and warns that "a wrong line is worse than none". Set membership was the only check, and
// a context line satisfies it, so a model miscounting by one to three lines within a hunk landed on context,
// passed every check, and posted. The two properties this file conflated are not the same: "GitHub will
// accept this anchor" is about avoiding a 422, and "this anchor is CORRECT" is about not telling a
// contributor their bug is on a line they did not write.
//
// Scoped to blockers deliberately. A blocker is the finding that can cost someone their PR, so a wrong line
// there is the expensive error; a misplaced nit is noise, and holding nits to the stricter rule would drop
// legitimate ones that genuinely concern surrounding context.
const addedLines = addedLinesByPath(files);
const out: AnchoredInlineFinding[] = [];
const seen = new Set<string>();
for (let index = 0; index < findings.length; index++) {
const finding = findings[index]!;
if (!shouldShowInlineFinding(finding.severity, minFindingSeverity)) continue;
const validLines = finding.severity === "blocker" ? addedLines.get(finding.path) : rightLinesByPath.get(finding.path);
if (!validLines || !validLines.has(finding.line)) continue;
const key = `${finding.path}:${finding.line}`;
if (seen.has(key)) continue;
seen.add(key);
out.push({ finding, index });
}
return out;
}

/** Select anchorable inline findings, optionally applying a per-category sub-cap before the total cap. */
export function selectAnchoredInlineFindings(
findings: InlineFinding[],
files: Pick<PullRequestFileRecord, "path" | "payload">[],
options: InlineCommentSelectOptions,
): InlineFinding[] {
const anchored = anchorableInlineFindings(findings, files, options.minFindingSeverity);
const maxComments = options.maxComments ?? DEFAULT_MAX_INLINE_COMMENTS;
const perCategoryCap = options.perCategoryCap;
const ordered =
perCategoryCap == null
? anchored
: [...anchored].sort((left, right) => {
const byPriority = compareInlineFindingPriority(left.finding, right.finding);
if (byPriority !== 0) return byPriority;
return left.index - right.index;
});
const perCategoryCounts = new Map<FindingCategory, number>();
const out: InlineFinding[] = [];
for (const { finding } of ordered) {
if (out.length >= maxComments) break;
if (perCategoryCap != null) {
const category = inlineFindingCategory(finding);
const count = perCategoryCounts.get(category) ?? 0;
if (count >= perCategoryCap) continue;
perCategoryCounts.set(category, count + 1);
}
out.push(finding);
}
return out;
}
19 changes: 17 additions & 2 deletions src/review/prompt-injection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,24 @@ export function hasPromptInjection(text: string | null | undefined): boolean {
export function neutralizePromptInjection(text: string): { text: string; injected: boolean } {
if (!text) return { text, injected: false };
let injected = false;
const cleaned = text.replace(new RegExp(INJECTION_SOURCE, "gi"), () => {
const cleaned = text.replace(new RegExp(INJECTION_SOURCE, "gi"), (match) => {
injected = true;
return "[external-instruction-redacted]";
// #9076: LINE-COUNT PRESERVING. The `[^.]{0,N}` gaps above deliberately span newlines (see the header),
// so one match can swallow two or three diff lines -- including their leading `+`/`-`/space markers and,
// worst case, an `@@` hunk header or a `### path` file header. Replacing all of that with a single-line
// literal collapsed those newlines away.
//
// That mattered far beyond readability. The reviewer is instructed to derive an inline finding's `line` by
// counting forward from the `+` start of the nearest `@@` header -- over THIS text -- but the finding is
// then validated and posted against the ORIGINAL patch. Every anchor after a multi-line redaction was
// therefore shifted by the number of collapsed newlines, and a shifted anchor that still landed inside the
// commentable set passed validation and posted publicly on the WRONG line of a contributor's PR.
//
// Re-emitting one newline per newline consumed keeps the defanged text line-for-line congruent with the
// original, so the two coordinate systems cannot drift apart. The redaction itself is unchanged: the
// attacker's literal text still never reaches the model.
const newlines = (match.match(/\n/g) ?? []).length;
return `[external-instruction-redacted]${"\n".repeat(newlines)}`;
});
return { text: cleaned, injected };
}
Expand Down
Loading
Loading