diff --git a/PLAN-inline-comments.md b/PLAN-inline-comments.md index f316507..18ca854 100644 --- a/PLAN-inline-comments.md +++ b/PLAN-inline-comments.md @@ -3,8 +3,8 @@ ## Goal Findings anchored to a line in the PR diff land as inline review comments on -that line. The main issue comment keeps every finding but renders an inlined -one SHORT: title + one-line rationale + link to the inline thread. The main +that line. The main issue comment does not list an inlined finding again: it +shows each inlined finding's id, linked to its thread, in one row per severity. The main comment stays the single durable state store (embedded state, dismissals, feedback records, pins, `/dismiss` target). Inline comments are a derived presentation layer, converged on every run. @@ -16,8 +16,9 @@ presentation layer, converged on every run. Individual review comments isolate failures and PATCH in place. - No multi-line comments; single `line` + `side: "RIGHT"` only. - No GraphQL thread resolution, and no detection of author-resolved/outdated - threads. Accepted risk: the PR author can collapse inline threads; the main - comment therefore keeps a one-line rationale per finding (see Short form). + threads. Accepted risk: the PR author can resolve (collapse) an inline thread. + The thread stays on the PR and anyone can expand it, and the author cannot + edit or delete the bot's comment, so the main comment shows only its id. - `ecr feedback` retroactive crawl keeps reading issue comments only. The review-comments fetch in `replyComments` is gated on the reporter having an ENABLED `inline` option, so the crawl (which passes none) gains no fetches. @@ -166,16 +167,15 @@ threads are STUBBED, bare ones deleted — never delete a thread humans wrote in covers the fp-namespace flip on per-scope↔single switches, symmetric with how main comments are cleared today. -### Main-comment short form +### Main-comment id rows - `LinkContext.inlineUrls?: Map`. NEVER mutate the shared routed-flow `link` object — render with a copy `{ ...link, inlineUrls }`. -- `renderFindingLines` with an inline URL: `- **title** — - [file:line](thread-url) _(category)_ · id:… · 💬 [inline](url)` + reply - annotation + ONE-LINE truncated rationale (~160 chars, word boundary) — the - audit trail survives the author collapsing/resolving inline threads. - Sources/suggestion/full rationale live inline only. Missing URL ⇒ full form - (tested). +- `renderSeveritySections` leaves every finding with an inline URL out of the + severity lists. Under `💬 **Inline comments on the diff (n)**` it adds one row + per severity with each finding's `id:` linked to its thread, plus a reply link + when an author answered. Severity headings count only the findings listed under + them. Missing URL ⇒ full form (tested). See LLP 0008 "Inline Threads". - Embedded state unchanged. ### Inline replies → feedback @@ -211,14 +211,14 @@ main comments are cleared today. + pure exported planner, replyComments extension, clear(), applyDismissal additive sync. - `src/core/render.ts` — inline marker helpers (anchored parse), inline body - renderer, `LinkContext.inlineUrls`, short form + rationale truncation. + renderer, `LinkContext.inlineUrls`, main-comment id rows. - `src/core/responses.ts` — threadFp, unquoted-marker backstop. - `src/core/schema.ts` — applyPins same-stream guard. - `src/commands/ci.ts` — wire config, `couldNotComplete: true` on both failure literals, aggregate teardown flag. `review.ts`, `post-review.ts`, `dismiss.ts` — wire config. - Tests: planner (create/patch/stub/delete, sticky cap, dedupe, additive mode), - marker anchor/forgery, short form + missing-URL fallback, backstop vs + marker anchor/forgery, id rows + missing-URL fallback, backstop vs quote-reply, threadFp in all match modes, applyPins stream guard, couldNotComplete literals. - `templates/config.jsonc`, `README.md`. diff --git a/README.md b/README.md index dad1c12..18b0ece 100644 --- a/README.md +++ b/README.md @@ -744,11 +744,13 @@ the defaults are asymmetric: [LLP 0011](./llp/0011-author-feedback.explainer.md) Off by default. When enabled, each finding anchored to a line that is actually in the PR's diff is posted as an inline review comment on that line, carrying the -full rationale, sources, and suggestion. The main comment still lists every -finding, but an inlined one renders short — title, links, category, id, and a -one-line rationale — plus a `💬 inline comment` link to the thread. The main -comment remains the single durable state store: `/dismiss`, embedded state, and -reply adjudication are unchanged. +full rationale, sources, and suggestion. The main comment does not list an +inlined finding again: under `💬 Inline comments on the diff` it shows one row per +severity with each finding's `id:`, linked to its thread, so `/dismiss` still has +the id. Findings that are not +inlined (outside the diff, over `maxComments`, or with no thread link) are listed +in full as before. The main comment remains the single durable state store: +`/dismiss`, embedded state, and reply adjudication are unchanged. ```jsonc "inline": { diff --git a/llp/0008-sources-and-reporters.explainer.md b/llp/0008-sources-and-reporters.explainer.md index 6eb87a1..f0c2550 100644 --- a/llp/0008-sources-and-reporters.explainer.md +++ b/llp/0008-sources-and-reporters.explainer.md @@ -91,6 +91,8 @@ With `inline.enabled`, `syncInline` converges one inline thread per in-diff find **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`). +**The main comment does not repeat an inlined finding.** A finding with a live thread is read in that thread, so the main comment shows only its `id:`, linked to the thread, in one row per severity under `💬 Inline comments on the diff`, with a link to a reply when an author answered it. The id stays because `/dismiss` takes it and `/dismiss` is an issue comment, not a thread reply. A finding without a live thread (outside the diff, over `maxComments`, a failed link, or a stubbed thread) is listed in full. Every render passes the thread links: a review run gets them from `syncInline`, and `/dismiss` and `/undismiss` read the threads before they render, so a dismissal does not bring the full list back. A `/undismiss` restores a stubbed thread before it renders, and links it only when the restore succeeded. When the threads can't be listed, every finding renders in full [observed] (`renderSeveritySections`, `splitInlined`, `inlinedLines` in [render.ts](../src/core/render.ts); `applyDismissal` in [github.ts](../src/reporters/github.ts)). The embedded state keeps every finding the comment shows, so dismissals and reply matching do not change. In the aggregate comment, the size cap trims inlined and listed findings on separate limits, inlined first, and both down to 3 per scope: a trimmed inlined finding keeps its thread on the PR, while a listed one appears nowhere else. Inlined findings must still be trimmable, because each renders as one link but carries its full text in the embedded state. The floor of 3 keeps a scope with findings from showing none. A trimmed finding leaves the state too, so `/dismiss` does not accept its id until a review run that fits (`renderAggregateMarkdown`). An earlier version kept a short entry with a one-line rationale as an audit trail in case the author resolved the thread. It was dropped because the author cannot edit or delete the bot's comment, and a resolved thread stays on the PR. + ## 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-comments.test.ts b/src/__tests__/inline-comments.test.ts index 08fc909..4c21417 100644 --- a/src/__tests__/inline-comments.test.ts +++ b/src/__tests__/inline-comments.test.ts @@ -10,15 +10,16 @@ import type { ExistingInlineComment } from "../reporters/github.js"; import { inlineCommentMarker, inlineStubBody, - oneLineRationale, parseInlineMarkerFp, renderInlineCommentBody, + parseReviewState, + renderAggregateMarkdown, renderMarkdown, } from "../core/render.js"; import type { LinkContext } from "../core/render.js"; import { hasUnquotedInlineMarker, matchReplies, replyIsNewer } from "../core/responses.js"; import type { ReplyComment } from "../core/responses.js"; -import { applyPins, fingerprintFinding } from "../core/schema.js"; +import { applyPins, fingerprintFinding, scopedFingerprint } from "../core/schema.js"; import type { CoordinatorOutput, FeedbackRecord, Finding } from "../core/schema.js"; const finding = (over: Partial = {}): Finding => ({ @@ -87,29 +88,6 @@ test("inlineStubBody: neutral — never claims resolved or dismissed", () => { expect(stub).toContain("main review comment"); }); -// ---- oneLineRationale ---- - -test("oneLineRationale: strips HTML tags, collapses whitespace, escapes <, truncates at a word boundary", () => { - expect(oneLineRationale("a\n\nb c")).toBe("a b c"); - expect(oneLineRationale("
x
")).toBe("x"); - expect(oneLineRationale("High — reason.
**Impact:** Medium
end")).toBe( - "High — reason. **Impact:** Medium end", - ); - expect(oneLineRationale("
Why\n\nbody
")).toBe("Why body"); - // Non-formatting angle brackets stay, escaped. - expect(oneLineRationale("returns `Promise` when a < b")).toBe( - "returns `Promise<void>` when a < b", - ); - const long = Array.from({ length: 60 }, (_, i) => `word${i}`).join(" "); - const short = oneLineRationale(long); - expect(short.length).toBeLessThanOrEqual(161); - expect(short.endsWith("…")).toBe(true); - // Cut lands on a word boundary: the kept prefix is followed by a space in the input. - const kept = short.slice(0, -1); - expect(long.startsWith(kept)).toBe(true); - expect(long[kept.length]).toBe(" "); -}); - // ---- planner ---- test("planInlineSync: creates new, patches live, stubs replied stale, deletes bare stale", () => { @@ -331,11 +309,18 @@ test("activeFindings: drops dismissed, reply-cleared, and requalified findings", expect(active.map((entry) => entry.fp)).toEqual(["keep01"]); }); -// ---- short form ---- +// ---- inlined findings leave the finding list ---- + +const threadUrl = (n: number): string => `https://github.com/o/r/pull/1#discussion_r${n}`; -test("renderMarkdown: an inlined finding renders short with the thread link; others render full", () => { +test("renderMarkdown: an inlined finding is only linked by id; others render full", () => { const inlined = finding({ suggestion: "cache the observer" }); - const other = finding({ file: "b.ts", title: "Other finding", suggestion: "do the thing" }); + const other = finding({ + file: "b.ts", + title: "Other finding", + rationale: "Other rationale.", + suggestion: "do the thing", + }); const review: CoordinatorOutput = { decision: "approve_with_comments", findings: [inlined, other], @@ -345,17 +330,265 @@ test("renderMarkdown: an inlined finding renders short with the thread link; oth const link: LinkContext = { repo: "o/r", prNumber: 1, - inlineUrls: new Map([ - [fingerprintFinding(inlined), "https://github.com/o/r/pull/1#discussion_r123"], - ]), + inlineUrls: new Map([[fingerprintFinding(inlined), threadUrl(123)]]), }; const body = renderMarkdown(review, TAG, [], link); - expect(body).toContain("[inline comment](https://github.com/o/r/pull/1#discussion_r123)"); - // Short form: the one-line rationale survives, the suggestion moves inline-only. - expect(body).toContain("getInstances() restarts the push token observer"); + // The inline thread holds the finding, so the list does not repeat it. + expect(body).not.toContain("Observer restarts for cached instances"); + expect(body).not.toContain("getInstances() restarts the push token observer"); expect(body).not.toContain("cache the observer"); + // Its id stays visible for `/dismiss`, linked to the thread. + expect(body).toContain("💬 **Inline comments on the diff (1)**"); + expect(body).toContain( + `- ⚠️ Warning: [\`id:${fingerprintFinding(inlined)}\`](${threadUrl(123)})`, + ); + // The heading counts only the findings listed under it. + expect(body).toContain("### ⚠️ Warning (1)"); // The non-inlined finding keeps its full form. + expect(body).toContain("**Other finding**"); expect(body).toContain("**Suggestion:** do the thing"); + // The embedded state still carries every finding. + expect(body).toContain(`"${fingerprintFinding(inlined)}"`); +}); + +test("renderMarkdown: when every finding is inlined, the list is only the inline rows", () => { + const warning = finding(); + const nitA = finding({ severity: "nit", file: "c.ts", title: "Nit A" }); + const nitB = finding({ severity: "nit", file: "d.ts", title: "Nit B" }); + const review: CoordinatorOutput = { + decision: "approve_with_comments", + findings: [nitA, warning, nitB], + summary: "s", + incomplete: [], + }; + const link: LinkContext = { + repo: "o/r", + prNumber: 1, + inlineUrls: new Map([ + [fingerprintFinding(nitA), threadUrl(2)], + [fingerprintFinding(warning), threadUrl(1)], + [fingerprintFinding(nitB), threadUrl(3)], + ]), + }; + const body = renderMarkdown(review, TAG, [], link); + expect(body).not.toContain("###"); + expect(body).not.toContain("No findings."); + expect(body).toContain("💬 **Inline comments on the diff (3)**"); + // One row per severity, most severe first. + const warningRow = body.indexOf("- ⚠️ Warning: "); + const nitRow = body.indexOf("- 🐧 Nit: "); + expect(warningRow).toBeGreaterThan(-1); + expect(nitRow).toBeGreaterThan(warningRow); + expect( + body + .slice(nitRow) + .split("\n")[0]! + .match(/discussion_r/g), + ).toHaveLength(2); +}); + +test("renderMarkdown: an inlined finding with a reply links the reply next to its id", () => { + const inlined = finding(); + const fp = fingerprintFinding(inlined); + const review: CoordinatorOutput = { + decision: "approve_with_comments", + findings: [inlined], + summary: "s", + incomplete: [], + }; + const reply: FeedbackRecord = { + fp, + by: "octocat", + commentId: 5, + url: "https://github.com/o/r/pull/1#issuecomment-5", + maintainer: false, + applied: false, + }; + const link: LinkContext = { repo: "o/r", prNumber: 1, inlineUrls: new Map([[fp, threadUrl(9)]]) }; + const body = renderMarkdown(review, TAG, [], link, [reply]); + expect(body).toContain( + `[\`id:${fp}\`](${threadUrl(9)}) (💬 [@octocat replied](https://github.com/o/r/pull/1#issuecomment-5))`, + ); +}); + +test("renderAggregateMarkdown: a named scope's inline threads match by the scope's ids", () => { + const inlined = finding(); + const listed = finding({ file: "b.ts", title: "Listed finding", rationale: "Listed." }); + const scopedId = scopedFingerprint("ios", inlined); + const link: LinkContext = { + repo: "o/r", + prNumber: 1, + // A plain fingerprint must not match: the scope renders scoped ids. + inlineUrls: new Map([ + [scopedId, threadUrl(4)], + [fingerprintFinding(listed), threadUrl(5)], + ]), + }; + const body = renderAggregateMarkdown( + [ + { + scope: "ios", + isDefault: false, + review: { + decision: "approve_with_comments", + findings: [inlined, listed], + summary: "s", + incomplete: [], + }, + }, + ], + TAG, + [], + link, + ); + expect(body).toContain(`[\`id:${scopedId}\`](${threadUrl(4)})`); + expect(body).not.toContain("Observer restarts for cached instances"); + expect(body).toContain("**Listed finding**"); + expect(body).not.toContain(threadUrl(5)); +}); + +test("renderAggregateMarkdown: a scope whose findings are all inlined stays open, with no 'No findings.'", () => { + const inlined = finding(); + const link: LinkContext = { + repo: "o/r", + prNumber: 1, + inlineUrls: new Map([[fingerprintFinding(inlined), threadUrl(6)]]), + }; + const body = renderAggregateMarkdown( + [ + { + scope: "root", + isDefault: true, + review: { + decision: "approve_with_comments", + findings: [inlined], + summary: "s", + incomplete: [], + }, + }, + ], + TAG, + [], + link, + ); + expect(body).toContain("
"); + expect(body).toContain("root — Ready for human review (with comments) (1)"); + expect(body).not.toContain("No findings."); + expect(body).toContain(threadUrl(6)); +}); + +const rootScope = (findings: Finding[]) => [ + { + scope: "root", + isDefault: true, + review: { + decision: "approve_with_comments" as const, + findings, + summary: "s", + incomplete: [], + }, + }, +]; + +test("renderAggregateMarkdown: truncation trims inlined findings before listed ones", () => { + // 30 inlined warnings and 10 listed nits, each nit large enough that the comment + // overflows and the per-scope limit must cut. + const inlined = Array.from({ length: 30 }, (_, index) => + finding({ file: `w${index}.ts`, title: `Warning ${index}` }), + ); + const listed = Array.from({ length: 10 }, (_, index) => + finding({ + severity: "nit", + file: `n${index}.ts`, + title: `Listed nit ${index}`, + rationale: "x".repeat(3_000), + }), + ); + const link: LinkContext = { + repo: "o/r", + prNumber: 1, + inlineUrls: new Map(inlined.map((f, index) => [fingerprintFinding(f), threadUrl(100 + index)])), + }; + const body = renderAggregateMarkdown(rootScope([...inlined, ...listed]), TAG, [], link); + expect(body.length).toBeLessThanOrEqual(60_000); + // Inlined findings went first, down to 3: the trimmed ones keep their threads. + expect(body).toContain("💬 **Inline comments on the diff (3)**"); + expect(body).toContain("…and 27 more inline comment(s) on the diff."); + // Then the listed nits, and the note counts only the listed ones left out. + const shownNits = listed.filter((f) => body.includes(`**${f.title}**`)).length; + expect(shownNits).toBeGreaterThan(0); + expect(shownNits).toBeLessThan(10); + expect(body).toContain(`…and ${10 - shownNits} more finding(s)`); + // The scope header still counts every kept finding. + expect(body).toContain("(40)"); +}); + +test("renderAggregateMarkdown: long inlined findings are trimmed so listed ones stay", () => { + // The inlined findings render as id links, but their full text rides the state. + const inlined = Array.from({ length: 20 }, (_, index) => + finding({ file: `w${index}.ts`, title: `Warning ${index}`, rationale: "y".repeat(4_000) }), + ); + const listed = Array.from({ length: 10 }, (_, index) => + finding({ severity: "nit", file: `n${index}.ts`, title: `Listed nit ${index}` }), + ); + const link: LinkContext = { + repo: "o/r", + prNumber: 1, + inlineUrls: new Map(inlined.map((f, index) => [fingerprintFinding(f), threadUrl(200 + index)])), + }; + const body = renderAggregateMarkdown(rootScope([...inlined, ...listed]), TAG, [], link); + expect(body.length).toBeLessThanOrEqual(60_000); + expect(listed.every((f) => body.includes(`**${f.title}**`))).toBe(true); + expect(body).toMatch(/…and \d+ more inline comment\(s\) on the diff\./); + expect(body).not.toContain("more finding(s)"); +}); + +test("renderAggregateMarkdown: a scope keeps 3 inlined findings in the comment and the state", () => { + // A scope with only inlined findings next to a scope whose large listed findings + // overflow the comment: the cap must not empty the first scope. + const inlined = Array.from({ length: 8 }, (_, index) => + finding({ file: `w${index}.ts`, title: `Warning ${index}` }), + ); + const listed = Array.from({ length: 10 }, (_, index) => + finding({ + severity: "nit", + file: `n${index}.ts`, + title: `Nit ${index}`, + rationale: "x".repeat(3_000), + }), + ); + const ids = inlined.map((f) => scopedFingerprint("ios", f)); + const link: LinkContext = { + repo: "o/r", + prNumber: 1, + inlineUrls: new Map(ids.map((id, index) => [id, threadUrl(300 + index)])), + }; + const scope = (name: string, findings: Finding[]) => ({ + scope: name, + isDefault: false, + inputHash: `hash-${name}`, + review: { + decision: "approve_with_comments" as const, + findings, + summary: "s", + incomplete: [], + }, + }); + const body = renderAggregateMarkdown( + [scope("ios", inlined), scope("web", listed)], + TAG, + [], + link, + ); + expect(body.length).toBeLessThanOrEqual(60_000); + expect(body).not.toContain("No findings."); + expect(body).toContain("…and 5 more inline comment(s) on the diff."); + const state = parseReviewState(body, TAG)!; + const ios = state.scopes!.find((entry) => entry.scope === "ios")!; + // The 3 shown inlined findings stay in the state, so `/dismiss` accepts their ids. + expect(ios.review.findings).toHaveLength(3); + // A trimmed scope is not the full result, so its input hash is left out. + expect(ios.inputHash).toBeUndefined(); }); test("renderMarkdown: a non-github inline URL is ignored (full form)", () => { diff --git a/src/__tests__/inline-review.test.ts b/src/__tests__/inline-review.test.ts index 367e58b..64e186e 100644 --- a/src/__tests__/inline-review.test.ts +++ b/src/__tests__/inline-review.test.ts @@ -4,9 +4,15 @@ import { tmpdir } from "node:os"; import path, { delimiter } from "node:path"; import { resetTrustedToolCache } from "../core/exec.js"; -import { renderInlineCommentBody } from "../core/render.js"; +import { + inlineStubBody, + renderAggregateMarkdown, + renderInlineCommentBody, + renderMarkdown, +} from "../core/render.js"; import type { LinkContext } from "../core/render.js"; -import type { Finding } from "../core/schema.js"; +import { fingerprintFinding, scopedFingerprint } from "../core/schema.js"; +import type { CoordinatorOutput, Finding } from "../core/schema.js"; import { GitHubReporter, ghApiFailure, inlineUrlsByFp } from "../reporters/github.js"; const TAG = "expo-ai-code-reviewer"; @@ -46,7 +52,9 @@ test("inlineUrlsByFp: maps our marked comments to their permalinks", () => { // 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. +// comments from pr-comments.json. The PR's issue comments come from +// issue-comments.json, and an issue comment edit is copied to issue-patch.json. +// FAKE_GH_PR_LIST_ERROR makes listing the PR's review comments fail. const FAKE_GH = `#!/bin/sh printf '%s\\n' "$*" >> "$FAKE_GH_DIR/calls.log" eval "last=\\\${$#}" @@ -75,7 +83,13 @@ case "$*" in 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 "*"/issues/"*"/comments"*) + if [ -f "$FAKE_GH_DIR/issue-comments.json" ]; then cat "$FAKE_GH_DIR/issue-comments.json"; else printf '[]'; fi ;; + *"-X PATCH "*"/issues/comments/"*) + cp "$last" "$FAKE_GH_DIR/issue-patch.json" + printf '{}' ;; *"-X GET "*"/comments"*) + if [ -n "$FAKE_GH_PR_LIST_ERROR" ]; then echo "$FAKE_GH_PR_LIST_ERROR" >&2; exit 1; fi 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 @@ -105,6 +119,7 @@ beforeEach(async () => { "FAKE_GH_PATCH_ERROR", "FAKE_GH_COMMENT_ERROR", "FAKE_GH_COMMENT_BODY", + "FAKE_GH_PR_LIST_ERROR", "GITHUB_STEP_SUMMARY", ]) { saved[name] = process.env[name]; @@ -120,6 +135,7 @@ beforeEach(async () => { delete process.env.FAKE_GH_PATCH_ERROR; delete process.env.FAKE_GH_COMMENT_ERROR; delete process.env.FAKE_GH_COMMENT_BODY; + delete process.env.FAKE_GH_PR_LIST_ERROR; delete process.env.GITHUB_STEP_SUMMARY; resetTrustedToolCache(); }); @@ -431,3 +447,195 @@ test("ghApiFailure: caps each message, not the whole text, so a late throttle su `creating the review failed: gh: Validation Failed (HTTP 422): Validation Failed; Line could not be resolved; ${"x".repeat(200)}; was submitted too quickly`, ); }); + +// ---- /dismiss re-renders with the live inline threads ---- + +test("applyDismissal: the re-rendered comment links findings with live threads, not stubs", async () => { + const live = finding(); + const stubbed = finding({ file: "b.ts", line: 9, title: "Stubbed thread finding" }); + const dismissed = finding({ file: "c.ts", line: 3, title: "Dismissed finding" }); + const [liveFp, stubbedFp, dismissedFp] = [live, stubbed, dismissed].map(fingerprintFinding); + const review: CoordinatorOutput = { + decision: "approve_with_comments", + findings: [live, stubbed, dismissed], + summary: "s", + incomplete: [], + }; + await writeFile( + path.join(dir, "issue-comments.json"), + JSON.stringify([{ id: 900, body: renderMarkdown(review, TAG), user: { login: "ecr-bot" } }]), + "utf8", + ); + await writeFile( + path.join(dir, "pr-comments.json"), + JSON.stringify([ + { + id: 81, + body: renderInlineCommentBody(live, TAG, liveFp!), + user: { login: "ecr-bot" }, + html_url: "https://github.com/o/r/pull/801#discussion_r81", + }, + { + id: 82, + body: inlineStubBody(TAG, stubbedFp!), + user: { login: "ecr-bot" }, + html_url: "https://github.com/o/r/pull/801#discussion_r82", + }, + ]), + "utf8", + ); + const reporter = new GitHubReporter({ + prNumber: 801, + repo: "o/r", + commentTag: TAG, + ownLogin: "ecr-bot", + headSha: HEAD, + inline: { enabled: true, maxComments: 20 }, + linkContext: { repo: "o/r", prNumber: 801 }, + }); + await reporter.applyDismissal([dismissedFp!], []); + const { body } = JSON.parse(await readFile(path.join(dir, "issue-patch.json"), "utf8")) as { + body: string; + }; + // The live thread is linked by id, not listed again. + expect(body).toContain(`[\`id:${liveFp}\`](https://github.com/o/r/pull/801#discussion_r81)`); + expect(body).not.toContain("**Observer restarts for cached instances**"); + // A stub is no live thread, so its finding stays listed in full. + expect(body).toContain("**Stubbed thread finding**"); + expect(body).not.toContain("discussion_r82"); + // The dismissed finding moves to the dismissed section. + expect(body).toContain("Dismissed on this PR (1)"); +}); + +// A dismissal setup: the main comment, and our review comments. +// Each test uses its own PR number: the reporter caches review comments per PR. +async function dismissalFixture( + prNumber: number, + issueBody: string, + threads: Array<{ id: number; body: string }>, +): Promise { + await writeFile( + path.join(dir, "issue-comments.json"), + JSON.stringify([{ id: 900, body: issueBody, user: { login: "ecr-bot" } }]), + "utf8", + ); + await writeFile( + path.join(dir, "pr-comments.json"), + JSON.stringify( + threads.map((thread) => ({ + ...thread, + user: { login: "ecr-bot" }, + html_url: `https://github.com/o/r/pull/${prNumber}#discussion_r${thread.id}`, + })), + ), + "utf8", + ); + return new GitHubReporter({ + prNumber, + repo: "o/r", + commentTag: TAG, + ownLogin: "ecr-bot", + headSha: HEAD, + inline: { enabled: true, maxComments: 20 }, + linkContext: { repo: "o/r", prNumber }, + }); +} + +async function patchedMainComment(): Promise { + return ( + JSON.parse(await readFile(path.join(dir, "issue-patch.json"), "utf8")) as { body: string } + ).body; +} + +test("applyDismissal: /undismiss restores the thread before the main comment links it", async () => { + const restored = finding({ title: "Restored finding" }); + const fp = fingerprintFinding(restored); + const review: CoordinatorOutput = { + decision: "approve_with_comments", + findings: [restored], + summary: "s", + incomplete: [], + }; + const reporter = await dismissalFixture(803, renderMarkdown(review, TAG, [{ fp }]), [ + { id: 91, body: inlineStubBody(TAG, fp) }, + ]); + await reporter.applyDismissal([], [fp]); + const calls = (await readFile(path.join(dir, "calls.log"), "utf8")).trim().split("\n"); + const patches = calls.filter((call) => call.includes("-X PATCH")); + // The thread PATCH comes first, then the main comment. + expect(patches[0]).toContain("repos/o/r/pulls/comments/91"); + expect(patches[1]).toContain("repos/o/r/issues/comments/900"); + expect(await patchedMainComment()).toContain( + `[\`id:${fp}\`](https://github.com/o/r/pull/803#discussion_r91)`, + ); +}); + +test("applyDismissal: a failed restore leaves the finding listed in full, not linked to a stub", async () => { + process.env.FAKE_GH_PATCH_ERROR = "HTTP 500"; + const restored = finding({ title: "Restored finding" }); + const fp = fingerprintFinding(restored); + const review: CoordinatorOutput = { + decision: "approve_with_comments", + findings: [restored], + summary: "s", + incomplete: [], + }; + const reporter = await dismissalFixture(804, renderMarkdown(review, TAG, [{ fp }]), [ + { id: 92, body: inlineStubBody(TAG, fp) }, + ]); + await reporter.applyDismissal([], [fp]); + const body = await patchedMainComment(); + expect(body).toContain("**Restored finding**"); + expect(body).not.toContain("discussion_r92"); +}); + +test("applyDismissal: when the threads can't be listed, every finding renders in full", async () => { + process.env.FAKE_GH_PR_LIST_ERROR = "HTTP 502"; + const live = finding(); + const other = finding({ file: "b.ts", title: "Other finding" }); + const review: CoordinatorOutput = { + decision: "approve_with_comments", + findings: [live, other], + summary: "s", + incomplete: [], + }; + const reporter = await dismissalFixture(805, renderMarkdown(review, TAG), [ + { id: 93, body: renderInlineCommentBody(live, TAG, fingerprintFinding(live)) }, + ]); + await reporter.applyDismissal([fingerprintFinding(other)], []); + const body = await patchedMainComment(); + expect(body).toContain("**Observer restarts for cached instances**"); + expect(body).not.toContain("discussion_r93"); + expect(body).toContain("Dismissed on this PR (1)"); +}); + +test("applyDismissal: an aggregate comment links threads by the scope's ids", async () => { + const live = finding(); + const dismissed = finding({ file: "c.ts", title: "Dismissed finding" }); + const liveId = scopedFingerprint("ios", live); + const dismissedId = scopedFingerprint("ios", dismissed); + const issueBody = renderAggregateMarkdown( + [ + { + scope: "ios", + isDefault: false, + review: { + decision: "approve_with_comments", + findings: [live, dismissed], + summary: "s", + incomplete: [], + }, + }, + ], + TAG, + [], + ); + const reporter = await dismissalFixture(806, issueBody, [ + { id: 94, body: renderInlineCommentBody(live, TAG, liveId) }, + ]); + const result = await reporter.applyDismissal([dismissedId], []); + expect(result.matched).toEqual([dismissedId]); + const body = await patchedMainComment(); + expect(body).toContain(`[\`id:${liveId}\`](https://github.com/o/r/pull/806#discussion_r94)`); + expect(body).not.toContain("**Observer restarts for cached instances**"); +}); diff --git a/src/config/schema.ts b/src/config/schema.ts index 19c3947..85a7707 100644 --- a/src/config/schema.ts +++ b/src/config/schema.ts @@ -234,8 +234,8 @@ export const ReviewConfigSchema = z.object({ maxAdjudications: 10, }), // Inline PR review comments for findings anchored to a line in the diff: the - // main comment stays the durable state store and renders such findings short, - // linking to the inline thread. ROOT-ONLY (the comment lifecycle is global) and + // main comment stays the durable state store and shows only each such finding's + // id, linked to the inline thread. ROOT-ONLY (the comment lifecycle is global) and // off by default — a feature that mutates N comments per run earns trust with // field data first. Turning it off later leaves existing threads up (documented); // clear() still sweeps them on comment-mode switches. diff --git a/src/core/render.ts b/src/core/render.ts index f6abcd2..a650824 100644 --- a/src/core/render.ts +++ b/src/core/render.ts @@ -44,9 +44,9 @@ export interface LinkContext { /** * Permalinks to this run's live inline finding comments, keyed by the SAME id the * comment renders (plain fingerprint, or scope-namespaced for an aggregate). A - * finding with an entry renders in SHORT form — title, links, one-line rationale — - * because its full text lives in the inline thread; a finding without one renders - * in full exactly as before. Callers must pass a per-render COPY of a shared + * finding with an entry is left out of the finding list and shown only as its linked id, because + * its full text lives in the inline thread; a finding without one renders in full. + * Callers must pass a per-render COPY of a shared * LinkContext (`{ ...link, inlineUrls }`), never mutate the shared object: the * routed ci flow shares one context across every scope reporter. */ @@ -409,7 +409,8 @@ function renderSeveritySections( feedbackById?: Map, ): string[] { const out: string[] = []; - const groups = groupBySeverity(sortFindings(findings)); + const { inlined, listed } = splitInlined(sortFindings(findings), link, idFor); + const groups = groupBySeverity(listed); for (const severity of SEVERITIES) { const group = groups[severity]; if (group.length === 0) { @@ -422,6 +423,62 @@ function renderSeveritySections( } out.push(""); } + out.push(...inlinedLines(inlined, feedbackById)); + return out; +} + +// @ref LLP 0008#inline-threads [implements] — an inlined finding shows only its linked id in the main comment +/** + * Split findings into those with a live inline thread and those the main comment + * lists in full. An inlined finding is read in its thread, on the flagged line, so + * the main comment does not repeat it. It stays in the embedded state, so `/dismiss` + * and reply matching still find it. Order is kept. + */ +function splitInlined( + findings: Finding[], + link: LinkContext | undefined, + idFor: (finding: Finding) => string, +): { inlined: { finding: Finding; id: string; url: string }[]; listed: Finding[] } { + const inlined: { finding: Finding; id: string; url: string }[] = []; + const listed: Finding[] = []; + for (const finding of findings) { + const id = idFor(finding); + const url = validInlineUrl(link, id); + if (url) { + inlined.push({ finding, id, url }); + } else { + listed.push(finding); + } + } + return { inlined, listed }; +} + +/** + * The findings posted as inline threads, one row per severity: each thread's id, + * linked to the thread, plus a reply link when an author answered it. The id is + * what `/dismiss` takes, so it must stay visible in the main comment. + */ +function inlinedLines( + inlined: { finding: Finding; id: string; url: string }[], + feedbackById?: Map, +): string[] { + if (inlined.length === 0) { + return []; + } + const out = [`💬 **Inline comments on the diff (${inlined.length})**`, ""]; + for (const severity of SEVERITIES) { + const group = inlined.filter((entry) => knownSeverity(entry.finding.severity) === severity); + if (group.length === 0) { + continue; + } + const threads = group.map(({ id, url }) => { + const reply = feedbackById?.get(id); + const replied = reply ? ` (💬 ${replyLink(`${replyAuthor(reply)} replied`, reply)})` : ""; + return `[\`id:${id}\`](${url})${replied}`; + }); + out.push(`- ${severityHeading(severity)}: ${threads.join(", ")}`); + } + out.push(""); return out; } @@ -450,9 +507,6 @@ function sourceLabel(value: string): string { .replace(/([\\[\]])/g, "\\$1"); } -/** Longest one-line rationale excerpt the short form keeps in the main comment. */ -const SHORT_RATIONALE_CHARS = 160; - /** * HTML formatting tags a model writes into a rationale (`
`, `
`, ...). * A fixed list, not "anything in angle brackets", so a type like `Promise` @@ -461,28 +515,6 @@ const SHORT_RATIONALE_CHARS = 160; const FORMATTING_TAG_RE = /<\/?(?:a|b|blockquote|br|code|del|details|div|em|h[1-6]|hr|i|kbd|li|ol|p|pre|s|span|strong|sub|summary|sup|table|tbody|td|th|thead|tr|u|ul)\b[^<>]*>/gi; -/** - * A rationale reduced to one plain line for the short (inlined) form: HTML - * formatting tags removed (an escaped `
` would show as literal text), - * whitespace collapsed, every remaining `<` escaped so a truncated HTML block can - * never leave an unclosed tag in the comment, cut at a word boundary with an - * ellipsis. This excerpt is the audit trail that survives the PR author collapsing - * or resolving the inline thread, so it must always be non-empty when the rationale is. - */ -export function oneLineRationale(text: string): string { - const flat = text - .replace(FORMATTING_TAG_RE, " ") - .replace(/ 0 ? atWord : cut}…`; -} - /** Only ever link to github.com from a stored/returned URL (see REPLY_URL_RE). */ function validInlineUrl(link: LinkContext | undefined, id: string): string | null { const url = link?.inlineUrls?.get(id); @@ -495,17 +527,6 @@ function renderFindingLines( id: string = fingerprintFinding(finding), reply?: FeedbackRecord, ): string[] { - const inlineUrl = validInlineUrl(link, id); - if (inlineUrl) { - // Short form: the full text lives in the inline thread on the flagged line. - // The one-line rationale stays here as the durable audit trail (the author - // can collapse/resolve the thread; they cannot edit this comment). - return [ - `- **${modelText(finding.title)}** — ${location(finding, link)} _(${finding.category})_ · \`id:${id}\` · 💬 [inline comment](${inlineUrl})${replyAnnotation(reply)}`, - ...indentContinuation(breakMentions(oneLineRationale(stripStateMarkers(finding.rationale)))), - "", - ]; - } const out = [ `- **${modelText(finding.title)}** — ${location(finding, link)} _(${finding.category})_ · \`id:${id}\`${replyAnnotation(reply)}`, ...indentContinuation(modelText(finding.rationale)), @@ -532,7 +553,7 @@ function renderFindingLines( * Full body of one inline (PR review) finding comment: identity marker at byte 0, * then the finding rendered whole — everything model-written passes * stripStateMarkers, same as the main comment. The footer teaches the clearing - * flow (an UNQUOTED id citation), since the main comment only shows the short form. + * flow (an UNQUOTED id citation), since the main comment shows only its linked id. * The body must never contain `:state=`/`:fingerprints=` (it would be dropped as * our own main comment by the reply matcher's backstop) — it doesn't. */ @@ -962,7 +983,11 @@ export function renderAggregateMarkdown( const worst = worstDecision(results.map((result) => result.review.decision)); const unmatched = opts?.unmatchedFiles ?? []; - const buildBody = (limitPerScope: number, withSummaries: boolean): string => { + const buildBody = ( + limitPerScope: number, + inlinedLimit: number, + withSummaries: boolean, + ): string => { const lines: string[] = [ commentMarker(tag), "## 🤖 AI code review", @@ -1007,31 +1032,49 @@ export function renderAggregateMarkdown( lines.push("> 🔗 **Review setup:**", ...setupLines, ""); } - // Shown = the most-severe N kept findings per scope (N = limitPerScope). The - // embedded state trims KEPT findings to the same set so a truncated comment + // @ref LLP 0008#inline-threads [implements] — the size cap trims inlined findings before listed ones + // Shown = the most-severe `limitPerScope` listed and `inlinedLimit` inlined kept + // findings per scope. The two limits are separate, and the loop below lowers + // `inlinedLimit` first: a trimmed inlined finding still has its thread on the PR, + // while a listed one appears nowhere else. Inlined findings must be trimmable at + // all, because each renders as one id link but carries its full text in the + // embedded state. Both limits stop at 3, so a scope with kept findings always + // shows some, and keeps them in the state for `/dismiss`. The embedded state trims KEPT findings to the same set so a truncated comment // still fits GitHub's body limit (the hidden findings are noted, not silently // carried) — but dismissed findings are always kept in state (see below). - const rendered = perScope.map(({ result, kept, requalified, dropped, feedback: replies }) => ({ - result, - replies, - shown: sortFindings(kept.map((entry) => entry.finding)).slice(0, limitPerScope), - hidden: Math.max(0, kept.length - limitPerScope), - // The requalified section is trimmed by the same per-scope limit as shown: it - // is coordinator-populated (a wide stack can requalify many findings at once), - // and an untrimmed section would keep the truncation loop below from ever - // converging under MAX_COMMENT_CHARS. The audit note carries the TOTAL count, - // so trimming never hides that requalification happened. - requalified: requalified.slice(0, limitPerScope), - requalifiedHidden: Math.max(0, requalified.length - limitPerScope), - requalifiedAll: requalified, - dropped, - })); + const rendered = perScope.map(({ result, kept, requalified, dropped, feedback: replies }) => { + const { inlined, listed } = splitInlined( + sortFindings(kept.map((entry) => entry.finding)), + link, + (finding) => idOf(result, finding), + ); + return { + result, + replies, + shown: [ + ...inlined.slice(0, inlinedLimit).map((entry) => entry.finding), + ...listed.slice(0, limitPerScope), + ], + hidden: Math.max(0, listed.length - limitPerScope), + inlinedHidden: Math.max(0, inlined.length - inlinedLimit), + // The requalified section is trimmed by the same per-scope limit as shown: it + // is coordinator-populated (a wide stack can requalify many findings at once), + // and an untrimmed section would keep the truncation loop below from ever + // converging under MAX_COMMENT_CHARS. The audit note carries the TOTAL count, + // so trimming never hides that requalification happened. + requalified: requalified.slice(0, limitPerScope), + requalifiedHidden: Math.max(0, requalified.length - limitPerScope), + requalifiedAll: requalified, + dropped, + }; + }); for (const { result, replies, shown, hidden, + inlinedHidden, requalified, requalifiedAll, requalifiedHidden, @@ -1045,7 +1088,7 @@ export function renderAggregateMarkdown( shown.length > 0 || requalifiedAll.length > 0 || replies.length > 0 || mentions.length > 0 ? " open" : ""; - const keptCount = shown.length + hidden; + const keptCount = shown.length + hidden + inlinedHidden; lines.push( ``, `${stripStateMarkers(result.scope)} — ${decisionLabel(result.review.decision)} (${keptCount})`, @@ -1072,6 +1115,9 @@ export function renderAggregateMarkdown( if (hidden > 0) { lines.push(`_…and ${hidden} more finding(s) — see the workflow log._`, ""); } + if (inlinedHidden > 0) { + lines.push(`_…and ${inlinedHidden} more inline comment(s) on the diff._`, ""); + } if (requalifiedAll.length > 0) { lines.push( `**🔁 Addressed in stacked PRs (${requalifiedAll.length})**`, @@ -1118,7 +1164,7 @@ export function renderAggregateMarkdown( // across re-renders. The per-scope data (`scopes`) plus a merged v1 `review` // keep both v2 and v1 consumers working. const stateScopes: ScopeReviewResult[] = rendered.map( - ({ result, shown, hidden, requalified, requalifiedHidden, dropped }) => { + ({ result, shown, hidden, inlinedHidden, requalified, requalifiedHidden, dropped }) => { // @ref LLP 0011#suppression-is-never-silent — strip any per-scope `feedback` // a freshly-reviewed scope's ReviewRunResult carries: the top-level feedback // array (reviewState below) is the single source of truth. A stale per-scope @@ -1139,7 +1185,7 @@ export function renderAggregateMarkdown( isDefault: result.isDefault, // A truncated state is not the full result and therefore cannot be a cache // source. Omit its hash so the next run retries instead of reusing a subset. - ...(hidden === 0 && requalifiedHidden === 0 + ...(hidden === 0 && inlinedHidden === 0 && requalifiedHidden === 0 ? result.inputHash ? { inputHash: result.inputHash } : {} @@ -1189,13 +1235,29 @@ export function renderAggregateMarkdown( }; let limit = Number.POSITIVE_INFINITY; + let inlinedLimit = Number.POSITIVE_INFINITY; let withSummaries = true; - let body = buildBody(limit, withSummaries); + let body = buildBody(limit, inlinedLimit, withSummaries); // Agent summaries go before any finding: they never affect the decision, and each // scope's summary budget does not bound the sum across scopes. if (body.length > MAX_COMMENT_CHARS) { withSummaries = false; - body = buildBody(limit, withSummaries); + body = buildBody(limit, inlinedLimit, withSummaries); + } + // Inlined findings go next, down to 3: each trimmed one keeps its thread on the PR. + const largestInlined = Math.max( + 0, + ...perScope.map( + ({ result, kept }) => + kept.filter(({ finding }) => validInlineUrl(link, idOf(result, finding))).length, + ), + ); + while (body.length > MAX_COMMENT_CHARS && inlinedLimit > 3) { + inlinedLimit = + inlinedLimit === Number.POSITIVE_INFINITY + ? Math.max(3, Math.floor(largestInlined / 2)) + : Math.max(3, Math.floor(inlinedLimit / 2)); + body = buildBody(limit, inlinedLimit, withSummaries); } // Seed from the largest per-scope section the limit applies to — kept OR // requalified — so the halving loop shrinks whichever one is oversized. @@ -1208,7 +1270,7 @@ export function renderAggregateMarkdown( limit === Number.POSITIVE_INFINITY ? Math.max(3, Math.floor(largestScope / 2)) : Math.max(3, Math.floor(limit / 2)); - body = buildBody(limit, withSummaries); + body = buildBody(limit, inlinedLimit, withSummaries); } return body; } diff --git a/src/reporters/github.ts b/src/reporters/github.ts index 263921c..ba445fd 100644 --- a/src/reporters/github.ts +++ b/src/reporters/github.ts @@ -741,7 +741,8 @@ export class GitHubReporter implements Reporter { : await this.computeFeedback(withFp, priorRecords, pinsIn); const link = await this.linkContextAsync(); // Inline threads converge to the ACTIVE findings before the body renders, so - // the short form can link each one. A failed run (couldNotComplete) posts an + // the body links those findings by id instead of listing them. A failed run + // (couldNotComplete) posts an // empty findings list that proves nothing — additive-only, no teardown. const inlineUrls = await this.syncInline(activeFindings(withFp, dismissed, records), link, { teardown: review.couldNotComplete !== true, @@ -1735,7 +1736,61 @@ export class GitHubReporter implements Reporter { } const isAggregate = Array.isArray(state.scopes) && state.scopes.length > 0; const next = applyDismissalToState(state, add, remove, this.options.feedback, by, reason); - const link = await this.linkContextAsync(); + // @ref LLP 0008#inline-threads [implements] — /dismiss renders with the live thread links + // Our inline threads, read BEFORE the render: the main comment links a finding + // with a live thread instead of listing it again, like a review run does. A stub + // is no live thread. Fail-soft: without the threads, every finding renders in + // full, and a dismissal must never fail on the inline layer. + let own: ExistingInlineComment[] | null = null; + if (this.options.inline?.enabled) { + try { + own = await this.ownInlineComments(); + } catch (error) { + process.stderr.write( + `inline sync: could not list review comments (findings render in the main comment): ${errorMessage(error)}\n`, + ); + } + } + // Restore `/undismiss`ed threads BEFORE the render, so the main comment links a + // thread only once it shows the finding again. A restore that fails leaves a stub, + // and its finding renders in full. + const bodies = new Map((own ?? []).map((comment) => [comment.fp, comment.body])); + if (own) { + try { + const byFp = new Map(own.map((comment) => [comment.fp, comment])); + const findingById = stateFindingsById(state); + for (const fp of remove) { + const comment = byFp.get(fp); + const finding = findingById.get(fp); + // A finding truncated out of the state (or a thread that was deleted) just + // renders full-form in the main comment until the next review re-creates it. + if (!comment || !finding) { + continue; + } + const full = renderInlineCommentBody(finding, this.options.commentTag, fp); + if (!inlineBodiesEqual(comment.body, full)) { + await this.patchReviewComment(comment.id, full); + bodies.set(fp, full); + } + } + } catch (error) { + process.stderr.write( + `inline sync: thread restore skipped (findings render in the main comment): ${errorMessage(error)}\n`, + ); + } + } + const inlineUrls = new Map( + (own ?? []).flatMap((comment) => + comment.url && + !inlineBodiesEqual( + bodies.get(comment.fp) ?? comment.body, + inlineStubBody(this.options.commentTag, comment.fp), + ) + ? [[comment.fp, comment.url] as const] + : [], + ), + ); + const link = withInlineUrls(await this.linkContextAsync(), inlineUrls); const body = isAggregate ? renderAggregateMarkdown( state.scopes!, @@ -1762,11 +1817,9 @@ export class GitHubReporter implements Reporter { // (renderAggregateMarkdown trims kept findings to fit GitHub's body cap), so // "not in state" here does not mean "gone" and must not tear a thread down. // Fail-soft: a dismissal must never fail on the inline layer. - if (this.options.inline?.enabled) { + if (own) { try { - const own = await this.ownInlineComments(); const byFp = new Map(own.map((comment) => [comment.fp, comment])); - const findingById = stateFindingsById(state); for (const fp of next.matched) { const comment = byFp.get(fp); if (!comment) { @@ -1777,19 +1830,6 @@ export class GitHubReporter implements Reporter { await this.patchReviewComment(comment.id, stub); } } - for (const fp of remove) { - const comment = byFp.get(fp); - const finding = findingById.get(fp); - // A finding truncated out of the state (or a thread that was deleted) just - // renders full-form in the main comment until the next review re-creates it. - if (!comment || !finding) { - continue; - } - const full = renderInlineCommentBody(finding, this.options.commentTag, fp); - if (!inlineBodiesEqual(comment.body, full)) { - await this.patchReviewComment(comment.id, full); - } - } } catch (error) { process.stderr.write( `inline sync: dismissal update skipped (main comment already updated): ${errorMessage(error)}\n`, diff --git a/templates/config.jsonc b/templates/config.jsonc index 163dbd3..e1b394a 100644 --- a/templates/config.jsonc +++ b/templates/config.jsonc @@ -167,9 +167,9 @@ // Inline PR comments (ROOT-ONLY, off by default). When enabled, each finding // anchored to a line in the PR diff is posted as an inline review comment on - // that line (full rationale/suggestion there), and the main comment renders it - // short — title, links, a one-line rationale — with a link to the thread. The - // main comment stays the single source of truth (state, /dismiss, replies). + // that line (full rationale/suggestion there). The main comment does not list + // it again; it shows only its id, linked to the thread. The main comment stays + // the single source of truth (state, /dismiss, replies). // 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).