Tononoff Journey

skill · 07

pr-brief: a reviewer for PRs nobody wants to read

It groups a huge PR into features, picks the reading order, and narrates each diff from the side.

Three honest reasons pr-brief exists. I had to review enormous PRs and couldn't ask everyone to stack them. The needles in that haystack aren't evenly distributed, so I wanted to know the right order to look. And reading code isn't the hard part — connecting a migration in one folder to a cache read in another is. That's the part you skip at file 140.

“PR Brief skill” — the whole loop. Watch on YouTube ↗

§Grouping before reading

Ask Claude to "review this pr" and one opus agent gets the file list, the diff and the PR body, and returns a features.json. A feature is a kebab-case change theme of one to three words — ownership-guards, asgi-middleware. Grouped by theme, not directory. A file carrying two themes joins them with a +: sse-stability + inbox-ux.

The ordering heuristic:

- Foundation first: migrations, schemas, core modules, shared utilities, types
- Domain/services next: business logic, repositories
- Entry points: controllers, API endpoints, middleware
- UI: components, stores, pages
- Last: docs, tests, build/ci
- Within each layer: higher blast_radius first

blast_radius (high for schema/auth/migrations, low for docs/tests/lockfiles) is only the tiebreak inside a layer. Sorting the whole PR by blast radius is the obvious move, and it's wrong: it puts the scary auth guard ahead of the migration that defines the column being guarded. Dependency order first, fear second. The agent also emits five to twelve edges, pairs like ["migrations","ai-enabled-flag"] meaning "understanding A helps read B".1SKILL.md mandates opus for every subagent: it "makes fewer mistakes on diff analysis and feature grouping, which is load-bearing". Past 200 files it falls back to top-level directories.

§Two kinds of pill

Explain pills are the purple ✨ callouts in the 400px track right of each diff. Each must do three of four jobs: what the block does, why this approach, where it connects (a specific call site by path:symbol), and how it fits the feature. Job three hurts to write:

{
  "title": "Pure ASGI to unblock SSE",
  "body": "Switched from Starlette's `BaseHTTPMiddleware` to raw ASGI because the former buffers `StreamingResponse` bodies through an asyncio queue.\n\n**Why now:** `inbox_service.stream_conversations` (next file) emits SSE — the buffer would have collapsed all events into a single chunk, which is the bug `sse-stability` fixes."
}

It names the approach that failed, the file you'll open next, and the feature it belongs to. None of that comes from reading one file. That's why step 6a exists — nothing but "walk the sequence, find the seam where feature A's output becomes feature B's input" before any pill is written.

§Severity that means something

Critique pills share that track in red, with an S1–S5 badge. As written:

  • S1 (nit) — style, naming, minor readability.
  • S2 (suggestion) — a cleaner approach exists, not a bug.
  • S3 (issue) — real concern: subtle bug, missing edge case, technical debt that will bite.
  • S4 (problem) — a bug, security/perf concern, broken contract. Should block until addressed.
  • S5 (blocker) — must-fix. Production-impacting bug, data-loss risk, security hole.

The definitions are the easy half. The distribution guidance is what stops the scale collapsing: a typical 50-file PR "might have 0 S5s, 1–3 S4s, 5–10 S3s, more S1–S2s", and a genuinely fine file gets zero critiques, not a forced S1. If everything is S5, nothing is. Vague critiques are banned — name the cleanup or skip the pill.

{
  "title": "Race in cache invalidation",
  "body": "**`session_management.py`** clears the cache *before* `db.commit()`. A concurrent reader between the clear and the commit will repopulate stale data from the old transaction snapshot, defeating the invalidation.\n\nMove the `cache.delete()` call after the commit, or use a write-through cache.",
  "severity": 4,
  "start_line": 142,
  "end_line": 147,
  "side": "RIGHT",
  "suggested_change": "        db.commit()\n        cache.delete(f\"ai:{phone_group_key}\")"
}

The good part is the mechanism — "repopulate stale data from the old transaction snapshot" — not the word "race". suggested_change is optional, and it's what earns the click. "Use as comment →" pre-fills the inline editor with the critique and wraps the replacement in a GitHub ```suggestion block, so the author gets the one-click accept UI.2Saves POST immediately, throttled server-side to one write per 1.5s (GitHub asks for ≥1s). On a secondary rate limit the server returns 429 and the UI flips itself into Batch mode, posting everything as one review.

§Positions before the browser

The build step flattens each file's diff into line objects carrying their GitHub position:

one file's diff, from the first @@ three coordinates, one physical line old_line new_line position @@ -1,5 +1,10 @@ hunk — — 1 foo context 1 1 2 +bar add — 2 3 -baz del 2 — 4 qux context 3 3 5 +quux add — 4 6 counts the @@ row too resets each file the pill author emits start_line 2 · end_line 4 · side RIGHT post-change numbers only RIGHT matches new_line LEFT matches old_line resolved at build time unmatched anchors dropped
The holes are the point: an add row has no old_line, a del row has no new_line, and only position counts the @@ row.
{ "position": 1, "type": "hunk", "content": "@@ -1,5 +1,10 @@" }
{ "position": 2, "type": "context", "content": " foo", "old_line": 1, "new_line": 1 }
{ "position": 3, "type": "add", "content": "+bar", "new_line": 2 }
{ "position": 4, "type": "del", "content": "-baz", "old_line": 2 }

position is not a line number. It's a 1-indexed offset into that file's unified diff, counting from the first @@ header. Hunk headers, context, adds and deletes all increment it. It resets per file, and \ No newline at end of file markers are skipped. So one physical line has three coordinates — old_line, new_line, position — and the pill author only ever sees the second. (I know.) Pills are written as start_line/end_line/side, and the build step resolves them — for RIGHT, the context-or-add line whose new_line matches; LEFT uses old_line — dropping any anchor missing from the diff with a warning.

Resolving that in the browser means shipping a diff parser and hiding an off-by-one in the one layer with no tests — and the failure isn't cosmetic: GitHub rejects the comment.

Then the twist: positions never reach GitHub for the comments you write. /api/post-comment sends line and side (plus start_line/start_side for a range), the newer shape that supports multi-line ranges. position is the app's internal coordinate system: data-position per row, data-start-position per pill, end_position as the local key for a pending comment. The only one crossing the wire is /api/post-briefs, pinning each file's brief at position: 1. One consequence: files are stacked, so rows restart at position 1 in each. A global querySelector opens the editor in the first match in document order — a file far up the page, invisible to whoever clicked. Lookups are scoped to .diff-wrap.3SKILL.md cites a build_data.py that isn't in the repo; step 4 has Claude write the stdlib parser inline each run.

§Chat that already knows

It starts as a context menu. Select any code inside a diff, right-click, and the only item is the one you want:

Selecting code in a diff and right-clicking surfaces a "Chat about `path:lines`" item — the whole affordance
Selecting code in a diff and right-clicking surfaces a "Chat about `path:lines`" item — the whole affordance
Click it and the drawer opens on the right, pre-loaded with the file, the line range and the selected code, streaming an answer
Click it and the drawer opens on the right, pre-loaded with the file, the line range and the selected code, streaming an answer

A still of a streaming panel is a poor likeness of one, so here's the demo from the repo:

Select, right-click, ask, read. The panel already knows what PR this is.

Select code, right-click, and a 440px drawer opens. Every panel — across files, across reloads — resumes the same Claude Code session that wrote the brief, via claude --output-format=stream-json --verbose --dangerously-skip-permissions --resume <id> -p <text>, streamed to the browser as newline-delimited JSON. So it starts with the whole PR in context. No re-seeding, no vector store.

Getting the id is fiddly. Just before launch the skill takes the most recently modified *.jsonl under ~/.claude/projects/<cwd-with-slashes-turned-into-dashes>/ and writes {session_id, cwd} to ~/.claude/pr-review/pr-<num>/session.json. The cwd matters as much as the id: --resume looks the id up scoped to the directory it runs in, so resuming from the wrong one gives "No conversation found" instead of a useful error.

The snippet travels only on a panel's first message; after that firstSent flips and follow-ups go verbatim. Every panel is one conversation, so panel three can say "compare this with the previous file" and mean it. One turn at a time: the server takes a non-blocking lock and answers 409 if you overlap. Stop SIGTERMs the subprocess, and -SIGTERM is deliberately not counted as a failure. If no id was captured, /api/chat-context reports enabled: false, the context menu is never wired, and the feature doesn't exist instead of half-working.

524 lines of SKILL.md, one 2241-line HTML file, a 511-line stdlib server, no node_modules.

§Ordering without the model

The two halves of this come apart, and only one needs a model. Being told that nine files are one idea called sse-stability, that the migration comes before the guard reading the column it adds, that understanding A helps with B — that's a sorting problem over a file list, and it removes most of the work before a single pill is written. I think you could delete the explain and critique tracks and it would still be worth running. The reverse is not true: pills hung on an arbitrary file order are annotations on a haystack.

The severity scale generalizes further. S1–S5 definitions are the free part; any tool can write them. What stops the scale being decoration is the distribution guidance — a fine file gets zero critiques, a 50-file PR shouldn't yield a dozen S4s. Without a stated expectation of how rare the top of the scale is, nothing stops a model spending it, and by the second file I stop reading the badge. That holds for any generated output carrying a number it assigned itself.


The pills are the nice part. The ordering is the useful part.

Sign in with GitHub and say something. Threads live in this repository's Discussions, so you keep your words and I keep no database.