Repository navigation
feat: show each pull request's Claude review round on its row and document - #11
Conversation
datapointchris
left a comment
There was a problem hiding this comment.
PR review — failure-paths
Used: Read the full diff and the head clone. Traced the tmux list-windows read, the window-open record and the diff-file write. Read gotermboard's Source and prs's run helper. Ran no failure.
Standards: No rule searched. No finding needed one.
Findings: 0 correctness · 0 design · 0 prose
No findings.
Findings as JSON
{
"used": "Read the full diff and the head clone. Traced the tmux list-windows read, the window-open record and the diff-file write. Read gotermboard's Source and prs's run helper. Ran no failure.",
"standards": "No rule searched. No finding needed one.",
"findings": []
}This review does not gate merge.
datapointchris
left a comment
There was a problem hiding this comment.
PR review — stale-prose
Used: I cloned the repo and checked out PR 11 at 7d2c9a0. I read every added sentence beside the code it describes. The files were README.md, CLAUDE.md, the prs config and prs tui help in cmd/prs, the doc comments in internal/board and internal/reviewrounds, and the comments in cmd/prs/tui_test.go. I searched the added lines for words that narrate the change, such as "now", "no longer" and "used to", and found none. I added a throwaway test that called MarkdownDocument while the detail read was still running and a round window was open, then deleted it.
Standards: I applied pull-requests.md § "Sentences the diff adds are checked for what goes stale, and the finding carries the rewrite". I also cited documentation.md § "A comment describes the code as it is, never the change or history that produced it". Both came from fleet standards search.
Findings: 0 correctness · 0 design · 3 prose
Prose
stale-prose.1 README.md:162 · internal/board/review_rounds.go:216 · README.md:146
False: three sentences say the Review section needs the detail read or a posted round, but an open round window draws it before either.
reviewSection returns early only on d == nil || !d.Held && in.roundWindow == "". It skips the empty-round case only where in.roundWindow == "". With a window open, it draws a Review section before the detail read lands, on a pull request no round has reviewed yet. The r key does exactly this.
I called MarkdownDocument with &pullrequests.DetailState{Reading: true} and the window review r #1. It returned:
# Review
A review round is running in the tmux window `review r #1`, and no reviewer has posted yet.
# Checks
...
_Reading._
The sentences:
README.md:162: "Review, Checks, Files and Comments fill in once the pull request's own read lands." Rewrite: "Checks, Files and Comments fill in once the pull request's own read lands, and say they are reading until then. Review fills in then too, except that a window running a review round shows it at once."internal/board/review_rounds.go:216: "A pull request no round reviewed has no Review section." Rewrite: "A pull request no round reviewed has no Review section unless a window running a round is open."README.md:146: "Review is the newest Claude review round, where one reviewed it". Rewrite: "Review is the newest Claude review round, where one reviewed it or a window is running one".
stale-prose.2 CLAUDE.md:101
False: a CLAUDE.md sentence is ungrammatical and does not say which posts show whole.
CLAUDE.md:101-102 reads: "a post whose marker names a version reviewrounds.Version is not shows whole rather than read wrong." The clause "a version reviewrounds.Version is not shows whole" does not parse. A reader cannot tell whether matching posts or non-matching posts show whole.
The code in placed keeps a post whole when its marker's version is not reviewrounds.Version. It also keeps a post whole when its marker names no round or no reviewer.
Rewrite: "a post whose marker names a version other than reviewrounds.Version shows whole rather than read wrong."
stale-prose.3 cmd/prs/tui_test.go:75
A value the code holds: the withRounds doc comment repeats a mapping that lives in another function.
cmd/prs/tui_test.go:75-76 says: "widget #13 all of it, gadget #5 its first three reviewers' reviews with no round comment yet, and widget #12 G1's review under a marker naming a version prs does not read." withRounds only sets g.rounds = true. The mapping is the switch number in detail, which cases on the bare numbers "13", "5" and "12" and slices reviews[:3]. A new case added to that switch leaves this comment, on a different function, out of date.
Rewrite the withRounds comment as: "withRounds has each detail answer carry the review round in testdata/review-round.json, a real one, cut per pull request as detail says." Then write one line above the switch in detail to say why each pull request gets its cut: "one finished round, one still posting, one under a version prs does not read."
Findings as JSON
{
"used": "I cloned the repo and checked out PR 11 at 7d2c9a0. I read every added sentence beside the code it describes. The files were README.md, CLAUDE.md, the `prs config` and `prs tui` help in cmd/prs, the doc comments in internal/board and internal/reviewrounds, and the comments in cmd/prs/tui_test.go. I searched the added lines for words that narrate the change, such as \"now\", \"no longer\" and \"used to\", and found none. I added a throwaway test that called MarkdownDocument while the detail read was still running and a round window was open, then deleted it.",
"standards": "I applied pull-requests.md § \"Sentences the diff adds are checked for what goes stale, and the finding carries the rewrite\". I also cited documentation.md § \"A comment describes the code as it is, never the change or history that produced it\". Both came from `fleet standards search`.",
"findings": [
{
"number": "stale-prose.1",
"section": "prose",
"sites": [
"README.md:162",
"internal/board/review_rounds.go:216",
"README.md:146"
],
"reach": "",
"body": "**False: three sentences say the Review section needs the detail read or a posted round, but an open round window draws it before either.**\n\n`reviewSection` returns early only on `d == nil || !d.Held && in.roundWindow == \"\"`. It skips the empty-round case only where `in.roundWindow == \"\"`. With a window open, it draws a Review section before the detail read lands, on a pull request no round has reviewed yet. The `r` key does exactly this.\n\nI called `MarkdownDocument` with `&pullrequests.DetailState{Reading: true}` and the window `review r #1`. It returned:\n\n```\n# Review\n\nA review round is running in the tmux window `review r #1`, and no reviewer has posted yet.\n\n# Checks\n...\n_Reading._\n```\n\nThe sentences:\n\n- `README.md:162`: \"Review, Checks, Files and Comments fill in once the pull request's own read lands.\" Rewrite: \"Checks, Files and Comments fill in once the pull request's own read lands, and say they are reading until then. Review fills in then too, except that a window running a review round shows it at once.\"\n- `internal/board/review_rounds.go:216`: \"A pull request no round reviewed has no Review section.\" Rewrite: \"A pull request no round reviewed has no Review section unless a window running a round is open.\"\n- `README.md:146`: \"**Review** is the newest Claude review round, where one reviewed it\". Rewrite: \"**Review** is the newest Claude review round, where one reviewed it or a window is running one\"."
},
{
"number": "stale-prose.2",
"section": "prose",
"sites": [
"CLAUDE.md:101"
],
"reach": "",
"body": "**False: a CLAUDE.md sentence is ungrammatical and does not say which posts show whole.**\n\n`CLAUDE.md:101-102` reads: \"a post whose marker names a version `reviewrounds.Version` is not shows whole rather than read wrong.\" The clause \"a version `reviewrounds.Version` is not shows whole\" does not parse. A reader cannot tell whether matching posts or non-matching posts show whole.\n\nThe code in `placed` keeps a post whole when its marker's version is not `reviewrounds.Version`. It also keeps a post whole when its marker names no round or no reviewer.\n\nRewrite: \"a post whose marker names a version other than `reviewrounds.Version` shows whole rather than read wrong.\""
},
{
"number": "stale-prose.3",
"section": "prose",
"sites": [
"cmd/prs/tui_test.go:75"
],
"reach": "",
"body": "**A value the code holds: the `withRounds` doc comment repeats a mapping that lives in another function.**\n\n`cmd/prs/tui_test.go:75-76` says: \"widget #13 all of it, gadget #5 its first three reviewers' reviews with no round comment yet, and widget #12 G1's review under a marker naming a version prs does not read.\" `withRounds` only sets `g.rounds = true`. The mapping is the `switch number` in `detail`, which cases on the bare numbers `\"13\"`, `\"5\"` and `\"12\"` and slices `reviews[:3]`. A new case added to that switch leaves this comment, on a different function, out of date.\n\nRewrite the `withRounds` comment as: \"withRounds has each detail answer carry the review round in testdata/review-round.json, a real one, cut per pull request as detail says.\" Then write one line above the `switch` in `detail` to say why each pull request gets its cut: \"one finished round, one still posting, one under a version prs does not read.\""
}
]
}This review does not gate merge.
datapointchris
left a comment
There was a problem hiding this comment.
PR review — code-architecture
Used: I cloned the repo, checked out PR 11 at 7d2c9a0, and read the whole diff against origin/main along with the code around it. That code covers internal/reviewrounds/rounds.go, internal/board/{review_rounds,actions,rows,table,document,board}.go, internal/config/config.go and the grammar's owner, which pull-requests.md names as builder/pr_review_round.py in the claude repo. I searched for every other site that fills a window name, matches a provider, names a finding section, orders columns or lists the posts the Review section shows. I did not build or run anything, because no design finding depends on runtime behaviour.
Standards: One rule applies: code-quality.md/Ask whatever owns a fact, the function or enum that computes it, never duplicate its logic at a second site. Searches for column-order and view-exclusion rules found nothing closer.
Findings: 0 correctness · 4 design · 0 prose
Design and architecture
code-architecture.1 internal/board/review_rounds.go:128 · internal/board/review_rounds.go:130 · internal/board/review_rounds.go:131 · internal/board/actions.go:349 · internal/board/actions.go:357 · internal/board/actions.go:358
Reach: Every new field in config.Fields (its members are Number, Repo and Slug) has to be added at both sites. So does a new placeholder in config.placeholders that a window template can use, and any change to how an action is matched to a provider. config.Placeholders() is already exported as an extension point, so new placeholders are expected. The new runs_review_round key makes this PR the first case where the two sites must give the same answer.
Two sites build a review window's name from a pull request
The name of the tmux window that runs a review round is now built in two places:
windowAction(actions.go:349,:357,:358) checks whether the action applies to the provider, buildsconfig.Fields{Number: pr.Number, Repo: pr.Repo, Slug: pr.Slug}and callsconfig.Fill(window.Window, fields). It then hands that name toopenWindowandroundWindows.openedHere.roundWindowName(review_rounds.go:128-131) repeats the provider check aslen(action.Providers) == 0 || slices.Contains(...)and builds the sameFieldsliteral with its ownFillcall.roundWindows.openthen looks the result up in tmux's window list.
The window watch only works if the two strings match exactly, and nothing makes them match. Suppose a fourth field such as Branch is added to the literal in windowAction but not in roundWindowName. The opener then creates review {branch}…, the watcher looks for a different name, and the CLAUDE cell never shows running.
Fix: add one function that both callers use, such as config.WindowAction.NameFor(pr) (string, bool) or a board helper windowFields(pr) plus appliesTo(action, pr). windowAction and roundWindowName would each call it once.
code-architecture.2 internal/reviewrounds/rounds.go:34 · internal/board/review_rounds.go:104 · internal/board/review_rounds.go:402 · internal/board/review_rounds.go:419
Reach: The next grammar version. The PR treats one as likely: reviewrounds.Version gates everything, the test fixture uses a pr-review v7 marker, and the PR body says pre-v6 rounds show whole. A v7 that renames, adds or reorders a section needs edits in step to reviewrounds.Sections, board.sectionHeading, and the hard-coded "correctness" check in claudeCell.
The finding-section vocabulary is split between the grammar package and the view
reviewrounds is the package that reads the v6 grammar, and pull-requests.md names builder/pr_review_round.py as the grammar's owner. reviewrounds holds only part of the section vocabulary:
rounds.go:34holds the section names and their order:correctness,design,prose.board/review_rounds.go:402maps each name to the heading the grammar writes (Correctness,Design and architecture,Prose) with its ownswitchon string literals.board/review_rounds.go:104decides that acorrectnessfinding colours the cell red, with a literal comparison.board/review_rounds.go:419decides the ranking order by readingreviewrounds.Sections.
Both the headings and the urgent section belong to the grammar. They are not decisions the view makes. Today a grammar version bump changes one package for parsing and a second package for display, and the compiler cannot tell when the two drift apart.
Fix: move the vocabulary into reviewrounds as one table, for example []SectionDef{{Name: "correctness", Heading: "Correctness", Blocking: true}, …}. Give it a Heading(section) function. board would then read that table and drop its own switch and literals.
code-architecture.3 internal/board/review_rounds.go:218 · internal/board/review_rounds.go:235 · internal/board/review_rounds.go:249 · internal/board/review_rounds.go:448
Reach: The next change to what the Review section shows. The PR body already names one: "Earlier rounds' posts stay there", in Conversation. Showing an earlier round's findings in Review, or a reviewer's superseded post that keptNewest drops, requires an edit to reviewSection and a matching edit to shownInReview. Otherwise the post appears in both sections or in neither.
Two functions list which posts the Review section shows
reviewSection (review_rounds.go:218) chooses what Review renders: the newest round's Reviews (:235), its Synthesis, its Finish, and every post in rounds.Whole (:249). shownInReview (:448) lists the same set a second time as a map of body strings, and conversationOf uses that map to leave those posts out of Comments. The PR body states the rule as "Comments leaves out exactly the posts Review shows". Two separate lists that happen to agree today enforce that rule.
Fix: compute the shown posts once and pass them to both functions. For example, a reviewrounds.Rounds.Shown() method, or a reviewPosts(rounds) function that reviewSection loops over and shownInReview turns into a map. Each change to Review's contents would then also change what Comments leaves out.
code-architecture.4 internal/board/rows.go:146 · internal/board/review_rounds.go:24 · internal/board/rows.go:187 · internal/board/rows.go:189
Reach: The next optional column, or any reorder of the first three columns. claudeAt = 3 is a positional index into columns. row() independently places claudeCell right after reviewCell. A column added before REVIEW moves the header without moving the cells. This PR is the first optional column, so the pattern is already set.
Three sites decide where the CLAUDE column sits
columns (rows.go:146) lists the headers. claudeAt = 3 (review_rounds.go:24) counts how many of those headers come before CLAUDE. row() (rows.go:187-189) builds cells in order and adds claudeCell after reviewCell when claudeShown is set. Before this PR, columns and row() had to agree only on order. Now they must also agree on claudeAt as a number. table.go pads cells by index against frameColumns(...), so a mismatch shifts every cell after the gap under the wrong header.
Fix: make the row a list of (Column, cell func) pairs with an optional flag. frameColumns and row() would then both read that one list, and claudeAt would no longer be needed.
Findings as JSON
{
"used": "I cloned the repo, checked out PR 11 at 7d2c9a0, and read the whole diff against origin/main along with the code around it. That code covers `internal/reviewrounds/rounds.go`, `internal/board/{review_rounds,actions,rows,table,document,board}.go`, `internal/config/config.go` and the grammar's owner, which `pull-requests.md` names as `builder/pr_review_round.py` in the claude repo. I searched for every other site that fills a window name, matches a provider, names a finding section, orders columns or lists the posts the Review section shows. I did not build or run anything, because no design finding depends on runtime behaviour.",
"standards": "One rule applies: code-quality.md/Ask whatever owns a fact, the function or enum that computes it, never duplicate its logic at a second site. Searches for column-order and view-exclusion rules found nothing closer.",
"findings": [
{
"number": "code-architecture.1",
"section": "design",
"sites": [
"internal/board/review_rounds.go:128",
"internal/board/review_rounds.go:130",
"internal/board/review_rounds.go:131",
"internal/board/actions.go:349",
"internal/board/actions.go:357",
"internal/board/actions.go:358"
],
"reach": "Every new field in `config.Fields` (its members are `Number`, `Repo` and `Slug`) has to be added at both sites. So does a new placeholder in `config.placeholders` that a window template can use, and any change to how an action is matched to a provider. `config.Placeholders()` is already exported as an extension point, so new placeholders are expected. The new `runs_review_round` key makes this PR the first case where the two sites must give the same answer.",
"body": "### Two sites build a review window's name from a pull request\n\nThe name of the tmux window that runs a review round is now built in two places:\n\n- `windowAction` (`actions.go:349`, `:357`, `:358`) checks whether the action applies to the provider, builds `config.Fields{Number: pr.Number, Repo: pr.Repo, Slug: pr.Slug}` and calls `config.Fill(window.Window, fields)`. It then hands that name to `openWindow` and `roundWindows.openedHere`.\n- `roundWindowName` (`review_rounds.go:128-131`) repeats the provider check as `len(action.Providers) == 0 || slices.Contains(...)` and builds the same `Fields` literal with its own `Fill` call. `roundWindows.open` then looks the result up in tmux's window list.\n\nThe window watch only works if the two strings match exactly, and nothing makes them match. Suppose a fourth field such as `Branch` is added to the literal in `windowAction` but not in `roundWindowName`. The opener then creates `review {branch}…`, the watcher looks for a different name, and the CLAUDE cell never shows running.\n\nFix: add one function that both callers use, such as `config.WindowAction.NameFor(pr) (string, bool)` or a board helper `windowFields(pr)` plus `appliesTo(action, pr)`. `windowAction` and `roundWindowName` would each call it once."
},
{
"number": "code-architecture.2",
"section": "design",
"sites": [
"internal/reviewrounds/rounds.go:34",
"internal/board/review_rounds.go:104",
"internal/board/review_rounds.go:402",
"internal/board/review_rounds.go:419"
],
"reach": "The next grammar version. The PR treats one as likely: `reviewrounds.Version` gates everything, the test fixture uses a `pr-review v7` marker, and the PR body says pre-v6 rounds show whole. A v7 that renames, adds or reorders a section needs edits in step to `reviewrounds.Sections`, `board.sectionHeading`, and the hard-coded \"correctness\" check in `claudeCell`.",
"body": "### The finding-section vocabulary is split between the grammar package and the view\n\n`reviewrounds` is the package that reads the v6 grammar, and `pull-requests.md` names `builder/pr_review_round.py` as the grammar's owner. `reviewrounds` holds only part of the section vocabulary:\n\n- `rounds.go:34` holds the section names and their order: `correctness`, `design`, `prose`.\n- `board/review_rounds.go:402` maps each name to the heading the grammar writes (`Correctness`, `Design and architecture`, `Prose`) with its own `switch` on string literals.\n- `board/review_rounds.go:104` decides that a `correctness` finding colours the cell red, with a literal comparison.\n- `board/review_rounds.go:419` decides the ranking order by reading `reviewrounds.Sections`.\n\nBoth the headings and the urgent section belong to the grammar. They are not decisions the view makes. Today a grammar version bump changes one package for parsing and a second package for display, and the compiler cannot tell when the two drift apart.\n\nFix: move the vocabulary into `reviewrounds` as one table, for example `[]SectionDef{{Name: \"correctness\", Heading: \"Correctness\", Blocking: true}, …}`. Give it a `Heading(section)` function. `board` would then read that table and drop its own `switch` and literals."
},
{
"number": "code-architecture.3",
"section": "design",
"sites": [
"internal/board/review_rounds.go:218",
"internal/board/review_rounds.go:235",
"internal/board/review_rounds.go:249",
"internal/board/review_rounds.go:448"
],
"reach": "The next change to what the Review section shows. The PR body already names one: \"Earlier rounds' posts stay there\", in Conversation. Showing an earlier round's findings in Review, or a reviewer's superseded post that `keptNewest` drops, requires an edit to `reviewSection` and a matching edit to `shownInReview`. Otherwise the post appears in both sections or in neither.",
"body": "### Two functions list which posts the Review section shows\n\n`reviewSection` (`review_rounds.go:218`) chooses what Review renders: the newest round's `Reviews` (`:235`), its `Synthesis`, its `Finish`, and every post in `rounds.Whole` (`:249`). `shownInReview` (`:448`) lists the same set a second time as a map of body strings, and `conversationOf` uses that map to leave those posts out of Comments. The PR body states the rule as \"Comments leaves out exactly the posts Review shows\". Two separate lists that happen to agree today enforce that rule.\n\nFix: compute the shown posts once and pass them to both functions. For example, a `reviewrounds.Rounds.Shown()` method, or a `reviewPosts(rounds)` function that `reviewSection` loops over and `shownInReview` turns into a map. Each change to Review's contents would then also change what Comments leaves out."
},
{
"number": "code-architecture.4",
"section": "design",
"sites": [
"internal/board/rows.go:146",
"internal/board/review_rounds.go:24",
"internal/board/rows.go:187",
"internal/board/rows.go:189"
],
"reach": "The next optional column, or any reorder of the first three columns. `claudeAt = 3` is a positional index into `columns`. `row()` independently places `claudeCell` right after `reviewCell`. A column added before REVIEW moves the header without moving the cells. This PR is the first optional column, so the pattern is already set.",
"body": "### Three sites decide where the CLAUDE column sits\n\n`columns` (`rows.go:146`) lists the headers. `claudeAt = 3` (`review_rounds.go:24`) counts how many of those headers come before CLAUDE. `row()` (`rows.go:187-189`) builds cells in order and adds `claudeCell` after `reviewCell` when `claudeShown` is set. Before this PR, `columns` and `row()` had to agree only on order. Now they must also agree on `claudeAt` as a number. `table.go` pads cells by index against `frameColumns(...)`, so a mismatch shifts every cell after the gap under the wrong header.\n\nFix: make the row a list of `(Column, cell func)` pairs with an `optional` flag. `frameColumns` and `row()` would then both read that one list, and `claudeAt` would no longer be needed."
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — system-design
Used: Cloned PR 11 at head 7d2c9a0. Read the full diff, review_rounds.go, rounds.go, github_detail.go (the far-side read) and the board/config wiring. Built and ran nothing.
Standards: No fleet standards search run. Both findings come from reading the code and the GraphQL read it depends on.
Findings: 2 correctness · 0 design · 0 prose
Correctness
system-design.1 internal/reviewrounds/rounds.go:155 · internal/reviewrounds/rounds.go:176 · internal/reviewrounds/rounds.go:282
Reach: Anyone who can comment or review on a watched PR. A public repo allows any GitHub user. Sequence: they post a comment opening <!-- pr-review-round v6 round=99 head=abc --> with a <details> JSON block. Read makes round 99 the Newest and the real round becomes Earlier. The row shows claude no findings in green and the document shows the forged synthesis and findings. A forged review marker with no round comment shows claude running indefinitely.
Any commenter can forge a Claude review round.
Read takes every review and comment whose first line is a pr-review or pr-review-round marker. It never checks the author. Post.Author is stored but nothing filters on it. The round number, head and findings come from the attacker's text.
The user trusts the CLAUDE cell as the verdict from their own review job. A third party can hide real findings or fake a clean result. They can also inject arbitrary markdown into the Review section.
Fix: add an allowed-author set to config, for example the login the review job posts as. Treat a marker post from any other author as a Whole post with the reason author not trusted. Never count it in a round.
system-design.2 internal/pullrequests/github_detail.go:28 · internal/board/review_rounds.go:49 · internal/reviewrounds/rounds.go:176
Reach: A PR with a finished round, then more than 50 later comments (human chatter, bots, CI). GitHub's comments(last: 50) drops the round comment. Reviews are read with last: 100 and survive. Finish is nil, so reviewOf sets running true. The row shows claude running in yellow and the document says the round has not posted its round comment. It stays that way until the PR closes.
A finished round reads as running forever once 50 later comments push its round comment out of the read.
The round comment is the only signal that a round finished. The detail read fetches only the last 50 issue comments. Review posts come from a separate 100-review window and outlast it. Finish == nil is then read as in progress. The same window drops the round comments of earlier rounds. Those rounds then read as never posted its round comment.
The far side returns a truncated list, and the reader treats missing data as a state.
Fix: read comments.totalCount against the number of nodes returned. When the list is truncated and the newest round has no Finish, show claude reviewed and do not show running. Or page the comments, or query only comments whose body starts with the round marker.
Findings as JSON
{
"used": "Cloned PR 11 at head 7d2c9a0. Read the full diff, review_rounds.go, rounds.go, github_detail.go (the far-side read) and the board/config wiring. Built and ran nothing.",
"standards": "No `fleet standards search` run. Both findings come from reading the code and the GraphQL read it depends on.",
"findings": [
{
"number": "system-design.1",
"section": "correctness",
"sites": [
"internal/reviewrounds/rounds.go:155",
"internal/reviewrounds/rounds.go:176",
"internal/reviewrounds/rounds.go:282"
],
"reach": "Anyone who can comment or review on a watched PR. A public repo allows any GitHub user. Sequence: they post a comment opening `<!-- pr-review-round v6 round=99 head=abc -->` with a `<details>` JSON block. `Read` makes round 99 the Newest and the real round becomes Earlier. The row shows `claude no findings` in green and the document shows the forged synthesis and findings. A forged review marker with no round comment shows `claude running` indefinitely.",
"body": "**Any commenter can forge a Claude review round.**\n\n`Read` takes every review and comment whose first line is a `pr-review` or `pr-review-round` marker. It never checks the author. `Post.Author` is stored but nothing filters on it. The round number, head and findings come from the attacker's text.\n\nThe user trusts the CLAUDE cell as the verdict from their own review job. A third party can hide real findings or fake a clean result. They can also inject arbitrary markdown into the Review section.\n\n**Fix:** add an allowed-author set to config, for example the login the review job posts as. Treat a marker post from any other author as a `Whole` post with the reason `author not trusted`. Never count it in a round."
},
{
"number": "system-design.2",
"section": "correctness",
"sites": [
"internal/pullrequests/github_detail.go:28",
"internal/board/review_rounds.go:49",
"internal/reviewrounds/rounds.go:176"
],
"reach": "A PR with a finished round, then more than 50 later comments (human chatter, bots, CI). GitHub's `comments(last: 50)` drops the round comment. Reviews are read with `last: 100` and survive. `Finish` is nil, so `reviewOf` sets `running` true. The row shows `claude running` in yellow and the document says the round has not posted its round comment. It stays that way until the PR closes.",
"body": "**A finished round reads as running forever once 50 later comments push its round comment out of the read.**\n\nThe round comment is the only signal that a round finished. The detail read fetches only the last 50 issue comments. Review posts come from a separate 100-review window and outlast it. `Finish == nil` is then read as in progress. The same window drops the round comments of earlier rounds. Those rounds then read as `never posted its round comment`.\n\nThe far side returns a truncated list, and the reader treats missing data as a state.\n\n**Fix:** read `comments.totalCount` against the number of nodes returned. When the list is truncated and the newest round has no `Finish`, show `claude reviewed` and do not show `running`. Or page the comments, or query only comments whose body starts with the round marker."
}
]
}This review does not gate merge.
datapointchris
left a comment
There was a problem hiding this comment.
PR review — fleet-patterns
Used: I cloned PR 11 and read internal/reviewrounds/rounds.go. I also read the other readers of pr-review v6: fleet's reviewrounds package, ccm's per_reviewer.py and the writer ~/.claude/src/builder/pr_review_round.py. I fed one fixture to two of them. The fixture holds G1 posting twice in round 1, plus two round comments. prs's Read ran through a throwaway test in my clone, which I deleted afterwards. fleet's reviewrounds.Read ran through a scratch module that pointed at ~/tools/fleet with a replace line. On that input prs counted 1 finding and listed no absent reviewer. fleet counted 2 findings and recorded G1 as not run.
Standards: go.md § "Ask the library to export what you would otherwise copy out of it" and code-quality.md § "Ask whatever owns a fact … never duplicate its logic at a second site" both apply. I searched for "two tools parse the same wire format share one library", "shared logic duplicated across repos one library one owner" and "client hand-copies the producer's format vocabulary".
Findings: 0 correctness · 1 design · 0 prose
Design and architecture
fleet-patterns.1 internal/reviewrounds/rounds.go:22 · internal/reviewrounds/rounds.go:146 · internal/reviewrounds/rounds.go:188 · internal/reviewrounds/rounds.go:384 · internal/reviewrounds/rounds.go:416 · fleet:reviewrounds/round.go:309 · fleet:reviewrounds/round.go:326 · fleet:reviewrounds/round.go:369 · fleet:reviewrounds/comment.go:289 · fleet:reviewrounds/comment.go:318 · claude-code-metrics:src/ccm/reviews/per_reviewer.py:32 · claude-code-metrics:src/ccm/reviews/per_reviewer.py:112 · claude:src/builder/pr_review_round.py:607 · claude:src/builder/pr_review_round.py:652
Reach: The next grammar change costs the most. The writer in ~/.claude owns the marker and the JSON block. Its next change to either, such as v7, a renamed key or a new block summary, must now be made in four places: prs, fleet reviewrounds, ccm and the writer. prs's own test already posts a v7 marker. On a v7 round, fleet's >= 6 gate reads the round while prs's == "v6" gate shows each post whole. A re-run that posts a reviewer or a round comment twice gives a different count in prs's CLAUDE cell than in fleet's ClickHouse rows for the same round. I reproduced that with one fixture.
prs adds a fourth reader of the pr-review v6 grammar, and it disagrees with fleet's
PR 11 adds internal/reviewrounds, which reads the markers, the <details> JSON blocks, the synthesis and the round record. fleet already exports a Go package with the same name, github.com/datapointchris/fleet/reviewrounds. Its doc comment calls it "fleet's one reader of that grammar". ccm carries a third reader in per_reviewer.py, and the writer has its own json_block and posted_in. The PR body never mentions fleet's package or why prs does not use it.
The four readers already disagree on these rules:
- A reviewer posting twice in a round. prs keeps the newest review (
keptNewest, rounds.go:416), and the writer'sposted_inkeeps the last one it reads. fleet appends every review's findings (round.go:369), and so does ccm'sfindings_of. - Two round comments for one round. prs keeps the newest (rounds.go:188). fleet keeps the first (round.go:326), and so does ccm (
setdefault, per_reviewer.py:112). - Version gate. prs reads only an exact
v6(rounds.go:22). fleet reads>= 6(comment.go:289). ccm reads only== 6. - Finding the block. prs takes any fence of three or more backticks after the last
<details>(rounds.go:384). fleet takes the first four-backtickjsonfence anywhere in the body (comment.go:318). The writer takes the last one. ccm requires a<summary>… as JSON</summary>heading in front of it.
I ran one fixture through prs's Read and through fleet's Read. In it, G1 posts two v6 reviews in round 1 with one finding each. Round comments follow with the status not run, then ran. prs returned counts=[{design 1}] absent=[]. fleet returned findings=2 reviewers=G1:not run. So prs's row and fleet's ClickHouse tables report different numbers for one round.
prs also hand-copies the writer's contract: the marker names pr-review and pr-review-round, the synthesis label, the ran status, and the Finding, Shape and Duplicate field names. ~/.claude/src/builder/pr_review_round.py owns all of these, and its reply schemas define them. That is the case go.md § "Ask the library to export what you would otherwise copy out of it" names: "least of all where the library owns the format being read".
Fix. Give the v6 grammar one Go reader and import it in prs. fleet's reviewrounds already has the logic. It cannot be imported as it stands, for two reasons. The fleet repo is private and prs is public. The package also pulls in fleet's ClickHouse writer. The fix is to move the grammar half into a small public library, such as goreviewrounds. That half is ReadMarker, ParseReview, grammar6Rounds and readRecord. Then fleet and prs both import it, and the duplicate and version rules get decided once. If prs keeps its own copy for now, the PR body should name fleet's reader. It should also say which side is right on the duplicate-review, duplicate-comment and version rules, and the side that is wrong should be fixed.
Findings as JSON
{
"used": "I cloned PR 11 and read `internal/reviewrounds/rounds.go`. I also read the other readers of `pr-review v6`: fleet's `reviewrounds` package, ccm's `per_reviewer.py` and the writer `~/.claude/src/builder/pr_review_round.py`. I fed one fixture to two of them. The fixture holds G1 posting twice in round 1, plus two round comments. prs's `Read` ran through a throwaway test in my clone, which I deleted afterwards. fleet's `reviewrounds.Read` ran through a scratch module that pointed at `~/tools/fleet` with a `replace` line. On that input prs counted 1 finding and listed no absent reviewer. fleet counted 2 findings and recorded G1 as `not run`.",
"standards": "go.md § \"Ask the library to export what you would otherwise copy out of it\" and code-quality.md § \"Ask whatever owns a fact … never duplicate its logic at a second site\" both apply. I searched for \"two tools parse the same wire format share one library\", \"shared logic duplicated across repos one library one owner\" and \"client hand-copies the producer's format vocabulary\".",
"findings": [
{
"number": "fleet-patterns.1",
"section": "design",
"sites": [
"internal/reviewrounds/rounds.go:22",
"internal/reviewrounds/rounds.go:146",
"internal/reviewrounds/rounds.go:188",
"internal/reviewrounds/rounds.go:384",
"internal/reviewrounds/rounds.go:416",
"fleet:reviewrounds/round.go:309",
"fleet:reviewrounds/round.go:326",
"fleet:reviewrounds/round.go:369",
"fleet:reviewrounds/comment.go:289",
"fleet:reviewrounds/comment.go:318",
"claude-code-metrics:src/ccm/reviews/per_reviewer.py:32",
"claude-code-metrics:src/ccm/reviews/per_reviewer.py:112",
"claude:src/builder/pr_review_round.py:607",
"claude:src/builder/pr_review_round.py:652"
],
"reach": "The next grammar change costs the most. The writer in `~/.claude` owns the marker and the JSON block. Its next change to either, such as `v7`, a renamed key or a new block summary, must now be made in four places: prs, fleet `reviewrounds`, ccm and the writer. prs's own test already posts a `v7` marker. On a `v7` round, fleet's `>= 6` gate reads the round while prs's `== \"v6\"` gate shows each post whole. A re-run that posts a reviewer or a round comment twice gives a different count in prs's `CLAUDE` cell than in fleet's ClickHouse rows for the same round. I reproduced that with one fixture.",
"body": "### prs adds a fourth reader of the `pr-review v6` grammar, and it disagrees with fleet's\n\nPR 11 adds `internal/reviewrounds`, which reads the markers, the `<details>` JSON blocks, the synthesis and the round record. fleet already exports a Go package with the same name, `github.com/datapointchris/fleet/reviewrounds`. Its doc comment calls it \"fleet's one reader of that grammar\". ccm carries a third reader in `per_reviewer.py`, and the writer has its own `json_block` and `posted_in`. The PR body never mentions fleet's package or why prs does not use it.\n\nThe four readers already disagree on these rules:\n\n- **A reviewer posting twice in a round.** prs keeps the newest review (`keptNewest`, rounds.go:416), and the writer's `posted_in` keeps the last one it reads. fleet appends every review's findings (round.go:369), and so does ccm's `findings_of`.\n- **Two round comments for one round.** prs keeps the newest (rounds.go:188). fleet keeps the first (round.go:326), and so does ccm (`setdefault`, per_reviewer.py:112).\n- **Version gate.** prs reads only an exact `v6` (rounds.go:22). fleet reads `>= 6` (comment.go:289). ccm reads only `== 6`.\n- **Finding the block.** prs takes any fence of three or more backticks after the last `<details>` (rounds.go:384). fleet takes the first four-backtick `json` fence anywhere in the body (comment.go:318). The writer takes the last one. ccm requires a `<summary>… as JSON</summary>` heading in front of it.\n\nI ran one fixture through prs's `Read` and through fleet's `Read`. In it, G1 posts two v6 reviews in round 1 with one finding each. Round comments follow with the status `not run`, then `ran`. prs returned `counts=[{design 1}] absent=[]`. fleet returned `findings=2 reviewers=G1:not run`. So prs's row and fleet's ClickHouse tables report different numbers for one round.\n\nprs also hand-copies the writer's contract: the marker names `pr-review` and `pr-review-round`, the `synthesis` label, the `ran` status, and the `Finding`, `Shape` and `Duplicate` field names. `~/.claude/src/builder/pr_review_round.py` owns all of these, and its reply schemas define them. That is the case go.md § \"Ask the library to export what you would otherwise copy out of it\" names: \"least of all where the library owns the format being read\".\n\n**Fix.** Give the v6 grammar one Go reader and import it in prs. fleet's `reviewrounds` already has the logic. It cannot be imported as it stands, for two reasons. The fleet repo is private and prs is public. The package also pulls in fleet's ClickHouse writer. The fix is to move the grammar half into a small public library, such as `goreviewrounds`. That half is `ReadMarker`, `ParseReview`, `grammar6Rounds` and `readRecord`. Then fleet and prs both import it, and the duplicate and version rules get decided once. If prs keeps its own copy for now, the PR body should name fleet's reader. It should also say which side is right on the duplicate-review, duplicate-comment and version rules, and the side that is wrong should be fixed."
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — useless-test
Used: Cloned PR 11 and checked it out. Ran the new review-round test, the edited board test and document_test with HOME pointed at my own directory; all pass. Broke shownInReview so round posts stayed in Comments: the new test failed ("G1.1 should read once … not 3 times"). Reverted the change.
Standards: testing.md/Test behavior the user cares about, not what this commit happened to change; pull-requests.md/Tests the diff adds are checked for whether they test the program or the change; testing.md/Never assert on rendered output — assert the value it was built from (its interpolated-value and computed-value exceptions keep both edited tests).
Findings: 0 correctness · 0 design · 0 prose
No findings.
Findings as JSON
{
"used": "Cloned PR 11 and checked it out. Ran the new review-round test, the edited board test and document_test with HOME pointed at my own directory; all pass. Broke shownInReview so round posts stayed in Comments: the new test failed (\"G1.1 should read once … not 3 times\"). Reverted the change.",
"standards": "testing.md/Test behavior the user cares about, not what this commit happened to change; pull-requests.md/Tests the diff adds are checked for whether they test the program or the change; testing.md/Never assert on rendered output — assert the value it was built from (its interpolated-value and computed-value exceptions keep both edited tests).",
"findings": []
}This review does not gate merge.
There was a problem hiding this comment.
PR review — contracts
Used: I built the PR head with go build. I ran prs config show against an isolated XDG config that holds runs_review_round on two actions. It printed key r claude review offered, runs review rounds. I checked prs's parser against the program that writes the posts, claude:src/builder/pr_review_round.py (marker, as_json, render_round, Run.status), and against fleet's reader in fleet:reviewrounds/grammar.go. I checked the skill's round rules in claude:skills/review-pr/SKILL.md and the dotfiles template dotfiles:configs/common/.config/prs/config.yml.tmpl. A throwaway Go test fed reviewrounds.Read and the board's claudeCell and roundStanding two inputs. The first was a round with reviews and no round comment. The second was a round comment whose synthesis was skipped. I deleted the test afterwards.
Standards: These rules apply: pull-requests.md § "The comment carries a version marker and a stable finding grammar", cli-design.md § "A lifecycle is one --status enum, spelled in the field's own words", and release.md § "A release that needs another repos change ships after that change is live". prs reads the v6 marker, the JSON block and the round record the way the first rule fixes them. The PR body already orders the dotfiles change after the release, as the third rule asks.
Findings: 0 correctness · 2 design · 0 prose
Design and architecture
contracts.1 internal/board/review_rounds.go:49 · internal/board/review_rounds.go:93 · internal/board/review_rounds.go:278
Reach: This happens on every pull request whose newest v6 round has reviews and no round comment. The writer leaves a round that way when the round comment fails to post (claude:src/builder/pr_review_round.py:1634, RoundError) or when fleet agents cancel stops the dispatch. claude:skills/review-pr/SKILL.md:96-113 names this state "a round cut short" and says to dispatch again to continue it. My probe fed reviewrounds.Read one G1 review from 2026-09-01 with no round comment. The cell read claude running. The document read Round 2 of abc1234 is running, and its round comment has not posted.
claude running also means a round that was cut short
prs counts a round as running whenever its newest round has no round comment (newest.Finish == nil). The program that writes the posts uses that same state for something else. The /review-pr skill says: "A round cut short is continued, never started again. A dispatch on a pull request whose newest round has reviews and no round comment continues that round under the same number." It also says: "Where the dispatch ended with no round comment posted, dispatching once more continues the round."
So a round that died has no round comment, just like a live one. prs shows claude running in yellow for it, with no time limit. The document says "is running". The reader is told to wait, but the writer's own docs say the fix is to dispatch again. The cut-short round would read as running for weeks, until someone guesses.
prs already has the honest words for this state. earlierLine writes "never posted its round comment" for an earlier round in exactly this state.
Run: a throwaway test fed one v6 review dated 2026-09-01, with no round comment, and evaluated it on 2026-11-08. The cell was claude running. The standing line was Round 2 of abc1234 is running, and its round comment has not posted. G1 raised 1 finding so far.
Fix: use running only for a window prs saw open. Name the other state in the writer's words. For example, the cell could read claude unfinished and the document could say "Round 2 has no round comment, posted 67d ago. Dispatching again continues it." A cheaper option is to read running only while the round's newest post is younger than the slowest reviewer's run, and unfinished after that.
Rule: cli-design.md § "A lifecycle is one --status enum, spelled in the field's own words". The writer has two states here, running and cut short, and prs gives both one word.
contracts.2 internal/reviewrounds/rounds.go:370 · internal/board/review_rounds.go:312
Reach: This happens on every finished round whose synthesis the writer skipped. claude:src/builder/pr_review_round.py:1336 sets run.skipped = 'fewer than two findings to share a cause'. The record then carries status: "skipped". My probe fed roundStanding that record. It returned synthesis did not run: fewer than two findings to share a cause.
A skipped reviewer reads as one that did not run
The writer's round record has four statuses: ran, skipped, not run and not posted (Run.status, claude:src/builder/pr_review_round.py:360). Its own round comment keeps them apart. Failures go under **Did not run**, and skips go under **Skipped** (render_round, lines 1218-1226).
finishOf keeps every status other than ran as Absent. roundStanding then writes each one as "X did not run: why". So a synthesis skipped on purpose for a round with fewer than two findings reads in prs as a failure. The round comment it came from says the opposite. This is common: any round with zero or one finding skips the synthesis.
Run: a round record with {"label":"synthesis","status":"skipped","why":"fewer than two findings to share a cause"} produced Round 2 of abc1234 finished 67d ago. G1 raised 1 finding: 1 correctness. synthesis did not run: fewer than two findings to share a cause.
Fix: keep skipped apart in finishOf, for example as Absent{Label, Why, Skipped bool} or as a separate Skipped slice. Write it as "synthesis skipped: fewer than two findings to share a cause", in the round comment's own word.
Findings as JSON
{
"used": "I built the PR head with `go build`. I ran `prs config show` against an isolated XDG config that holds `runs_review_round` on two actions. It printed `key r claude review offered, runs review rounds`. I checked prs's parser against the program that writes the posts, `claude:src/builder/pr_review_round.py` (marker, `as_json`, `render_round`, `Run.status`), and against fleet's reader in `fleet:reviewrounds/grammar.go`. I checked the skill's round rules in `claude:skills/review-pr/SKILL.md` and the dotfiles template `dotfiles:configs/common/.config/prs/config.yml.tmpl`. A throwaway Go test fed `reviewrounds.Read` and the board's `claudeCell` and `roundStanding` two inputs. The first was a round with reviews and no round comment. The second was a round comment whose synthesis was `skipped`. I deleted the test afterwards.\n",
"standards": "These rules apply: pull-requests.md § \"The comment carries a version marker and a stable finding grammar\", cli-design.md § \"A lifecycle is one --status enum, spelled in the field's own words\", and release.md § \"A release that needs another repos change ships after that change is live\". prs reads the v6 marker, the JSON block and the round record the way the first rule fixes them. The PR body already orders the dotfiles change after the release, as the third rule asks.",
"findings": [
{
"number": "contracts.1",
"section": "design",
"sites": [
"internal/board/review_rounds.go:49",
"internal/board/review_rounds.go:93",
"internal/board/review_rounds.go:278"
],
"reach": "This happens on every pull request whose newest v6 round has reviews and no round comment. The writer leaves a round that way when the round comment fails to post (`claude:src/builder/pr_review_round.py:1634`, `RoundError`) or when `fleet agents cancel` stops the dispatch. `claude:skills/review-pr/SKILL.md:96-113` names this state \"a round cut short\" and says to dispatch again to continue it. My probe fed `reviewrounds.Read` one G1 review from 2026-09-01 with no round comment. The cell read `claude running`. The document read `Round 2 of `abc1234` is running, and its round comment has not posted.`",
"body": "### `claude running` also means a round that was cut short\n\nprs counts a round as running whenever its newest round has no round comment (`newest.Finish == nil`). The program that writes the posts uses that same state for something else. The `/review-pr` skill says: \"A round cut short is continued, never started again. A dispatch on a pull request whose newest round has reviews and no round comment continues that round under the same number.\" It also says: \"Where the dispatch ended with no round comment posted, dispatching once more continues the round.\"\n\nSo a round that died has no round comment, just like a live one. prs shows `claude running` in yellow for it, with no time limit. The document says \"is running\". The reader is told to wait, but the writer's own docs say the fix is to dispatch again. The cut-short round would read as running for weeks, until someone guesses.\n\nprs already has the honest words for this state. `earlierLine` writes \"never posted its round comment\" for an earlier round in exactly this state.\n\nRun: a throwaway test fed one v6 review dated 2026-09-01, with no round comment, and evaluated it on 2026-11-08. The cell was ` claude running`. The standing line was `Round 2 of `abc1234` is running, and its round comment has not posted. G1 raised 1 finding so far`.\n\n**Fix:** use `running` only for a window prs saw open. Name the other state in the writer's words. For example, the cell could read `claude unfinished` and the document could say \"Round 2 has no round comment, posted 67d ago. Dispatching again continues it.\" A cheaper option is to read `running` only while the round's newest post is younger than the slowest reviewer's run, and `unfinished` after that.\n\n**Rule:** cli-design.md § \"A lifecycle is one --status enum, spelled in the field's own words\". The writer has two states here, running and cut short, and prs gives both one word."
},
{
"number": "contracts.2",
"section": "design",
"sites": [
"internal/reviewrounds/rounds.go:370",
"internal/board/review_rounds.go:312"
],
"reach": "This happens on every finished round whose synthesis the writer skipped. `claude:src/builder/pr_review_round.py:1336` sets `run.skipped = 'fewer than two findings to share a cause'`. The record then carries `status: \"skipped\"`. My probe fed `roundStanding` that record. It returned `synthesis did not run: fewer than two findings to share a cause.`",
"body": "### A skipped reviewer reads as one that did not run\n\nThe writer's round record has four statuses: `ran`, `skipped`, `not run` and `not posted` (`Run.status`, `claude:src/builder/pr_review_round.py:360`). Its own round comment keeps them apart. Failures go under `**Did not run**`, and skips go under `**Skipped**` (`render_round`, lines 1218-1226).\n\n`finishOf` keeps every status other than `ran` as `Absent`. `roundStanding` then writes each one as \"X did not run: why\". So a synthesis skipped on purpose for a round with fewer than two findings reads in prs as a failure. The round comment it came from says the opposite. This is common: any round with zero or one finding skips the synthesis.\n\nRun: a round record with `{\"label\":\"synthesis\",\"status\":\"skipped\",\"why\":\"fewer than two findings to share a cause\"}` produced `Round 2 of `abc1234` finished 67d ago. G1 raised 1 finding: 1 correctness. synthesis did not run: fewer than two findings to share a cause.`\n\n**Fix:** keep `skipped` apart in `finishOf`, for example as `Absent{Label, Why, Skipped bool}` or as a separate `Skipped` slice. Write it as \"synthesis skipped: fewer than two findings to share a cause\", in the round comment's own word."
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — G1
Used: I built the PR head and ran go test ./..., which passed. I put a scratch program in my clone that calls reviewrounds.Read on live reviews and comments fetched with gh api. It ran on prs #7, #8 and #10, on claude #16 and #21, on dotfiles #97–#100 and on ichrisbirch #277. I called claudeCell, roundStanding and earlierLine on claude#21's round 1. I ran prs tui in a private tmux server against real GitHub, with HOME, XDG paths and the registry inside my directory. I read claude/src/builder/pr_review_round.py, which renders the posts.
Standards: code-quality.md § "Parsing or escaping JSON, YAML, CSV, a URL or a shell word follows the format's grammar, never a regex of the values seen so far" · code-quality.md § "A result that can come back partial carries the signal that tells it from a whole one" · searches for a bound on an inferred running state found no rule.
Findings: 3 correctness · 0 design · 1 prose
Correctness
G1.1 internal/reviewrounds/rounds.go:385
Reach: Any reviewer whose finding body or synthesis shape contains the text <details>. as_json in claude/src/builder/pr_review_round.py:1023 writes the block with json.dumps(..., ensure_ascii=False), which leaves < unescaped. This PR's own body asks reviewers to check "that the block is found after the last <details>", so reviews of PR 11 are the first likely hit. This finding contains the string too.
A finding that mentions <details> makes prs drop that reviewer's whole review.
jsonBlock starts at strings.LastIndex(body, "<details>"). A finding whose text contains <details> puts that string inside the JSON block itself, after the real <details>. The scan then starts mid-JSON and finds no opening fence. The result is "its collapsed block holds no json fence".
I ran it on PR #10's live round. I appended "The block is found after the last <details> in the body." to G1's first finding and re-rendered the review the way as_json does. Read then gave G1 findings=0 unread="its collapsed block holds no json fence". The round's counts went from 1 correctness · 5 design · 2 prose to 4 design · 2 prose. The row's cell turns from red to cyan, and the round's only correctness finding disappears from it. Nothing on the row says a review went unread.
This breaks code-quality.md § "Parsing or escaping JSON, YAML, CSV, a URL or a shell word follows the format's grammar, never a regex of the values seen so far". The extractor is bounded by a string the JSON content can contain, not by the format's own delimiter.
Fix: find the fence by lines, not by <details>. JSON escapes newlines inside strings, so a line that is exactly three or more backticks followed by json can only be the real opener or markdown above it. Take the last such line in the body, and read to the matching closing fence. Add a test case where a finding body contains <details>.
G1.2 internal/board/review_rounds.go:101 · internal/board/review_rounds.go:388 · internal/reviewrounds/rounds.go:212 · internal/reviewrounds/rounds.go:370
Reach: claude#21, round 1. A usage limit refused all eight reviewers, and the round comment posted with **Ran**: none. Until round 2 ran, prs would have shown that row as green claude no findings.
A round where no reviewer ran reads as a green claude no findings.
Counts() sums findings from the reviews that posted. claudeCell reads an empty count as a clean result. It never checks whether anyone reviewed.
I fed claude#21's round-1 comment alone to the board's functions. They returned:
- cell:
claude no findings, in green - earlier line:
Round 1 ofc46521cfinished 4d ago, with no findings.
The Review section's text is correct. It lists every reviewer under "did not run". The row and the earlier-rounds line say the opposite.
Two neighbouring cases have the same cause:
- Unreadable reviews drop out silently. A review whose block cannot be read adds nothing to the counts, with no mark on the cell.
- Skipped reviewers read as failed.
finishOftreats every status other thanranas absent. That includesskipped, which the round comment lists separately under Skipped. The section then prints "synthesis did not run: fewer than two findings to share a cause".
This is code-quality.md § "A result that can come back partial carries the signal that tells it from a whole one". The round record carries that signal, and the cell drops it.
Fix:
- Keep each reviewer's status in
Finish, and keepskippedapart fromnot runandnot posted. - Where no reviewer ran, show the cell as something like
claude did not run, in yellow. Write the earlier line the same way. - Where some reviewers did not run or could not be read, add that to the counts, for example
claude 2 correctness · 3 not run.
G1.3 internal/board/review_rounds.go:50 · internal/board/review_rounds.go:277
Reach: unproven — what would settle it is a v6 round whose reviews posted and whose round comment never did. pull-requests.md § "The comment carries a version marker and a stable finding grammar" names that state: "the newest round's reviews posted and its round comment did not". It is cleared only when someone runs the round again. I found no such round in the 20-odd PRs I scanned.
A round whose round comment never posts shows claude running forever.
reviewOf sets running whenever the newest round has no Finish, and puts no time limit on it. A round whose posting step died keeps the row yellow claude running indefinitely. The document says "Round N … is running, and its round comment has not posted". That reads as work in progress. Nothing tells the user to start the round again. The earlier-rounds line already handles this state: earlierLine writes "never posted its round comment". Only the newest round assumes the comment is still coming.
Fix: give the running state an age. Past a bound longer than a normal round, say an hour, show the cell as claude stalled and the section line as "Round N's last post landed X ago and its round comment never posted". Use PostedAt() for that age. The window-open case can keep its current rule.
Prose
G1.4 CLAUDE.md:103
The CLAUDE.md sentence about unread versions does not parse.
CLAUDE.md:102–103 says: "a post whose marker names a version reviewrounds.Version is not shows whole rather than read wrong." The words "is not shows" break the sentence. A reader has to work out that the missing word is "that".
Rewrite: "A post whose marker names any version other than reviewrounds.Version shows whole rather than read wrong."
Findings as JSON
{
"used": "I built the PR head and ran `go test ./...`, which passed. I put a scratch program in my clone that calls `reviewrounds.Read` on live reviews and comments fetched with `gh api`. It ran on prs #7, #8 and #10, on claude #16 and #21, on dotfiles #97–#100 and on ichrisbirch #277. I called `claudeCell`, `roundStanding` and `earlierLine` on claude#21's round 1. I ran `prs tui` in a private tmux server against real GitHub, with HOME, XDG paths and the registry inside my directory. I read `claude/src/builder/pr_review_round.py`, which renders the posts.",
"standards": "code-quality.md § \"Parsing or escaping JSON, YAML, CSV, a URL or a shell word follows the format's grammar, never a regex of the values seen so far\" · code-quality.md § \"A result that can come back partial carries the signal that tells it from a whole one\" · searches for a bound on an inferred running state found no rule.",
"findings": [
{
"number": "G1.1",
"section": "correctness",
"sites": [
"internal/reviewrounds/rounds.go:385"
],
"reach": "Any reviewer whose finding body or synthesis shape contains the text `<details>`. `as_json` in `claude/src/builder/pr_review_round.py:1023` writes the block with `json.dumps(..., ensure_ascii=False)`, which leaves `<` unescaped. This PR's own body asks reviewers to check \"that the block is found after the last `<details>`\", so reviews of PR 11 are the first likely hit. This finding contains the string too.",
"body": "**A finding that mentions `<details>` makes prs drop that reviewer's whole review.**\n\n`jsonBlock` starts at `strings.LastIndex(body, \"<details>\")`. A finding whose text contains `<details>` puts that string inside the JSON block itself, after the real `<details>`. The scan then starts mid-JSON and finds no opening fence. The result is \"its collapsed block holds no json fence\".\n\nI ran it on PR #10's live round. I appended \"The block is found after the last `<details>` in the body.\" to G1's first finding and re-rendered the review the way `as_json` does. `Read` then gave `G1 findings=0 unread=\"its collapsed block holds no json fence\"`. The round's counts went from `1 correctness · 5 design · 2 prose` to `4 design · 2 prose`. The row's cell turns from red to cyan, and the round's only correctness finding disappears from it. Nothing on the row says a review went unread.\n\nThis breaks `code-quality.md` § \"Parsing or escaping JSON, YAML, CSV, a URL or a shell word follows the format's grammar, never a regex of the values seen so far\". The extractor is bounded by a string the JSON content can contain, not by the format's own delimiter.\n\nFix: find the fence by lines, not by `<details>`. JSON escapes newlines inside strings, so a line that is exactly three or more backticks followed by `json` can only be the real opener or markdown above it. Take the last such line in the body, and read to the matching closing fence. Add a test case where a finding body contains `<details>`."
},
{
"number": "G1.2",
"section": "correctness",
"sites": [
"internal/board/review_rounds.go:101",
"internal/board/review_rounds.go:388",
"internal/reviewrounds/rounds.go:212",
"internal/reviewrounds/rounds.go:370"
],
"reach": "claude#21, round 1. A usage limit refused all eight reviewers, and the round comment posted with `**Ran**: none`. Until round 2 ran, prs would have shown that row as green `claude no findings`.",
"body": "**A round where no reviewer ran reads as a green `claude no findings`.**\n\n`Counts()` sums findings from the reviews that posted. `claudeCell` reads an empty count as a clean result. It never checks whether anyone reviewed.\n\nI fed claude#21's round-1 comment alone to the board's functions. They returned:\n\n- cell: `claude no findings`, in green\n- earlier line: `Round 1 of `c46521c` finished 4d ago, with no findings.`\n\nThe Review section's text is correct. It lists every reviewer under \"did not run\". The row and the earlier-rounds line say the opposite.\n\nTwo neighbouring cases have the same cause:\n\n- **Unreadable reviews drop out silently.** A review whose block cannot be read adds nothing to the counts, with no mark on the cell.\n- **Skipped reviewers read as failed.** `finishOf` treats every status other than `ran` as absent. That includes `skipped`, which the round comment lists separately under **Skipped**. The section then prints \"synthesis did not run: fewer than two findings to share a cause\".\n\nThis is `code-quality.md` § \"A result that can come back partial carries the signal that tells it from a whole one\". The round record carries that signal, and the cell drops it.\n\nFix:\n\n- Keep each reviewer's status in `Finish`, and keep `skipped` apart from `not run` and `not posted`.\n- Where no reviewer ran, show the cell as something like `claude did not run`, in yellow. Write the earlier line the same way.\n- Where some reviewers did not run or could not be read, add that to the counts, for example `claude 2 correctness · 3 not run`."
},
{
"number": "G1.3",
"section": "correctness",
"sites": [
"internal/board/review_rounds.go:50",
"internal/board/review_rounds.go:277"
],
"reach": "unproven — what would settle it is a v6 round whose reviews posted and whose round comment never did. `pull-requests.md` § \"The comment carries a version marker and a stable finding grammar\" names that state: \"the newest round's reviews posted and its round comment did not\". It is cleared only when someone runs the round again. I found no such round in the 20-odd PRs I scanned.",
"body": "**A round whose round comment never posts shows `claude running` forever.**\n\n`reviewOf` sets `running` whenever the newest round has no `Finish`, and puts no time limit on it. A round whose posting step died keeps the row yellow `claude running` indefinitely. The document says \"Round N … is running, and its round comment has not posted\". That reads as work in progress. Nothing tells the user to start the round again. The earlier-rounds line already handles this state: `earlierLine` writes \"never posted its round comment\". Only the newest round assumes the comment is still coming.\n\nFix: give the running state an age. Past a bound longer than a normal round, say an hour, show the cell as `claude stalled` and the section line as \"Round N's last post landed X ago and its round comment never posted\". Use `PostedAt()` for that age. The window-open case can keep its current rule."
},
{
"number": "G1.4",
"section": "prose",
"sites": [
"CLAUDE.md:103"
],
"reach": "",
"body": "**The CLAUDE.md sentence about unread versions does not parse.**\n\nCLAUDE.md:102–103 says: \"a post whose marker names a version `reviewrounds.Version` is not shows whole rather than read wrong.\" The words \"is not shows\" break the sentence. A reader has to work out that the missing word is \"that\".\n\nRewrite: \"A post whose marker names any version other than `reviewrounds.Version` shows whole rather than read wrong.\""
}
]
}This review does not gate merge.
datapointchris
left a comment
There was a problem hiding this comment.
PR review — synthesis
Used: the 16 findings of round 1, from 8 review(s), and nothing else
Standards: none, because this step reads only the findings
Findings: 0 correctness · 3 design · 0 prose
Design and architecture
synthesis.1 explains G1.3 · contracts.1 · system-design.2
The running state is decided from a missing round comment (Finish == nil), and the missing comment has several causes. The view treats absent data as a state. G1.3 shows a round whose comment never posted. contracts.1 shows a round cut short, with the same claude running reading. system-design.2 shows a comment pushed out of the 50-comment read window. In each case the cell and the section say running for a round that is not running. Sites: review_rounds.go:49 and :278.
synthesis.2 explains G1.2 · contracts.2
The round record's skipped status is collapsed into the absent or empty case in finishOf and the counts. The cell and the section then lose the difference between a skipped reviewer, a reviewer that did not run, and a round where no reviewer ran. G1.2 names the skipped case and the no-reviewer-ran case. contracts.2 names the skipped synthesis case. Sites: rounds.go:370 and review_rounds.go.
synthesis.3 explains code-architecture.1 · code-architecture.2 · code-architecture.3 · code-architecture.4
The same fact is decided at two or more sites, and nothing ties the copies together. The window name is built in windowAction and roundWindowName. The Review post set is listed in reviewSection and shownInReview. The CLAUDE column position is set in columns, claudeAt and row(). The section vocabulary is split between reviewrounds and board.
One defect, raised more than once
G1.4 · stale-prose.2: The CLAUDE.md sentence about unread versions is ungrammatical ("is not shows whole") at CLAUDE.md:101-103.
Shapes as JSON
{
"shapes": [
{
"number": "synthesis.1",
"section": "design",
"explains": [
"G1.3",
"contracts.1",
"system-design.2"
],
"body": "The running state is decided from a missing round comment (`Finish == nil`), and the missing comment has several causes. The view treats absent data as a state. G1.3 shows a round whose comment never posted. contracts.1 shows a round cut short, with the same `claude running` reading. system-design.2 shows a comment pushed out of the 50-comment read window. In each case the cell and the section say `running` for a round that is not running. Sites: review_rounds.go:49 and :278."
},
{
"number": "synthesis.2",
"section": "design",
"explains": [
"G1.2",
"contracts.2"
],
"body": "The round record's `skipped` status is collapsed into the absent or empty case in `finishOf` and the counts. The cell and the section then lose the difference between a skipped reviewer, a reviewer that did not run, and a round where no reviewer ran. G1.2 names the skipped case and the no-reviewer-ran case. contracts.2 names the skipped synthesis case. Sites: rounds.go:370 and review_rounds.go."
},
{
"number": "synthesis.3",
"section": "design",
"explains": [
"code-architecture.1",
"code-architecture.2",
"code-architecture.3",
"code-architecture.4"
],
"body": "The same fact is decided at two or more sites, and nothing ties the copies together. The window name is built in `windowAction` and `roundWindowName`. The Review post set is listed in `reviewSection` and `shownInReview`. The CLAUDE column position is set in `columns`, `claudeAt` and `row()`. The section vocabulary is split between `reviewrounds` and `board`."
}
],
"duplicates": [
{
"defect": "The CLAUDE.md sentence about unread versions is ungrammatical (\"is not shows whole\") at CLAUDE.md:101-103.",
"findings": [
"G1.4",
"stale-prose.2"
]
}
]
}This review does not gate merge.
PR review round 1Ran: G1, useless-test, code-architecture, fleet-patterns, system-design, stale-prose, failure-paths, contracts, synthesis
Round record{
"grammar": 6,
"round": 1,
"repo": "datapointchris/prs",
"pr": 11,
"head": "7d2c9a01ac62129babb2254cbdf2884ee18fb2bd",
"experiment": "none",
"dispatch": "20261108T074255Z-pr-review",
"roster": "1e89b64f0fb2c38851e32d4f454928d3f98e04ab",
"harness": "1b0df0090dc84b9bf846796f6afafba9a2ef8e48",
"machine": "archlinux",
"started": "2026-11-08T07:43:14Z",
"finished": "2026-11-08T07:52:10Z",
"reviewers": [
{
"label": "G1",
"agent": "pr-reviewer",
"model": "opus",
"effort": "high",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/general.md",
"prompt_sha": "a0f85e3b82b4",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 437,
"tokens": {
"input": 70,
"cache_write": 74345,
"cache_read": 1780196,
"output": 19731
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 70,
"cache_write": 74345,
"cache_read": 1780196,
"output": 19731
}
},
"session": "01a1ba1c-8573-72df-b62f-0f43eca0ffbb",
"worklog": "1329.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/11#pullrequestreview-5817102772",
"review_id": 5817102772,
"findings": {
"correctness": 3,
"design": 0,
"prose": 1
},
"numbers": [
"G1.1",
"G1.2",
"G1.3",
"G1.4"
],
"rules": {
"proposed": 1,
"keys": [
"rule/code-quality/a-running-state-inferred-from-a-missing-completion-record-carries-its-age-because-the-record-can-fail-to-arrive"
],
"filed": true,
"why": "",
"path": "/home/fleet/.cache/run-pr-reviewers/datapointchris-prs-pr11-round1-20261108T074255Z/records/G1.rules.json"
},
"head": "7d2c9a01ac62129babb2254cbdf2884ee18fb2bd",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-prs-pr11-round1-20261108T074255Z/records",
"started": "2026-11-08T07:44:08Z",
"finished": "2026-11-08T07:51:32Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
},
{
"label": "useless-test",
"agent": "pr-useless-test-reviewer",
"model": "opus",
"effort": "high",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/useless-test.md",
"prompt_sha": "3700d72236e3",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 153,
"tokens": {
"input": 20,
"cache_write": 28180,
"cache_read": 233893,
"output": 4655
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 20,
"cache_write": 28180,
"cache_read": 233893,
"output": 4655
}
},
"session": "01a1ba1c-af95-70b1-b545-1b5ccdbc0426",
"worklog": "1329.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/11#pullrequestreview-5817091724",
"review_id": 5817091724,
"findings": {
"correctness": 0,
"design": 0,
"prose": 0
},
"numbers": [],
"rules": {},
"head": "7d2c9a01ac62129babb2254cbdf2884ee18fb2bd",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-prs-pr11-round1-20261108T074255Z/records",
"started": "2026-11-08T07:44:20Z",
"finished": "2026-11-08T07:47:00Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
},
{
"label": "code-architecture",
"agent": "pr-code-architecture-reviewer",
"model": "opus",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "d9be86eb4324",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 87,
"tokens": {
"input": 22,
"cache_write": 37615,
"cache_read": 330536,
"output": 7153
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 22,
"cache_write": 37615,
"cache_read": 330536,
"output": 7153
}
},
"session": "01a1ba1c-7d73-73e5-ba17-164562961fb0",
"worklog": "1329.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/11#pullrequestreview-5817083830",
"review_id": 5817083830,
"findings": {
"correctness": 0,
"design": 4,
"prose": 0
},
"numbers": [
"code-architecture.1",
"code-architecture.2",
"code-architecture.3",
"code-architecture.4"
],
"rules": {},
"head": "7d2c9a01ac62129babb2254cbdf2884ee18fb2bd",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-prs-pr11-round1-20261108T074255Z/records",
"started": "2026-11-08T07:43:14Z",
"finished": "2026-11-08T07:44:43Z",
"account": "live",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "fleet-patterns",
"agent": "pr-fleet-patterns-reviewer",
"model": "opus",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "eae343d1d8d5",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 215,
"tokens": {
"input": 44,
"cache_write": 57300,
"cache_read": 928782,
"output": 12110
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 44,
"cache_write": 57300,
"cache_read": 928782,
"output": 12110
}
},
"session": "01a1ba1c-7f31-7a21-ba6a-ce8e5d50057f",
"worklog": "1329.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/11#pullrequestreview-5817091386",
"review_id": 5817091386,
"findings": {
"correctness": 0,
"design": 1,
"prose": 0
},
"numbers": [
"fleet-patterns.1"
],
"rules": {},
"head": "7d2c9a01ac62129babb2254cbdf2884ee18fb2bd",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-prs-pr11-round1-20261108T074255Z/records",
"started": "2026-11-08T07:43:15Z",
"finished": "2026-11-08T07:46:52Z",
"account": "live",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "system-design",
"agent": "pr-system-design-reviewer",
"model": "sonnet",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "c284e39c894a",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 57,
"tokens": {
"input": 14,
"cache_write": 26539,
"cache_read": 138852,
"output": 3441
},
"tokens_from": "result",
"tokens_by_model": {
"claude-sonnet-5-5": {
"input": 14,
"cache_write": 26539,
"cache_read": 138852,
"output": 3441
}
},
"session": "01a1ba1c-8cf8-7e19-932c-41b819a985f2",
"worklog": "1329.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/11#pullrequestreview-5817085027",
"review_id": 5817085027,
"findings": {
"correctness": 2,
"design": 0,
"prose": 0
},
"numbers": [
"system-design.1",
"system-design.2"
],
"rules": {},
"head": "7d2c9a01ac62129babb2254cbdf2884ee18fb2bd",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-prs-pr11-round1-20261108T074255Z/records",
"started": "2026-11-08T07:44:08Z",
"finished": "2026-11-08T07:45:14Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
},
{
"label": "stale-prose",
"agent": "pr-stale-prose-reviewer",
"model": "opus",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "50506a05f936",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 80,
"tokens": {
"input": 28,
"cache_write": 31538,
"cache_read": 397302,
"output": 6411
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 28,
"cache_write": 31538,
"cache_read": 397302,
"output": 6411
}
},
"session": "01a1ba1c-7da7-769a-b57d-d92ccefde0f7",
"worklog": "1329.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/11#pullrequestreview-5817083539",
"review_id": 5817083539,
"findings": {
"correctness": 0,
"design": 0,
"prose": 3
},
"numbers": [
"stale-prose.1",
"stale-prose.2",
"stale-prose.3"
],
"rules": {},
"head": "7d2c9a01ac62129babb2254cbdf2884ee18fb2bd",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-prs-pr11-round1-20261108T074255Z/records",
"started": "2026-11-08T07:43:14Z",
"finished": "2026-11-08T07:44:36Z",
"account": "live",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "failure-paths",
"agent": "pr-failure-paths-reviewer",
"model": "sonnet",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "56bdd640764b",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 31,
"tokens": {
"input": 16,
"cache_write": 17354,
"cache_read": 132242,
"output": 2112
},
"tokens_from": "result",
"tokens_by_model": {
"claude-sonnet-5-5": {
"input": 16,
"cache_write": 17354,
"cache_read": 132242,
"output": 2112
}
},
"session": "01a1ba1c-ff56-733d-9a14-2ef2869244f4",
"worklog": "1329.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/11#pullrequestreview-5817082995",
"review_id": 5817082995,
"findings": {
"correctness": 0,
"design": 0,
"prose": 0
},
"numbers": [],
"rules": {},
"head": "7d2c9a01ac62129babb2254cbdf2884ee18fb2bd",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-prs-pr11-round1-20261108T074255Z/records",
"started": "2026-11-08T07:43:50Z",
"finished": "2026-11-08T07:44:23Z",
"account": "live",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "contracts",
"agent": "pr-contracts-reviewer",
"model": "opus",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "88b65626bceb",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 261,
"tokens": {
"input": 66,
"cache_write": 66107,
"cache_read": 1641040,
"output": 13644
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 66,
"cache_write": 66107,
"cache_read": 1641040,
"output": 13644
}
},
"session": "01a1ba1c-a990-7d79-aa13-84295069b4fa",
"worklog": "1329.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/11#pullrequestreview-5817095781",
"review_id": 5817095781,
"findings": {
"correctness": 0,
"design": 2,
"prose": 0
},
"numbers": [
"contracts.1",
"contracts.2"
],
"rules": {},
"head": "7d2c9a01ac62129babb2254cbdf2884ee18fb2bd",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-prs-pr11-round1-20261108T074255Z/records",
"started": "2026-11-08T07:44:08Z",
"finished": "2026-11-08T07:48:35Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
},
{
"label": "synthesis",
"agent": null,
"model": "sonnet",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/synthesis.md",
"prompt_sha": "40dcb5a50df0",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 8,
"tokens": {
"input": 2,
"cache_write": 11550,
"cache_read": 1433,
"output": 670
},
"tokens_from": "result",
"tokens_by_model": {
"claude-sonnet-5-5": {
"input": 2,
"cache_write": 11550,
"cache_read": 1433,
"output": 670
}
},
"session": "01a1ba24-6949-7ba1-9e08-462a32426153",
"worklog": "1329.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/11#pullrequestreview-5817104276",
"review_id": 5817104276,
"findings": {
"correctness": 0,
"design": 3,
"prose": 0
},
"numbers": [
"synthesis.1",
"synthesis.2",
"synthesis.3"
],
"rules": {},
"head": "7d2c9a01ac62129babb2254cbdf2884ee18fb2bd",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-prs-pr11-round1-20261108T074255Z/records",
"started": "2026-11-08T07:51:59Z",
"finished": "2026-11-08T07:52:10Z",
"account": "live",
"api_key_source": "none",
"machine": "archlinux"
}
],
"rules": {
"proposed": 1,
"keys": [
"rule/code-quality/a-running-state-inferred-from-a-missing-completion-record-carries-its-age-because-the-record-can-fail-to-arrive"
],
"unfiled": []
},
"kept": []
} |
…its document A review round posts a review per reviewer, a synthesis review and a round comment, each opening with a versioned marker and closing with a collapsed JSON block. The detail read already fetches every review and comment, so internal/reviewrounds reads rounds out of it and costs no read of its own. The row gains a CLAUDE cell after the review, from the newest round: its findings by section once the round comment posts, claude running while its reviews post, or claude reviewed where the newest review is of a version prs does not read. The column draws only while a row fills it. The document gains a Review section after Description: where the round stands, its synthesis's shapes and duplicates, then every finding under a heading for its section, read from the JSON rather than the markdown. Each earlier round gets a line, and a post of an unknown version shows whole. Comments leaves out exactly the posts Review shows. A window action marked runs_review_round reads running while its window is open, which prs asks tmux every five seconds. A session that ran a round keeps its window, so the window counts only until a round finishes after prs opened it, and one prs did not open counts only where none finished. The test fixture is a real round, with machine names and paths replaced. Agent-Name: claude-code-unattended Agent-Session: e999b398-dbff-4616-bec4-8a433b70f632 Agent-Run: 01a1ba02-f445-7d6e-ae62-ae0826bc7073 Worklog: 1329.1
A post's author type (User, Bot) and its association with the repo (OWNER, COLLABORATOR, NONE and so on) come from the same detail read, at no extra cost. They tell a post from the repo's own review automation or someone who can write to it from a post anyone could have left, which a review round's reader needs before it counts a post. The threads file d hands nvim carries both on each comment. Agent-Name: claude-code-unattended Agent-Session: e999b398-dbff-4616-bec4-8a433b70f632 Agent-Run: 01a1ba1c-5efd-7540-ae4b-b95176a64f72 Worklog: 1329.1
…ound fact once A round's JSON block is the last json fence in the post, found by its own lines. A finding quoting `<details>` put that string inside the block, so anchoring on the last `<details>` dropped the whole review. A marked post counts in a round only where an app on the repo or someone who can write to it posted it. Anyone can comment with a round's marker, and a forged round 99 would otherwise have become the newest round. A round with no round comment reads running only for an hour after its newest post, or while a window runs one. After that it reads unfinished, because a round cut short looks the same and is continued by running it again. Where the detail read holds only the newest comments, the round reads its counts, because its round comment may sit outside the read. The round record's statuses stay apart: a skipped synthesis reads as skipped, a reviewer that did not run counts as a gap beside the findings, and a round none ran reads `claude did not run` rather than as clean. The window name, the columns, the finding sections and the posts Review shows each come from one place: windowOn, rowColumns, reviewrounds' Sections table and reviewShows. Agent-Name: claude-code-unattended Agent-Session: e999b398-dbff-4616-bec4-8a433b70f632 Agent-Run: 01a1ba1c-5efd-7540-ae4b-b95176a64f72 Worklog: 1329.1
7d2c9a0 to
2d4b8c6
Compare
prs reads a window action's `runs_review_round` to know which tmux window runs a Claude review round. Without it on `r`, a round started from prs reads nothing on its row until its first reviewer posts, about four minutes on prs PR 7's round. This marks `r` in the prs config template. **Merge this only after datapointchris/prs#11 merges and its release is out.** Every prs before that release refuses the whole config with `field runs_review_round not found in type config.WindowAction`, so `prs tui` would not start on any box that applies this first. ## What to look at `configs/common/.config/prs/config.yml.tmpl`: one key on the `r` action, and the comment above it. ## How it was verified prs#11's build reads the rendered template as `key r claude review offered, runs review rounds` under `prs config show`. The installed prs refuses it with the error above, which is the ordering this PR waits on. The commit hooks passed. ## What changes After the next `dotfiles apply`, a row whose `r` window is open reads `claude running` until a round finishes after the window opened. ## Risk and rollback Applied before prs's release, prs refuses its config on that box until the release installs. A revert of this file alone restores it. Session e999b398-dbff-4616-bec4-8a433b70f632.
A Claude review round posts a review per reviewer, a synthesis review and a round comment on a pull request. prs showed them only as long bodies in Comments. Nobody could tell from the list whether a round had run, whether it was still running, or what it found, without opening each pull request and reading every review.
This reads each round from the reviews and comments the detail read already fetches, so it costs no GitHub read of its own:
CLAUDEcell after the review cell gives the newest round. Once its round comment posts, the cell is its finding counts, asclaude 5 correctness · 3 design · 4 prose. It adds1 not runor1 unreadwhere a reviewer did not run or its review could not be read, and readsclaude did not runwhere none ran. Before the round comment, it readsclaude runningwhile reviews post, up to an hour after the last, thenclaude unfinished. It readsclaude reviewedwhere the newest review's marker names a version prs does not read. The column draws only while a row fills it.runs_review_roundreads running while its tmux window is open, until a round finishes after prs opened it.What to look at
internal/reviewrounds/rounds.go:Read,outsiderWhyandjsonBlock. Check that the block is the last json fence, matched by lines, that an outsider's post never reaches a round, and that the marker's version gates the rest.internal/board/review_rounds.go:roundStateOf, which the cell and the Review section both read, androundWindowOfwithroundWindows.open, the window rule. A session keeps its window after its round, so a window prs opened counts until a round finishes after the opening. One prs did not open counts only where no round has finished.internal/board/rows.go:rowColumns, each column with the cell it draws, the optionalCLAUDEone among them.cmd/prs/testdata/review-round.json, a real round copied from this repo's PR 7. Machine names, home paths and a tracker id are replaced, and each author carries the type and association GitHub sends.How it was verified
go test ./...passes under the commit hook.TestEachRowReadsItsNewestReviewRoundAndItsDocumentHoldsTheRounddrives the built binary on a pty against the stand-in GitHub:<details>, and an outsider's forged round 99.v7marker.dhands nvim for all three. It then pressesr, waits forclaude running, empties the stand-in tmux's window list and waits for the counts to return.jsonBlockanchored on the last<details>again, the test times out waiting for feat: draw each pull request as two lines, ruled apart and spread across the width #13's counts. It does the same with the author check removed.claude did not run. A skipped synthesis beside a reviewer not posted readclaude 1 design · 1 not run, with "synthesis skipped: fewer than two findings to share a cause." A round whose round comment fell outside the comments read read its counts and "may have finished". A round posting ten minutes ago readclaude running.prs config showreads the dotfiles template with the new key askey r claude review offered, runs review rounds. The installed prs refuses it:field runs_review_round not found in type config.WindowAction.What changes
CLAUDEcolumn betweenREVIEWandAGE. A board with no round never draws it.prs listnever does, because it reads no detail.dhands nvim gainsauthor_typeandauthor_associationon each comment, both required indiff-threads.schema.json. A reader holding the file to the earlier schema refuses it, because its comment object allows no other keys.runs_review_roundis a new window action key. Every earlier prs refuses a config holding it, so the dotfiles template that adds it tormerges after this releases.Decisions, and what they rejected
reviews(last: 100)with bodies andcomments(last: 50). A shared per-PR cache for details, diffs and rounds was rejected, because rounds add no read to share.claude reviewedwould hide them.runs_review_roundon the window action. The window's name is config, so prs needs to know which action runs a round. Hard-codingreview {repo} #{number}would tie prs to one config. A top-levelreview_round_windowkey was rejected, because it would repeat the action'swindow.claude runningon a finished round for as long as the window stayed open.Risk and rollback
A revert removes the cell, the section and the key. A config holding
runs_review_roundis then refused, so the dotfiles template change reverts with it.What this does not do
--json, which reads no detail.The review
#11 (comment)
G1.1fixed — 2d4b8c6.jsonBlocktakes the last line of three or more backticks andjson, then reads to the line of the same backticks. JSON escapes a newline inside a string, so no line of a finding's text can open a fence, whatever it quotes. The test's widget feat: draw each pull request as two lines, ruled apart and spread across the width #13 carries a G1 finding quoting<details>.G1.2fixed — same assynthesis.2.G1.3fixed — same assynthesis.1.G1.4fixed — same asstale-prose.2.stale-prose.1fixed — 2d4b8c6. The three sentences now say a window running a round shows Review before the detail read lands, in the words proposed.stale-prose.2fixed — 2d4b8c6. CLAUDE.md reads "a post whose marker names a version other thanreviewrounds.Versionshows whole rather than read wrong."stale-prose.3fixed — 2d4b8c6.withRoundssays each pull request's cut is inwithRoundOf, and one line above that switch says why each gets its cut.code-architecture.1fixed — same assynthesis.3.code-architecture.2fixed — same assynthesis.3.code-architecture.3fixed — same assynthesis.3.code-architecture.4fixed — same assynthesis.3.system-design.1fixed — 2d4b8c6. The detail read asks for each author's__typenameandauthorAssociation. A marked post counts only where its author is a Bot, which is an app installed on the repo, or is an OWNER, MEMBER or COLLABORATOR. Any other shows whole with why, moves no cell, and never reads as the newest review. The test's widget feat: draw each pull request as two lines, ruled apart and spread across the width #13 carries an outsider's round 99. The proposed config allow-list was rejected, because GitHub already sends what decides it and a stranger's first run would have none.system-design.2fixed — same assynthesis.1.contracts.1fixed — same assynthesis.1.contracts.2fixed — same assynthesis.2.fleet-patterns.1deferred — one public reader of the grammar, shared by every program that reads it, is filed. It needs a new public module, and the existing reader cannot be imported as it stands. Until then prs keeps the newest review and the newest round comment, as the grammar's writer does. It reads only v6, the rule settled for prs, and takes the last json fence, as the writer does.synthesis.1fixed — 2d4b8c6.roundStateOfdecides the round's state once, and the cell and Review both read it. A round with no round comment reads running while a window runs it, or for an hour after its newest post. After that it readsclaude unfinished, and Review says it stopped before its round comment posted and that running a review again continues it. Where the pull request has more comments than the read holds, the round reads its counts, and Review says it may have finished. An earlier round there no longer reads "never posted its round comment". Gadget feat: prs tui paints its last listing at once, then rereads on a timer and on focus #5 in the test readsclaude unfinished.synthesis.2fixed — 2d4b8c6.Finishkeeps each reviewer's status. A skipped reviewer reads "synthesis skipped: why", apart from "X did not run: why". A round none ran readsclaude did not runin yellow, and its earlier line says none of its reviewers ran. A reviewer not run and a review not read add1 not runand1 unreadto the counts, and turn a cell with no correctness finding yellow.synthesis.3fixed — 2d4b8c6. Each fact has one home.windowOnbuilds a window's name and checks its provider, for both the key that opens it and the watch that finds it.reviewrounds.Sectionsholds each section's name, heading and whether it blocks a merge, and the board's heading switch and"correctness"literal are gone.reviewShowsis the set Review draws, and itsbodiesthe set Comments leaves out.rowColumnsholds each column with its cell, andclaudeAtis gone.Session e999b398-dbff-4616-bec4-8a433b70f632.