Commitfd87bb9unknown_key
feat(ai-review): wire triggerAiReview from stub to real Claude review
feat(ai-review): wire triggerAiReview from stub to real Claude review
Previously triggerAiReview() was a stub that did nothing — it logged
when DEBUG_AI_REVIEW=1 was set but never actually called Claude or
posted a comment. The PR open path (src/routes/pulls.tsx:317) was
firing-and-forgetting into a no-op, leaving "AI review" advertised
on the scorecard but functionally absent on every PR.
This pass connects the wires:
1. On PR open (non-draft, API key present), trigger now:
- Idempotency check: skips if a prior review summary exists
(detected via AI_REVIEW_MARKER, an HTML comment embedded in the
summary body — invisible in rendered markdown but searchable
via LIKE).
- Computes base...head merge-base diff via `git diff` against the
bare repo. Empty/unreadable diff → no-op.
- 100KB diff cap (matches reviewDiff's prompt cap; downstream
model context isn't the limiter, our prompt budget is).
- Calls existing reviewDiff() for the structured response.
- Persists one summary comment + N inline comments. Inline
comments populate filePath + lineNumber so the existing PR view
renders them anchored. authorId is the PR author (no synthetic
bot user yet — tracked alongside H2 app-bot identity work).
- Anthropic API failure → posts a single "AI review unavailable"
advisory comment instead of silent failure.
2. Never-throw guarantee. The whole function is wrapped in try/catch
with an inner try/catch around the AI call. The fire-and-forget
call site stays unchanged.
3. AI_REVIEW_MARKER exported as a public constant — future code that
surfaces "this PR has been AI-reviewed" badges can reuse it.
4. __test helpers (diffBetweenBranches, alreadyReviewed) exported
for unit tests; pure non-AI behaviour can be exercised without
mocking Anthropic.
Tests: 9 new (marker stability, isAiReviewEnabled return type, three
graceful-degrade cases for triggerAiReview, two fail-open cases for
__test internals). Total suite 1073 / 8 skip / 0 fail (was 1064 / 8 / 0).
https://claude.ai/code/session_0163vJChUuZqqtBBznUW6xBU2 files changed+325−10fd87bb9deb2d8b2e741f9a2924e8f2e2ccfd7915
2 changed files+325−10
Addedsrc/__tests__/ai-review.test.ts+127−0View fileUnifiedSplit
@@ -0,0 +1,127 @@
1/**
2 * Tests for src/lib/ai-review.ts.
3 *
4 * The Anthropic-calling path (reviewDiff) and the DB-touching paths
5 * (triggerAiReview's idempotency probe + insert) require external
6 * services we don't stand up in unit tests. We therefore cover the
7 * pure invariants:
8 *
9 * - The marker constant is stable so future searches keep working.
10 * - isAiReviewEnabled is a boolean reflecting config.anthropicApiKey.
11 * - triggerAiReview is a no-op (resolves cleanly) when the API key
12 * is absent — this is the documented graceful-degrade contract.
13 * - triggerAiReview never throws even when called with garbage.
14 * - The internal __test helpers are exported and shaped correctly.
15 */
16
17import { describe, it, expect } from "bun:test";
18import {
19 AI_REVIEW_MARKER,
20 isAiReviewEnabled,
21 triggerAiReview,
22 __test,
23} from "../lib/ai-review";
24
25describe("AI_REVIEW_MARKER", () => {
26 it("is the documented stable string", () => {
27 // Important: any change to this string is a breaking change for
28 // idempotency. New version → write a migration to back-fill old
29 // markers so older AI summaries still suppress duplicates.
30 expect(AI_REVIEW_MARKER).toBe("<!-- gluecron-ai-review:summary -->");
31 });
32
33 it("is an HTML comment so it doesn't render in markdown", () => {
34 expect(AI_REVIEW_MARKER.startsWith("<!--")).toBe(true);
35 expect(AI_REVIEW_MARKER.endsWith("-->")).toBe(true);
36 });
37});
38
39describe("isAiReviewEnabled", () => {
40 it("returns a boolean", () => {
41 const v = isAiReviewEnabled();
42 expect(typeof v).toBe("boolean");
43 });
44});
45
46describe("triggerAiReview — graceful degrade + crash-free", () => {
47 // Note: in the test sandbox ANTHROPIC_API_KEY is unset, so the
48 // function should resolve immediately without touching the DB. We
49 // also pass garbage repo names + nonexistent PR ids to confirm the
50 // overall try/catch holds.
51 it("resolves without throwing when API key is absent", async () => {
52 const before = process.env.ANTHROPIC_API_KEY;
53 delete process.env.ANTHROPIC_API_KEY;
54 try {
55 await triggerAiReview(
56 "alice",
57 "demo",
58 "00000000-0000-0000-0000-000000000000",
59 "Test PR",
60 "Body",
61 "main",
62 "feature"
63 );
64 } finally {
65 if (before) process.env.ANTHROPIC_API_KEY = before;
66 }
67 expect(true).toBe(true);
68 });
69
70 it("never throws even with invalid inputs", async () => {
71 let threw = false;
72 try {
73 await triggerAiReview(
74 "",
75 "",
76 "not-a-uuid",
77 "",
78 "",
79 "",
80 ""
81 );
82 } catch {
83 threw = true;
84 }
85 expect(threw).toBe(false);
86 });
87
88 it("survives an unknown branch combination without throwing", async () => {
89 let threw = false;
90 try {
91 await triggerAiReview(
92 "definitely-not-a-real-owner",
93 "definitely-not-a-real-repo",
94 "00000000-0000-0000-0000-000000000000",
95 "Title",
96 "Body",
97 "definitely-not-a-real-base",
98 "definitely-not-a-real-head"
99 );
100 } catch {
101 threw = true;
102 }
103 expect(threw).toBe(false);
104 });
105});
106
107describe("__test internals", () => {
108 it("exports diffBetweenBranches and alreadyReviewed", () => {
109 expect(typeof __test.diffBetweenBranches).toBe("function");
110 expect(typeof __test.alreadyReviewed).toBe("function");
111 });
112
113 it("diffBetweenBranches returns '' for a nonexistent repo", async () => {
114 const out = await __test.diffBetweenBranches(
115 "definitely-not-a-real-owner",
116 "definitely-not-a-real-repo",
117 "main",
118 "feature"
119 );
120 expect(out).toBe("");
121 });
122
123 it("alreadyReviewed returns false for an unknown PR id (fail-open)", async () => {
124 const out = await __test.alreadyReviewed("00000000-0000-0000-0000-000000000000");
125 expect(out).toBe(false);
126 });
127});
Modifiedsrc/lib/ai-review.ts+198−10View fileUnifiedSplit
@@ -6,6 +6,10 @@
66 */
77
88import Anthropic from "@anthropic-ai/sdk";
9import { eq, and, like } from "drizzle-orm";
10import { db } from "../db";
11import { pullRequests, prComments } from "../db/schema";
12import { getRepoPath } from "../git/repository";
913import { config } from "./config";
1014
1115interface ReviewComment {
@@ -20,6 +24,16 @@ interface ReviewResult {
2024 approved: boolean;
2125}
2226
27/**
28 * Marker we drop into the AI summary comment body. Used to detect a
29 * prior review and short-circuit duplicate runs (e.g. when a PR is
30 * marked draft → ready → draft → ready).
31 */
32export const AI_REVIEW_MARKER = "<!-- gluecron-ai-review:summary -->";
33
34/** Max bytes of diff we send to Claude. Matches reviewDiff's internal cap. */
35const DIFF_BYTE_CAP = 100_000;
36
2337let _client: Anthropic | null = null;
2438
2539function getClient(): Anthropic {
@@ -125,20 +139,194 @@ export function isAiReviewEnabled(): boolean {
125139}
126140
127141/**
128 * Fire-and-forget AI review trigger. Callers .catch() failures.
129 * Currently a stub that defers to reviewDiff once the diff is available.
142 * Compute the merge-base diff between two branches in a bare repo.
143 * Returns "" on any error so callers can no-op cleanly. Uses the
144 * three-dot `base...head` form so the diff is what changed on `head`
145 * relative to the common ancestor with `base` (which is the PR
146 * conventional view, not a literal range diff).
147 */
148async function diffBetweenBranches(
149 ownerName: string,
150 repoName: string,
151 baseBranch: string,
152 headBranch: string
153): Promise<string> {
154 try {
155 const cwd = getRepoPath(ownerName, repoName);
156 const proc = Bun.spawn(
157 [
158 "git",
159 "diff",
160 `${baseBranch}...${headBranch}`,
161 "--",
162 ],
163 { cwd, stdout: "pipe", stderr: "pipe" }
164 );
165 const text = await new Response(proc.stdout).text();
166 await proc.exited;
167 return text;
168 } catch {
169 return "";
170 }
171}
172
173/**
174 * Has this PR already been reviewed by the AI? Detected by an existing
175 * PR comment carrying our summary marker. Cheap LIKE query — if it
176 * fails (DB hiccup) we fall back to "not yet" and re-review, which is
177 * idempotent at worst (a duplicate summary), never destructive.
178 */
179async function alreadyReviewed(prId: string): Promise<boolean> {
180 try {
181 const [row] = await db
182 .select({ id: prComments.id })
183 .from(prComments)
184 .where(
185 and(
186 eq(prComments.pullRequestId, prId),
187 eq(prComments.isAiReview, true),
188 like(prComments.body, `%${AI_REVIEW_MARKER}%`)
189 )
190 )
191 .limit(1);
192 return !!row;
193 } catch {
194 return false;
195 }
196}
197
198/**
199 * Real AI review trigger. Replaces the previous stub. Pipeline:
200 *
201 * 1. Idempotency check — bail if a prior review summary exists.
202 * 2. Compute the base...head diff via the bare repo.
203 * 3. Call reviewDiff for a structured response (summary + per-file
204 * comments + approved boolean).
205 * 4. Persist:
206 * - one summary comment (isAiReview=true, marker embedded), and
207 * - one comment per inline finding (isAiReview=true, filePath +
208 * lineNumber populated).
209 *
210 * Always fire-and-forget at the call site (`.catch(...)`); this
211 * function still never throws so the catch is belt-and-braces. AI
212 * comments are authored by the PR author so the existing comment
213 * rendering can group them naturally — there is no synthetic bot user
214 * yet (tracked alongside H2 app-bot identity work).
130215 */
131216export async function triggerAiReview(
132217 ownerName: string,
133218 repoName: string,
134 _prId: string,
135 _title: string,
136 _body: string,
137 _baseBranch: string,
138 _headBranch: string,
219 prId: string,
220 title: string,
221 body: string,
222 baseBranch: string,
223 headBranch: string,
139224): Promise<void> {
140 if (!isAiReviewEnabled()) return;
141 if (process.env.DEBUG_AI_REVIEW === "1") {
142 console.log("[ai-review] queued", ownerName, repoName, _prId);
225 try {
226 if (!isAiReviewEnabled()) return;
227 if (await alreadyReviewed(prId)) return;
228
229 const [pr] = await db
230 .select({ id: pullRequests.id, authorId: pullRequests.authorId })
231 .from(pullRequests)
232 .where(eq(pullRequests.id, prId))
233 .limit(1);
234 if (!pr) return;
235
236 let diffText = await diffBetweenBranches(
237 ownerName,
238 repoName,
239 baseBranch,
240 headBranch
241 );
242 if (!diffText.trim()) return;
243 if (diffText.length > DIFF_BYTE_CAP) {
244 diffText = diffText.slice(0, DIFF_BYTE_CAP);
245 }
246
247 let result: ReviewResult;
248 try {
249 result = await reviewDiff(
250 `${ownerName}/${repoName}`,
251 title,
252 body || null,
253 baseBranch,
254 headBranch,
255 diffText
256 );
257 } catch (err) {
258 // Anthropic API failure — degrade to a single advisory comment so
259 // PR authors see the attempt rather than silence.
260 const reason = err instanceof Error ? err.message : "unknown error";
261 await db
262 .insert(prComments)
263 .values({
264 pullRequestId: prId,
265 authorId: pr.authorId,
266 isAiReview: true,
267 body: `${AI_REVIEW_MARKER}\n## AI review unavailable\n\nThe AI review attempt failed: ${reason}. The PR is otherwise unchanged.`,
268 })
269 .catch(() => {});
270 return;
271 }
272
273 const verdict = result.approved
274 ? "**AI review:** no blocking issues found."
275 : `**AI review:** flagged ${result.comments.length} item(s) for human attention.`;
276 const summaryBody = `${AI_REVIEW_MARKER}\n## AI Code Review\n\n${verdict}\n\n${result.summary}`;
277 await db
278 .insert(prComments)
279 .values({
280 pullRequestId: prId,
281 authorId: pr.authorId,
282 isAiReview: true,
283 body: summaryBody,
284 })
285 .catch(() => {});
286
287 for (const c of result.comments) {
288 if (!c || !c.body) continue;
289 const filePath =
290 typeof c.filePath === "string" && c.filePath ? c.filePath : null;
291 const lineNumber =
292 Number.isInteger(c.lineNumber) && (c.lineNumber as number) > 0
293 ? (c.lineNumber as number)
294 : null;
295 await db
296 .insert(prComments)
297 .values({
298 pullRequestId: prId,
299 authorId: pr.authorId,
300 isAiReview: true,
301 body: c.body,
302 filePath,
303 lineNumber,
304 })
305 .catch(() => {});
306 }
307
308 if (process.env.DEBUG_AI_REVIEW === "1") {
309 console.log(
310 "[ai-review] reviewed",
311 ownerName,
312 repoName,
313 prId,
314 `comments=${result.comments.length}`,
315 `approved=${result.approved}`
316 );
317 }
318 } catch (err) {
319 // Belt-and-braces: never escape into the request path.
320 if (process.env.DEBUG_AI_REVIEW === "1") {
321 console.error("[ai-review] crashed:", err);
322 }
143323 }
144324}
325
326/**
327 * Test-only export: the internal helpers. Not part of the public API.
328 */
329export const __test = {
330 diffBetweenBranches,
331 alreadyReviewed,
332};
145333