From 0fd032cbc725b7e8d3bb56102b861521ce22c232 Mon Sep 17 00:00:00 2001 From: Tomasz Sapeta Date: Tue, 20 Oct 2026 22:29:18 +0200 Subject: [PATCH 1/6] feat: post a run's new inline comments in one review Each new inline comment went out as its own `POST /pulls/{n}/comments`, and GitHub makes every such comment its own review. A run with three findings put three "reviewed" entries and three notifications on the PR. The inline sync now posts all of a run's new threads in one `POST /pulls/{n}/reviews` with `event: "COMMENT"`, the reviewed head as `commit_id`, and a short fixed body, which GitHub requires for a COMMENT review. The response doesn't list the comments, so the sync reads the review's comments and maps each one to its finding by the inline marker. Patches, stubs and deletes stay per comment and create no review. GitHub rejects the whole review when it refuses one comment, so any failure other than a rate limit falls back to one comment per thread. A review that was created but returned no usable id is not posted again; its findings render in full in the main comment until the next run links them. --- llp/0008-sources-and-reporters.explainer.md | 12 +- src/__tests__/inline-review.test.ts | 198 ++++++++++++++++++++ src/reporters/github.ts | 162 ++++++++++++++-- 3 files changed, 359 insertions(+), 13 deletions(-) create mode 100644 src/__tests__/inline-review.test.ts diff --git a/llp/0008-sources-and-reporters.explainer.md b/llp/0008-sources-and-reporters.explainer.md index fa4b559..d498f12 100644 --- a/llp/0008-sources-and-reporters.explainer.md +++ b/llp/0008-sources-and-reporters.explainer.md @@ -57,7 +57,7 @@ The base OID is fetchable by SHA even though the base ref may have moved since t ## GitHub Reporter Identity -The reporter maintains exactly one PR comment and is comment-only — it never touches the review-thread / GitHub "reviews" APIs, by explicit design [observed] ([github.ts:77-82](../src/reporters/github.ts)). The hard problem is: which comment on the PR is *ours*? +The reporter maintains exactly one PR comment, plus optional inline threads (see [Inline Threads](#inline-threads)), and it never approves or requests changes: the only review it creates is a `COMMENT` review that carries a run's new inline threads [observed] ([github.ts](../src/reporters/github.ts) `GitHubReporter` doc comment). The hard problem is: which comment on the PR is *ours*? **Author + marker, never marker alone.** The comment marker is a hardcoded public literal, readable in the base-branch config, so anyone who can comment on the PR (including the untrusted PR author) could post a comment carrying the marker plus a forged embedded review state. A newest-marker-wins lookup would adopt that state and carry its `dismissed` list forward, silently suppressing real findings. So a comment counts as ours only if it carries the marker AND was authored by `ownLogin` — GitHub sets a comment's author from the authenticated identity and it cannot be spoofed [observed] ([github.ts:51-75](../src/reporters/github.ts); commit 93aaf30: "identify our own comment by author + marker, not the public marker alone, so a forged comment can't carry review state forward"; regression tests `src/__tests__/github-reporter.test.ts:25-34`). Any rewrite that reintroduces marker-only matching reopens this forged-dismissal-state attack. @@ -79,6 +79,16 @@ The reporter maintains exactly one PR comment and is comment-only — it never t **Links fail soft.** `linkContextAsync` can be prebuilt and injected once so a CI fan-out builds one diff-lines + base-SHA context for all scopes instead of one pair of `gh` calls per scope — the reporter-side mirror of `memoizeSource` [observed] ([github.ts:33-37, 200-205](../src/reporters/github.ts)). Both underlying fetches fail soft: a missing diff index degrades in-diff findings to plain-text `file:line`, a missing base SHA degrades out-of-diff findings the same way — never a dead link, never a hard failure of the whole report [observed] ([github.ts:194-235](../src/reporters/github.ts); commits f9fecd5, 9446b42). This follows the repo rule that `ecr ci` must never fail a PR's checks on reviewer error [observed] (AGENTS.md: "`ecr ci` must never fail a PR's checks — reviewer errors degrade gracefully"). +## Inline Threads + +With `inline.enabled`, `syncInline` converges one inline thread per in-diff finding: `planInlineSync` decides which threads to create, patch, stub or delete, and the executor runs the plan [observed] ([github.ts](../src/reporters/github.ts) `planInlineSync`, `syncInline`). + +**One review per run for new threads.** GitHub makes every `POST /pulls/{n}/comments` its own review, so posting threads one by one put one "reviewed" entry and one notification per finding on the PR. The executor instead posts all of a run's new threads in one `POST /pulls/{n}/reviews` with `event: "COMMENT"`, the reviewed head as `commit_id`, and a short fixed body, which GitHub requires for a `COMMENT` review [observed] ([github.ts](../src/reporters/github.ts) `createInlineReview`, `INLINE_REVIEW_BODY`). Patches, stubs and deletes stay per comment: they edit existing threads and create no review. + +**The batch is all or nothing.** GitHub rejects the whole review when it refuses one comment, for example a line it will not anchor. Any failure other than a rate limit therefore falls back to one `POST /pulls/{n}/comments` per thread, so only the refused comment is lost; a rate limit aborts the rest of the sync as before [observed] (`syncInline`; tests `src/__tests__/inline-review.test.ts`). + +**Links come from a second call.** The review response does not list its comments, so the executor reads `GET /pulls/{n}/reviews/{id}/comments` and maps each comment to its finding by the anchored marker in its body (`inlineUrlsByFp`). When that read fails, the threads exist but the main comment renders those findings in full for this run; the next run finds the threads through `ownInlineComments` and links them [observed] (`syncInline`). + ## Terminal Reporter The terminal reporter writes the primary report to **stdout** (progress and logging go to stderr) specifically so `ecr review > out.txt` captures a clean report and shell redirection works as expected [observed] ([terminal.ts:45-47](../src/reporters/terminal.ts)). It maps `review.decision` to a process exit code via `decisionExitCode`, which is what makes it usable as a pre-push/pre-commit gate; `--json` disables color and `noFail` forces exit 0 [observed] ([terminal.ts:28-32, 38, 49](../src/reporters/terminal.ts)). Color is enabled only on a TTY and only when not `--json` [observed] ([terminal.ts:38](../src/reporters/terminal.ts): `!options.json && Boolean(process.stdout.isTTY)`) — naively coloring whenever output is not JSON, ignoring `isTTY`, would corrupt piped or redirected output with ANSI codes. diff --git a/src/__tests__/inline-review.test.ts b/src/__tests__/inline-review.test.ts new file mode 100644 index 0000000..7a86a7d --- /dev/null +++ b/src/__tests__/inline-review.test.ts @@ -0,0 +1,198 @@ +import { test, expect, afterEach, beforeEach } from "bun:test"; +import { chmod, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path, { delimiter } from "node:path"; + +import { resetTrustedToolCache } from "../core/exec.js"; +import { renderInlineCommentBody } from "../core/render.js"; +import type { LinkContext } from "../core/render.js"; +import type { Finding } from "../core/schema.js"; +import { GitHubReporter, INLINE_REVIEW_BODY, inlineUrlsByFp } from "../reporters/github.js"; + +const TAG = "expo-ai-code-reviewer"; +const HEAD = "0123456789abcdef0123456789abcdef01234567"; + +const finding = (over: Partial = {}): Finding => ({ + severity: "warning", + category: "quality", + file: "a.ts", + line: 5, + title: "Observer restarts for cached instances", + rationale: "getInstances() restarts the push token observer for cached instances.", + ...over, +}); + +test("inlineUrlsByFp: maps our marked comments to their permalinks", () => { + const ours = renderInlineCommentBody(finding(), TAG, "aaaa11"); + const urls = inlineUrlsByFp( + [ + { id: 1, body: ours, html_url: "https://github.com/o/r/pull/7#discussion_r1" }, + { id: 2, body: renderInlineCommentBody(finding(), "other-tag", "bbbb22"), html_url: "u2" }, + { id: 3, body: "a human reply", html_url: "u3" }, + { id: 4, body: renderInlineCommentBody(finding(), TAG, "cccc33") }, + ], + TAG, + ); + expect([...urls]).toEqual([["aaaa11", "https://github.com/o/r/pull/7#discussion_r1"]]); +}); + +// ---- the sync posts new threads through a fake `gh` on PATH ---- + +// A fake `gh` that logs each call and answers the endpoints the inline sync uses. +// FAKE_GH_REVIEW_ERROR makes the review POST fail with that stderr, and +// FAKE_GH_REVIEW_NO_ID makes it succeed with a response that has no id. +const FAKE_GH = `#!/bin/sh +printf '%s\\n' "$*" >> "$FAKE_GH_DIR/calls.log" +eval "last=\\\${$#}" +case "$*" in + "pr view"*) printf '%s' "$FAKE_GH_HEAD" ;; + *"/reviews/99/comments"*) cat "$FAKE_GH_DIR/review-comments.json" ;; + *"-X POST "*"/reviews --input "*) + cp "$last" "$FAKE_GH_DIR/review.json" + if [ -n "$FAKE_GH_REVIEW_ERROR" ]; then echo "$FAKE_GH_REVIEW_ERROR" >&2; exit 1; fi + if [ -n "$FAKE_GH_REVIEW_NO_ID" ]; then printf '{}'; exit 0; fi + printf '{"id": 99}' ;; + *"-X POST "*"/comments --input "*) + n=$(wc -l < "$FAKE_GH_DIR/calls.log" | tr -d ' ') + printf '{"html_url": "https://github.com/o/r/pull/7#discussion_r%s"}' "$n" ;; + *"-X GET "*"/comments"*) printf '[]' ;; + *) echo "unexpected gh call: $*" >&2; exit 1 ;; +esac +`; + +let dir = ""; +const saved: Record = {}; + +beforeEach(async () => { + dir = await mkdtemp(path.join(tmpdir(), "ecr-inline-review-")); + await writeFile(path.join(dir, "gh"), FAKE_GH, "utf8"); + await chmod(path.join(dir, "gh"), 0o755); + await writeFile(path.join(dir, "calls.log"), "", "utf8"); + for (const name of [ + "PATH", + "FAKE_GH_DIR", + "FAKE_GH_HEAD", + "FAKE_GH_REVIEW_ERROR", + "FAKE_GH_REVIEW_NO_ID", + "GITHUB_STEP_SUMMARY", + ]) { + saved[name] = process.env[name]; + } + process.env.PATH = `${dir}${delimiter}${saved.PATH ?? ""}`; + process.env.FAKE_GH_DIR = dir; + process.env.FAKE_GH_HEAD = HEAD; + delete process.env.FAKE_GH_REVIEW_ERROR; + delete process.env.FAKE_GH_REVIEW_NO_ID; + delete process.env.GITHUB_STEP_SUMMARY; + resetTrustedToolCache(); +}); + +afterEach(async () => { + for (const [name, value] of Object.entries(saved)) { + if (value === undefined) { + delete process.env[name]; + } else { + process.env[name] = value; + } + } + resetTrustedToolCache(); + await rm(dir, { recursive: true, force: true }); +}); + +async function syncNewThreads(prNumber: number): Promise<{ + urls: Map; + calls: string[]; +}> { + const targets = [ + { fp: "aaaa11", finding: finding() }, + { fp: "bbbb22", finding: finding({ file: "b.ts", line: 9, severity: "suggestion" }) }, + ]; + await writeFile( + path.join(dir, "review-comments.json"), + JSON.stringify( + targets.map((target, index) => ({ + id: 500 + index, + body: renderInlineCommentBody(target.finding, TAG, target.fp), + html_url: `https://github.com/o/r/pull/${prNumber}#discussion_r${500 + index}`, + })), + ), + "utf8", + ); + const reporter = new GitHubReporter({ + prNumber, + repo: "o/r", + commentTag: TAG, + ownLogin: "ecr-bot", + headSha: HEAD, + inline: { enabled: true, maxComments: 20 }, + }); + const link: LinkContext = { + repo: "o/r", + prNumber, + diffLines: new Map([ + ["a.ts", new Set([5])], + ["b.ts", new Set([9])], + ]), + }; + // syncInline is private; the test drives it directly so the payload GitHub would + // receive is checked without stubbing the rest of report(). + const sync = ( + reporter as unknown as { + syncInline: ( + targets: Array<{ finding: Finding; fp: string }>, + link: LinkContext, + opts: { teardown: boolean }, + ) => Promise>; + } + ).syncInline.bind(reporter); + const urls = await sync(targets, link, { teardown: true }); + const calls = (await readFile(path.join(dir, "calls.log"), "utf8")).trim().split("\n"); + return { urls, calls }; +} + +test("syncInline: posts all new threads in one COMMENT review and links each one", async () => { + const { urls, calls } = await syncNewThreads(701); + expect(calls.filter((call) => call.includes("-X POST"))).toEqual([ + expect.stringContaining("repos/o/r/pulls/701/reviews --input"), + ]); + const review = JSON.parse(await readFile(path.join(dir, "review.json"), "utf8")); + expect(review).toMatchObject({ commit_id: HEAD, event: "COMMENT", body: INLINE_REVIEW_BODY }); + expect( + review.comments.map((c: { path: string; line: number; side: string }) => [ + c.path, + c.line, + c.side, + ]), + ).toEqual([ + ["a.ts", 5, "RIGHT"], + ["b.ts", 9, "RIGHT"], + ]); + expect(urls.get("aaaa11")).toBe("https://github.com/o/r/pull/701#discussion_r500"); + expect(urls.get("bbbb22")).toBe("https://github.com/o/r/pull/701#discussion_r501"); +}); + +test("syncInline: a rejected review falls back to one comment per thread", async () => { + process.env.FAKE_GH_REVIEW_ERROR = "gh: Unprocessable Entity (HTTP 422)"; + const { urls, calls } = await syncNewThreads(702); + const posts = calls.filter((call) => call.includes("-X POST")); + expect(posts).toHaveLength(3); + expect(posts[0]).toContain("repos/o/r/pulls/702/reviews --input"); + expect( + posts.slice(1).every((call) => call.includes("repos/o/r/pulls/702/comments --input")), + ).toBe(true); + expect([...urls.keys()].sort()).toEqual(["aaaa11", "bbbb22"]); +}); + +test("syncInline: a rate-limited review posts nothing else", async () => { + process.env.FAKE_GH_REVIEW_ERROR = "gh: You have exceeded a secondary rate limit (HTTP 403)"; + const { urls, calls } = await syncNewThreads(703); + expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1); + expect(urls.size).toBe(0); +}); + +test("syncInline: a created review without an id is never posted again", async () => { + process.env.FAKE_GH_REVIEW_NO_ID = "1"; + const { urls, calls } = await syncNewThreads(704); + expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1); + expect(urls.size).toBe(0); +}); diff --git a/src/reporters/github.ts b/src/reporters/github.ts index 055b5b1..8faed78 100644 --- a/src/reporters/github.ts +++ b/src/reporters/github.ts @@ -197,6 +197,25 @@ export interface ExistingInlineComment { hasReplies: boolean; } +/** Body of the review that carries one run's new inline comments. GitHub requires one. */ +export const INLINE_REVIEW_BODY = + "🤖 AI code review: inline comments for this run's findings. The AI code review comment has the summary."; + +/** + * fp → permalink for the inline comments of ours among `comments`, read from the + * anchored marker in each body (see parseInlineMarkerFp). + */ +export function inlineUrlsByFp(comments: ReviewCommentApi[], tag: string): Map { + const urls = new Map(); + for (const comment of comments) { + const fp = parseInlineMarkerFp(comment.body ?? "", tag); + if (fp && comment.html_url) { + urls.set(fp, comment.html_url); + } + } + return urls; +} + /** What one inline sync run will do. Bodies are rendered by the executor. */ export interface InlineSyncPlan { /** Findings to POST a new inline comment for (needs a commit id). */ @@ -553,7 +572,8 @@ export function selectOwnComments( /** * Maintains exactly one PR comment, updating it in place across re-reviews (and * cleaning up duplicates) so the review converges instead of churning. Runs the - * break-glass check first. Comment-only: never calls the review-state APIs. + * break-glass check first. Never approves or requests changes: a run's new inline + * comments go out in one COMMENT review. */ export class GitHubReporter implements Reporter { private readonly marker: string; @@ -1361,18 +1381,68 @@ export class GitHubReporter implements Reporter { counts.patched++; }); } - for (const entry of creates) { - await attempt(async () => { - const created = await this.createReviewComment( - renderInlineCommentBody(entry.finding, this.options.commentTag, entry.fp), - entry.finding.file, - entry.finding.line!, - ); - counts.created++; - if (created?.html_url) { - urls.set(entry.fp, created.html_url); + if (creates.length > 0 && !aborted) { + const comments = creates.map((entry) => ({ + fp: entry.fp, + body: renderInlineCommentBody(entry.finding, this.options.commentTag, entry.fp), + path: entry.finding.file, + line: entry.finding.line!, + })); + // @ref LLP 0008#inline-threads [implements] — one COMMENT review per run for new threads; per-comment fallback when GitHub rejects the review + let posted = false; + let reviewId: number | null = null; + try { + reviewId = await this.createInlineReview(comments); + posted = true; + } catch (error) { + if (GitHubReporter.isRateLimitError(error)) { + counts.failed += comments.length; + aborted = true; + process.stderr.write( + `inline sync: rate-limited; aborting remaining inline mutations this run: ${errorMessage(error)}\n`, + ); + } else { + // GitHub rejects the whole review when it refuses one comment, so retry one + // by one: only the comment it refuses is lost. + process.stderr.write( + `inline sync: the review with ${comments.length} new comment(s) failed; posting them one by one: ${errorMessage(error)}\n`, + ); } - }); + } + if (posted) { + counts.created += comments.length; + try { + if (reviewId == null) { + throw new Error("the created review has no id"); + } + for (const [fp, url] of inlineUrlsByFp( + await this.listInlineReviewComments(reviewId), + this.options.commentTag, + )) { + urls.set(fp, url); + } + } catch (error) { + // The comments exist; without their links the findings render full-form in + // the main comment this run, and the next run finds them as live threads. + process.stderr.write( + `inline sync: could not list the new review's comments (they render in the main comment): ${errorMessage(error)}\n`, + ); + } + } else { + for (const comment of comments) { + await attempt(async () => { + const created = await this.createReviewComment( + comment.body, + comment.path, + comment.line, + ); + counts.created++; + if (created?.html_url) { + urls.set(comment.fp, created.html_url); + } + }); + } + } } for (const comment of plan.stub) { const body = inlineStubBody(this.options.commentTag, comment.fp); @@ -1404,6 +1474,74 @@ export class GitHubReporter implements Reporter { return urls; } + /** + * Post `comments` as one COMMENT review anchored to the reviewed head and return + * the review id, or null when the review was created but its id can't be read. + * Throws only when GitHub rejects the review; it rejects all of the comments + * together. + */ + private async createInlineReview( + comments: Array<{ body: string; path: string; line: number }>, + ): Promise { + this.invalidateReviewComments(); + const gh = await resolveTrustedTool("gh"); + const payload = JSON.stringify({ + commit_id: this.options.headSha, + event: "COMMENT", + body: INLINE_REVIEW_BODY, + comments: comments.map(({ body, path: path_, line }) => ({ + body, + path: path_, + line, + side: "RIGHT", + })), + }); + const dir = await mkdtemp(path.join(tmpdir(), "ecr-")); + const jsonPath = path.join(dir, "review.json"); + try { + await writeFile(jsonPath, payload, "utf8"); + const { stdout } = await run( + gh, + [ + "api", + "-X", + "POST", + `repos/${this.options.repo}/pulls/${this.options.prNumber}/reviews`, + "--input", + jsonPath, + ], + { cwd: this.options.cwd }, + ); + try { + const review = JSON.parse(stdout) as { id?: unknown }; + return typeof review.id === "number" ? review.id : null; + } catch { + return null; + } + } finally { + await rm(dir, { recursive: true, force: true }); + } + } + + /** The comments of one review. A review holds at most `inline.maxComments` of ours. */ + private async listInlineReviewComments(reviewId: number): Promise { + const gh = await resolveTrustedTool("gh"); + const { stdout } = await run( + gh, + [ + "api", + "-X", + "GET", + `repos/${this.options.repo}/pulls/${this.options.prNumber}/reviews/${reviewId}/comments`, + "-f", + "per_page=100", + ], + { cwd: this.options.cwd }, + ); + const comments = JSON.parse(stdout) as ReviewCommentApi[]; + return Array.isArray(comments) ? comments : []; + } + private async createReviewComment( body: string, path_: string, From e9ad5347398d860cfdced5bddf9fbf41d9da4da4 Mon Sep 17 00:00:00 2001 From: Tomasz Sapeta Date: Tue, 20 Oct 2026 22:45:00 +0200 Subject: [PATCH 2/6] docs: describe the single review for new inline comments in the README --- README.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index fe46d73..1980eed 100644 --- a/README.md +++ b/README.md @@ -732,8 +732,11 @@ Behavior notes: - **Replies on inline threads count as author feedback.** Replying in a thread matches the reply to that finding structurally; clearing still requires the `id:…` token in the replier's own words, same as main-comment replies. -- **Notification fan-out.** The first enabled run on a PR sends one notification - per created inline comment; `maxComments` bounds it. +- **One review per run.** A run's new inline comments go out in one `COMMENT` + review, so the PR gets one review entry and one notification per run, not one + per comment. The reviewer never approves or requests changes. If GitHub + rejects the review, for example for a line it won't anchor, the comments are + posted one by one instead, each as its own review. - **Disabling later** leaves existing threads up (the sync no longer runs); a comment-mode switch — or the reporter's clear path — stubs/removes them. From b8746cb4c198307f91be9338842b7cac1ee707f8 Mon Sep 17 00:00:00 2001 From: Tomasz Sapeta Date: Tue, 20 Oct 2026 22:45:21 +0200 Subject: [PATCH 3/6] docs: describe the single review for new inline comments in the config template --- templates/config.jsonc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/templates/config.jsonc b/templates/config.jsonc index 5d3f5a0..52be6b0 100644 --- a/templates/config.jsonc +++ b/templates/config.jsonc @@ -170,8 +170,8 @@ // Threads update in place across re-reviews; a retired thread with human // replies is stubbed, never deleted. Replies on inline threads are matched to // their finding like main-comment replies (same clearing rules). - // NOTE: the first enabled run on a PR sends one notification per created - // inline comment — `maxComments` bounds that. Disabling later leaves existing + // A run's new inline comments go out in one COMMENT review, so the PR gets one + // review entry and one notification per run. Disabling later leaves existing // threads up until a comment-mode switch (or manual cleanup) clears them. // "inline": { // "enabled": false, From e443e50565b9db2e04317f7d661cdafe590d47d5 Mon Sep 17 00:00:00 2001 From: Tomasz Sapeta Date: Tue, 20 Oct 2026 22:52:07 +0200 Subject: [PATCH 4/6] fix: retry inline comments one by one only after a 422, and read every page of the review A review POST that failed with a 5xx or a dropped connection fell back to posting each comment, but GitHub can have created the review anyway, so the fallback could post every thread twice. Only a 422, which means GitHub created nothing, now falls back. Any other failure counts the threads as failed and does not retry; the main comment renders those findings in full, and the next run creates the threads that are still missing. `inline.maxComments` has no upper bound, so the read of the new review's comments now goes through every page, like the other review-comment reads. --- llp/0008-sources-and-reporters.explainer.md | 4 +- src/__tests__/inline-review.test.ts | 95 +++++++++++++++------ src/reporters/github.ts | 68 ++++++++++----- 3 files changed, 117 insertions(+), 50 deletions(-) diff --git a/llp/0008-sources-and-reporters.explainer.md b/llp/0008-sources-and-reporters.explainer.md index d498f12..6be4094 100644 --- a/llp/0008-sources-and-reporters.explainer.md +++ b/llp/0008-sources-and-reporters.explainer.md @@ -85,9 +85,9 @@ With `inline.enabled`, `syncInline` converges one inline thread per in-diff find **One review per run for new threads.** GitHub makes every `POST /pulls/{n}/comments` its own review, so posting threads one by one put one "reviewed" entry and one notification per finding on the PR. The executor instead posts all of a run's new threads in one `POST /pulls/{n}/reviews` with `event: "COMMENT"`, the reviewed head as `commit_id`, and a short fixed body, which GitHub requires for a `COMMENT` review [observed] ([github.ts](../src/reporters/github.ts) `createInlineReview`, `INLINE_REVIEW_BODY`). Patches, stubs and deletes stay per comment: they edit existing threads and create no review. -**The batch is all or nothing.** GitHub rejects the whole review when it refuses one comment, for example a line it will not anchor. Any failure other than a rate limit therefore falls back to one `POST /pulls/{n}/comments` per thread, so only the refused comment is lost; a rate limit aborts the rest of the sync as before [observed] (`syncInline`; tests `src/__tests__/inline-review.test.ts`). +**The batch is all or nothing, and only a 422 is retried.** GitHub rejects the whole review with a 422 when it refuses one comment, for example a line it will not anchor, and then creates nothing. Only that error falls back to one `POST /pulls/{n}/comments` per thread, so only the refused comment is lost. Any other failure, such as a 5xx or a dropped connection, may come after GitHub created the review, so a retry could post every thread twice: the executor counts those threads as failed and does not retry, the main comment renders the findings in full, and the next run creates whatever is still missing. A rate limit aborts the rest of the sync as before [observed] (`syncInline`, `isUnprocessableError`; tests `src/__tests__/inline-review.test.ts`). -**Links come from a second call.** The review response does not list its comments, so the executor reads `GET /pulls/{n}/reviews/{id}/comments` and maps each comment to its finding by the anchored marker in its body (`inlineUrlsByFp`). When that read fails, the threads exist but the main comment renders those findings in full for this run; the next run finds the threads through `ownInlineComments` and links them [observed] (`syncInline`). +**Links come from a second call.** The review response does not list its comments, so the executor reads `GET /pulls/{n}/reviews/{id}/comments`, every page of it, because `inline.maxComments` has no upper bound, and maps each comment to its finding by the anchored marker in its body (`inlineUrlsByFp`). When that read fails, the threads exist but the main comment renders those findings in full for this run; the next run finds the threads through `ownInlineComments` and links them [observed] (`syncInline`). ## Terminal Reporter diff --git a/src/__tests__/inline-review.test.ts b/src/__tests__/inline-review.test.ts index 7a86a7d..dc046b5 100644 --- a/src/__tests__/inline-review.test.ts +++ b/src/__tests__/inline-review.test.ts @@ -39,14 +39,23 @@ test("inlineUrlsByFp: maps our marked comments to their permalinks", () => { // ---- the sync posts new threads through a fake `gh` on PATH ---- // A fake `gh` that logs each call and answers the endpoints the inline sync uses. -// FAKE_GH_REVIEW_ERROR makes the review POST fail with that stderr, and -// FAKE_GH_REVIEW_NO_ID makes it succeed with a response that has no id. +// FAKE_GH_REVIEW_ERROR makes the review POST fail with that stderr, +// FAKE_GH_REVIEW_NO_ID makes it succeed with a response that has no id, and +// FAKE_GH_LIST_ERROR makes reading the review's comments fail. The review's comments +// are served from review-comments-.json. const FAKE_GH = `#!/bin/sh printf '%s\\n' "$*" >> "$FAKE_GH_DIR/calls.log" eval "last=\\\${$#}" case "$*" in "pr view"*) printf '%s' "$FAKE_GH_HEAD" ;; - *"/reviews/99/comments"*) cat "$FAKE_GH_DIR/review-comments.json" ;; + *"/reviews/99/comments"*) + if [ -n "$FAKE_GH_LIST_ERROR" ]; then echo "$FAKE_GH_LIST_ERROR" >&2; exit 1; fi + page=$(printf '%s' "$*" | sed -n 's/.*page=\\([0-9]*\\).*/\\1/p') + if [ -f "$FAKE_GH_DIR/review-comments-$page.json" ]; then + cat "$FAKE_GH_DIR/review-comments-$page.json" + else + printf '[]' + fi ;; *"-X POST "*"/reviews --input "*) cp "$last" "$FAKE_GH_DIR/review.json" if [ -n "$FAKE_GH_REVIEW_ERROR" ]; then echo "$FAKE_GH_REVIEW_ERROR" >&2; exit 1; fi @@ -74,6 +83,7 @@ beforeEach(async () => { "FAKE_GH_HEAD", "FAKE_GH_REVIEW_ERROR", "FAKE_GH_REVIEW_NO_ID", + "FAKE_GH_LIST_ERROR", "GITHUB_STEP_SUMMARY", ]) { saved[name] = process.env[name]; @@ -83,6 +93,7 @@ beforeEach(async () => { process.env.FAKE_GH_HEAD = HEAD; delete process.env.FAKE_GH_REVIEW_ERROR; delete process.env.FAKE_GH_REVIEW_NO_ID; + delete process.env.FAKE_GH_LIST_ERROR; delete process.env.GITHUB_STEP_SUMMARY; resetTrustedToolCache(); }); @@ -99,41 +110,48 @@ afterEach(async () => { await rm(dir, { recursive: true, force: true }); }); -async function syncNewThreads(prNumber: number): Promise<{ +// Two findings by default; `count` findings (one per file) for the pagination test. +async function syncNewThreads( + prNumber: number, + count?: number, +): Promise<{ urls: Map; calls: string[]; }> { - const targets = [ - { fp: "aaaa11", finding: finding() }, - { fp: "bbbb22", finding: finding({ file: "b.ts", line: 9, severity: "suggestion" }) }, - ]; - await writeFile( - path.join(dir, "review-comments.json"), - JSON.stringify( - targets.map((target, index) => ({ - id: 500 + index, - body: renderInlineCommentBody(target.finding, TAG, target.fp), - html_url: `https://github.com/o/r/pull/${prNumber}#discussion_r${500 + index}`, - })), - ), - "utf8", - ); + const targets = count + ? Array.from({ length: count }, (_, index) => ({ + fp: (0xa00000 + index).toString(16), + finding: finding({ file: `f${index}.ts`, line: 1 }), + })) + : [ + { fp: "aaaa11", finding: finding() }, + { fp: "bbbb22", finding: finding({ file: "b.ts", line: 9, severity: "suggestion" }) }, + ]; + const reviewComments = targets.map((target, index) => ({ + id: 500 + index, + body: renderInlineCommentBody(target.finding, TAG, target.fp), + html_url: `https://github.com/o/r/pull/${prNumber}#discussion_r${500 + index}`, + })); + for (let page = 1; (page - 1) * 100 < reviewComments.length; page++) { + await writeFile( + path.join(dir, `review-comments-${page}.json`), + JSON.stringify(reviewComments.slice((page - 1) * 100, page * 100)), + "utf8", + ); + } const reporter = new GitHubReporter({ prNumber, repo: "o/r", commentTag: TAG, ownLogin: "ecr-bot", headSha: HEAD, - inline: { enabled: true, maxComments: 20 }, + inline: { enabled: true, maxComments: Math.max(20, targets.length) }, }); - const link: LinkContext = { - repo: "o/r", - prNumber, - diffLines: new Map([ - ["a.ts", new Set([5])], - ["b.ts", new Set([9])], - ]), - }; + const diffLines = new Map>(); + for (const { finding: target } of targets) { + diffLines.set(target.file, new Set([...(diffLines.get(target.file) ?? []), target.line!])); + } + const link: LinkContext = { repo: "o/r", prNumber, diffLines }; // syncInline is private; the test drives it directly so the payload GitHub would // receive is checked without stubbing the rest of report(). const sync = ( @@ -196,3 +214,24 @@ test("syncInline: a created review without an id is never posted again", async ( expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1); expect(urls.size).toBe(0); }); + +test("syncInline: a review that fails without a 422 is not retried, since it may exist", async () => { + process.env.FAKE_GH_REVIEW_ERROR = "gh: Bad Gateway (HTTP 502)"; + const { urls, calls } = await syncNewThreads(705); + expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1); + expect(urls.size).toBe(0); +}); + +test("syncInline: a created review whose comments can't be read is not posted again", async () => { + process.env.FAKE_GH_LIST_ERROR = "gh: Internal Server Error (HTTP 500)"; + const { urls, calls } = await syncNewThreads(706); + expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1); + expect(urls.size).toBe(0); +}); + +test("syncInline: links every thread of a review with more than one page of comments", async () => { + const { urls, calls } = await syncNewThreads(707, 150); + expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1); + expect(calls.filter((call) => call.includes("/reviews/99/comments"))).toHaveLength(2); + expect(urls.size).toBe(150); +}); diff --git a/src/reporters/github.ts b/src/reporters/github.ts index 8faed78..da78a15 100644 --- a/src/reporters/github.ts +++ b/src/reporters/github.ts @@ -1271,6 +1271,11 @@ export class GitHubReporter implements Reporter { return /HTTP 403|HTTP 429|rate limit|secondary rate/i.test(errorMessage(error)); } + /** 422: GitHub validated the request and created nothing. */ + private static isUnprocessableError(error: unknown): boolean { + return /HTTP 422/.test(errorMessage(error)); + } + // @ref LLP 0008#github-reporter-identity [constrained-by] — inline threads are // patched/deleted only when author + anchored marker both match; creates also require // a resolved own login, or every run would duplicate what it cannot recognize @@ -1390,6 +1395,7 @@ export class GitHubReporter implements Reporter { })); // @ref LLP 0008#inline-threads [implements] — one COMMENT review per run for new threads; per-comment fallback when GitHub rejects the review let posted = false; + let retryOneByOne = false; let reviewId: number | null = null; try { reviewId = await this.createInlineReview(comments); @@ -1401,11 +1407,20 @@ export class GitHubReporter implements Reporter { process.stderr.write( `inline sync: rate-limited; aborting remaining inline mutations this run: ${errorMessage(error)}\n`, ); - } else { + } else if (GitHubReporter.isUnprocessableError(error)) { // GitHub rejects the whole review when it refuses one comment, so retry one // by one: only the comment it refuses is lost. + retryOneByOne = true; process.stderr.write( - `inline sync: the review with ${comments.length} new comment(s) failed; posting them one by one: ${errorMessage(error)}\n`, + `inline sync: GitHub rejected the review with ${comments.length} new comment(s); posting them one by one: ${errorMessage(error)}\n`, + ); + } else { + // A 5xx or a dropped connection can still have created the review, so a retry + // could post every comment twice. The findings render in the main comment, + // and the next run creates the threads that are still missing. + counts.failed += comments.length; + process.stderr.write( + `inline sync: the review with ${comments.length} new comment(s) failed and may exist; not retrying: ${errorMessage(error)}\n`, ); } } @@ -1428,7 +1443,7 @@ export class GitHubReporter implements Reporter { `inline sync: could not list the new review's comments (they render in the main comment): ${errorMessage(error)}\n`, ); } - } else { + } else if (retryOneByOne) { for (const comment of comments) { await attempt(async () => { const created = await this.createReviewComment( @@ -1477,8 +1492,9 @@ export class GitHubReporter implements Reporter { /** * Post `comments` as one COMMENT review anchored to the reviewed head and return * the review id, or null when the review was created but its id can't be read. - * Throws only when GitHub rejects the review; it rejects all of the comments - * together. + * Throws when the POST fails. After a 422, GitHub created nothing, because it + * rejects all of the comments together; after any other failure, such as a 5xx + * or a dropped connection, the review may still exist. */ private async createInlineReview( comments: Array<{ body: string; path: string; line: number }>, @@ -1523,23 +1539,35 @@ export class GitHubReporter implements Reporter { } } - /** The comments of one review. A review holds at most `inline.maxComments` of ours. */ + /** All comments of one review, every page (`inline.maxComments` has no upper bound). */ private async listInlineReviewComments(reviewId: number): Promise { + const all: ReviewCommentApi[] = []; const gh = await resolveTrustedTool("gh"); - const { stdout } = await run( - gh, - [ - "api", - "-X", - "GET", - `repos/${this.options.repo}/pulls/${this.options.prNumber}/reviews/${reviewId}/comments`, - "-f", - "per_page=100", - ], - { cwd: this.options.cwd }, - ); - const comments = JSON.parse(stdout) as ReviewCommentApi[]; - return Array.isArray(comments) ? comments : []; + for (let page = 1; page <= GitHubReporter.MAX_COMMENT_PAGES; page++) { + const { stdout } = await run( + gh, + [ + "api", + "-X", + "GET", + `repos/${this.options.repo}/pulls/${this.options.prNumber}/reviews/${reviewId}/comments`, + "-f", + "per_page=100", + "-f", + `page=${page}`, + ], + { cwd: this.options.cwd }, + ); + const batch = JSON.parse(stdout) as ReviewCommentApi[]; + if (!Array.isArray(batch) || batch.length === 0) { + break; + } + all.push(...batch); + if (batch.length < 100) { + break; + } + } + return all; } private async createReviewComment( From 8c86a3efa32c6d18687c8b8cb01daa514836acc4 Mon Sep 17 00:00:00 2001 From: Brent Vatne Date: Wed, 21 Oct 2026 14:28:38 -0700 Subject: [PATCH 5/6] fix: classify review and comment POST failures by GitHub's response body - Both inline POSTs run with check: false and throw ghApiFailure, which keeps the response body's distinct messages (each capped) next to gh's one-line stderr, since gh prints errors[] only to stdout. - The spam throttle ("submitted too quickly") counts as a rate limit, so it aborts the sync for the review and inside the one-by-one fallback. A pending review on the posting account fails the new threads without a fallback; stubs and deletes still run. - Reading back the review's comments goes page by page: a failed page keeps the links before it, and a rate limit there stops the remaining mutations. - Tests: payload bodies, a mixed create/delete run, read-back rate limit and partial pages, pending-review and spam 422s, a rate limit while patching, spam during the fallback, and ghApiFailure. - README, template and LLP 0008: one review per scope in per-scope mode, the review body stays after its threads are deleted, and GitHub and gh behavior that hasn't been checked live is tagged [inferred]. --- README.md | 10 +- llp/0008-sources-and-reporters.explainer.md | 6 +- src/__tests__/inline-review.test.ts | 217 +++++++++++++++++++- src/reporters/github.ts | 103 ++++++++-- templates/config.jsonc | 7 +- 5 files changed, 306 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 1980eed..cc6d7ae 100644 --- a/README.md +++ b/README.md @@ -733,10 +733,12 @@ Behavior notes: matches the reply to that finding structurally; clearing still requires the `id:…` token in the replier's own words, same as main-comment replies. - **One review per run.** A run's new inline comments go out in one `COMMENT` - review, so the PR gets one review entry and one notification per run, not one - per comment. The reviewer never approves or requests changes. If GitHub - rejects the review, for example for a line it won't anchor, the comments are - posted one by one instead, each as its own review. + review, so each run puts one review entry and one notification on the PR (one + per scope in per-scope comment mode), not one per comment. The reviewer never + approves or requests changes. If GitHub refuses one of the comments, for + example for a line it won't anchor, the comments are posted one by one + instead, each as its own review. The review's short body stays on the PR + after its threads are deleted, because GitHub can't delete a submitted review. - **Disabling later** leaves existing threads up (the sync no longer runs); a comment-mode switch — or the reporter's clear path — stubs/removes them. diff --git a/llp/0008-sources-and-reporters.explainer.md b/llp/0008-sources-and-reporters.explainer.md index 6be4094..ddc959e 100644 --- a/llp/0008-sources-and-reporters.explainer.md +++ b/llp/0008-sources-and-reporters.explainer.md @@ -83,11 +83,11 @@ The reporter maintains exactly one PR comment, plus optional inline threads (see With `inline.enabled`, `syncInline` converges one inline thread per in-diff finding: `planInlineSync` decides which threads to create, patch, stub or delete, and the executor runs the plan [observed] ([github.ts](../src/reporters/github.ts) `planInlineSync`, `syncInline`). -**One review per run for new threads.** GitHub makes every `POST /pulls/{n}/comments` its own review, so posting threads one by one put one "reviewed" entry and one notification per finding on the PR. The executor instead posts all of a run's new threads in one `POST /pulls/{n}/reviews` with `event: "COMMENT"`, the reviewed head as `commit_id`, and a short fixed body, which GitHub requires for a `COMMENT` review [observed] ([github.ts](../src/reporters/github.ts) `createInlineReview`, `INLINE_REVIEW_BODY`). Patches, stubs and deletes stay per comment: they edit existing threads and create no review. +**One review per run for new threads.** GitHub makes every `POST /pulls/{n}/comments` its own review, so posting threads one by one put one "reviewed" entry and one notification per finding on the PR. The executor instead posts all of a run's new threads in one `POST /pulls/{n}/reviews` with `event: "COMMENT"`, the reviewed head as `commit_id`, and a short fixed body [observed] ([github.ts](../src/reporters/github.ts) `createInlineReview`, `INLINE_REVIEW_BODY`). GitHub documents the body as required for a `COMMENT` review, and a submitted review can't be deleted, so that body stays on the PR after its threads are deleted [inferred: from GitHub's REST docs, not checked against a live PR]. In per-scope comment mode every scope's reporter syncs its own threads, so a run posts one review per scope with new threads [observed] (`reporterFor` in `src/commands/ci.ts` passes the root `inline` config to each scope). Patches, stubs and deletes stay per comment: they edit existing threads and create no review. -**The batch is all or nothing, and only a 422 is retried.** GitHub rejects the whole review with a 422 when it refuses one comment, for example a line it will not anchor, and then creates nothing. Only that error falls back to one `POST /pulls/{n}/comments` per thread, so only the refused comment is lost. Any other failure, such as a 5xx or a dropped connection, may come after GitHub created the review, so a retry could post every thread twice: the executor counts those threads as failed and does not retry, the main comment renders the findings in full, and the next run creates whatever is still missing. A rate limit aborts the rest of the sync as before [observed] (`syncInline`, `isUnprocessableError`; tests `src/__tests__/inline-review.test.ts`). +**The batch is all or nothing, and only a 422 a comment caused is retried.** GitHub rejects the whole review with a 422 when it refuses one comment, for example a line it will not anchor, and then creates nothing [inferred: from GitHub's REST docs, not checked against a live PR]. That error falls back to one `POST /pulls/{n}/comments` per thread, so only the refused comment is lost. Two 422s are about the account, not a comment, and one-by-one posts would hit them again: GitHub's spam throttle ("submitted too quickly") aborts the rest of the sync like a rate limit, for the review and for each one-by-one post, and a pending review the posting account already has on the PR (GitHub allows one) fails the new threads with a message saying to submit or delete it, while stubs and deletes still run. Telling them apart needs the response body: `gh` prints only the top-level message to stderr and the body to stdout [inferred: from the gh CLI source, not checked against a live PR], so both POSTs run with `check: false` and `ghApiFailure` keeps the body's distinct messages, each capped, in the error. Any other failure, such as a 5xx or a dropped connection, may come after GitHub created the review, so a retry could post every thread twice: the executor counts those threads as failed and does not retry, the main comment renders the findings in full, and the next run creates whatever is still missing. A rate limit aborts the rest of the sync as before [observed] (`syncInline`, `isPendingReviewError`, `ghApiFailure`; tests `src/__tests__/inline-review.test.ts`). -**Links come from a second call.** The review response does not list its comments, so the executor reads `GET /pulls/{n}/reviews/{id}/comments`, every page of it, because `inline.maxComments` has no upper bound, and maps each comment to its finding by the anchored marker in its body (`inlineUrlsByFp`). When that read fails, the threads exist but the main comment renders those findings in full for this run; the next run finds the threads through `ownInlineComments` and links them [observed] (`syncInline`). +**Links come from a second call.** The review response does not list its comments, so the executor reads `GET /pulls/{n}/reviews/{id}/comments` a page at a time, up to the same 30-page bound as every comment listing, and maps each comment to its finding by the anchored marker in its body (`inlineUrlsByFp`). When a page fails, the pages before it stay linked, the threads exist, and the main comment renders the rest in full for this run; the next run finds the threads through `ownInlineComments` and links them. A rate limit on that read aborts the stubs and deletes still to come [observed] (`syncInline`, `inlineReviewCommentPages`). ## Terminal Reporter diff --git a/src/__tests__/inline-review.test.ts b/src/__tests__/inline-review.test.ts index dc046b5..7da2a13 100644 --- a/src/__tests__/inline-review.test.ts +++ b/src/__tests__/inline-review.test.ts @@ -7,7 +7,12 @@ import { resetTrustedToolCache } from "../core/exec.js"; import { renderInlineCommentBody } from "../core/render.js"; import type { LinkContext } from "../core/render.js"; import type { Finding } from "../core/schema.js"; -import { GitHubReporter, INLINE_REVIEW_BODY, inlineUrlsByFp } from "../reporters/github.js"; +import { + GitHubReporter, + INLINE_REVIEW_BODY, + ghApiFailure, + inlineUrlsByFp, +} from "../reporters/github.js"; const TAG = "expo-ai-code-reviewer"; const HEAD = "0123456789abcdef0123456789abcdef01234567"; @@ -39,18 +44,24 @@ test("inlineUrlsByFp: maps our marked comments to their permalinks", () => { // ---- the sync posts new threads through a fake `gh` on PATH ---- // A fake `gh` that logs each call and answers the endpoints the inline sync uses. -// FAKE_GH_REVIEW_ERROR makes the review POST fail with that stderr, -// FAKE_GH_REVIEW_NO_ID makes it succeed with a response that has no id, and -// FAKE_GH_LIST_ERROR makes reading the review's comments fail. The review's comments -// are served from review-comments-.json. +// FAKE_GH_REVIEW_ERROR makes the review POST fail with that stderr and +// FAKE_GH_REVIEW_BODY as the response body on stdout, FAKE_GH_REVIEW_NO_ID makes it +// succeed with a response that has no id, and FAKE_GH_LIST_ERROR makes reading the +// review's comments fail (only page FAKE_GH_LIST_ERROR_PAGE, when set), and +// FAKE_GH_PATCH_ERROR makes editing an existing comment fail. FAKE_GH_COMMENT_ERROR +// and FAKE_GH_COMMENT_BODY do for a single comment POST what the REVIEW pair does. The review's +// comments are served from review-comments-.json, and the PR's existing review +// comments from pr-comments.json. const FAKE_GH = `#!/bin/sh printf '%s\\n' "$*" >> "$FAKE_GH_DIR/calls.log" eval "last=\\\${$#}" case "$*" in "pr view"*) printf '%s' "$FAKE_GH_HEAD" ;; *"/reviews/99/comments"*) - if [ -n "$FAKE_GH_LIST_ERROR" ]; then echo "$FAKE_GH_LIST_ERROR" >&2; exit 1; fi page=$(printf '%s' "$*" | sed -n 's/.*page=\\([0-9]*\\).*/\\1/p') + if [ -n "$FAKE_GH_LIST_ERROR" ] && [ "\${FAKE_GH_LIST_ERROR_PAGE:-$page}" = "$page" ]; then + echo "$FAKE_GH_LIST_ERROR" >&2; exit 1 + fi if [ -f "$FAKE_GH_DIR/review-comments-$page.json" ]; then cat "$FAKE_GH_DIR/review-comments-$page.json" else @@ -58,13 +69,23 @@ case "$*" in fi ;; *"-X POST "*"/reviews --input "*) cp "$last" "$FAKE_GH_DIR/review.json" - if [ -n "$FAKE_GH_REVIEW_ERROR" ]; then echo "$FAKE_GH_REVIEW_ERROR" >&2; exit 1; fi + if [ -n "$FAKE_GH_REVIEW_ERROR" ]; then + printf '%s' "$FAKE_GH_REVIEW_BODY"; echo "$FAKE_GH_REVIEW_ERROR" >&2; exit 1 + fi if [ -n "$FAKE_GH_REVIEW_NO_ID" ]; then printf '{}'; exit 0; fi printf '{"id": 99}' ;; *"-X POST "*"/comments --input "*) + if [ -n "$FAKE_GH_COMMENT_ERROR" ]; then + printf '%s' "$FAKE_GH_COMMENT_BODY"; echo "$FAKE_GH_COMMENT_ERROR" >&2; exit 1 + fi n=$(wc -l < "$FAKE_GH_DIR/calls.log" | tr -d ' ') printf '{"html_url": "https://github.com/o/r/pull/7#discussion_r%s"}' "$n" ;; - *"-X GET "*"/comments"*) printf '[]' ;; + *"-X GET "*"/comments"*) + if [ -f "$FAKE_GH_DIR/pr-comments.json" ]; then cat "$FAKE_GH_DIR/pr-comments.json"; else printf '[]'; fi ;; + *"-X PATCH "*) + if [ -n "$FAKE_GH_PATCH_ERROR" ]; then echo "$FAKE_GH_PATCH_ERROR" >&2; exit 1; fi + printf '{}' ;; + *"-X DELETE "*) ;; *) echo "unexpected gh call: $*" >&2; exit 1 ;; esac `; @@ -82,8 +103,13 @@ beforeEach(async () => { "FAKE_GH_DIR", "FAKE_GH_HEAD", "FAKE_GH_REVIEW_ERROR", + "FAKE_GH_REVIEW_BODY", "FAKE_GH_REVIEW_NO_ID", "FAKE_GH_LIST_ERROR", + "FAKE_GH_LIST_ERROR_PAGE", + "FAKE_GH_PATCH_ERROR", + "FAKE_GH_COMMENT_ERROR", + "FAKE_GH_COMMENT_BODY", "GITHUB_STEP_SUMMARY", ]) { saved[name] = process.env[name]; @@ -92,8 +118,13 @@ beforeEach(async () => { process.env.FAKE_GH_DIR = dir; process.env.FAKE_GH_HEAD = HEAD; delete process.env.FAKE_GH_REVIEW_ERROR; + delete process.env.FAKE_GH_REVIEW_BODY; delete process.env.FAKE_GH_REVIEW_NO_ID; delete process.env.FAKE_GH_LIST_ERROR; + delete process.env.FAKE_GH_LIST_ERROR_PAGE; + delete process.env.FAKE_GH_PATCH_ERROR; + delete process.env.FAKE_GH_COMMENT_ERROR; + delete process.env.FAKE_GH_COMMENT_BODY; delete process.env.GITHUB_STEP_SUMMARY; resetTrustedToolCache(); }); @@ -185,6 +216,15 @@ test("syncInline: posts all new threads in one COMMENT review and links each one ["a.ts", 5, "RIGHT"], ["b.ts", 9, "RIGHT"], ]); + // The read-back fixture is built separately, so only this catches a lost body. + expect(review.comments.map((c: { body: string }) => c.body)).toEqual([ + renderInlineCommentBody(finding(), TAG, "aaaa11"), + renderInlineCommentBody( + finding({ file: "b.ts", line: 9, severity: "suggestion" }), + TAG, + "bbbb22", + ), + ]); expect(urls.get("aaaa11")).toBe("https://github.com/o/r/pull/701#discussion_r500"); expect(urls.get("bbbb22")).toBe("https://github.com/o/r/pull/701#discussion_r501"); }); @@ -235,3 +275,164 @@ test("syncInline: links every thread of a review with more than one page of comm expect(calls.filter((call) => call.includes("/reviews/99/comments"))).toHaveLength(2); expect(urls.size).toBe(150); }); + +// One thread of ours from an earlier run whose finding is gone, so the sync deletes it +// after posting the new review. +async function writeStaleThread(): Promise { + await writeFile( + path.join(dir, "pr-comments.json"), + JSON.stringify([ + { + id: 77, + body: renderInlineCommentBody(finding({ file: "c.ts", line: 3 }), TAG, "dddd44"), + user: { login: "ecr-bot" }, + html_url: "https://github.com/o/r/pull/7#discussion_r77", + }, + ]), + "utf8", + ); +} + +test("syncInline: one run posts the review, links it, and deletes a retired thread", async () => { + await writeStaleThread(); + const { urls, calls } = await syncNewThreads(708); + expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1); + expect(calls.filter((call) => call.includes("-X DELETE"))).toEqual([ + expect.stringContaining("repos/o/r/pulls/comments/77"), + ]); + expect([...urls.keys()].sort()).toEqual(["aaaa11", "bbbb22"]); +}); + +test("syncInline: a rate limit while reading the review's comments stops later mutations", async () => { + await writeStaleThread(); + process.env.FAKE_GH_LIST_ERROR = "gh: API rate limit exceeded (HTTP 429)"; + const { urls, calls } = await syncNewThreads(709); + expect(calls.filter((call) => call.includes("-X DELETE"))).toHaveLength(0); + expect(urls.size).toBe(0); +}); + +test("syncInline: a review page that fails keeps the links of the pages before it", async () => { + process.env.FAKE_GH_LIST_ERROR = "gh: Internal Server Error (HTTP 500)"; + process.env.FAKE_GH_LIST_ERROR_PAGE = "2"; + const { urls } = await syncNewThreads(710, 150); + expect(urls.size).toBe(100); +}); + +test("syncInline: a review refused for a pending review isn't posted one by one", async () => { + await writeStaleThread(); + process.env.FAKE_GH_REVIEW_ERROR = "gh: Unprocessable Entity (HTTP 422)"; + process.env.FAKE_GH_REVIEW_BODY = JSON.stringify({ + message: "Unprocessable Entity", + errors: ["User can only have one pending review per pull request"], + }); + const { urls, calls } = await syncNewThreads(711); + expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1); + // Only the new threads fail: the rest of the sync still runs. + expect(calls.filter((call) => call.includes("-X DELETE"))).toHaveLength(1); + expect(urls.size).toBe(0); +}); + +test("syncInline: a review refused as spam aborts the rest of the sync", async () => { + await writeStaleThread(); + process.env.FAKE_GH_REVIEW_ERROR = "gh: Validation Failed (HTTP 422)"; + process.env.FAKE_GH_REVIEW_BODY = JSON.stringify({ + message: "Validation Failed", + errors: [ + { resource: "PullRequestReview", code: "custom", message: "was submitted too quickly" }, + ], + }); + const { urls, calls } = await syncNewThreads(712); + expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1); + expect(calls.filter((call) => call.includes("-X DELETE"))).toHaveLength(0); + expect(urls.size).toBe(0); +}); + +test("syncInline: a rate limit while editing a thread skips the review", async () => { + // aaaa11's thread exists with an outdated body, so the sync patches it before it + // posts bbbb22 in a review. + await writeFile( + path.join(dir, "pr-comments.json"), + JSON.stringify([ + { + id: 78, + body: renderInlineCommentBody(finding({ title: "An older title" }), TAG, "aaaa11"), + user: { login: "ecr-bot" }, + html_url: "https://github.com/o/r/pull/7#discussion_r78", + }, + ]), + "utf8", + ); + process.env.FAKE_GH_PATCH_ERROR = "gh: API rate limit exceeded (HTTP 429)"; + const { calls } = await syncNewThreads(713); + expect(calls.filter((call) => call.includes("-X PATCH"))).toHaveLength(1); + expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(0); +}); + +test("syncInline: the spam throttle during the one-by-one fallback aborts the rest", async () => { + await writeStaleThread(); + process.env.FAKE_GH_REVIEW_ERROR = "gh: Unprocessable Entity (HTTP 422)"; + process.env.FAKE_GH_REVIEW_BODY = JSON.stringify({ + message: "Unprocessable Entity", + errors: ["Line could not be resolved"], + }); + process.env.FAKE_GH_COMMENT_ERROR = "gh: Validation Failed (HTTP 422)"; + process.env.FAKE_GH_COMMENT_BODY = JSON.stringify({ + message: "Validation Failed", + errors: [ + { + resource: "PullRequestReviewComment", + code: "custom", + message: "was submitted too quickly", + }, + ], + }); + const { calls } = await syncNewThreads(714); + expect(calls.filter((call) => call.includes("/comments --input"))).toHaveLength(1); + expect(calls.filter((call) => call.includes("-X DELETE"))).toHaveLength(0); +}); + +test("ghApiFailure: keeps the response body's messages, not the body", () => { + const json = ghApiFailure("creating the review", { + code: 1, + stderr: "gh: Validation Failed (HTTP 422)\n", + stdout: JSON.stringify({ + message: "Validation Failed", + errors: ["one", { message: "two" }, { code: "custom" }, null], + documentation_url: "https://docs.github.com", + }), + }); + expect(json.message).toBe( + "creating the review failed: gh: Validation Failed (HTTP 422): Validation Failed; one; two", + ); + const html = ghApiFailure("creating the review", { + code: 1, + stderr: "gh: HTTP 502\n", + stdout: "Bad Gateway", + }); + expect(html.message).toBe("creating the review failed: gh: HTTP 502"); + const silent = ghApiFailure("creating the review", { + code: 1, + stderr: "", + stdout: "", + signal: "SIGKILL", + }); + expect(silent.message).toBe("creating the review failed: gh exited 1 (SIGKILL)"); +}); + +test("ghApiFailure: caps each message, not the whole text, so a late throttle survives", () => { + const error = ghApiFailure("creating the review", { + code: 1, + stderr: "gh: Validation Failed (HTTP 422)", + stdout: JSON.stringify({ + message: "Validation Failed", + errors: [ + ...Array.from({ length: 30 }, () => ({ message: "Line could not be resolved" })), + { message: "x".repeat(1000) }, + { message: "was submitted too quickly" }, + ], + }), + }); + expect(error.message).toBe( + `creating the review failed: gh: Validation Failed (HTTP 422): Validation Failed; Line could not be resolved; ${"x".repeat(200)}; was submitted too quickly`, + ); +}); diff --git a/src/reporters/github.ts b/src/reporters/github.ts index da78a15..fb2b273 100644 --- a/src/reporters/github.ts +++ b/src/reporters/github.ts @@ -4,6 +4,7 @@ import { tmpdir } from "node:os"; import path from "node:path"; import { resolveTrustedTool, run } from "../core/exec.js"; +import type { RunResult } from "../core/exec.js"; import { errorMessage } from "../core/util.js"; import { parseUnifiedDiff } from "../core/diff.js"; import { @@ -197,6 +198,39 @@ export interface ExistingInlineComment { hasReplies: boolean; } +/** + * The error for a failed `gh api` call run with `check: false`. gh prints only the + * top-level message ("Validation Failed (HTTP 422)") to stderr; why GitHub refused, + * such as its spam throttle or a pending review, is in the response body on stdout. + * The body's distinct messages are kept, each capped, not the body: an HTML error page + * or one error per refused comment can't flood the log, and a cap on the whole text + * could cut off the message syncInline classifies by. Credentials never appear in + * either stream. + */ +export function ghApiFailure(action: string, result: RunResult): Error { + const details = new Set(); + try { + const body = JSON.parse(result.stdout) as { message?: unknown; errors?: unknown }; + if (typeof body.message === "string") { + details.add(body.message.slice(0, 200)); + } + for (const item of Array.isArray(body.errors) ? body.errors : []) { + const message = + typeof item === "string" ? item : (item as { message?: unknown } | null)?.message; + if (typeof message === "string") { + details.add(message.slice(0, 200)); + } + } + } catch { + // Not JSON (an HTML error page, or nothing): stderr alone says what happened. + } + const stderr = result.stderr.trim(); + const reason = + [stderr, [...details].join("; ")].filter(Boolean).join(": ") || + `gh exited ${result.code}${result.signal ? ` (${result.signal})` : ""}`; + return new Error(`${action} failed: ${reason}`); +} + /** Body of the review that carries one run's new inline comments. GitHub requires one. */ export const INLINE_REVIEW_BODY = "🤖 AI code review: inline comments for this run's findings. The AI code review comment has the summary."; @@ -1265,10 +1299,13 @@ export class GitHubReporter implements Reporter { } } - /** Secondary-rate-limit shapes (403/429): abort the rest of the sync, not just - * this mutation — hammering on would dig the hole deeper. */ + /** Secondary-rate-limit shapes (403/429), and the spam throttle's 422 ("submitted + * too quickly", only in errors from ghApiFailure): abort the rest of the sync, not + * just this mutation — hammering on would dig the hole deeper. */ private static isRateLimitError(error: unknown): boolean { - return /HTTP 403|HTTP 429|rate limit|secondary rate/i.test(errorMessage(error)); + return /HTTP 403|HTTP 429|rate limit|secondary rate|submitted too quickly/i.test( + errorMessage(error), + ); } /** 422: GitHub validated the request and created nothing. */ @@ -1276,6 +1313,16 @@ export class GitHubReporter implements Reporter { return /HTTP 422/.test(errorMessage(error)); } + /** + * A 422 because the posting account already has a pending review on this PR, which + * GitHub allows only one of. Posting the comments one by one would hit it too. + */ + private static isPendingReviewError(error: unknown): boolean { + return ( + GitHubReporter.isUnprocessableError(error) && /one pending review/i.test(errorMessage(error)) + ); + } + // @ref LLP 0008#github-reporter-identity [constrained-by] — inline threads are // patched/deleted only when author + anchored marker both match; creates also require // a resolved own login, or every run would duplicate what it cannot recognize @@ -1407,6 +1454,11 @@ export class GitHubReporter implements Reporter { process.stderr.write( `inline sync: rate-limited; aborting remaining inline mutations this run: ${errorMessage(error)}\n`, ); + } else if (GitHubReporter.isPendingReviewError(error)) { + counts.failed += comments.length; + process.stderr.write( + `inline sync: the posting account has a pending review on this PR, and GitHub allows only one; submit or delete it so new inline comments can post: ${errorMessage(error)}\n`, + ); } else if (GitHubReporter.isUnprocessableError(error)) { // GitHub rejects the whole review when it refuses one comment, so retry one // by one: only the comment it refuses is lost. @@ -1430,18 +1482,23 @@ export class GitHubReporter implements Reporter { if (reviewId == null) { throw new Error("the created review has no id"); } - for (const [fp, url] of inlineUrlsByFp( - await this.listInlineReviewComments(reviewId), - this.options.commentTag, - )) { - urls.set(fp, url); + for await (const page of this.inlineReviewCommentPages(reviewId)) { + for (const [fp, url] of inlineUrlsByFp(page, this.options.commentTag)) { + urls.set(fp, url); + } } } catch (error) { // The comments exist; without their links the findings render full-form in // the main comment this run, and the next run finds them as live threads. process.stderr.write( - `inline sync: could not list the new review's comments (they render in the main comment): ${errorMessage(error)}\n`, + `inline sync: could not list the new review's comments (the unlinked ones render in the main comment): ${errorMessage(error)}\n`, ); + if (GitHubReporter.isRateLimitError(error)) { + aborted = true; + process.stderr.write( + "inline sync: rate-limited; aborting remaining inline mutations this run.\n", + ); + } } } else if (retryOneByOne) { for (const comment of comments) { @@ -1516,7 +1573,7 @@ export class GitHubReporter implements Reporter { const jsonPath = path.join(dir, "review.json"); try { await writeFile(jsonPath, payload, "utf8"); - const { stdout } = await run( + const result = await run( gh, [ "api", @@ -1526,8 +1583,12 @@ export class GitHubReporter implements Reporter { "--input", jsonPath, ], - { cwd: this.options.cwd }, + { cwd: this.options.cwd, check: false }, ); + if (result.code !== 0) { + throw ghApiFailure("creating the review", result); + } + const { stdout } = result; try { const review = JSON.parse(stdout) as { id?: unknown }; return typeof review.id === "number" ? review.id : null; @@ -1539,9 +1600,11 @@ export class GitHubReporter implements Reporter { } } - /** All comments of one review, every page (`inline.maxComments` has no upper bound). */ - private async listInlineReviewComments(reviewId: number): Promise { - const all: ReviewCommentApi[] = []; + /** + * The comments of one review, a page at a time (`inline.maxComments` has no upper + * bound), so a page that fails doesn't cost the links of the pages before it. + */ + private async *inlineReviewCommentPages(reviewId: number): AsyncGenerator { const gh = await resolveTrustedTool("gh"); for (let page = 1; page <= GitHubReporter.MAX_COMMENT_PAGES; page++) { const { stdout } = await run( @@ -1562,12 +1625,11 @@ export class GitHubReporter implements Reporter { if (!Array.isArray(batch) || batch.length === 0) { break; } - all.push(...batch); + yield batch; if (batch.length < 100) { break; } } - return all; } private async createReviewComment( @@ -1588,7 +1650,7 @@ export class GitHubReporter implements Reporter { const jsonPath = path.join(dir, "review-comment.json"); try { await writeFile(jsonPath, payload, "utf8"); - const { stdout } = await run( + const result = await run( gh, [ "api", @@ -1598,10 +1660,13 @@ export class GitHubReporter implements Reporter { "--input", jsonPath, ], - { cwd: this.options.cwd }, + { cwd: this.options.cwd, check: false }, ); + if (result.code !== 0) { + throw ghApiFailure("creating the comment", result); + } try { - return JSON.parse(stdout) as { html_url?: string }; + return JSON.parse(result.stdout) as { html_url?: string }; } catch { return null; } diff --git a/templates/config.jsonc b/templates/config.jsonc index 52be6b0..b4bac07 100644 --- a/templates/config.jsonc +++ b/templates/config.jsonc @@ -170,9 +170,10 @@ // Threads update in place across re-reviews; a retired thread with human // replies is stubbed, never deleted. Replies on inline threads are matched to // their finding like main-comment replies (same clearing rules). - // A run's new inline comments go out in one COMMENT review, so the PR gets one - // review entry and one notification per run. Disabling later leaves existing - // threads up until a comment-mode switch (or manual cleanup) clears them. + // A run's new inline comments go out in one COMMENT review, so each run puts + // one review entry and one notification on the PR (one per scope in per-scope + // mode). Disabling later leaves existing threads up until a comment-mode switch + // (or manual cleanup) clears them. // "inline": { // "enabled": false, // "maxComments": 20 // per PR (single comment mode) / per scope (per-scope mode) From dd543a75dbb775f09cfc798b0e6b085966589096 Mon Sep 17 00:00:00 2001 From: Brent Vatne Date: Thu, 22 Oct 2026 11:57:56 -0700 Subject: [PATCH 6/6] fix: post the inline review with an empty body, and record GitHub's observed behavior A probe against a private scratch PR showed that GitHub accepts a COMMENT review with comments and an empty or missing body, despite its docs. A submitted review can't be deleted, so the fixed body stayed on the PR after its threads were torn down; an empty one leaves nothing. The same probe confirmed that one unanchorable line makes GitHub refuse the whole review and create nothing, that gh prints errors[] only to stdout, and what both POSTs return while the account has a pending review. LLP 0008 now tags those [observed]. --- README.md | 4 ++-- llp/0008-sources-and-reporters.explainer.md | 4 ++-- src/__tests__/inline-review.test.ts | 9 ++------- src/reporters/github.ts | 9 ++++----- 4 files changed, 10 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index cc6d7ae..3b222ba 100644 --- a/README.md +++ b/README.md @@ -737,8 +737,8 @@ Behavior notes: per scope in per-scope comment mode), not one per comment. The reviewer never approves or requests changes. If GitHub refuses one of the comments, for example for a line it won't anchor, the comments are posted one by one - instead, each as its own review. The review's short body stays on the PR - after its threads are deleted, because GitHub can't delete a submitted review. + instead, each as its own review. The review has no body of its own, so + nothing is left on the PR once its threads are deleted. - **Disabling later** leaves existing threads up (the sync no longer runs); a comment-mode switch — or the reporter's clear path — stubs/removes them. diff --git a/llp/0008-sources-and-reporters.explainer.md b/llp/0008-sources-and-reporters.explainer.md index ddc959e..f22ef01 100644 --- a/llp/0008-sources-and-reporters.explainer.md +++ b/llp/0008-sources-and-reporters.explainer.md @@ -83,9 +83,9 @@ The reporter maintains exactly one PR comment, plus optional inline threads (see With `inline.enabled`, `syncInline` converges one inline thread per in-diff finding: `planInlineSync` decides which threads to create, patch, stub or delete, and the executor runs the plan [observed] ([github.ts](../src/reporters/github.ts) `planInlineSync`, `syncInline`). -**One review per run for new threads.** GitHub makes every `POST /pulls/{n}/comments` its own review, so posting threads one by one put one "reviewed" entry and one notification per finding on the PR. The executor instead posts all of a run's new threads in one `POST /pulls/{n}/reviews` with `event: "COMMENT"`, the reviewed head as `commit_id`, and a short fixed body [observed] ([github.ts](../src/reporters/github.ts) `createInlineReview`, `INLINE_REVIEW_BODY`). GitHub documents the body as required for a `COMMENT` review, and a submitted review can't be deleted, so that body stays on the PR after its threads are deleted [inferred: from GitHub's REST docs, not checked against a live PR]. In per-scope comment mode every scope's reporter syncs its own threads, so a run posts one review per scope with new threads [observed] (`reporterFor` in `src/commands/ci.ts` passes the root `inline` config to each scope). Patches, stubs and deletes stay per comment: they edit existing threads and create no review. +**One review per run for new threads.** GitHub makes every `POST /pulls/{n}/comments` its own review, so posting threads one by one put one "reviewed" entry and one notification per finding on the PR. The executor instead posts all of a run's new threads in one `POST /pulls/{n}/reviews` with `event: "COMMENT"`, the reviewed head as `commit_id`, and an empty body [observed] ([github.ts](../src/reporters/github.ts) `createInlineReview`). GitHub's docs call the body required for a `COMMENT` review, but it accepts an empty or missing one when the review has comments [observed: probe against a private scratch PR, 2026-10-21]. A submitted review can't be deleted, so a body would stay on the PR after its threads are torn down; an empty one leaves nothing. In per-scope comment mode every scope's reporter syncs its own threads, so a run posts one review per scope with new threads [observed] (`reporterFor` in `src/commands/ci.ts` passes the root `inline` config to each scope). Patches, stubs and deletes stay per comment: they edit existing threads and create no review. -**The batch is all or nothing, and only a 422 a comment caused is retried.** GitHub rejects the whole review with a 422 when it refuses one comment, for example a line it will not anchor, and then creates nothing [inferred: from GitHub's REST docs, not checked against a live PR]. That error falls back to one `POST /pulls/{n}/comments` per thread, so only the refused comment is lost. Two 422s are about the account, not a comment, and one-by-one posts would hit them again: GitHub's spam throttle ("submitted too quickly") aborts the rest of the sync like a rate limit, for the review and for each one-by-one post, and a pending review the posting account already has on the PR (GitHub allows one) fails the new threads with a message saying to submit or delete it, while stubs and deletes still run. Telling them apart needs the response body: `gh` prints only the top-level message to stderr and the body to stdout [inferred: from the gh CLI source, not checked against a live PR], so both POSTs run with `check: false` and `ghApiFailure` keeps the body's distinct messages, each capped, in the error. Any other failure, such as a 5xx or a dropped connection, may come after GitHub created the review, so a retry could post every thread twice: the executor counts those threads as failed and does not retry, the main comment renders the findings in full, and the next run creates whatever is still missing. A rate limit aborts the rest of the sync as before [observed] (`syncInline`, `isPendingReviewError`, `ghApiFailure`; tests `src/__tests__/inline-review.test.ts`). +**The batch is all or nothing, and only a 422 a comment caused is retried.** GitHub rejects the whole review with a 422 when it refuses one comment, for example a line it will not anchor, and then creates nothing: a review with one valid and one unanchorable line came back 422 `Line could not be resolved` and created neither comment [observed: probe against a private scratch PR, 2026-10-21]. That error falls back to one `POST /pulls/{n}/comments` per thread, so only the refused comment is lost. Two 422s are about the account, not a comment, and one-by-one posts would hit them again: GitHub's spam throttle ("submitted too quickly") aborts the rest of the sync like a rate limit, for the review and for each one-by-one post, and a pending review the posting account already has on the PR (GitHub allows one, and refuses both POSTs with "one pending review per pull request" while it exists) fails the new threads with a message saying to submit or delete it, while stubs and deletes still run. Telling them apart needs the response body: `gh` prints only the top-level message to stderr (`gh: Unprocessable Entity (HTTP 422)`) and the body, with `errors[]`, to stdout [observed: probe against a private scratch PR, 2026-10-21; the spam throttle's text is from reports, not reproduced], so both POSTs run with `check: false` and `ghApiFailure` keeps the body's distinct messages, each capped, in the error. Any other failure, such as a 5xx or a dropped connection, may come after GitHub created the review, so a retry could post every thread twice: the executor counts those threads as failed and does not retry, the main comment renders the findings in full, and the next run creates whatever is still missing. A rate limit aborts the rest of the sync as before [observed] (`syncInline`, `isPendingReviewError`, `ghApiFailure`; tests `src/__tests__/inline-review.test.ts`). **Links come from a second call.** The review response does not list its comments, so the executor reads `GET /pulls/{n}/reviews/{id}/comments` a page at a time, up to the same 30-page bound as every comment listing, and maps each comment to its finding by the anchored marker in its body (`inlineUrlsByFp`). When a page fails, the pages before it stay linked, the threads exist, and the main comment renders the rest in full for this run; the next run finds the threads through `ownInlineComments` and links them. A rate limit on that read aborts the stubs and deletes still to come [observed] (`syncInline`, `inlineReviewCommentPages`). diff --git a/src/__tests__/inline-review.test.ts b/src/__tests__/inline-review.test.ts index 7da2a13..367e58b 100644 --- a/src/__tests__/inline-review.test.ts +++ b/src/__tests__/inline-review.test.ts @@ -7,12 +7,7 @@ import { resetTrustedToolCache } from "../core/exec.js"; import { renderInlineCommentBody } from "../core/render.js"; import type { LinkContext } from "../core/render.js"; import type { Finding } from "../core/schema.js"; -import { - GitHubReporter, - INLINE_REVIEW_BODY, - ghApiFailure, - inlineUrlsByFp, -} from "../reporters/github.js"; +import { GitHubReporter, ghApiFailure, inlineUrlsByFp } from "../reporters/github.js"; const TAG = "expo-ai-code-reviewer"; const HEAD = "0123456789abcdef0123456789abcdef01234567"; @@ -205,7 +200,7 @@ test("syncInline: posts all new threads in one COMMENT review and links each one expect.stringContaining("repos/o/r/pulls/701/reviews --input"), ]); const review = JSON.parse(await readFile(path.join(dir, "review.json"), "utf8")); - expect(review).toMatchObject({ commit_id: HEAD, event: "COMMENT", body: INLINE_REVIEW_BODY }); + expect(review).toMatchObject({ commit_id: HEAD, event: "COMMENT", body: "" }); expect( review.comments.map((c: { path: string; line: number; side: string }) => [ c.path, diff --git a/src/reporters/github.ts b/src/reporters/github.ts index fb2b273..49a8df9 100644 --- a/src/reporters/github.ts +++ b/src/reporters/github.ts @@ -231,10 +231,6 @@ export function ghApiFailure(action: string, result: RunResult): Error { return new Error(`${action} failed: ${reason}`); } -/** Body of the review that carries one run's new inline comments. GitHub requires one. */ -export const INLINE_REVIEW_BODY = - "🤖 AI code review: inline comments for this run's findings. The AI code review comment has the summary."; - /** * fp → permalink for the inline comments of ours among `comments`, read from the * anchored marker in each body (see parseInlineMarkerFp). @@ -1561,7 +1557,10 @@ export class GitHubReporter implements Reporter { const payload = JSON.stringify({ commit_id: this.options.headSha, event: "COMMENT", - body: INLINE_REVIEW_BODY, + // Empty on purpose. GitHub accepts it next to comments, despite its docs, and a + // submitted review can't be deleted: a body would stay on the PR after its + // threads are torn down. + body: "", comments: comments.map(({ body, path: path_, line }) => ({ body, path: path_,