From 639aa65a25c60599721e9524bdd1bc22d9c4a1f4 Mon Sep 17 00:00:00 2001 From: Tomasz Sapeta Date: Sun, 25 Oct 2026 10:22:29 +0100 Subject: [PATCH 1/4] feat: stop repeating inline findings in the main comment --- PLAN-inline-comments.md | 25 +++++---- README.md | 11 ++-- src/__tests__/inline-comments.test.ts | 75 ++++++++++++++++----------- src/core/render.ts | 75 ++++++++++++--------------- src/reporters/github.ts | 2 +- templates/config.jsonc | 6 +-- 6 files changed, 101 insertions(+), 93 deletions(-) diff --git a/PLAN-inline-comments.md b/PLAN-inline-comments.md index f316507..38724ed 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 +counts the inlined findings by severity and links the most severe thread. 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 only counts it. - `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,14 @@ 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 count line - `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 and adds one line: `💬 **Inline comments on the diff (n):**`, + a count per severity, and a link to the most severe thread. Severity headings + count only the findings listed under them. Missing URL ⇒ full form (tested). - Embedded state unchanged. ### Inline replies → feedback @@ -211,14 +210,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 count line. - `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, count line + 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..59ad107 100644 --- a/README.md +++ b/README.md @@ -744,11 +744,12 @@ 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: it shows one `💬 Inline comments on the diff` line with a +count per severity and a link to the most severe thread. 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/src/__tests__/inline-comments.test.ts b/src/__tests__/inline-comments.test.ts index 08fc909..ad74c53 100644 --- a/src/__tests__/inline-comments.test.ts +++ b/src/__tests__/inline-comments.test.ts @@ -10,7 +10,6 @@ import type { ExistingInlineComment } from "../reporters/github.js"; import { inlineCommentMarker, inlineStubBody, - oneLineRationale, parseInlineMarkerFp, renderInlineCommentBody, renderMarkdown, @@ -87,29 +86,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 +307,16 @@ 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 ---- -test("renderMarkdown: an inlined finding renders short with the thread link; others render full", () => { +test("renderMarkdown: an inlined finding is only counted, with a link to its thread; 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], @@ -350,12 +331,46 @@ test("renderMarkdown: an inlined finding renders short with the thread link; oth ]), }; 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"); + expect(body).toContain( + "💬 **Inline comments on the diff (1):** ⚠️ Warning 1 · [first thread](https://github.com/o/r/pull/1#discussion_r123)", + ); + // 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 count line", () => { + const warning = finding(); + const nit = finding({ severity: "nit", file: "c.ts", title: "Nit title" }); + const review: CoordinatorOutput = { + decision: "approve_with_comments", + findings: [nit, warning], + summary: "s", + incomplete: [], + }; + const link: LinkContext = { + repo: "o/r", + prNumber: 1, + inlineUrls: new Map([ + [fingerprintFinding(nit), "https://github.com/o/r/pull/1#discussion_r2"], + [fingerprintFinding(warning), "https://github.com/o/r/pull/1#discussion_r1"], + ]), + }; + const body = renderMarkdown(review, TAG, [], link); + expect(body).not.toContain("###"); + expect(body).not.toContain("No findings."); + // Most severe first, and the link goes to the most severe thread. + expect(body).toContain( + "💬 **Inline comments on the diff (2):** ⚠️ Warning 1 · 🐧 Nit 1 · [first thread](https://github.com/o/r/pull/1#discussion_r1)", + ); }); test("renderMarkdown: a non-github inline URL is ignored (full form)", () => { diff --git a/src/core/render.ts b/src/core/render.ts index f6abcd2..031dcbd 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 only counted, 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,16 @@ function renderSeveritySections( feedbackById?: Map, ): string[] { const out: string[] = []; - const groups = groupBySeverity(sortFindings(findings)); + const sorted = sortFindings(findings); + // A finding with a live inline thread is read there, on the flagged line, so the + // list does not repeat it. It stays in the embedded state, so `/dismiss` and reply + // matching still find it. + const inlined = sorted.flatMap((finding) => { + const url = validInlineUrl(link, idFor(finding)); + return url ? [{ finding, url }] : []; + }); + const listed = sorted.filter((finding) => !validInlineUrl(link, idFor(finding))); + const groups = groupBySeverity(listed); for (const severity of SEVERITIES) { const group = groups[severity]; if (group.length === 0) { @@ -422,9 +431,29 @@ function renderSeveritySections( } out.push(""); } + out.push(...inlinedCountLines(inlined)); return out; } +/** + * One line that counts the findings posted as inline threads, by severity, and links + * the most severe thread. `inlined` is sorted, so the first entry is that thread. + */ +function inlinedCountLines(inlined: { finding: Finding; url: string }[]): string[] { + const first = inlined[0]; + if (!first) { + return []; + } + const groups = groupBySeverity(inlined.map((entry) => entry.finding)); + const counts = SEVERITIES.filter((severity) => groups[severity].length > 0).map( + (severity) => `${severityHeading(severity)} ${groups[severity].length}`, + ); + return [ + `💬 **Inline comments on the diff (${inlined.length}):** ${[...counts, `[first thread](${first.url})`].join(" · ")}`, + "", + ]; +} + // @ref LLP 0005#comment-rendering [constrained-by] — blank lines must stay truly empty or
escapes the list (euxy#45) /** * Indent every line of a multi-line value to a list item's content column. @@ -450,9 +479,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 +487,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 +499,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 +525,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 only counts this finding. * 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. */ diff --git a/src/reporters/github.ts b/src/reporters/github.ts index 263921c..1b4e5c8 100644 --- a/src/reporters/github.ts +++ b/src/reporters/github.ts @@ -741,7 +741,7 @@ 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 knows which findings to count instead of list. 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, diff --git a/templates/config.jsonc b/templates/config.jsonc index 163dbd3..7f4350f 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 one line that counts the inline comments and links the + // most severe 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). From 8c5a8d3217231051a8218569b68ffde033ad39d3 Mon Sep 17 00:00:00 2001 From: Tomasz Sapeta Date: Sun, 25 Oct 2026 10:45:27 +0100 Subject: [PATCH 2/4] fix: keep inlined finding ids in the main comment, and cover dismissals and truncation --- PLAN-inline-comments.md | 13 +- README.md | 5 +- llp/0008-sources-and-reporters.explainer.md | 2 + src/__tests__/inline-comments.test.ts | 174 ++++++++++++++++++-- src/__tests__/inline-review.test.ts | 72 +++++++- src/config/schema.ts | 4 +- src/core/render.ts | 120 +++++++++----- src/reporters/github.ts | 30 +++- templates/config.jsonc | 3 +- 9 files changed, 352 insertions(+), 71 deletions(-) diff --git a/PLAN-inline-comments.md b/PLAN-inline-comments.md index 38724ed..f8d7288 100644 --- a/PLAN-inline-comments.md +++ b/PLAN-inline-comments.md @@ -4,7 +4,7 @@ Findings anchored to a line in the PR diff land as inline review comments on that line. The main issue comment does not list an inlined finding again: it -counts the inlined findings by severity and links the most severe thread. The main +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. @@ -172,9 +172,10 @@ main comments are cleared today. - `LinkContext.inlineUrls?: Map`. NEVER mutate the shared routed-flow `link` object — render with a copy `{ ...link, inlineUrls }`. - `renderSeveritySections` leaves every finding with an inline URL out of the - severity lists and adds one line: `💬 **Inline comments on the diff (n):**`, - a count per severity, and a link to the most severe thread. Severity headings - count only the findings listed under them. Missing URL ⇒ full form (tested). + 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 @@ -210,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`, main-comment count line. + 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, count line + 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 59ad107..18b0ece 100644 --- a/README.md +++ b/README.md @@ -745,8 +745,9 @@ 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 does not list an -inlined finding again: it shows one `💬 Inline comments on the diff` line with a -count per severity and a link to the most severe thread. Findings that are not +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. diff --git a/llp/0008-sources-and-reporters.explainer.md b/llp/0008-sources-and-reporters.explainer.md index 6eb87a1..5153bc1 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 never brings the full list back [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, so dismissals and reply matching do not change. In the aggregate comment, the size cap trims only listed findings: an inlined one costs one link, `maxComments` bounds how many there are, and cutting the combined list would hide listed findings, which appear nowhere else (`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 ad74c53..c6a9ca0 100644 --- a/src/__tests__/inline-comments.test.ts +++ b/src/__tests__/inline-comments.test.ts @@ -12,12 +12,13 @@ import { inlineStubBody, parseInlineMarkerFp, renderInlineCommentBody, + 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 => ({ @@ -309,7 +310,9 @@ test("activeFindings: drops dismissed, reply-cleared, and requalified findings", // ---- inlined findings leave the finding list ---- -test("renderMarkdown: an inlined finding is only counted, with a link to its thread; others render full", () => { +const threadUrl = (n: number): string => `https://github.com/o/r/pull/1#discussion_r${n}`; + +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", @@ -326,17 +329,17 @@ test("renderMarkdown: an inlined finding is only counted, with a link to its thr 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); // 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( - "💬 **Inline comments on the diff (1):** ⚠️ Warning 1 · [first thread](https://github.com/o/r/pull/1#discussion_r123)", + `- ⚠️ Warning: [\`id:${fingerprintFinding(inlined)}\`](${threadUrl(123)})`, ); // The heading counts only the findings listed under it. expect(body).toContain("### ⚠️ Warning (1)"); @@ -347,12 +350,13 @@ test("renderMarkdown: an inlined finding is only counted, with a link to its thr expect(body).toContain(`"${fingerprintFinding(inlined)}"`); }); -test("renderMarkdown: when every finding is inlined, the list is only the count line", () => { +test("renderMarkdown: when every finding is inlined, the list is only the inline rows", () => { const warning = finding(); - const nit = finding({ severity: "nit", file: "c.ts", title: "Nit title" }); + 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: [nit, warning], + findings: [nitA, warning, nitB], summary: "s", incomplete: [], }; @@ -360,17 +364,161 @@ test("renderMarkdown: when every finding is inlined, the list is only the count repo: "o/r", prNumber: 1, inlineUrls: new Map([ - [fingerprintFinding(nit), "https://github.com/o/r/pull/1#discussion_r2"], - [fingerprintFinding(warning), "https://github.com/o/r/pull/1#discussion_r1"], + [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."); - // Most severe first, and the link goes to the most severe thread. + 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( - "💬 **Inline comments on the diff (2):** ⚠️ Warning 1 · 🐧 Nit 1 · [first thread](https://github.com/o/r/pull/1#discussion_r1)", + `[\`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)); +}); + +test("renderAggregateMarkdown: truncation trims listed findings only, never for inlined 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(6_000), + }), + ); + const link: LinkContext = { + repo: "o/r", + prNumber: 1, + inlineUrls: new Map(inlined.map((f, index) => [fingerprintFinding(f), threadUrl(100 + index)])), + }; + const body = renderAggregateMarkdown( + [ + { + scope: "root", + isDefault: true, + review: { + decision: "approve_with_comments", + findings: [...inlined, ...listed], + summary: "s", + incomplete: [], + }, + }, + ], + TAG, + [], + link, ); + // Every inlined thread is still linked. + expect(body).toContain("💬 **Inline comments on the diff (30)**"); + // Some listed nits are shown, 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)`); }); 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..faf05e9 100644 --- a/src/__tests__/inline-review.test.ts +++ b/src/__tests__/inline-review.test.ts @@ -4,9 +4,10 @@ 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, renderInlineCommentBody, renderMarkdown } from "../core/render.js"; import type { LinkContext } from "../core/render.js"; -import type { Finding } from "../core/schema.js"; +import { fingerprintFinding } 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 +47,8 @@ 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. const FAKE_GH = `#!/bin/sh printf '%s\\n' "$*" >> "$FAKE_GH_DIR/calls.log" eval "last=\\\${$#}" @@ -75,6 +77,11 @@ 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 [ -f "$FAKE_GH_DIR/pr-comments.json" ]; then cat "$FAKE_GH_DIR/pr-comments.json"; else printf '[]'; fi ;; *"-X PATCH "*) @@ -431,3 +438,62 @@ 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/7#discussion_r81", + }, + { + id: 82, + body: inlineStubBody(TAG, stubbedFp!), + user: { login: "ecr-bot" }, + html_url: "https://github.com/o/r/pull/7#discussion_r82", + }, + ]), + "utf8", + ); + const reporter = new GitHubReporter({ + prNumber: 7, + repo: "o/r", + commentTag: TAG, + ownLogin: "ecr-bot", + headSha: HEAD, + inline: { enabled: true, maxComments: 20 }, + linkContext: { repo: "o/r", prNumber: 7 }, + }); + 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/7#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)"); +}); 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 031dcbd..325426e 100644 --- a/src/core/render.ts +++ b/src/core/render.ts @@ -409,15 +409,7 @@ function renderSeveritySections( feedbackById?: Map, ): string[] { const out: string[] = []; - const sorted = sortFindings(findings); - // A finding with a live inline thread is read there, on the flagged line, so the - // list does not repeat it. It stays in the embedded state, so `/dismiss` and reply - // matching still find it. - const inlined = sorted.flatMap((finding) => { - const url = validInlineUrl(link, idFor(finding)); - return url ? [{ finding, url }] : []; - }); - const listed = sorted.filter((finding) => !validInlineUrl(link, idFor(finding))); + const { inlined, listed } = splitInlined(sortFindings(findings), link, idFor); const groups = groupBySeverity(listed); for (const severity of SEVERITIES) { const group = groups[severity]; @@ -431,27 +423,63 @@ function renderSeveritySections( } out.push(""); } - out.push(...inlinedCountLines(inlined)); + 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 /** - * One line that counts the findings posted as inline threads, by severity, and links - * the most severe thread. `inlined` is sorted, so the first entry is that thread. + * 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 inlinedCountLines(inlined: { finding: Finding; url: string }[]): string[] { - const first = inlined[0]; - if (!first) { +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 groups = groupBySeverity(inlined.map((entry) => entry.finding)); - const counts = SEVERITIES.filter((severity) => groups[severity].length > 0).map( - (severity) => `${severityHeading(severity)} ${groups[severity].length}`, - ); - return [ - `💬 **Inline comments on the diff (${inlined.length}):** ${[...counts, `[first thread](${first.url})`].join(" · ")}`, - "", - ]; + 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; } // @ref LLP 0005#comment-rendering [constrained-by] — blank lines must stay truly empty or
escapes the list (euxy#45) @@ -1000,25 +1028,37 @@ 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 listed findings only + // Shown = every inlined kept finding plus the most-severe N listed ones per scope + // (N = limitPerScope). The limit applies to listed findings only: an inlined + // finding renders as one short id link, and the inline cap (`maxComments`) + // already bounds how many there are. Cutting the combined list would drop listed + // findings for inlined ones, and a listed finding has nowhere else to appear. + // 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.map((entry) => entry.finding), ...listed.slice(0, limitPerScope)], + hidden: Math.max(0, listed.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, + }; + }); for (const { result, diff --git a/src/reporters/github.ts b/src/reporters/github.ts index 1b4e5c8..78750d3 100644 --- a/src/reporters/github.ts +++ b/src/reporters/github.ts @@ -1735,7 +1735,32 @@ 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, unless this action restores it below. Fail-soft: without + // the threads, every finding renders in full. + 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`, + ); + } + } + const restored = new Set(remove); + const inlineUrls = new Map( + (own ?? []).flatMap((comment) => + comment.url && + (restored.has(comment.fp) || + !inlineBodiesEqual(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,9 +1787,8 @@ 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) { diff --git a/templates/config.jsonc b/templates/config.jsonc index 7f4350f..f3a6fa3 100644 --- a/templates/config.jsonc +++ b/templates/config.jsonc @@ -168,8 +168,7 @@ // 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). The main comment does not list - // it again; it shows one line that counts the inline comments and links the - // most severe thread. The main comment stays the single source of truth (state, /dismiss, replies). + // 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). From 6e67ccf8d14ea8739a40770f8a5daa3128d96bce Mon Sep 17 00:00:00 2001 From: Tomasz Sapeta Date: Sun, 25 Oct 2026 11:44:25 +0100 Subject: [PATCH 3/4] fix: trim inlined findings first when the aggregate comment is too large --- llp/0008-sources-and-reporters.explainer.md | 2 +- src/__tests__/inline-comments.test.ts | 65 ++++++++++++++------- src/core/render.ts | 52 ++++++++++++----- 3 files changed, 81 insertions(+), 38 deletions(-) diff --git a/llp/0008-sources-and-reporters.explainer.md b/llp/0008-sources-and-reporters.explainer.md index 5153bc1..36f6a74 100644 --- a/llp/0008-sources-and-reporters.explainer.md +++ b/llp/0008-sources-and-reporters.explainer.md @@ -91,7 +91,7 @@ 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 never brings the full list back [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, so dismissals and reply matching do not change. In the aggregate comment, the size cap trims only listed findings: an inlined one costs one link, `maxComments` bounds how many there are, and cutting the combined list would hide listed findings, which appear nowhere else (`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. +**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 never brings the full list back [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, 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 down to none: 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 (`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 diff --git a/src/__tests__/inline-comments.test.ts b/src/__tests__/inline-comments.test.ts index c6a9ca0..7e5d4c6 100644 --- a/src/__tests__/inline-comments.test.ts +++ b/src/__tests__/inline-comments.test.ts @@ -476,7 +476,20 @@ test("renderAggregateMarkdown: a scope whose findings are all inlined stays open expect(body).toContain(threadUrl(6)); }); -test("renderAggregateMarkdown: truncation trims listed findings only, never for inlined ones", () => { +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) => @@ -487,7 +500,7 @@ test("renderAggregateMarkdown: truncation trims listed findings only, never for severity: "nit", file: `n${index}.ts`, title: `Listed nit ${index}`, - rationale: "x".repeat(6_000), + rationale: "x".repeat(3_000), }), ); const link: LinkContext = { @@ -495,30 +508,38 @@ test("renderAggregateMarkdown: truncation trims listed findings only, never for prNumber: 1, inlineUrls: new Map(inlined.map((f, index) => [fingerprintFinding(f), threadUrl(100 + index)])), }; - const body = renderAggregateMarkdown( - [ - { - scope: "root", - isDefault: true, - review: { - decision: "approve_with_comments", - findings: [...inlined, ...listed], - summary: "s", - incomplete: [], - }, - }, - ], - TAG, - [], - link, - ); - // Every inlined thread is still linked. - expect(body).toContain("💬 **Inline comments on the diff (30)**"); - // Some listed nits are shown, and the note counts only the listed ones left out. + const body = renderAggregateMarkdown(rootScope([...inlined, ...listed]), TAG, [], link); + expect(body.length).toBeLessThanOrEqual(60_000); + // Every inlined finding went first: their threads stay on the PR. + expect(body).not.toContain("💬 **Inline comments on the diff"); + expect(body).toContain("…and 30 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("renderMarkdown: a non-github inline URL is ignored (full form)", () => { diff --git a/src/core/render.ts b/src/core/render.ts index 325426e..a933c4c 100644 --- a/src/core/render.ts +++ b/src/core/render.ts @@ -983,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", @@ -1028,13 +1032,13 @@ export function renderAggregateMarkdown( lines.push("> 🔗 **Review setup:**", ...setupLines, ""); } - // @ref LLP 0008#inline-threads [implements] — the size cap trims listed findings only - // Shown = every inlined kept finding plus the most-severe N listed ones per scope - // (N = limitPerScope). The limit applies to listed findings only: an inlined - // finding renders as one short id link, and the inline cap (`maxComments`) - // already bounds how many there are. Cutting the combined list would drop listed - // findings for inlined ones, and a listed finding has nowhere else to appear. - // 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. 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 }) => { @@ -1046,8 +1050,12 @@ export function renderAggregateMarkdown( return { result, replies, - shown: [...inlined.map((entry) => entry.finding), ...listed.slice(0, limitPerScope)], + 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 @@ -1065,6 +1073,7 @@ export function renderAggregateMarkdown( replies, shown, hidden, + inlinedHidden, requalified, requalifiedAll, requalifiedHidden, @@ -1078,7 +1087,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})`, @@ -1105,6 +1114,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})**`, @@ -1151,7 +1163,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 @@ -1172,7 +1184,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 } : {} @@ -1222,13 +1234,23 @@ 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 none: each one keeps its thread on the PR. + const largestInlined = Math.max(0, ...perScope.map((entry) => entry.kept.length)); + while (body.length > MAX_COMMENT_CHARS && inlinedLimit > 0) { + inlinedLimit = + inlinedLimit === Number.POSITIVE_INFINITY + ? Math.floor(largestInlined / 2) + : 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. @@ -1241,7 +1263,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; } From 2eccf266e652d1e4fdbf036548f0201b651811f4 Mon Sep 17 00:00:00 2001 From: Tomasz Sapeta Date: Sun, 25 Oct 2026 15:07:52 +0100 Subject: [PATCH 4/4] fix: keep 3 inlined findings per scope, and restore threads before the /undismiss render --- PLAN-inline-comments.md | 4 +- llp/0008-sources-and-reporters.explainer.md | 2 +- src/__tests__/inline-comments.test.ts | 55 ++++++- src/__tests__/inline-review.test.ts | 156 +++++++++++++++++++- src/core/render.ts | 23 ++- src/reporters/github.ts | 56 ++++--- templates/config.jsonc | 3 +- 7 files changed, 257 insertions(+), 42 deletions(-) diff --git a/PLAN-inline-comments.md b/PLAN-inline-comments.md index f8d7288..18ca854 100644 --- a/PLAN-inline-comments.md +++ b/PLAN-inline-comments.md @@ -18,7 +18,7 @@ presentation layer, converged on every run. - No GraphQL thread resolution, and no detection of author-resolved/outdated 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 only counts it. + 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. @@ -167,7 +167,7 @@ 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 count line +### Main-comment id rows - `LinkContext.inlineUrls?: Map`. NEVER mutate the shared routed-flow `link` object — render with a copy `{ ...link, inlineUrls }`. diff --git a/llp/0008-sources-and-reporters.explainer.md b/llp/0008-sources-and-reporters.explainer.md index 36f6a74..f0c2550 100644 --- a/llp/0008-sources-and-reporters.explainer.md +++ b/llp/0008-sources-and-reporters.explainer.md @@ -91,7 +91,7 @@ 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 never brings the full list back [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, 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 down to none: 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 (`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. +**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 diff --git a/src/__tests__/inline-comments.test.ts b/src/__tests__/inline-comments.test.ts index 7e5d4c6..4c21417 100644 --- a/src/__tests__/inline-comments.test.ts +++ b/src/__tests__/inline-comments.test.ts @@ -12,6 +12,7 @@ import { inlineStubBody, parseInlineMarkerFp, renderInlineCommentBody, + parseReviewState, renderAggregateMarkdown, renderMarkdown, } from "../core/render.js"; @@ -510,9 +511,9 @@ test("renderAggregateMarkdown: truncation trims inlined findings before listed o }; const body = renderAggregateMarkdown(rootScope([...inlined, ...listed]), TAG, [], link); expect(body.length).toBeLessThanOrEqual(60_000); - // Every inlined finding went first: their threads stay on the PR. - expect(body).not.toContain("💬 **Inline comments on the diff"); - expect(body).toContain("…and 30 more inline comment(s) on the diff."); + // 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); @@ -542,6 +543,54 @@ test("renderAggregateMarkdown: long inlined findings are trimmed so listed ones 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)", () => { const f = finding({ suggestion: "s1" }); const review: CoordinatorOutput = { diff --git a/src/__tests__/inline-review.test.ts b/src/__tests__/inline-review.test.ts index faf05e9..64e186e 100644 --- a/src/__tests__/inline-review.test.ts +++ b/src/__tests__/inline-review.test.ts @@ -4,9 +4,14 @@ import { tmpdir } from "node:os"; import path, { delimiter } from "node:path"; import { resetTrustedToolCache } from "../core/exec.js"; -import { inlineStubBody, renderInlineCommentBody, renderMarkdown } from "../core/render.js"; +import { + inlineStubBody, + renderAggregateMarkdown, + renderInlineCommentBody, + renderMarkdown, +} from "../core/render.js"; import type { LinkContext } from "../core/render.js"; -import { fingerprintFinding } 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"; @@ -49,6 +54,7 @@ test("inlineUrlsByFp: maps our marked comments to their permalinks", () => { // comments are served from review-comments-.json, and the PR's existing review // 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=\\\${$#}" @@ -83,6 +89,7 @@ case "$*" in 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 @@ -112,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]; @@ -127,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(); }); @@ -464,32 +473,32 @@ test("applyDismissal: the re-rendered comment links findings with live threads, id: 81, body: renderInlineCommentBody(live, TAG, liveFp!), user: { login: "ecr-bot" }, - html_url: "https://github.com/o/r/pull/7#discussion_r81", + 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/7#discussion_r82", + html_url: "https://github.com/o/r/pull/801#discussion_r82", }, ]), "utf8", ); const reporter = new GitHubReporter({ - prNumber: 7, + prNumber: 801, repo: "o/r", commentTag: TAG, ownLogin: "ecr-bot", headSha: HEAD, inline: { enabled: true, maxComments: 20 }, - linkContext: { repo: "o/r", prNumber: 7 }, + 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/7#discussion_r81)`); + 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**"); @@ -497,3 +506,136 @@ test("applyDismissal: the re-rendered comment links findings with live threads, // 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/core/render.ts b/src/core/render.ts index a933c4c..a650824 100644 --- a/src/core/render.ts +++ b/src/core/render.ts @@ -44,7 +44,7 @@ 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 is left out of the finding list and only counted, because + * 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 @@ -553,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 counts this finding. + * 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. */ @@ -1038,7 +1038,8 @@ export function renderAggregateMarkdown( // `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. The embedded state trims KEPT findings to the same set so a truncated comment + // 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 }) => { @@ -1243,13 +1244,19 @@ export function renderAggregateMarkdown( withSummaries = false; body = buildBody(limit, inlinedLimit, withSummaries); } - // Inlined findings go next, down to none: each one keeps its thread on the PR. - const largestInlined = Math.max(0, ...perScope.map((entry) => entry.kept.length)); - while (body.length > MAX_COMMENT_CHARS && inlinedLimit > 0) { + // 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.floor(largestInlined / 2) - : Math.floor(inlinedLimit / 2); + ? 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 diff --git a/src/reporters/github.ts b/src/reporters/github.ts index 78750d3..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 body knows which findings to count instead of list. 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, @@ -1738,8 +1739,8 @@ export class GitHubReporter implements Reporter { // @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, unless this action restores it below. Fail-soft: without - // the threads, every finding renders in full. + // 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 { @@ -1750,12 +1751,41 @@ export class GitHubReporter implements Reporter { ); } } - const restored = new Set(remove); + // 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 && - (restored.has(comment.fp) || - !inlineBodiesEqual(comment.body, inlineStubBody(this.options.commentTag, comment.fp))) + !inlineBodiesEqual( + bodies.get(comment.fp) ?? comment.body, + inlineStubBody(this.options.commentTag, comment.fp), + ) ? [[comment.fp, comment.url] as const] : [], ), @@ -1790,7 +1820,6 @@ export class GitHubReporter implements Reporter { if (own) { try { 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) { @@ -1801,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 f3a6fa3..e1b394a 100644 --- a/templates/config.jsonc +++ b/templates/config.jsonc @@ -168,7 +168,8 @@ // 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). 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). + // 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).