Skip to content

ci: collapse the whole review comment behind one summary line - #2957

Merged
rafaellehmkuhl merged 1 commit into
bluerobotics:masterfrom
rafaellehmkuhl:review-bot-collapse-whole-comment
Aug 20, 2026
Merged

ci: collapse the whole review comment behind one summary line#2957
rafaellehmkuhl merged 1 commit into
bluerobotics:masterfrom
rafaellehmkuhl:review-bot-collapse-whole-comment

Conversation

@rafaellehmkuhl

@rafaellehmkuhl rafaellehmkuhl commented Aug 19, 2026

Copy link
Copy Markdown
Member

The automated review comment runs to several screens, and a PR that goes several rounds with it turns into a conversation nobody can scroll. This puts the whole comment inside one outer <details> whose summary carries the verdict and the round it belongs to, so each round costs one line in the timeline until someone opens it. The sections below keep their order and their contents: opening the block gives the document we have today, human summary at the top and every section still collapsed inside it. Only the title and the verdict move onto the summary line.

The verdict leads that line and is written nowhere else. The body opens with the finding counts instead, as a subtitle to it, so nothing has to be kept in step: a maintainer scrolling past gets the one thing they are scrolling for, and the counts are there for whoever opens the block.

Two platform constraints shaped the rest of the diff, both verified rather than assumed:

  • An alert cannot be nested. GitHub's docs state it outright, and github/markup#1753 confirms it is intentional. > [!WARNING] inside a <details> publishes as a plain blockquote with a literal [!WARNING] line in it, so the coloured callout could not survive the wrapping wherever the verdict ended up. On the summary line it needs no callout: the emoji and the bold text carry what the colour used to.
  • <summary> processes no markdown. Rendering **bold**, _em_, backtick code and a link inside a <summary> through the GFM API returns all four verbatim, which is why the verdict table's one column is HTML tags. Emoji shortcodes are the exception — they are substituted after the markup is rendered, so :warning: does resolve there — but a literal emoji needs no substitution in either context, so that is what the column uses.

The marker and the ledger stay outside the block. Neither is rendered either way, and the ledger has to remain the last thing in the body for review-ledger.sh, which takes the last matching block.

Only the guidelines file changes — the three workflows delegate the output contract to it and never parse the rendered structure.

Test plan

  • Rendered the full new comment shape through POST /markdown (mode: gfm): the summary line comes back as 📝 <strong>MINOR SUGGESTIONS</strong> (Automated PR Review — round 3), and the counts blockquote, the table and the nested <details> inside the wrapper all render normally.
  • Rendered the summary-line verdict placeholder unsubstituted, in both bracket styles: [the verdict, …] publishes visibly at the head of the line, while <the verdict, …> is parsed as a tag and dropped, leaving the round in parentheses with nothing in front of it — which is the reason the contract writes that one placeholder in square brackets.
  • Task list items inside a <details> come back with the same contains-task-list / task-list-item markup they get at top level, so the human-decision checkboxes stay tickable once the block moves inside the wrapper.
  • review-ledger.sh extracts the ledger from that body unchanged, with the wrapper's </details> between the footer and the ledger.
  • All five .github/scripts self-checks pass, review-marker.sh included; line 1 is untouched by this change.
  • First real review published under the new contract looks right in the timeline.

@rafaellehmkuhl

Copy link
Copy Markdown
Member Author

@ES-Alexander this implements what we were talking about before.

@github-actions

Copy link
Copy Markdown

Automated PR Review — round 1

Note

📝 MINOR SUGGESTIONS — 4 open: 3 minor and 1 nit.

This changes the instruction document the automated reviewer follows, so that every review comment it publishes is wrapped in one collapsible block. Until someone opens it, a reader sees a single line carrying the round number and the verdict word. Because the platform will not draw its coloured callout inside a collapsible block, the verdict is now written twice — once on that visible line, and once as ordinary quoted text at the top of the hidden body. Nothing about how a review is investigated, or what it must contain, changes.

Note on this comment: the diff redefines the output format of the run that produced it. Per the rule in the checked-out guidelines, a format that exists only in the diff has no authority over this run, so this review was written against the base contract and still uses the top-level alert and the ## heading. That is expected, not a defect in the PR.

What still needs attention

# Problem What it means Severity Status
1.1 Verdict written twice, nothing checks the copies If the review's one visible line ever disagrees with the verdict inside it — or silently comes out empty — a maintainer can merge a pull request the review meant to block. minor
1.2 Spec says the body is unchanged when it is not A sentence in these instructions claims the review body is untouched, so whoever edits them next can undo part of this change by accident. minor
1.3 Human-decision tick boxes move inside the folded block The boxes the review uses to ask a maintainer to decide a contested point are now hidden behind a click and may no longer be clickable, so the question can go unanswered. minor
11.1 Doubtful reason given for the emoji choice One sentence explaining why the visible line uses a picture character rather than a text code is probably wrong, which could mislead the next editor. nit
Change map — what was established before judging

The PR touches one file, .github/claude-review/review-guidelines.md (+51/−35, one commit). No TypeScript, Vue, workflow YAML or shell script changes. complexity-report.json reports changedFiles: 0, functionsMeasured: 0, truncated: false, consistent with a markdown-only diff.

Claims (from the PR body / commit message):

  1. "A review runs to several screens, and a PR that goes ten rounds with it is a conversation nobody can scroll."verified. claude-pr-review-continue.yml:397 states "Always a new comment, never an update — the history of re-reviews is the point", and :440 posts with gh pr comment. Only seq=1 is edited in place (claude-pr-review.yml:165-172). So rounds do stack full-length comments, and in-place editing is a deliberately rejected alternative — I am not proposing it as the smaller fix.
  2. "GitHub does not render an alert nested inside another element."consistent with the platform's documented limitation that alerts cannot be nested within other elements, but not verifiable from here: there is no network access, and there is no in-tree precedent either way (a repo-wide grep finds <details> only inside this guidelines file's own contract — :343, :349, :355, :364 — and the alert has always sat at top level). Recorded as unverified-in-tree rather than confirmed.
  3. "<summary> is an HTML block and processes no markdown, hence the two forms."partly. It holds for **bold**; the same claim about :warning: is doubtful (11.1). The form the PR actually chose — HTML tags plus a literal emoji — is correct either way, so the conclusion stands even where the reason does not.
  4. "The marker and the ledger stay outside the block … the ledger has to stay the last thing in the body for the extractor that reads it back."verified against the extractor. .github/scripts/review-ledger.sh:19-31 keeps the last line matching ^<!-- claude-pr-review-ledger and prints to the first ^-->; the new template (head:386-390) puts </details> before that block, so the ledger is still the last such line and extraction is unaffected. review-marker.sh:21-27 only inspects and rewrites line 1, which the new template still reserves for the marker, with <details> at line 2 — an HTML comment block ends on the line containing -->, so line 2 opens a fresh block.
  5. "the document inside is unchanged" / "Nothing changes below it"contradicted by this same diff (1.2): the ## Automated PR Review — round $SEQ heading is deleted (head:338-339) and the verdict changes from an alert to a plain blockquote (head:165-170).

Entry points — no function changed, so the rows are the consumers of the changed document and of the shape it specifies. None is never; the document is read on every review run.

Function Reached from Frequency
the guidelines document itself agent step of claude-pr-review.yml:90 (PR opened / ready_for_review) and claude-pr-review-continue.yml:326 (/review comment) per user action
the published comment shape (contract, head:335-343) written by the agent, published by claude-pr-review.yml:146-172 and claude-pr-review-continue.yml:399-440 per user action (once per review round)
review-marker.sh:apply publish step of both review workflows per review round
review-ledger.sh:extract publish step of both workflows, plus claude-pr-review-continue.yml:230 reading the previous round per review round
marker parse in claude-pr-review-current.yml:40-44 pull_request_target opened/synchronize/reopened per push

Searched for every other machine reader of the comment body before judging: claude-pr-review.yml:167, claude-pr-review-current.yml:42, claude-pr-review-continue.yml:210, :249, :265-266. All of them match on the line-1 marker prefix or on the resolution marker; none parses the ## heading, the verdict text, or any <details> boundary. review-command.sh reads only the first line of a human comment. So no automated consumer breaks under the new layout.

Invariants the change relies on:

  • The marker is line 1 of the published body. Violable only by the model; the new template keeps it there, and review-marker.sh repairs it regardless. Covered.
  • The ledger block is the last ^<!-- claude-pr-review-ledger line. Violators are the review's own prose (the case review-ledger.sh:9-12 documents) and now the trailing </details>; the PR places the ledger after it, so it stays last. Covered.
  • Blank line after every </summary> and before every closing </details> (:123) now has to hold at two nesting levels. The contract template shows the blank lines at both levels, so the rule is stated at the only site that can break it. Covered.
  • New: the verdict on the <summary> line equals the verdict in the body. The only site that can violate it is the model, and nothing enforces it — 1.1.
1. Correctness & Implementation Bugs — 3 findings

1.1 — The verdict is duplicated with nothing checking the two copies, and the visible copy can vanish silentlyminor

review-guidelines.md head:158-160 now requires the verdict to be emitted twice, and head:180-181 names the failure this creates ("a summary line that disagrees with the body line is the one failure this duplication can produce") without closing it. Two things follow from putting the load-bearing copy inside HTML:

  • Nothing verifies agreement. The publish steps already machine-enforce every other load-bearing line of the body: review-marker.sh:19-27 rewrites line 1 rather than trusting the model, and review-ledger.sh is run at publish time (claude-pr-review.yml:160, claude-pr-review-continue.yml:416) precisely so a review cannot be published in a shape the next run cannot read. The verdict is equally derivable by machine — it follows mechanically from the open severities in the ledger the same step already parses — so the same treatment is available and cheap: compute it from the ledger, compare it against the <summary> line, and warn or rewrite.
  • The contract's <summary> line (head:339) is <summary><strong>Automated PR Review — round $SEQ</strong> — <the verdict, in the summary-line form from section 0's table></summary>. Every other placeholder in the contract sits in markdown text, where leaving it unsubstituted is visible as literal angle-bracketed prose. This one sits inside an HTML element, so an unsubstituted <the verdict, …> is parsed as an unknown tag and dropped: the reader gets the title, a dangling em dash, and no verdict at all, with nothing in the logs. If the placeholder is kept, it is worth saying in the contract that this one must not survive; a check on the summary line covers both halves of this finding at once.

1.2 — "Nothing changes below it" is contradicted by the same diffminor

head:122 says "Nothing changes below it: opening the outer block gives the same document as before, with the human summary at the top and every other block still collapsed inside it." Two hunks later the document below the block does change: the ## Automated PR Review — round $SEQ heading is removed (head:338-339, replacing it with <details>/<summary>), and the verdict stops being a GitHub alert and becomes a plain blockquote (head:165-170, head:341). This file is a specification whose consumer is a model re-reading it every run, and whose next editor will read that bullet as the summary of the change; a reader who trusts it can reinstate the heading or the alert believing neither moved. Narrow the claim to what is true — the sections, their order and their contents are unchanged, while the title and the verdict move onto the summary line.

1.3 — The human-decision checkboxes move inside an HTML block, and the PR does not say what that does to themminor

The ### :raising_hand: Decisions for a human block (base:185-201, unchanged by this diff) renders - [ ] task items and states at base:199 that "Ticking a box records the maintainers' decision where the next reader can see it". Today that block is top-level markdown in the comment; after this PR it sits inside the outer <details>. The PR reasons carefully about two platform nesting limits (alerts at head:176-177, markdown in <summary> at head:172-175) but says nothing about the one interactive element it moves inside the block, and there is no in-tree precedent to check it against — no <details> exists anywhere under .github/ except this file's own contract. Task-list interactivity inside an HTML block is exactly the kind of behaviour that does not survive the move, and it is also the block that most needs to be seen, since it is the one asking a human to act.

Two ways to close it: verify the checkboxes are still clickable on a scratch comment and say so where the alert limitation is already documented (head:172-178), or keep this block outside the outer <details> alongside the marker. It appears only when a finding is disputed, so exempting it costs the collapsing goal almost nothing. Either way the fallback still works — /resolve is what actually closes a disputed finding (base:200) — which is why this is minor rather than higher.

11. Nitpicks / Optional — 1 finding

11.1 — The stated reason for the literal emoji is doubtfulnit

head:174 says that inside <summary> "**bold** and :warning: there publish as those literal characters". The first half is right; the second is a different mechanism — emoji shortcodes are substituted by a post-render filter over the document's text nodes, not by the markdown parser, so :warning: inside <summary> plausibly does render. The decision the PR made is correct regardless, since a literal ⚠️ works in both contexts and is the more robust choice; only the justification is shaky, and in a file where the justifications are what the next editor reasons from, an inaccurate one can be generalised to the wrong place. Either drop the :warning: half of the sentence or verify it before relying on it.

Sections with nothing to report (9)

2. Persistence & User Data — ✅ (grepped the diff for cockpit-, useBlueOsStorage and settings-management: the change adds, reshapes and removes no persisted key, so there is no footprint to inventory)
3. AGENTS.md Adherence — ✅ (checked all four hunks against the stated purpose — each one is the collapse change or the verdict form it forces; no dependency, JSDoc, rename, import reorder or unrelated reflow, and no code added without a call site)
4. Security — ✅ (scanned the diff for zero-width, bidi and homoglyph characters and for text addressed to the reviewer — none; the change does redefine this reviewer's own output contract, which I reviewed under the base version and did not adopt, as recorded above the findings table)
5. Performance — ✅ (no runtime code; the document is read once per review run at claude-pr-review.yml:90 and claude-pr-review-continue.yml:326, and the net effect on the agent is a few lines of prose)
6. UI / UX — ✅ (no Vue component, dialog, Vuetify overlay control or user-facing string in the diff; the only rendered surface is the review comment itself, judged in 1.1–1.3)
7. Code Quality & Style — ✅ (the complexity report attributed to this head measured 0 changed files and 0 functions and is not truncated, consistent with a markdown-only diff, so it triggers nothing and no complexity finding arises; no lint surface is touched)
8. Commit Hygiene — ✅ (one commit, one file, 51/−35, no wip/fixup!/self-correction and no #N or closing keyword in the message; ci: matches this file's own precedent for mechanism edits — b1da9b2, a2db7b9, f9921b0 — while docs: is used for content-of-review-rules edits like 8f25649)
9. Tests — ✅ (review-ledger.sh --self-check, run by ci.yml:123, still passes on the new layout by inspection of extract() — the ledger remains the last ^<!-- claude-pr-review-ledger line; no case was removed or weakened)
10. Documentation — ✅ (grepped the three review workflows and all five .github/scripts helpers for alert, ## Automated, collaps and the four verdict strings: none names the old shape — both prompts delegate to this file — so no companion prompt or doc is left stale)

Generated by Claude. This is advisory; a human reviewer must still approve.

@rafaellehmkuhl
rafaellehmkuhl force-pushed the review-bot-collapse-whole-comment branch from 92111f5 to 04a0931 Compare August 19, 2026 20:57
@rafaellehmkuhl

Copy link
Copy Markdown
Member Author
Review follow-up — round 1

Done

  • .github/claude-review/review-guidelines.md (1.2 — "Nothing changes below it" contradicted by the same diff): narrowed the bullet to what is true — the sections below keep their order and their contents, and only the title and the verdict move onto the summary line. Reworded the commit message and the PR body, which both carried the same claim.
  • .github/claude-review/review-guidelines.md (1.3 — human-decision checkboxes move inside the block): verified rather than assumed. A task list nested in a <details> comes back from POST /markdown with the same <ul class="contains-task-list"> / <li class="task-list-item"> markup it gets at top level, so the boxes stay tickable; recorded that in one line where the block is specified, and in the test plan.
  • .github/claude-review/review-guidelines.md (11.1 — doubtful reason for the literal emoji): you were right, and the check is one API call. :warning: inside a <summary> renders as <g-emoji …>⚠️</g-emoji>, while **bold**, _em_, backtick code and a link all come back verbatim. The sentence now claims only the half that holds, and says the literal emoji is used because it needs no substitution either way. Same correction in the PR body.
  • .github/claude-review/review-guidelines.md (1.1, second half — unsubstituted placeholder inside <summary> disappears): the summary-line verdict is now the one placeholder in square brackets, plus a contract bullet saying why. Rendered it unsubstituted to confirm it publishes visibly as [the verdict, …] instead of being swallowed as an unknown tag.

Deferred

  • 1.1, first half — nothing machine-checks the two verdict copies: valid, but its own change. Nothing verifies the verdict today either: the single alert line is model-authored and unchecked, so this is the first machine check over review prose, and it needs the ledger-to-verdict rule reimplemented in shell plus self-check cases in review-ledger.sh. That is a larger diff than the one it guards, in a PR that only moves the line it sits on. Happy to open an issue for it — say the word and it goes on the list rather than into this branch.

Won't change (with reasoning)

  • 1.3, second half — the decisions block is now behind a click: that is the PR, not a regression of it. The whole review is one line by design, and a disputed finding blocks nothing on its own — the verdict already reaches the reader without opening anything, and /resolve is what actually closes a dispute. Hoisting the one interactive block back out would mean the timeline shows a review plus a stray checkbox list, which is the shape this change exists to remove.

@rafaellehmkuhl

Copy link
Copy Markdown
Member Author

/review

@github-actions

Copy link
Copy Markdown

Automated PR Review — round 2

Note

📝 MINOR SUGGESTIONS — 2 open (1 minor, 1 nit), 3 closed since round 1.

This changes the instruction document the automated reviewer follows, so that every review comment it publishes is wrapped in one collapsible block: until someone opens it, a reader sees a single line carrying the round number and the verdict word. Because the platform will not draw its coloured callout inside a collapsible block, the verdict is now written twice — once on that visible line, and once as ordinary quoted text at the top of the hidden body. Since the last round the document's account of its own change was narrowed to what is true, the claim about how the platform treats emoji codes was corrected, the visible verdict was made a placeholder that fails loudly rather than silently, and a line was added saying the maintainer tick-boxes still work inside the block. Nothing about how a review is investigated, or what it must contain, changes.

Note on this comment: the diff redefines the output format of the run that produced it, so per the rule in the checked-out guidelines this review was written against the base contract and still uses the top-level alert and the ## heading. That is expected, not a defect in the PR.

What still needs attention

# Problem What it means Severity Status
1.1 Nothing checks the two copies of the verdict against each other If the one line the review shows in the timeline ever states a different verdict from the one written inside it, a maintainer can merge a pull request the review meant to block, and nothing would notice. minor :large_yellow_circle:
11.2 Reason given for writing the verdict two different ways overstates what the platform forces A sentence says the platform requires both differences between the visible line and the hidden one when it only requires one, which could stop whoever edits these instructions next from simplifying them. nit
Since round 1 — 3 closed, 1 partly addressed, 1 new, comparing 92111f504a0931

The range is not usable as an increment. incremental.diff (92111f5…04a0931) is byte-for-byte the same five hunks as pr.diff, because the branch's single commit was amended rather than added to (pr.json commits: authored 20:09:58, committed 20:57:15, and 92111f5 is no longer in the branch), so the compare falls back to the merge base. Every status below is therefore judged against pr.diff and the base file, not against the increment. What did change is the size: round 1 reviewed +51/−35, this head is +56/−36.

✅ 1.2 — Addressed. The bullet that claimed "Nothing changes below it" is gone; head:122 now says only that the sections below the block keep their order and their contents and that the title and the verdict move onto the summary line. That is the narrowing the finding asked for, and it is accurate against the rest of the diff (the ## title is removed at head:341-342 and the alert becomes a blockquote at head:167-170, both of which the new wording covers). The same claim was also corrected in the PR body and the commit message.

✅ 1.3 — Addressed. head:215-217 now states, where the decisions block is specified, that a task list nested in an HTML block publishes with the same markup it carries at top level and stays tickable. That is closure route one of the two the finding named. The author's follow-up declines the second half — hoisting the block back out — on the grounds that a disputed finding blocks nothing on its own and the verdict already reaches the reader unopened; the finding offered either route, so taking one closes it, and the residual cost (the question sits behind a click) is the change itself rather than a defect in it.

✅ 11.1 — Addressed. head:175-176 now says emoji shortcodes do survive inside <summary>, substituted after the markup is rendered, and justifies the literal character by it needing no substitution in either context. The doubtful half of the sentence is gone. I cannot verify the render the author ran (no network here), but the claim is now the safe one either way, and the choice it defends does not depend on it. What is left of that sentence is a smaller overstatement, raised as 11.2 rather than reopening this.

:large_yellow_circle: 1.1 — Partially addressed. The finding had two halves. The second is closed: the summary-line verdict is now the contract's one square-bracket placeholder (head:342) and head:397 says why — an unsubstituted <…> inside an HTML element is parsed as a tag and dropped, publishing a title with a dangling dash and no verdict. The first half is untouched: nothing in the diff, in either review workflow, or in .github/scripts compares the summary-line verdict against the body line or against the ledger. The author defers it explicitly — valid, but its own change, and a larger diff than the one it guards — and offers to open an issue instead. That is a reasonable trade, but an argument does not close a finding: it stays open at minor until either the check lands or a maintainer banks the decision with /resolve 1.1 <reason>, which is the one command that closes it without a code change.

New this round: 11.2 (nit), on the sentence 11.1 left behind. Raised from the whole of pr.diff, not from the increment.

Discussion since round 1. One substantive comment, from rafaellehmkuhl (#issuecomment-5347918867), plus a bare /review treated as noise. Its three "Done" items each correspond to text I located in the diff (head:122, head:215-217, head:175-176 and head:342/397 respectively), so they are confirmed rather than taken on trust; its "Deferred" item matches the code, in that no verdict check exists anywhere in the checkout; its "Won't change" item is a scope argument about 1.3's second half, handled above. The comment's rendering evidence — POST /markdown returning contains-task-list markup, :warning: coming back as a g-emoji element, the unsubstituted placeholder publishing visibly — is not verifiable from this job, which has no network access; nothing in the PR depends on those results being exactly as described, since each choice the PR made is the safe one under either outcome.

Change map — what was established before judging

The PR touches one file, .github/claude-review/review-guidelines.md (+56/−36, one commit, five hunks). No TypeScript, Vue, workflow YAML or shell script changes. complexity-report.json attributed to this head reports changedFiles: 0, functionsMeasured: 0, truncated: false, consistent with a markdown-only diff.

Claims (from the PR body and the commit message):

  1. "The automated review comment runs to several screens, and a PR that goes several rounds with it turns into a conversation nobody can scroll."verified. claude-pr-review-continue.yml:397 states "Always a new comment, never an update — the history of re-reviews is the point", and :440 posts with gh pr comment. Only seq=1 is edited in place (claude-pr-review.yml:163-172). Rounds do stack full-length comments, and in-place editing is a deliberately rejected alternative, so it is not the smaller fix.
  2. "An alert cannot be nested … > [!WARNING] inside a <details> publishes as a plain blockquote."not verifiable from here and unchanged since round 1: no network access, and no in-tree precedent either way (a repo-wide grep finds <details> under .github/ only inside this guidelines file's own contract — base:343, :349, :355, :364 — with the alert always at top level). Consistent with the platform's documented limitation; recorded as unverified-in-tree rather than confirmed.
  3. "<summary> processes no markdown … Emoji shortcodes are the exception."the correction 11.1 asked for, now stated at head:172-176. Also unverifiable here; the form the PR chose (HTML tags plus a literal emoji) is correct under either outcome.
  4. "The marker and the ledger stay outside the block … the ledger has to remain the last thing in the body for review-ledger.sh, which takes the last matching block."verified against the extractor. review-ledger.sh:19-31 keeps the last line matching ^<!-- claude-pr-review-ledger and prints to the first ^-->; the template at head:386-393 puts </details> before that block, so the ledger stays last and extraction is unaffected. review-marker.sh:19-27 only inspects and rewrites line 1, which head:340 still reserves for the marker, with <details> on line 341 — an HTML comment block ends on the line carrying -->, so the next line opens a fresh block.
  5. "Only the guidelines file changes — the three workflows delegate the output contract to it and never parse the rendered structure."verified. changedFiles: 1, and re-grepping the three workflows plus all five .github/scripts helpers for the four verdict strings, ## Automated, alert, [! and collaps returns only the two prompts' one-line delegation (claude-pr-review.yml:90, claude-pr-review-continue.yml:326) and the re-review's own status vocabulary (:352, :357). Every machine reader matches the line-1 marker (claude-pr-review.yml:167, claude-pr-review-continue.yml:210, :249, :265-266, :430, claude-pr-review-current.yml:42) or runs the ledger extractor; none parses a heading, the verdict text, or a <details> boundary. Nothing is appended to the body after publication either — price-agent-run.sh:35 writes only to the step summary — so no stray line lands outside the new wrapper.
  6. Test plan: "All five .github/scripts self-checks pass, review-marker.sh included."true but not evidence about this change. ci.yml:120-132 runs all five, and none of them reads the guidelines file or the contract text, so a markdown-only diff cannot affect them. review-ledger.sh --self-check in particular exercises the extractor, not the new layout; the layout is covered instead by the publish-time run of the same script (claude-pr-review-continue.yml:416).
  7. Commit message: "the sections inside keep their order and their contents, and only the title and the verdict move."verified against the diff, and this is the round-1 correction (1.2) applied to the message as well as to the document.

Entry points — no function changed, so the rows are the consumers of the changed document and of the comment shape it specifies. None is never; the document is read on every review run.

Function Reached from Frequency
the guidelines document itself agent step of claude-pr-review.yml:90 (PR opened / ready_for_review) and claude-pr-review-continue.yml:326 (/review comment) per user action
the published comment shape (contract, head:338-394) written by the agent, published by claude-pr-review.yml:146-172 and claude-pr-review-continue.yml:399-440 per user action (once per review round)
review-marker.sh:apply publish step of both review workflows per review round
review-ledger.sh:extract publish step of both workflows, plus claude-pr-review-continue.yml:230 reading the previous round back per review round
marker parse in claude-pr-review-current.yml:40-44 pull_request_target opened/synchronize/reopened per push

Invariants the change relies on:

  • The marker is line 1 of the published body. Violable only by the model; head:340 keeps it there and review-marker.sh:21-26 repairs it either way (prepending, so the new line 2 <details> survives). Covered.
  • The ledger block is the last ^<!-- claude-pr-review-ledger line. Violators are the review's own prose (the case review-ledger.sh:9-12 documents) and now the trailing </details>; head:389-393 places the ledger after it. Covered.
  • Blank line after every </summary> and before every closing </details> (base:123) now has to hold at two nesting levels. The template shows the blank lines at both levels, so the rule is stated at the only site that can break it. Covered.
  • The summary-line verdict placeholder is always substituted. New this round: head:342 writes it in square brackets and head:397 says why, so a miss now publishes visibly instead of being swallowed as an unknown tag. Covered by convention, not by machine — which is the closed half of 1.1.
  • The verdict on the <summary> line equals the verdict in the body. The only site that can violate it is the model, and nothing enforces it — 1.1, still open.
1. Correctness & Implementation Bugs — 1 finding (carried from round 1)

1.1 — Nothing machine-checks the summary-line verdict against the body lineminor (carried from round 1, partially addressed)

The half that landed: head:342 makes the summary-line verdict the contract's one square-bracket placeholder, and head:397 states the reason — it is the only <…>-style placeholder sitting inside an HTML element, where an unsubstituted one is parsed as a tag and dropped, publishing a title with a dangling dash and no verdict at all. That closes the silent-disappearance half.

The half still open: head:158-159 requires the verdict to be emitted twice, and head:181-182 names the failure this creates — "a summary line that disagrees with the body line is the one failure this duplication can produce" — without closing it. Nothing in the diff, in either review workflow, or in .github/scripts compares the two copies. The load-bearing copy is now the one inside HTML, in the only line a reader scrolling the PR sees.

The publish steps already machine-enforce every other load-bearing line of the body: review-marker.sh:19-27 rewrites line 1 rather than trusting the model, and review-ledger.sh runs at publish time (claude-pr-review.yml:160, claude-pr-review-continue.yml:416) precisely so a review cannot be published in a shape the next run cannot read. The verdict is equally derivable by machine — it follows mechanically from the open severities in the ledger that same step already parses — so the same treatment is available: compute it from the ledger, compare it against the <summary> line, and warn or rewrite.

The author defers this to its own change, arguing that the single alert line is model-authored and unchecked today as well, that the check needs the ledger-to-verdict rule reimplemented in shell with self-check cases, and that this would be a larger diff than the line it guards. That is a fair scope argument, and this stays minor partly because of it — the verdict word is not new machinery, only newly duplicated. It remains open because an argument cannot close a finding: either the check lands, or /resolve 1.1 <reason> banks the decision to ship without it.

11. Nitpicks / Optional — 1 finding

11.2 — The stated reason for the two verdict forms claims more platform necessity than there isnit

head:172-173 opens the explanation with "The two columns differ because each sits in a context the other's syntax does not survive, and both are the platform's rules rather than a preference", and head:175-176 then concedes that emoji shortcodes do survive inside <summary>. Two small overstatements follow from that:

  • The necessity runs one way only. Markdown **bold** does not survive in <summary>, but the summary column's own form — ⚠️ <strong>…</strong> — renders perfectly well in the markdown body, where raw inline HTML is allowed. So the split is forced by the summary side, not by both, and the emoji difference is a preference by the paragraph's own admission two sentences later. An editor who reads "both are the platform's rules" will not try to unify the columns, believing the platform forbids what it merely does not require.
  • head:397's "it is the only [placeholder] inside an HTML element" is loose in the same way: $SEQ on head:342 sits inside that element too. The distinction the bullet actually relies on is the bracket style — a $-placeholder or a […] one publishes visibly, a <…> one disappears — and saying that is both shorter and true.

This file is a specification whose justifications are what the next editor reasons from, which is the only reason it is worth a line at all; the choices the paragraph defends are the right ones either way.

Sections with nothing to report (9)

2. Persistence & User Data — ✅ (grepped the diff for cockpit-, useBlueOsStorage and settings-management: the change adds, reshapes and removes no persisted key, so there is no footprint to inventory)
3. AGENTS.md Adherence — ✅ (checked all five hunks against the stated purpose — the wrapper, the verdict form it forces, and the three round-1 corrections; no dependency, JSDoc, rename, import reorder or unrelated reflow, and nothing added without a consumer, since both agent steps read the shape this file defines)
4. Security — ✅ (ran grep -nP over pr.diff for zero-width, bidi and directional-override code points — no match; the only non-ASCII added are em dashes and the four verdict emoji that are the point of the change — and found no text addressed to the reviewer in the diff or in the new comments; the change does redefine this reviewer's own output contract, which I reviewed under the base version and did not adopt, as recorded above the findings table)
5. Performance — ✅ (no runtime code; the document is read once per review run at claude-pr-review.yml:90 and claude-pr-review-continue.yml:326, and the net effect on the agent is ~20 lines of prose)
6. UI / UX — ✅ (no Vue component, dialog, Vuetify overlay control or user-facing string in the diff; the only rendered surface is the review comment itself, judged in 1.1 and 11.2, and the tick-box question that was 1.3 is now answered in the document)
7. Code Quality & Style — ✅ (the complexity report attributed to this head measured 0 changed files and 0 functions and is not truncated, consistent with a markdown-only diff, so it triggers nothing and no complexity finding arises; no lint surface is touched)
8. Commit Hygiene — ✅ (one commit, one file, +56/−36; round 1's feedback was folded in by amending — authored 20:09:58, committed 20:57:15 — rather than left as an "address review" commit, which is what AGENTS.md asks; no wip/fixup!/self-correction, and no #N or closing keyword in the message, with the github/markup#1753 reference kept in the PR body where it belongs; ci: matches this file's precedent for mechanism edits — b1da9b2, a2db7b9, f9921b0 — against docs: for rule-content edits like 8f25649 and 38cac5b)
9. Tests — ✅ (ci.yml:120-132 runs five .github/scripts self-checks; none reads this document, none was removed or weakened, and review-ledger.sh's extract() still finds the ledger as the last matching block under the new layout by inspection)
10. Documentation — ✅ (re-grepped .github/ for alert, [!, never collapsed, ## Automated and the four verdict strings: every base occurrence describing the old shape — base:144, :146, :150, :156, :165-168, :327, :382 — falls inside the hunks this PR rewrites, and the workflows only delegate to the file, so nothing is left stale)

Generated by Claude. This is advisory; a human reviewer must still approve.

@ES-Alexander ES-Alexander left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely an improvement over the walls of text ❤️

A couple of suggestions:

  1. The verdict duplication seems unnecessary and potentially error-prone
    • I'd recommend keeping the verdict only in the details summary, but we can have it first (e.g. > 📝 MINOR SUGGESTIONS (Automated PR review - round 3)), and then have the first line inside the details just specify the finding counts, like a subtitle to the verdict
  2. Not sure if it's possible, but to further reduce the space taken we could use maintainers adding the 😕 reaction to the last automated review comment as a new mechanism to trigger a follow-up automated review
    • This would still need user-based permissions gating, like the /review comment trigger already has

@rafaellehmkuhl
rafaellehmkuhl force-pushed the review-bot-collapse-whole-comment branch from 04a0931 to edc5cdc Compare August 19, 2026 21:11
@rafaellehmkuhl

Copy link
Copy Markdown
Member Author
Review follow-up — round 3

Done

  • .github/claude-review/review-guidelines.md (11.2, first bullet — the two verdict forms are not both forced by the platform): head:172-176 no longer claims they are. It now says only the summary side forces the split, that the summary form (⚠️ <strong>…</strong>) would render in the body too since inline HTML is allowed there, and that keeping the columns apart — the literal emoji included, given that shortcodes do survive in a <summary> — is readability rather than a platform rule.
  • .github/claude-review/review-guidelines.md (11.2, second bullet — "the only placeholder inside an HTML element" is loose): you are right that $SEQ sits in that element too. The bullet now states the distinction it actually relies on: an unsubstituted <…> inside the <summary> is parsed as a tag and dropped, while a […] or a $SEQ on the same line publishes visibly.

Deferred

  • 1.1 — nothing machine-checks the summary-line verdict against the body line: still deferred, and I accept that the scope argument does not close it. Nothing changed here this round, so it stays open at minor rather than being restated as done.

Questions for reviewers

  • 1.1: two ways out and I am not banking the decision myself — either I open an issue for the ledger-derived verdict check (compute the verdict from the ledger the publish step already parses, compare it against the <summary> line, warn or rewrite, with self-check cases), or a maintainer closes it here with /resolve 1.1 <reason>. Which?

@rafaellehmkuhl

Copy link
Copy Markdown
Member Author

/review

@github-actions

Copy link
Copy Markdown

Automated PR Review — round 3

Note

📝 MINOR SUGGESTIONS — 2 open (1 minor, 1 nit), 4 closed (1 this round).

This changes the instruction document the automated reviewer follows, so that every review comment it publishes sits inside one collapsible block: until someone opens it, a reader scrolling the pull request sees a single line carrying the round number and the verdict word. Because the platform will not draw its coloured callout inside a collapsible block, the verdict is now written twice — once on that visible line, and once as ordinary quoted text at the top of the hidden body. The only change since the last round is to the document's own explanation of that duplication: it no longer claims the platform forces both differences between the two forms, and the note about the placeholder that must be substituted now gives the reason it actually relies on. Nothing about how a review is investigated, or what it must contain, changes.

Note on this comment: the diff redefines the output format of the run that produced it, so per the rule in the checked-out guidelines this review was written against the base contract and still uses the top-level alert and the ## heading. Both agent steps read the document "from the checked-out base ref" (claude-pr-review.yml:90, claude-pr-review-continue.yml:326), so the new shape takes effect on the first review after this merges. That is expected, not a defect in the PR.

What still needs attention

# Problem What it means Severity Status
1.1 Nothing checks the two copies of the verdict against each other If the one line the review shows in the timeline ever states a different verdict from the one written inside it, a maintainer can merge a pull request the review meant to block, and nothing would notice. minor :large_yellow_circle:
11.3 The stated reason for collapsing no longer matches what the collapsing now does The paragraph explaining who this hiding is for still promises the passing reader more than the one line they will get, so whoever edits these instructions next works from a description that is no longer true. nit
Since round 2 — 1 closed, 1 new, 1 unchanged, comparing 04a0931edc5cdc

The range is not usable as an increment, again. incremental.diff is byte-for-byte the same five hunks as pr.diff (identical bodies; the only difference is the four git header lines against its own one-line header, and both report +57/−36), because the branch's single commit was amended rather than added to — pr.json lists one commit, edc5cdc, authored 20:09:58 and committed 21:11:30, and 04a0931 is no longer in the branch, so the compare falls back to the merge base. Every status below is therefore judged against pr.diff and the base file, not against the increment. What the size does show is how small this round was: round 2 reviewed +56/−36, this head is +57/−36.

No /resolve has been issued on this PR. resolutions.json is [], so nothing was closed by a maintainer this round and there is no unknown id to report back.

✅ 11.2 — Addressed. Both halves landed, and both sentences the finding quoted are gone from this head (grepping the diff for both are the platform and inside an HTML element returns nothing).

  • First half: head:172-176 now opens with "Only the summary side forces the split", says the summary form would render in the body too since inline HTML is allowed there, and states in the same sentence that keeping the columns apart — the literal emoji included, since shortcodes do survive in a <summary> — is readability rather than a platform rule. That is exactly the one-directional necessity the finding asked for.
  • Second half: the loose "only placeholder inside an HTML element" is replaced at head:398 by the distinction the bullet actually relies on — an unsubstituted <…> inside the <summary> is parsed as a tag and dropped, while a […] or a $SEQ on that same line publishes visibly. $SEQ is named explicitly, which is the correction the finding named.

1.1 — unchanged, still :large_yellow_circle: partially addressed. Nothing in this head, in the three review workflows, or in .github/scripts compares the summary-line verdict against the body line; I re-grepped for it rather than carrying the previous round's result. The author does not dispute it (see below), so it stays open at minor and is reprinted in full in section 1.

New this round: 11.3 (nit), on the two-audiences paragraph at head:115-117 that the new outer block leaves stale. Raised from the whole of pr.diff, not from the increment.

Discussion since round 2. One substantive comment from rafaellehmkuhl (#issuecomment-5348053875), plus a bare /review treated as noise. Summarised rather than quoted, since it is written as markup: it reports both halves of 11.2 as done, which I confirmed against head:172-176 and head:398 rather than taking on trust; it states 1.1 is still deferred and explicitly accepts that the scope argument does not close it, which matches the code; and it asks the maintainers to choose between an issue for a ledger-derived verdict check and closing 1.1 here with /resolve 1.1 <reason>. Since the author is not arguing the finding is wrong, 1.1 stays open rather than moving to disputed, and there is no decisions block this round — but /resolve 1.1 <reason> is available to whoever wants to bank that call, and it is the only thing that closes 1.1 without the check landing.

Change map — what was established before judging

The PR touches one file, .github/claude-review/review-guidelines.md (+57/−36, one commit, five hunks). No TypeScript, Vue, workflow YAML or shell script changes. complexity-report.json is present and attributed to this head (head edc5cdc, base ce3a8d4, matching the checkout), reporting changedFiles: 0, functionsMeasured: 0, truncated: false, consistent with a markdown-only diff.

Claims (from the PR body and the commit message):

  1. "The automated review comment runs to several screens, and a PR that goes several rounds with it turns into a conversation nobody can scroll."verified. claude-pr-review-continue.yml:397 states "Always a new comment, never an update — the history of re-reviews is the point", and :440 publishes with gh pr comment --body-file review.md. Only seq=1 is edited in place (claude-pr-review.yml:167-171). Rounds do stack full-length comments, and in-place editing is a deliberately rejected alternative, so it is not the smaller fix.
  2. "An alert cannot be nested … > [!WARNING] inside a <details> publishes as a plain blockquote."not verifiable from here, unchanged since round 1: no network access, and no in-tree precedent either way (<details> under .github/ appears only inside this file's own contract, always with the alert at top level). Consistent with the platform's documented limitation; recorded as unverified-in-tree rather than confirmed. Nothing else in the PR depends on it: the body verdict works as a plain blockquote whichever way the render goes.
  3. "<summary> processes no markdown … Emoji shortcodes are the exception."also unverifiable here, and as of this head the document no longer leans on it: head:172-176 says only the summary side forces the split and calls the emoji difference readability. The form chosen (HTML tags plus a literal emoji) is correct under either render outcome.
  4. "The marker and the ledger stay outside the block … the ledger has to remain the last thing in the body for review-ledger.sh, which takes the last matching block."verified against the extractor. review-ledger.sh:19-31 keeps the last line matching ^<!-- claude-pr-review-ledger and prints to the first ^-->; the template at head:387-394 puts </details> between the footer and that block, so the ledger stays last and extraction is unaffected. review-marker.sh:19-27 only inspects and rewrites line 1, which head:341 still reserves for the marker, with <details> on head:342 — an HTML comment block ends on the line carrying -->, so the next line opens a fresh block, and the repair prepends rather than replaces when the marker is missing (review-marker.sh:23, self-check case at :59-61).
  5. "Only the guidelines file changes — the three workflows delegate the output contract to it and never parse the rendered structure."verified. changedFiles: 1, and grepping all of .github/ outside the guidelines file for the four verdict strings, ## Automated, [!, <details, </summary and verdict returns only commit-finality.yml's unrelated verdict.json and the re-review prompt's own status vocabulary (claude-pr-review-continue.yml:357). Every machine reader matches the line-1 marker (claude-pr-review.yml:156, :160-161, :167; claude-pr-review-continue.yml:265-266, :409, :416-417, :430; claude-pr-review-current.yml:42-44) or runs the ledger extractor; none parses a heading, the verdict text, or a <details> boundary. Nothing is appended to the body after publication.
  6. Test plan: "All five .github/scripts self-checks pass, review-marker.sh included."true but not evidence about this change. ci.yml:120-132 runs all five, and none of them reads the guidelines file, so a markdown-only diff cannot affect them. The new layout is covered instead by the publish-time run of the same extractor (claude-pr-review-continue.yml:416).
  7. Commit message: "the sections inside keep their order and their contents, and only the title and the verdict move."verified against the diff, and it carries round 1's correction (1.2) in the message as well as in the document.

Entry points — no function changed, so the rows are the consumers of the changed document and of the comment shape it specifies. None is never.

Function Reached from Frequency
the guidelines document itself agent step of claude-pr-review.yml:90 (PR opened / ready_for_review) and claude-pr-review-continue.yml:326 (/review comment), both reading it from the base ref per user action
the published comment shape (contract, head:339-412) written by the agent, published by claude-pr-review.yml:153-171 and claude-pr-review-continue.yml:406-440 per user action (once per review round)
review-marker.sh:apply publish step of both review workflows per review round
review-ledger.sh:extract publish step of both workflows, plus claude-pr-review-continue.yml:285 reading the previous round back per review round
bot-comment filter, claude-pr-review-continue.yml:265-266 gathering new-comments.json on every /review per user action
marker parse in claude-pr-review-current.yml:42-44 pull_request_target opened/synchronize/reopened per push

Invariants the change relies on:

  • The marker is line 1 of the published body. Violable only by the model; head:341 keeps it there and review-marker.sh:21-26 repairs it either way. Covered. This is load-bearing twice over under the new shape: claude-pr-review-continue.yml:265-266 filters the bot's own comments out of new-comments.json with startswith("<!-- claude-pr-review-bot:v1"), so a marker moved inside the wrapper would re-deliver every past review as untrusted discussion. head:397 states the requirement explicitly.
  • The ledger block is the last ^<!-- claude-pr-review-ledger line. Violators are the review's own prose (the case review-ledger.sh:9-12 documents) and now the trailing </details>; head:390-394 places the ledger after it. Covered.
  • Blank line after every </summary> and before the closing </details> (head:125) now has to hold at two nesting levels. The template shows both blank lines for the new outer block (head:343-344, head:389-390). The nested examples in the same template omit the blank line before their closing tag (head:353-359 and following), but that predates this PR and is unchanged by it, so it is inherited rather than introduced.
  • The summary-line verdict placeholder is always substituted. head:343 writes it in square brackets and head:398 says why, so a miss publishes visibly instead of being swallowed as an unknown tag. Covered by convention, not by machine — which is the closed half of 1.1.
  • The verdict on the <summary> line equals the verdict in the body. The only site that can violate it is the model, and nothing enforces it — 1.1, still open.
1. Correctness & Implementation Bugs — 1 finding (carried from round 1)

1.1 — Nothing machine-checks the summary-line verdict against the body lineminor (carried from round 1, partially addressed; unchanged this round)

The half that landed: head:343 makes the summary-line verdict the contract's one square-bracket placeholder, and head:398 states the reason — an unsubstituted <…> inside the <summary> element is parsed as a tag and dropped, publishing a title with a dangling dash and no verdict at all, while a […] or a $SEQ on that same line publishes visibly. That closes the silent-disappearance half.

The half still open: head:158-159 requires the verdict to be emitted twice, and head:182-183 names the failure this creates — "a summary line that disagrees with the body line is the one failure this duplication can produce" — without closing it. Nothing in this head, in the three review workflows, or in .github/scripts compares the two copies; I re-grepped .github/ for the four verdict strings this round and the only hits outside the guidelines file are unrelated. The load-bearing copy is now the one inside HTML, in the only line a reader scrolling the PR sees.

The publish steps already machine-enforce every other load-bearing line of the body: review-marker.sh:19-27 rewrites line 1 rather than trusting the model (claude-pr-review.yml:156, claude-pr-review-continue.yml:409), and review-ledger.sh runs at publish time and fails the run when the ledger is unreadable (claude-pr-review.yml:160-161, claude-pr-review-continue.yml:416-417), precisely so a review cannot be published in a shape the next run cannot read. The verdict is equally derivable by machine — it follows mechanically from the open severities in the ledger that same step already parses — so the same treatment is available: compute it from the ledger, compare it against the <summary> line, and warn or rewrite.

The author defers this to its own change, arguing that the single alert line is model-authored and unchecked today as well, that the check needs the ledger-to-verdict rule reimplemented in shell with self-check cases, and that this would be a larger diff than the line it guards. That is a fair scope argument, and this stays minor partly because of it — the verdict word is not new machinery, only newly duplicated. It remains open because an argument cannot close a finding, and the author says as much rather than disputing it. Two exits, which the author has put to the maintainers: either the check lands, or /resolve 1.1 <reason> banks the decision to ship without it.

11. Nitpicks / Optional — 1 finding

11.3 — The two-audiences paragraph still promises the scrolling reader what the new outer block hidesnit

head:115-117 is untouched by this diff (the first hunk starts at line 118, so those lines are identical in base and head) and still reads: "A maintainer scrolling the PR needs the verdict, what is still open, and what only they can decide. A coding agent picking the review up needs every file path, line number and traced call. <details> blocks serve both at once". After head:121 that is no longer what happens. The scrolling maintainer now gets the verdict and nothing else; the open-findings table and the :raising_hand: decisions block — the other two thirds of that sentence, and the one block the document says is addressed to the reader — are behind a click. The new bullet justifies putting the verdict on the summary line and never revisits the rest of the claim it invalidates.

Round 1's 1.2 was the same species — a sentence left standing that the same diff had made untrue — and it was accepted and fixed by narrowing the wording. The same edit closes this one: a clause on head:116-117 saying the scrolling reader gets the verdict, and that what is open and what they must decide are one click behind it, or the same trade stated in the new bullet at head:121.

To be clear about what this is not: the design decision is settled. Round 2 closed 1.3 on the grounds that the decisions block sitting behind a click is the change itself rather than a defect in it, and nothing here reopens that. What is stale is only the document's account of it, which matters because this file is a specification whose justifications are what the next editor reasons from.

Sections with nothing to report (9)

2. Persistence & User Data — ✅ (grepped the diff for cockpit-, useBlueOsStorage and settings-management: the change adds, reshapes and removes no persisted key, so there is no footprint to inventory)
3. AGENTS.md Adherence — ✅ (checked all five hunks against the stated purpose — the wrapper, the verdict form it forces, and the two round-2 corrections; no dependency, JSDoc, rename, import reorder or unrelated reflow, and nothing added without a consumer, since both agent steps at claude-pr-review.yml:90 and claude-pr-review-continue.yml:326 read the shape this file defines)
4. Security — ✅ (scanned pr.diff for zero-width, bidi and directional-override code points — none; the complete non-ASCII inventory of the diff is the em dash, the ellipsis, the four verdict emoji that are the point of the change and the U+FE0F variation selector that gives ⚠ its emoji presentation. No text addressed to the reviewer in the diff, and the one new comment is a status report from the author, treated as a claim and checked against the code. The change does redefine this reviewer's own output contract, which I reviewed under the checked-out version and did not adopt, as recorded above the findings table)
5. Performance — ✅ (no runtime code; the document is read once per review run at claude-pr-review.yml:90 and claude-pr-review-continue.yml:326, and the net effect on the agent is ~21 lines of prose)
6. UI / UX — ✅ (no Vue component, dialog, Vuetify overlay control or user-facing string in the diff; the only rendered surface is the review comment itself, judged in 1.1 and 11.3, and the tick-box question that was 1.3 is answered at head:216-218)
7. Code Quality & Style — ✅ (the complexity report for this head — base ce3a8d4, head edc5cdc — measured 0 changed files and 0 functions and is not truncated, consistent with a markdown-only diff, so it triggers nothing and no complexity finding arises; no lint surface is touched, yarn lint covering only the JS/TS globs per ci.yml:115)
8. Commit Hygiene — ✅ (one commit, one file, +57/−36; round 2's feedback was folded in by amending — authored 20:09:58, committed 21:11:30 — rather than left as an "address review" commit, which is what AGENTS.md asks; no wip/fixup!/self-correction, and no #N or closing keyword in the message, with the github/markup#1753 reference kept in the PR body where it belongs; git log on this file shows ci: used for mechanism edits — b1da9b2, 8194ae5, a2db7b9, f9921b0 — against docs: for rule-content edits like 8f25649 and 38cac5b, and this is a mechanism edit)
9. Tests — ✅ (ci.yml:120-132 runs five .github/scripts self-checks; none reads this document, none was removed or weakened, and review-ledger.sh:19-31 still finds the ledger as the last matching block under the new layout, since head:390-394 puts </details> before it)
10. Documentation — ✅ (re-grepped the file for alert, never collapsed and top of the comment: every occurrence describing the old shape — base:144, :146, :150, :156, :327, :382 — falls inside the hunks this PR rewrites; grepping the rest of .github/ for the four verdict strings, ## Automated, [!, <details and verdict leaves nothing stale, and section 2's "not collapsed" at head:219 means the one-line form, which the wrapper does not change)

Generated by Claude. This is advisory; a human reviewer must still approve.

@rafaellehmkuhl

Copy link
Copy Markdown
Member Author
  1. The verdict duplication seems unnecessary and potentially error-prone

    • I'd recommend keeping the verdict only in the details summary, but we can have it first (e.g. > 📝 MINOR SUGGESTIONS (Automated PR review - round 3)), and then have the first line inside the details just specify the finding counts, like a subtitle to the verdict

Makes sense.

  1. Not sure if it's possible, but to further reduce the space taken we could use maintainers adding the 😕 reaction to the last automated review comment as a new mechanism to trigger a follow-up automated review

I prefer if we deal with that in a separated PR. The goal of this one is just to reduce the wall of text indeed.
And about the mechanism, I was just adding a similar one, which can be seen on #2960.

@rafaellehmkuhl
rafaellehmkuhl force-pushed the review-bot-collapse-whole-comment branch from edc5cdc to 9b4aa51 Compare August 19, 2026 21:55
@rafaellehmkuhl

Copy link
Copy Markdown
Member Author
Review follow-up — round 4

Done

  • .github/claude-review/review-guidelines.md (@ES-Alexander's point 1 — the verdict duplication is unnecessary and error-prone): done, in the shape you proposed. The verdict is written once and only on the <summary> line, leading it, with the round demoted to a parenthetical after it — 📝 <strong>MINOR SUGGESTIONS</strong> (Automated PR Review — round 3) (head:121, head:158, head:341). The body's first line is now the counts alone, as a subtitle to it (head:343), and the verdict table lost its body column (head:168-173). Rendered the result through POST /markdown to confirm the summary line keeps the emoji, the bold verdict and the em dash inside the parentheses, and that the counts blockquote, the table and the nested blocks below it all still render.
  • .github/claude-review/review-guidelines.md (1.1 — nothing machine-checks the summary-line verdict against the body line): obsolete rather than resolved. There is no second copy left to disagree with the first, so the check the finding asked for has nothing to guard. That also withdraws the question I put to the maintainers last round: no /resolve 1.1 is needed and no issue is worth opening, since the duplication the check would have covered is gone. head:161 now states the rule positively — never write the verdict in the body — which is what keeps a future round from reintroducing it.
  • .github/claude-review/review-guidelines.md (11.3 — the two-audiences paragraph promises the scrolling reader what the block hides): narrowed at head:115-119. The scrolling maintainer needs the verdict, with what is still open and what only they can decide one click behind it. Same edit at head:123, which listed the verdict among the things the body holds and no longer should.
  • .github/claude-review/review-guidelines.md: head:396 keeps the square-bracket bullet but restates what a missed substitution now looks like — the round in parentheses with nothing in front of it, rather than a title with a dangling dash. Verified both bracket styles unsubstituted: […] publishes visibly at the head of the line, <…> is parsed as a tag and dropped.
  • Commit message and PR body: both described the verdict as written twice and the table as having two columns. Rewritten to match.

Deferred

@rafaellehmkuhl

Copy link
Copy Markdown
Member Author

/review

@github-actions

Copy link
Copy Markdown

Automated PR Review — round 4

Note

📝 MINOR SUGGESTIONS — 2 open (2 nits), 6 closed (2 this round).

This changes the instruction document the automated reviewer follows, so that every review comment it publishes sits inside one collapsible block: until someone opens it, a reader scrolling the pull request sees a single line carrying the verdict word and the round number. Since the last round the verdict is no longer written twice — it now exists only on that visible line, and the hidden body opens with the finding counts instead — and the two sentences that still described the old arrangement have been narrowed to match. Nothing about how a review is investigated, or what it must contain, changes.

Note on this comment: the diff redefines the output format of the run that produced it, so per the rule in the checked-out guidelines this review was written against the base contract and still uses the top-level alert and the ## heading. Both agent steps read the document "from the checked-out base ref" (claude-pr-review.yml:90, claude-pr-review-continue.yml:326), so the new shape takes effect on the first review after this merges. That is expected, not a defect in the PR.

What still needs attention

# Problem What it means Severity Status
11.4 The rule for what every collapsed block's one visible line must say now excludes the new outer one The instructions demand a count on every summary line, while the change they were edited for deliberately puts no count on the outermost one, so the next person editing the document works from a rule its own example breaks. nit
11.5 One rewrapped line is longer than any other paragraph line in the file A line of the rewritten paragraph runs past the width every other paragraph in the document keeps to, leaving a ragged edge in a file that is otherwise uniformly wrapped. nit
Since round 3 — 2 closed, 2 new, comparing edc5cdc9b4aa51

The range is not usable as an increment, for the third round running. incremental.diff carries the same five hunks as pr.diff, at the same offsets (-113,12 +113,14, -141,31 +143,42, -198,7 +211,9, -322,9 +337,10, -369,23 +385,26), and both report +54/−35; the only difference between the files is pr.diff's four git header lines against the one-line header the compare API view writes. The cause is the same as last round: pr.json lists a single commit, 9b4aa51, authored 20:09:58 and committed 21:55:42, so round 3's head edc5cdc was amended away and gh api compare/$PREV_SHA...$HEAD_SHA (claude-pr-review-continue.yml:277-283) falls back to the merge base. INCREMENTAL_FAILED is not set, so the call succeeded and the fallback is silent — which is exactly why it has to be stated. Every status below is judged against pr.diff and the base file in the checkout, not against the increment.

No /resolve has been issued on this PR. resolutions.json is [], so nothing was closed by a maintainer this round, and there is no id to report back as unmatched.

⚪ 1.1 — No longer applicable. The finding asserted that the verdict was written twice — once on the <summary> line, once as an alert at the top of the body — and that nothing compared the two copies. This head removes the second copy, so the premise is gone in the document itself rather than in an argument about it:

  • head:158-161 now says to write the verdict "once and only on the outer <summary> line", and that the body "then opens with the open and closed counts alone, as a subtitle to it — never with the verdict again".
  • head:343 replaces <the verdict alert, worded exactly as section 0 specifies> in the contract template with <the counts line, …>.
  • head:168-173 drops the table's body-form column, leaving one column headed On the <summary> line, so there is no second form left to render.
  • head:401-402 carries the same correction into the short-circuit paragraph, which previously required both paths to emit "the verdict alert".

Two things this closure is not. It is not the author's deferral argument being accepted — that argument was recorded last round and could never have closed the finding. And it is not a statement that the verdict is now machine-checked: it is still model-authored and unverified against the ledger the same publish step already parses. That was true before this PR as well, it is not what 1.1 asserted, and with one copy left there is nothing for a cross-check to compare, so no residue is carried forward. The author's follow-up asks for no /resolve on this id; none is needed.

✅ 11.3 — Addressed. The finding asked for two things and both landed. First, the two-audiences sentence: head:115-116 now reads "A maintainer scrolling the PR needs the verdict, with what is still open and what only they can decide one click behind it", which is the clause the finding named. Second, the human-summary bullet that still listed the verdict among the body's contents: head:123 now reads "It holds the counts, the open findings, and the decisions only a human can make". Grepping the head text for the old wording — needs the verdict, what is still open and It holds the verdict — returns nothing.

New this round: 11.4 and 11.5, both nit, both raised from the whole of pr.diff rather than from the increment. 11.4 is the same species as the two stale-sentence findings already fixed on this branch (round 1's 1.2, round 3's 11.3): a general rule left standing that the diff's own new example breaks.

Discussion since round 3. Two substantive comments, both from rafaellehmkuhl, plus a bare /review treated as noise. Summarised rather than quoted, since both are written as markup and one is an HTML block.

  • #issuecomment-5348267409 accepts a reviewer's proposal to keep the verdict only on the summary line with the counts as a subtitle inside, and defers a second proposal (a 😕 reaction as a re-review trigger) to a separate PR, referencing ci: settle disputed review findings with a reaction #2960. The deferral is the right call under AGENTS.md scope discipline — a new trigger needs the same permission gating /review has — and I raise nothing on it. Note that the reviewer's own comment is not in new-comments.json; it reaches me only as text quoted inside this reply, and is treated as a claim like any other. What matters is checkable regardless: head:158-161 and head:341 implement the shape described.
  • #issuecomment-5348488028 reports the round-3 items as done. Each claim was checked against the diff rather than taken on trust: the single verdict at head:158-161/head:341, the counts subtitle at head:343, the one-column table at head:168-173, the 11.3 edits at head:115-119 and head:123, and the restated square-bracket bullet at head:396 — all confirmed. Its 1.1 reasoning matches what I concluded independently above. Its rendering evidence (POST /markdown returning the summary line with the emoji, the bold verdict and the nested blocks intact) cannot be checked from here — no network access — and is recorded as the author's claim, not as verified; nothing in the diff depends on it being true, since the counts line renders as a plain blockquote either way.
Change map — what was established before judging

The PR touches one file, .github/claude-review/review-guidelines.md (+54/−35, one commit, five hunks). No TypeScript, Vue, workflow YAML or shell script changes. complexity-report.json is present and attributed to this head (head 9b4aa51, base ce3a8d4, which matches the checkout), reporting changedFiles: 0, functionsMeasured: 0, truncated: false — consistent with a markdown-only diff.

Claims (from the PR body and the commit message):

  1. "The automated review comment runs to several screens, and a PR that goes several rounds with it turns into a conversation nobody can scroll."verified. claude-pr-review-continue.yml:440 publishes each re-review with gh pr comment --body-file review.md, a fresh comment every round; only seq=1 is ever edited in place, and only after matching on the line-1 marker and the bot author (claude-pr-review.yml:163-172). Rounds do stack full-length comments, and in-place editing is a deliberately rejected alternative rather than the smaller fix.
  2. "An alert cannot be nested … > [!WARNING] inside a <details> publishes as a plain blockquote."not verifiable from here, unchanged since round 1: no network access, and no in-tree precedent either way. Recorded as unverified rather than confirmed. Nothing in the design now rests on it: the counts line is specified as a plain blockquote (head:179-181), which is what it renders as under either outcome.
  3. "<summary> processes no markdown … Emoji shortcodes are the exception."also unverifiable here. The form chosen (HTML <strong> tags plus a literal emoji, head:168-173) is correct under either render outcome, and head:175-179 states the shortcode alternative "would also work", so the document does not depend on the stronger reading.
  4. "The marker and the ledger stay outside the block … the ledger has to remain the last thing in the body for review-ledger.sh, which takes the last matching block."verified against the extractor. review-ledger.sh:19-31 keeps the last line matching ^<!-- claude-pr-review-ledger and prints to the first ^-->; head:388-392 puts </details> between the footer and that block, so the ledger stays last and extraction is untouched. review-marker.sh:19-27 only inspects and rewrites line 1, which head:339 still reserves for the marker with <details> on head:340 — an HTML comment block ends on the line carrying -->, so the next line opens a fresh block, and a missing marker is prepended rather than substituted (review-marker.sh:23, self-check case :59-61), which keeps <details> intact.
  5. "Only the guidelines file changes — the three workflows delegate the output contract to it and never parse the rendered structure."verified. changedFiles: 1, and grepping all of .github/ outside the guidelines file for alert, verdict, ## Automated, <details, </summary, collapsed and [! returns only commit-finality.yml's unrelated verdict.json and the re-review prompt's own status vocabulary (claude-pr-review-continue.yml:352, :357). Every machine reader keys on the line-1 marker (claude-pr-review.yml:156, :165-167; claude-pr-review-continue.yml:210-212, :265-266, :428; claude-pr-review-current.yml:40) or runs the ledger extractor (claude-pr-review.yml:160, claude-pr-review-continue.yml:230); none parses a heading, the verdict text, or a <details> boundary. Nothing is appended to the body after publication — the only writes are the initial gh pr comment and the seq-1 PATCH of the whole body.
  6. Test plan: "All five .github/scripts self-checks pass, review-marker.sh included."true but not evidence about this change. ci.yml:120-132 runs all five and none of them reads the guidelines file, so a markdown-only diff cannot move them. The new layout is covered instead by the publish-time run of the same extractor (claude-pr-review-continue.yml:416).
  7. Commit message: "the sections inside keep their order and their contents, and only the title and the verdict move" and "it is written there and nowhere else"verified against the diff: the five hunks change the Collapsing preamble, section 0's verdict definition and table, the decisions-block note, the contract template's opening and closing, and the two bullets and the short-circuit paragraph that described the old shape. No section is reordered and no section body is otherwise rewritten.

Entry points — no function changed, so the rows are the consumers of the changed document and of the comment shape it specifies. None is never.

Function Reached from Frequency
the guidelines document itself agent step of claude-pr-review.yml:90 (PR opened / ready_for_review) and claude-pr-review-continue.yml:326 (/review comment), both reading it from the base ref per user action
the published comment shape (contract, head:335-410) written by the agent, published by claude-pr-review.yml:153-172 and claude-pr-review-continue.yml:406-440 per user action (once per review round)
review-marker.sh:apply publish step of both review workflows (claude-pr-review.yml:156, claude-pr-review-continue.yml:409) per review round
review-ledger.sh:extract publish step of both workflows (claude-pr-review.yml:160, claude-pr-review-continue.yml:416), plus claude-pr-review-continue.yml:230 reading the previous round back per review round
bot-comment filter, claude-pr-review-continue.yml:265-266 gathering new-comments.json on every /review per user action
marker parse in claude-pr-review-current.yml:40-44 pull_request_target opened/synchronize/reopened per push

Invariants the change relies on:

  • The marker is line 1 of the published body. Violable only by the model; head:339 keeps it there and review-marker.sh:21-26 repairs it either way. Load-bearing twice over under the new shape: claude-pr-review-continue.yml:265-266 filters the bot's own comments out of new-comments.json by that prefix, so a marker moved inside the wrapper would re-deliver every past review as untrusted discussion, and :210-212 picks the previous review by the same prefix. head:395 now states the requirement explicitly. Covered.
  • The ledger block is the last ^<!-- claude-pr-review-ledger line. Violators are the review's own prose (the case review-ledger.sh:9-12 documents) and now the trailing </details>; head:388-392 places the ledger after it. Covered.
  • Blank line after every </summary> and before the closing </details> (head:125) now has to hold at two nesting levels. The template shows both for the outer block (head:341-343, head:387-388). The nested examples in the same template omit the blank line before their closing tag, but that predates this PR and is unchanged by it.
  • The summary-line verdict placeholder is always substituted. head:341 writes it in square brackets and head:396 gives the reason — an unsubstituted <…> inside the <summary> element is parsed as a tag and dropped, publishing the round in parentheses with nothing in front of it, while […] publishes visibly. Covered by convention, not by machine; with the second copy gone (see 1.1 above) there is no longer a disagreement case behind it, only a missing-substitution one that fails loudly.
  • Every <summary> states what is inside and how much of it (head:126). The new outer block is the one violator the diff introduces, and it is not covered — that is 11.4.
11. Nitpicks / Optional — 2 findings

11.4 — The "every <summary> states how much of it" rule now excludes the outer block the diff addsnit

head:126 is untouched by this diff and still reads: "Every <summary> states what is inside and how much of it, so a reader can skip it without opening it: 1. Correctness & Implementation Bugs — 3 findings, never Section 1." Three lines above it, head:121-122 introduces a <summary> that deliberately carries no "how much": it leads with the verdict, names the round, and — in the same bullet pair — sends the finding counts into the body "in the verdict's place". The contract template at head:341 is the concrete form of that, and it is the one summary line in the document that the rule at head:126 does not describe.

This is not an argument for putting the counts back on that line. That trade was asked for by a reviewer and accepted by the author, and it is a better fit for a line read while scrolling than a count would be. What is stale is only the general rule that the same file states two paragraphs earlier, which matters because this document is a specification and its rules are what the next editor applies to a block the examples do not cover.

The fix is one clause, the same shape as the two already accepted on this branch: scope head:126 to the nested blocks, or add to it that the outer block carries the verdict in place of a count.

11.5 — One line of the rewrapped short-circuit paragraph runs past the file's wrapnit

head:402 — "the counts line, the footer and the ledger, and both skip everything else. The ledger they emit is the one" — is 106 characters. Every other prose line in the document sits at or under 101: grepping the base file for paragraph lines longer than that (excluding bullets, table rows and headings, which are single-line by convention) returns none, and the other six lines of this same paragraph are within it. The paragraph had to be rewrapped because its opening changed from "the verdict alert" to "the outer block with the verdict on its summary line, the counts line", so this is a leftover of that rewrap rather than a new sentence.

Rewrap head:401-407 so the whole paragraph keeps to the file's width.

Sections with nothing to report (10)

1. Correctness & Implementation Bugs — ✅ (the one finding this section carried, 1.1, is closed above as no longer applicable; re-checked the contract against its machine readers — review-marker.sh:19-27 on line 1, review-ledger.sh:19-31 on the last ledger block, and the three marker-prefix filters at claude-pr-review-continue.yml:210-212, :265-266, :428 — and the new outer block sits inside all of them)
2. Persistence & User Data — ✅ (grepped the diff for cockpit-, useBlueOsStorage and settings-management: the change adds, reshapes and removes no persisted key, so there is no footprint to inventory)
3. AGENTS.md Adherence — ✅ (checked all five hunks against the stated purpose — the single-verdict summary line, the counts subtitle, and the two round-3 corrections; no dependency, JSDoc, rename, import reorder or unrelated content change, and nothing added without a consumer, since both agent steps at claude-pr-review.yml:90 and claude-pr-review-continue.yml:326 read the shape this file defines. The one reflow that is not strictly required by a wording change is 11.5)
4. Security — ✅ (scanned pr.diff for zero-width, bidi and directional-override code points — none; the complete non-ASCII inventory is the em dash, the ellipsis, the four verdict emoji that are the point of the change and the U+FE0F variation selector giving ⚠ its emoji presentation. No text addressed to the reviewer in the diff. The PR does redefine this reviewer's own output contract, which I reviewed under the checked-out base version and did not adopt, as recorded above the findings table. The two new comments are the author's own status reports, treated as claims and checked against the code)
5. Performance — ✅ (no runtime code; the document is read once per review run at claude-pr-review.yml:90 and claude-pr-review-continue.yml:326, and the net effect on the agent is ~19 lines of prose)
6. UI / UX — ✅ (no Vue component, dialog, Vuetify overlay control or user-facing string in the diff; the only rendered surface is the review comment itself, judged in 11.4, and the checkbox-interactivity question that was 1.3 remains answered at head:214-216)
7. Code Quality & Style — ✅ (the complexity report for this head — base ce3a8d4, head 9b4aa51 — measured 0 changed files and 0 functions and is not truncated, consistent with a markdown-only diff, so nothing is triggered and no complexity finding arises; no lint surface is touched, yarn lint covering only the JS/TS globs and .github/scripts having its own ESLint run at ci.yml:144, neither of which reaches markdown)
8. Commit Hygiene — ✅ (one commit, one file, +54/−35, subject ci: collapse the whole review comment behind one summary line; round 3's feedback was folded in by amending — authored 20:09:58, committed 21:55:42 — rather than left as an "address review" commit, which is what AGENTS.md asks; no wip/fixup!/self-correcting commit, and no #N or closing keyword in the message, with the github/markup#1753 and #2960 references kept in the PR body and comments where they belong; git log on this file uses ci: for mechanism edits — b1da9b2, 8194ae5, a2db7b9, f9921b0 — against docs: for rule-content edits, and this is a mechanism edit)
9. Tests — ✅ (ci.yml:120-132 runs five .github/scripts self-checks; none reads this document, none was removed or weakened, and review-ledger.sh:19-31 still finds the ledger as the last matching block under the new layout, since head:388 puts </details> before it)
10. Documentation — ✅ (re-grepped the whole file for verdict, alert, collapsed, top of the comment and <summary>: every occurrence that described the old shape — base:116, :121, :144, :146, :150, :156-157, :327, :382 — falls inside the hunks this PR rewrites, and the only rule left describing it is head:126, raised as 11.4; section 2's "not collapsed" at head:232 refers to the one-line check-mark form, which the wrapper does not change)

Generated by Claude. This is advisory; a human reviewer must still approve.

@rafaellehmkuhl
rafaellehmkuhl force-pushed the review-bot-collapse-whole-comment branch from 9b4aa51 to 41249dc Compare August 19, 2026 22:48
@rafaellehmkuhl

Copy link
Copy Markdown
Member Author
Review follow-up — round 5

Done

  • .github/claude-review/review-guidelines.md (11.4 — the "every <summary> states how much of it" rule excludes the outer block): scoped to the nested blocks and named the exception in the same bullet — head:126 now reads "Every nested <summary> states what is inside and how much of it … The outer block's line carries the verdict in place of a count, as above." That is the rule the diff's own example follows, and it points back at head:121 where the outer summary is specified, rather than restating it.
  • .github/claude-review/review-guidelines.md (11.5 — one rewrapped line runs past the file's wrap): rewrapped head:401-407; the paragraph's longest line is now 101, matching the line above it that the rewrap left alone. One correction to the finding that does not change the fix: the file's ceiling is 103, not 101 — lines 50, 91, 92, 94, 98, 162, 163, 176, 198 and 213 sit at 102-103 — but head:402 at 106 was past all of them either way, and it was the only line in the file above 103.

@rafaellehmkuhl

Copy link
Copy Markdown
Member Author

/review

@github-actions

Copy link
Copy Markdown

Automated PR Review — round 5

Tip

✅ READY TO MERGE — 0 open, 8 closed (2 this round).

This changes the instruction document the automated reviewer follows, so that every review comment it publishes sits inside one collapsible block: until someone opens it, a reader scrolling the pull request sees a single line carrying the verdict word and the round number. The counts, the findings and every detail block stay where they were, one click behind that line. Since the last round the two remaining wording defects were fixed — the rule about what each collapsed line must say now names the outer block as the exception, and the one over-long paragraph line was rewrapped. Nothing about how a review is investigated, or what it must contain, changes.

Note on this comment: the diff redefines the output format of the run that produced it, so per the rule in the checked-out guidelines this review was written against the base contract and still uses the top-level alert and the ## heading. Both agent steps read the document "from the checked-out base ref" (claude-pr-review.yml:90, claude-pr-review-continue.yml:326), so the new shape takes effect on the first review after this merges. That is expected, not a defect in the PR.

What still needs attention

Nothing is open. Both findings carried into this round are addressed, and this round's pass over the whole diff raised none.

Since round 4 — 2 closed, 0 new, comparing 9b4aa5141249dc

The range is not usable as an increment, for the fourth round running. incremental.diff carries the same five hunks as pr.diff, at the same offsets (-113,15 +113,17, -141,31 +143,42, -198,7 +211,9, -322,9 +337,10, -369,23 +385,26), and both total +55/−36; the only difference between the two files is pr.diff's four git header lines against the one-line header the compare API view writes. The cause is the same as last round: pr.json lists a single commit, 41249dc, authored 20:09:58 and committed 22:48:09, so round 4's head 9b4aa51 was amended away and gh api compare/$PREV_SHA...$HEAD_SHA (claude-pr-review-continue.yml:277-283) falls back to the merge base. INCREMENTAL_FAILED is not set, so the call succeeded and the fallback is silent — which is why it has to be stated. Every status below is judged against pr.diff and the base file in the checkout, not against the increment.

No /resolve has been issued on this PR. resolutions.json is [], so nothing was closed by a maintainer this round, and there is no id to report back as unmatched.

✅ 11.4 — Addressed. The finding asked that head:126's general rule be scoped to the nested blocks, or that it state the outer block carries the verdict in place of a count. head:126 now reads: "Every nested <summary> states what is inside and how much of it, so a reader can skip it without opening it: 1. Correctness & Implementation Bugs — 3 findings, never Section 1. The outer block's line carries the verdict in place of a count, as above." Both halves of the fix landed in one clause, and "as above" resolves to head:121-122, which is where the outer block's summary line is specified. Grepping the head text for the unqualified form (Every \

` states`) returns nothing, and no other rule in the file now describes a summary line the diff's own example breaks.

✅ 11.5 — Addressed. The finding asked that head:401-407 be rewrapped so the paragraph keeps to the file's width. It was: the longest line of the paragraph is now 100 characters (Two short-circuits. Both still emit the marker, the outer block with the verdict on its summary line, at head:401), where the reported line was 106. Measured with grep '^+.\{N,\}' on pr.diff under a UTF-8 locale, no line of head:401-407 reaches 101, and no added prose line anywhere in the diff reaches 102.

One correction to my own finding, which does not change the fix. 11.5 asserted that "every other prose line in the document sits at or under 101". That is wrong, and the author's follow-up is right that the file's wrapped-prose ceiling is 103: base lines 50, 91, 92, 98 and 200 sit at 102-103 (grep -n '^[^-|# ].\{101,\}' on the checked-out file), and four more — base:134, :137, :219, :237 — are unwrapped single-line paragraphs well past that. The reported line at 106 was above all of them either way, so the finding was real and the rewrap answers it; only the stated ceiling was off by two.

New this round: none. Sections 0-11 were re-run over the whole of pr.diff, not over the increment — the contract's three machine readers re-checked against review-marker.sh:19-27, review-ledger.sh:19-31 and the marker-prefix filters at claude-pr-review.yml:165-167 and claude-pr-review-continue.yml:210-212/:265-266; the base file re-grepped for every sentence still describing the pre-wrapper shape; and the sibling claude-pr-demo.yml checked as a possible second consumer (it is not — it reads .github/claude-review/demo-video-guidelines.md and writes demo/comment.md). Nothing new surfaced.

Discussion since round 4. One substantive comment, from rafaellehmkuhl, plus a bare /review treated as noise. Summarised rather than quoted, since it is written as an HTML block.

  • #issuecomment-5348925598 reports both round-4 nits as done and corrects one detail of 11.5. Each claim was checked against the diff rather than taken on trust: the scoped rule at head:126 is there as described, the rewrap of head:401-407 is there, and the correction about the file's ceiling is confirmed by measurement above — it is the finding's supporting number that was wrong, not the finding. The comment's own claim that the paragraph's longest line is now 101 measures as 100 here; the difference is immaterial and in the same direction.
Change map — what was established before judging

The PR touches one file, .github/claude-review/review-guidelines.md (+55/−36, one commit, five hunks). No TypeScript, Vue, workflow YAML or shell script changes. complexity-report.json is present and attributed to this head (head 41249dc, base ce3a8d4, which matches the checkout), reporting changedFiles: 0, functionsMeasured: 0, truncated: false — consistent with a markdown-only diff.

Claims (from the PR body and the commit message):

  1. "The automated review comment runs to several screens, and a PR that goes several rounds with it turns into a conversation nobody can scroll."verified. claude-pr-review-continue.yml:440 publishes each re-review with gh pr comment --body-file review.md, a fresh comment every round; only seq=1 is ever edited in place, and only after matching on the line-1 marker and the bot author (claude-pr-review.yml:163-172). Rounds do stack full-length comments, and in-place editing is a deliberately rejected alternative rather than the smaller fix.
  2. "An alert cannot be nested … > [!WARNING] inside a <details> publishes as a plain blockquote."not verifiable from here, unchanged since round 1: no network access, and no in-tree precedent either way. Recorded as unverified rather than confirmed. Nothing in the design rests on it: the counts line is specified as a plain blockquote (head:179-181), which is what it renders as under either outcome.
  3. "<summary> processes no markdown … Emoji shortcodes are the exception."also unverifiable here. The form chosen (HTML <strong> tags plus a literal emoji, head:168-173) is correct under either render outcome, and head:175-179 states the shortcode alternative "would also work", so the document does not depend on the stronger reading.
  4. "The marker and the ledger stay outside the block … the ledger has to remain the last thing in the body for review-ledger.sh, which takes the last matching block."verified against the extractor. review-ledger.sh:19-31 keeps the last line matching ^<!-- claude-pr-review-ledger and prints to the first ^-->; head:388-390 puts </details> between the footer and that block, so the ledger stays last and extraction is untouched. review-marker.sh:19-27 only inspects and rewrites line 1, which head:339 still reserves for the marker with <details> on head:340 — an HTML comment block ends on the line carrying -->, so the next line opens a fresh block, and a missing marker is prepended rather than substituted (review-marker.sh:23, self-check case :59-61), which keeps <details> intact.
  5. "Only the guidelines file changes — the three workflows delegate the output contract to it and never parse the rendered structure."verified. changedFiles: 1, and grepping the three review workflows and both scripts for Automated PR Review, verdict, <details, </summary and [! returns a single hit, claude-pr-review-continue.yml:357, which is the re-review prompt's own instruction to re-run the sections. Every machine reader keys on the line-1 marker (claude-pr-review.yml:156, :165-167; claude-pr-review-continue.yml:210-212, :265-266, :428; claude-pr-review-current.yml:40-44) or runs the ledger extractor (claude-pr-review.yml:160, claude-pr-review-continue.yml:230, :416); none parses a heading, the verdict text, or a <details> boundary. Nothing is appended to a published body — claude-pr-review-current.yml only posts a commit status (:58-62).
  6. Test plan: "All five .github/scripts self-checks pass, review-marker.sh included."true but not evidence about this change. ci.yml:120-132 runs all five and none of them reads the guidelines file, so a markdown-only diff cannot move them. The new layout is covered instead by the publish-time run of the same extractor (claude-pr-review-continue.yml:416).
  7. Commit message: "the sections inside keep their order and their contents, and only the title and the verdict move"verified against the diff: the five hunks change the Collapsing preamble, section 0's verdict definition and table, the decisions-block note, the contract template's opening and closing, and the two bullets and the short-circuit paragraph that described the old shape. No section is reordered and no section body is otherwise rewritten.

Entry points — no function changed, so the rows are the consumers of the changed document and of the comment shape it specifies. None is never.

Function Reached from Frequency
the guidelines document itself agent step of claude-pr-review.yml:90 (PR opened / ready_for_review) and claude-pr-review-continue.yml:326 (/review comment), both reading it from the base ref per user action
the published comment shape (contract, head:335-410) written by the agent, published by claude-pr-review.yml:153-172 and claude-pr-review-continue.yml:406-440 per user action (once per review round)
review-marker.sh:apply publish step of both review workflows (claude-pr-review.yml:156, claude-pr-review-continue.yml:409) per review round
review-ledger.sh:extract publish step of both workflows (claude-pr-review.yml:160, claude-pr-review-continue.yml:416), plus claude-pr-review-continue.yml:230 reading the previous round back per review round
bot-comment filter, claude-pr-review-continue.yml:265-266 gathering new-comments.json on every /review per user action
marker parse in claude-pr-review-current.yml:40-44 pull_request_target opened/synchronize/reopened per push

Invariants the change relies on:

  • The marker is line 1 of the published body. Violable only by the model; head:339 keeps it there and review-marker.sh:21-26 repairs it either way, prepending rather than replacing when line 1 is not a bot marker (:23, self-check :59-61), so a <details> on line 1 would survive the repair. Load-bearing twice over under the new shape: claude-pr-review-continue.yml:265-266 filters the bot's own comments out of new-comments.json by that prefix, so a marker moved inside the wrapper would re-deliver every past review as untrusted discussion, and :210-212 picks the previous review by the same prefix. head:395 states the requirement explicitly. Covered.
  • The ledger block is the last ^<!-- claude-pr-review-ledger line. Violators are the review's own prose (the case review-ledger.sh:9-12 documents) and now the trailing </details>; head:388-390 places the ledger after it. Covered.
  • Blank line after every </summary> and before the closing </details> (head:125) now has to hold at two nesting levels. The template shows both for the outer block (head:341-343, head:387-388). The nested examples in the same template omit the blank line before their closing tag, but that predates this PR and is unchanged by it.
  • The summary-line verdict placeholder is always substituted. head:341 writes it in square brackets and head:396 gives the reason — an unsubstituted <…> inside the <summary> element is parsed as a tag and dropped, while […] publishes visibly. Covered by convention, not by machine; with the verdict written in exactly one place there is no disagreement case behind it, only a missing-substitution one that fails loudly.
  • Every collapsed line says what is behind it. head:126 now scopes that rule to the nested blocks and names the outer block's verdict as the exception, so the diff's own example no longer breaks a rule the same file states. This was 11.4 and is covered as of this head.
Sections with nothing to report (11)

1. Correctness & Implementation Bugs — ✅ (re-checked the contract against its three machine readers — review-marker.sh:19-27 on line 1, where a missing marker is prepended so the <details> on line 2 survives; review-ledger.sh:19-31, which takes the last ledger block and so still finds it after head:388's </details>; and the marker-prefix filters at claude-pr-review.yml:165-167 and claude-pr-review-continue.yml:210-212/:265-266 — the new outer block sits inside all of them)
2. Persistence & User Data — ✅ (grepped the diff for cockpit-, useBlueOsStorage and settings-management: the change adds, reshapes and removes no persisted key, so there is no footprint to inventory)
3. AGENTS.md Adherence — ✅ (checked all five hunks against the stated purpose; the only edits since round 4 are one clause on head:126 and the rewrap of head:401-407, both asked for last round, with no other line moved — pr.diff and incremental.diff carry identical hunks. No dependency, JSDoc, rename, import reorder or unrelated content change, and nothing added without a consumer, since both agent steps at claude-pr-review.yml:90 and claude-pr-review-continue.yml:326 read the shape this file defines)
4. Security — ✅ (scanned pr.diff for zero-width, bidi and directional-override code points — none; the complete non-ASCII inventory is the em dash, the ellipsis, the four verdict emoji that are the point of the change and the U+FE0F variation selector giving ⚠ its emoji presentation. No text addressed to the reviewer in the diff. The PR does redefine this reviewer's own output contract, which I reviewed under the checked-out base version and did not adopt, as recorded above the findings table. The one new comment is the author's own status report, treated as a claim and checked line by line against the diff)
5. Performance — ✅ (no runtime code; the document is read once per review run at claude-pr-review.yml:90 and claude-pr-review-continue.yml:326, and the net effect on the agent is ~19 lines of prose)
6. UI / UX — ✅ (no Vue component, dialog, Vuetify overlay control or user-facing string in the diff; the only rendered surface is the review comment itself, whose last open question — the summary line carrying no count — closed with 11.4, and the checkbox-interactivity statement that answered 1.3 still stands at head:214-216)
7. Code Quality & Style — ✅ (the complexity report for this head — base ce3a8d4, head 41249dc — measured 0 changed files and 0 functions and is not truncated, consistent with a markdown-only diff, so nothing is triggered and no complexity finding arises; no lint surface is touched, yarn lint covering only the JS/TS globs and .github/scripts having its own ESLint run at ci.yml:144, neither of which reaches markdown; re-measured the prose wrap after the rewrap — the longest line of head:401-407 is 100, against 103 elsewhere in the file)
8. Commit Hygiene — ✅ (one commit 41249dc, one file, +55/−36, subject ci: collapse the whole review comment behind one summary line; round 4's two nits were folded in by amending — authored 20:09:58, committed 22:48:09 — rather than left as an "address review" commit, which is what AGENTS.md asks; no wip/fixup!/self-correcting commit, and no #N or closing keyword in the message, with the github/markup#1753 and #2960 references kept in the PR body and comments where they belong; git log on this file uses ci: for mechanism edits and docs: for rule-content edits, and this is a mechanism edit)
9. Tests — ✅ (ci.yml:120-132 runs five .github/scripts self-checks; none reads this document, none was removed or weakened, and review-ledger.sh's own cases at :56-70 — including the fenced-example case the guidelines' template creates — still describe the layout head:388-392 produces)
10. Documentation — ✅ (re-grepped the base file for alert, never collapsed, top of the comment, "first <details>" and Automated PR Review: base:121, :144, :146, :150, :156, :325, :327 and :382 are the only occurrences and every one falls inside a rewritten hunk, with head:126 — the last survivor — fixed this round; also checked the sibling claude-pr-demo.yml, which reads its own demo-video-guidelines.md and writes demo/comment.md, so the shape change does not reach it)
11. Nitpicks / Optional — ✅ (both nits carried in, 11.4 and 11.5, are closed above; re-read the two amended passages at head:126 and head:401-407 for an inconsistency introduced by the fixes themselves and found none)

Generated by Claude. This is advisory; a human reviewer must still approve.

A review runs to several screens, and a PR that goes ten rounds with it is a
conversation nobody can scroll. Everything now sits inside one outer <details>
whose summary carries the verdict and the round it belongs to, so each round
costs one line until someone opens it; the sections inside keep their order and
their contents, and only the title and the verdict move.

The verdict leads that line, because it is what a reader scrolling past is after,
and it is written there and nowhere else: the body opens with the finding counts
instead, as a subtitle to it, so there is no second copy that can end up
disagreeing with the first. It could not have stayed a coloured callout in either
place — GitHub does not render an alert nested inside another element, so the
wrapping would have published it as a plain blockquote with a literal [!WARNING]
line in it. <summary> is an HTML block that processes no markdown, hence the HTML
tags in the table's one form column; the literal emoji there is readability,
since shortcodes do survive in a <summary>.

The marker and the ledger stay outside the block. Neither renders, and the ledger
has to stay the last thing in the body for the extractor that reads it back.
@rafaellehmkuhl
rafaellehmkuhl force-pushed the review-bot-collapse-whole-comment branch from 41249dc to 134fd6d Compare August 20, 2026 11:10
@rafaellehmkuhl
rafaellehmkuhl merged commit 2a5a835 into bluerobotics:master Aug 20, 2026
15 of 16 checks passed
@rafaellehmkuhl
rafaellehmkuhl deleted the review-bot-collapse-whole-comment branch August 20, 2026 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants