Commitc166384

fix(ai-review): consolidate 3 drifted merge-approval heuristics, fail open on provider errors

fix(ai-review): consolidate 3 drifted merge-approval heuristics, fail open on provider errors

Three separate call sites (the MCP gluecron_merge_pr tool, the PR-page
gate preview, and the web merge handler) each computed "did AI review
approve this PR" with their own ad-hoc string check -- none of which
matched what triggerAiReview actually writes ("no blocking issues
found." / "flagged N item(s) for human attention."). The MCP tool
checked for "**Approved**"/"approved: true"/"lgtm", strings the real
generator never produces, so it would have hard-blocked every MCP
merge the instant a genuine, clean AI review ran.

None of the three distinguished a review that ran and found problems
from a review that never ran at all -- "AI review skipped" (quota
exhausted) and "AI review unavailable" (provider error) both post a
summary-marker comment without "no blocking issues found", so all
three heuristics treated them identically to real blocking findings.

Reproduced live: after wiring AI review into gluecron_create_pr
(previous commit), agent-journey.ts's merge step failed with "AI
review found blocking issues" -- the actual comment read "AI review
unavailable: Your credit balance is too low to access the Anthropic
API." A billing lapse on the Anthropic side would have silently
blocked every merge platform-wide.

Adds computeAiReviewApproval() as a pure, directly-testable function
(no DB) picking the most recent summary-marked comment and failing
open on skipped/unavailable; isAiReviewApproved() is the thin DB
wrapper. All three call sites now delegate to it instead of keeping
their own copy.

ccantynz-alt committed on July 20, 2026Parent: 79a52c8

4 files changed+119−40c1663846aa3beb3d4b559cf5aa42fc58ff865a6c

4 changed files+119−40

Modifiedsrc/__tests__/ai-review.test.ts+54−0View fileUnified Split

@@ -19,6 +19,7 @@ import {

1919 AI_REVIEW_MARKER,

2020 isAiReviewEnabled,

2121 triggerAiReview,

22 computeAiReviewApproval,

2223 __test,

2324} from "../lib/ai-review";

2425

@@ -104,6 +105,59 @@ describe("triggerAiReview — graceful degrade + crash-free", () => {

104105 });

105106});

106107

108describe("computeAiReviewApproval — merge-gate decision", () => {

109 // Regression coverage for a real bug: three call sites (the MCP merge

110 // tool, the PR-page gate preview, and the web merge handler) each had

111 // their own ad-hoc heuristic for "did AI review approve this", and none

112 // of them matched the literal verdict strings triggerAiReview actually

113 // writes -- so the MCP path in particular hard-blocked every merge the

114 // instant AI review ran at all, clean or not. Reproduced live via

115 // scripts/agent-journey.ts against production.

116 const marker = AI_REVIEW_MARKER;

117

118 it("approves when there are no AI comments at all", () => {

119 expect(computeAiReviewApproval([])).toBe(true);

120 });

121

122 it("approves on the real clean-verdict string", () => {

123 const body = `${marker}\n## AI Code Review\n\n**AI review:** no blocking issues found.\n\nLooks good.`;

124 expect(computeAiReviewApproval([{ body, createdAt: new Date() }])).toBe(true);

125 });

126

127 it("blocks on the real flagged-items verdict string", () => {

128 const body = `${marker}\n## AI Code Review\n\n**AI review:** flagged 2 item(s) for human attention.\n\nSee inline comments.`;

129 expect(computeAiReviewApproval([{ body, createdAt: new Date() }])).toBe(false);

130 });

131

132 it("fails open on 'AI review unavailable' (provider error, e.g. no API credit)", () => {

133 const body = `${marker}\n## AI review unavailable\n\nThe AI review attempt failed: 400 credit balance too low. The PR is otherwise unchanged.`;

134 expect(computeAiReviewApproval([{ body, createdAt: new Date() }])).toBe(true);

135 });

136

137 it("fails open on 'AI review skipped' (quota exhausted)", () => {

138 const body = `${marker}\n## AI review skipped\n\nYour monthly AI token budget has been reached.`;

139 expect(computeAiReviewApproval([{ body, createdAt: new Date() }])).toBe(true);

140 });

141

142 it("ignores non-summary AI comments (e.g. inline findings, triage) with no marker", () => {

143 const inlineFinding = { body: "This line looks risky.", createdAt: new Date() };

144 expect(computeAiReviewApproval([inlineFinding])).toBe(true);

145 });

146

147 it("uses the most recent summary when a PR was re-reviewed after a push", () => {

148 const older = {

149 body: `${marker}\n**AI review:** flagged 1 item(s) for human attention.`,

150 createdAt: new Date("2026-01-01T00:00:00Z"),

151 };

152 const newer = {

153 body: `${marker}\n**AI review:** no blocking issues found.`,

154 createdAt: new Date("2026-01-02T00:00:00Z"),

155 };

156 expect(computeAiReviewApproval([older, newer])).toBe(true);

157 expect(computeAiReviewApproval([newer, older])).toBe(true);

158 });

159});

160

107161describe("__test internals", () => {

108162 it("exports diffBetweenBranches and alreadyReviewed", () => {

109163 expect(typeof __test.diffBetweenBranches).toBe("function");

Modifiedsrc/lib/ai-review.ts+60−0View fileUnified Split

@@ -486,6 +486,66 @@ export async function triggerAiReview(

486486 }

487487}

488488

489/**

490 * Merge-gate signal: did AI review clear this PR?

491 *

492 * The single source of truth for "no blocking issues" is the literal

493 * verdict line `triggerAiReview` writes (`"no blocking issues found"` vs.

494 * `"flagged N item(s) for human attention"`) -- three call sites

495 * (mcp-tools.ts's merge tool, pulls.tsx's PR-page gate preview, and

496 * pulls.tsx's actual merge handler) had each grown their own ad-hoc

497 * heuristic, checking for strings like "**Approved**" / "approved: true"

498 * / "lgtm" that the real generator never produces. Left alone, that meant

499 * a genuinely clean review could still hard-block a merge depending on

500 * which code path ran it.

501 *

502 * Also treats a degraded review ("AI review skipped" on quota exhaustion,

503 * "AI review unavailable" on a provider error -- e.g. the Anthropic

504 * account running out of credit) as non-blocking, same fail-open

505 * precedent already applied to the security-scan gate: a review that

506 * never ran is not the same signal as a review that ran and found real

507 * problems, and treating them identically means any Anthropic-side

508 * hiccup silently blocks every merge platform-wide.

509 *

510 * No comment at all (review disabled, or hasn't posted yet) also

511 * approves -- gating on AI review's mere presence isn't this function's

512 * job, `runAllGateChecks`'s `runAiReview`/settings check already covers

513 * "should this gate even run".

514 */

515export async function isAiReviewApproved(prId: string): Promise<boolean> {

516 const aiComments = await db

517 .select({ body: prComments.body, createdAt: prComments.createdAt })

518 .from(prComments)

519 .where(

520 and(eq(prComments.pullRequestId, prId), eq(prComments.isAiReview, true))

521 );

522 return computeAiReviewApproval(aiComments);

523}

524

525/**

526 * Pure decision logic behind `isAiReviewApproved`, split out so it's

527 * directly unit-testable without a DB connection. Takes the PR's

528 * isAiReview=true comments (any order) and picks the most recent one

529 * carrying the summary marker.

530 */

531export function computeAiReviewApproval(

532 aiComments: Array<{ body: string; createdAt: string | Date }>

533): boolean {

534 const summaryComments = aiComments

535 .filter((c) => c.body.includes(AI_REVIEW_MARKER))

536 .sort((a, b) => +new Date(b.createdAt) - +new Date(a.createdAt));

537

538 const latest = summaryComments[0];

539 if (!latest) return true;

540 if (

541 latest.body.includes("AI review skipped") ||

542 latest.body.includes("AI review unavailable")

543 ) {

544 return true;

545 }

546 return latest.body.includes("no blocking issues found");

547}

548

489549/**

490550 * Test-only export: the internal helpers. Not part of the public API.

491551 */

Modifiedsrc/lib/mcp-tools.ts+2−19View fileUnified Split

@@ -43,7 +43,7 @@ import {

4343 evaluateProtection,

4444} from "./branch-protection";

4545import { mergeWithAutoResolve } from "./merge-resolver";

46import { isAiReviewEnabled, triggerAiReview } from "./ai-review";

46import { isAiReviewEnabled, triggerAiReview, isAiReviewApproved } from "./ai-review";

4747import { triggerPrTriage } from "./pr-triage";

4848import { requiredOwnersApproved } from "./codeowners";

4949import {

@@ -1236,24 +1236,7 @@ const mergePr: McpToolHandler = {

12361236 return { merged: false, reason: "Head branch not found" };

12371237 }

12381238

1239 // AI review approval signal — same heuristic as routes/pulls.tsx.

1240 const aiComments = await db

1241 .select()

1242 .from(prComments)

1243 .where(

1244 and(

1245 eq(prComments.pullRequestId, pr.id),

1246 eq(prComments.isAiReview, true)

1247 )

1248 );

1249 const aiApproved =

1250 aiComments.length === 0 ||

1251 aiComments.some(

1252 (c) =>

1253 c.body.includes("**Approved**") ||

1254 c.body.includes("approved: true") ||

1255 c.body.toLowerCase().includes("lgtm")

1256 );

1239 const aiApproved = await isAiReviewApproved(pr.id);

12571240

12581241 const gateResult = await runAllGateChecks(

12591242 owner,

Modifiedsrc/routes/pulls.tsx+3−21View fileUnified Split

@@ -55,7 +55,7 @@ import {

5555 notifyOwnerOfPendingComment,

5656 countPendingForRepo,

5757} from "../lib/comment-moderation";

58import { isAiReviewEnabled, triggerAiReview } from "../lib/ai-review";

58import { isAiReviewEnabled, triggerAiReview, isAiReviewApproved } from "../lib/ai-review";

5959import {

6060 TRIO_COMMENT_MARKER,

6161 TRIO_SUMMARY_MARKER,

@@ -4162,10 +4162,7 @@ pulls.get("/:owner/:repo/pulls/:number", softAuth, requireRepoAccess("read"), as

41624162 try {

41634163 const headSha = await resolveRef(ownerName, repoName, pr.headBranch);

41644164 if (headSha) {

4165 const aiComments = comments.filter(({ comment }) => comment.isAiReview);

4166 const aiApproved = aiComments.length === 0 || aiComments.some(

4167 ({ comment }) => comment.body.includes("**Approved**")

4168 );

4165 const aiApproved = await isAiReviewApproved(pr.id);

41694166 const [gateResult, fetchedCiStatuses] = await Promise.all([

41704167 runAllGateChecks(

41714168 ownerName, repoName, pr.baseBranch, pr.headBranch, headSha, aiApproved

@@ -6267,22 +6264,7 @@ pulls.post(

62676264 // "**AI review:** flagged N item(s)..." → not approved

62686265 // "severity: blocking" → explicit blocking (future)

62696266 // If no AI comments exist yet, treat as approved (gate hasn't run).

6270 const aiComments = await db

6271 .select()

6272 .from(prComments)

6273 .where(

6274 and(

6275 eq(prComments.pullRequestId, pr.id),

6276 eq(prComments.isAiReview, true)

6277 )

6278 );

6279 const aiSummaryComment = aiComments.find((c) =>

6280 c.body.includes("")

6281 );

6282 const aiApproved =

6283 !aiSummaryComment ||

6284 (aiSummaryComment.body.includes("no blocking issues found") &&

6285 !aiSummaryComment.body.includes("severity: blocking"));

6267 const aiApproved = await isAiReviewApproved(pr.id);

62866268

62876269 // Run all green gate checks (GateTest + mergeability + AI review)

62886270 const gateResult = await runAllGateChecks(

62896271