diff --git a/README.md b/README.md index fe46d73..3b222ba 100644 --- a/README.md +++ b/README.md @@ -732,8 +732,13 @@ 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 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 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 fa4b559..f22ef01 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 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: 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`). + ## 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..367e58b --- /dev/null +++ b/src/__tests__/inline-review.test.ts @@ -0,0 +1,433 @@ +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, ghApiFailure, 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_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"*) + 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 + printf '[]' + fi ;; + *"-X POST "*"/reviews --input "*) + cp "$last" "$FAKE_GH_DIR/review.json" + 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"*) + 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 +`; + +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_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]; + } + 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_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(); +}); + +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 }); +}); + +// 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 = 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: Math.max(20, targets.length) }, + }); + 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 = ( + 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: "" }); + 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"], + ]); + // 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"); +}); + +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); +}); + +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); +}); + +// 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 055b5b1..49a8df9 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,54 @@ 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}`); +} + +/** + * 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 +602,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; @@ -1245,10 +1295,28 @@ 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. */ + private static isUnprocessableError(error: unknown): boolean { + 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 @@ -1361,18 +1429,88 @@ 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 retryOneByOne = 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 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. + retryOneByOne = true; + process.stderr.write( + `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`, + ); } - }); + } + if (posted) { + counts.created += comments.length; + try { + if (reviewId == null) { + throw new Error("the created review has no id"); + } + 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 (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) { + 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 +1542,95 @@ 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 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 }>, + ): Promise { + this.invalidateReviewComments(); + const gh = await resolveTrustedTool("gh"); + const payload = JSON.stringify({ + commit_id: this.options.headSha, + event: "COMMENT", + // 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_, + 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 result = await run( + gh, + [ + "api", + "-X", + "POST", + `repos/${this.options.repo}/pulls/${this.options.prNumber}/reviews`, + "--input", + jsonPath, + ], + { 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; + } catch { + return null; + } + } finally { + await rm(dir, { recursive: true, force: true }); + } + } + + /** + * 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( + 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; + } + yield batch; + if (batch.length < 100) { + break; + } + } + } + private async createReviewComment( body: string, path_: string, @@ -1422,7 +1649,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", @@ -1432,10 +1659,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 5d3f5a0..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). - // NOTE: the first enabled run on a PR sends one notification per created - // inline comment — `maxComments` bounds that. 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)