mroberts
129354cda8
Claude Code delivers the hook payload on a socket. Opening it by path
('/dev/stdin' -> /proc/self/fd/0) fails with ENXIO, so bash-guard.mjs threw on
every invocation and its bare catch exited 0 silently. The guard looked like it
was never dispatched; it was dying on line 29 each time.
readFileSync(0) is read() on the descriptor with no open(), which works on a
socket. The catch now logs instead of swallowing, so this failure mode can never
again masquerade as non-dispatch.
Adds test-bash-guard.py, which drives the guard over a socketpair. A pipe would
not reproduce the bug, so the socket is load-bearing. Verified failing against
the pre-fix guard (silent, no output) and passing after.
Removes the five diagnostic probes and probe.mjs; they served their purpose.
Bumps to 1.0.2 because the plugin cache is keyed by version and an unchanged
version silently skips reinstall.
139 lines
9.3 KiB
Markdown
139 lines
9.3 KiB
Markdown
# PR Review Field Guide
|
||
|
||
A repeatable process for reviewing PRs without losing the big picture. No AI required — just git, `gh` (optional), and an editor with go-to-definition / find-references. The core move: **build a mental model first, then use the diff to verify it** — never the reverse.
|
||
|
||
---
|
||
|
||
## Phase 1 — Orient (before opening a single diff) · ~5 min
|
||
|
||
1. Read the PR description and the linked issue. The issue is usually the truer statement of intent.
|
||
2. Write a two-sentence hypothesis: *what should this change touch, and where should the hard part live?* Actually write it down — it's your anchor for the whole review.
|
||
3. Skim only the changed-file **list** (not contents). Compare against your hypothesis:
|
||
- Files you didn't expect → possible scope creep or coupling you didn't know about. Mark them.
|
||
- Files you expected but don't see → possibly incomplete change. Mark those too.
|
||
|
||
If the description is empty and the file list is confusing, stop and ask the author for a summary. That's a review comment in itself.
|
||
|
||
## Phase 2 — Get out of the diff viewer · ~2 min
|
||
|
||
```
|
||
gh pr checkout <number> # or: git fetch origin <branch> && git switch <branch>
|
||
```
|
||
|
||
Open the repo in your editor. From here on, the web diff is a *map* you glance at — the territory is the checked-out code, where go-to-definition and find-references work.
|
||
|
||
## Phase 3 — Find the load-bearing change · ~5 min
|
||
|
||
One change forces most of the others. Find it. Priority order of suspects:
|
||
|
||
schema / migration → public interface, type, or API contract → core algorithm → everything else
|
||
|
||
Test: *if this change were reverted, would most of the rest of the diff become unnecessary?* Read that file **in full, in context** — not the hunk.
|
||
|
||
## Phase 4 — Read outward, in dependency order · bulk of the review
|
||
|
||
Contracts → core logic → mechanical ripples → tests. At each stop ask:
|
||
|
||
- Does this change follow necessarily from the load-bearing one?
|
||
- Is anything *extra* hiding here that isn't part of the stated intent?
|
||
|
||
Batch the mechanical stuff (renames, import churn, call-site updates, lockfiles) into one fast skim pass — just confirm nothing substantive is buried in it. Spending equal attention per file is how reviewers burn out and miss the real issue.
|
||
|
||
## Phase 5 — Trace the blast radius · ~10 min
|
||
|
||
The classic integration bug: **unchanged callers of changed code.**
|
||
|
||
1. For every changed public symbol (signature, type, endpoint, event, config key): find-references. Check each call site that is *not* in the diff — is it still correct under the new behavior?
|
||
2. Grep for the old name / old pattern. Anything left over that should be gone?
|
||
3. Check the seams the diff doesn't show: serialization boundaries, DB reads of migrated data, consumers in other services.
|
||
|
||
## Phase 6 — Run it · ~10 min
|
||
|
||
- Run the targeted tests for the changed area (not just CI-green — read what they assert).
|
||
- Exercise the changed path once yourself — app, REPL, or debugger. Step through the load-bearing function with real values.
|
||
- Trigger the error path at least once. Error handling is the least-reviewed, most-shipped-broken code there is.
|
||
|
||
## Phase 7 — Step back (the holistic pass) · ~5 min
|
||
|
||
Close the diff. Answer from memory:
|
||
|
||
- Does the *whole* accomplish the stated intent — and nothing meaningfully more or less?
|
||
- Is there a simpler design that occurs to you now that you understand it? (Mention it; don't demand it.)
|
||
- What breaks at 10× load / 10× data / concurrent use?
|
||
- What is *not* tested that worries you?
|
||
- Will someone (you) understand this in six months without the PR description?
|
||
|
||
If you can't answer these, the review isn't done — or the PR is too big, which is feedback in itself.
|
||
|
||
## Phase 8 — Write the review
|
||
|
||
1. **Lead with your understanding**: "My read: this does X by changing Y, with Z as the tricky part." If you've misread it, the author corrects the model, not just the comments.
|
||
2. Separate **blocking** from **take-it-or-leave-it**, explicitly.
|
||
3. Surface Phase 1 surprises and Phase 5 blast-radius findings — those are your highest-value comments, and the ones a hunk-by-hunk reviewer can't make.
|
||
|
||
---
|
||
|
||
### Calibration
|
||
|
||
- Small PR (< ~200 lines): Phases 1, 3, 5, 8 — maybe 15 minutes total.
|
||
- Large PR (> ~800 substantive lines): do Phases 1–3, then tell the author what you're reviewing first and what needs a second pass — or ask for a split. Reviewing 2,000 lines in one sitting produces approval, not review.
|
||
- Repeat offender friction (huge PRs, empty descriptions, tangled commits): fix upstream — PR templates, stacked PRs, self-review annotations by the author.
|
||
|
||
---
|
||
|
||
## Appendix — this workflow in *your* Neovim
|
||
|
||
Grounded in your LazyVim config after the PR-review upgrades (verified against your pinned plugin sources plus the implemented brief). Two facts shape everything: your `<leader>g*` keys dispatch **git vs. jj** per-repo via `plugins.custom.vcs.is_jj()`, and your localleader is `\`, which all octo.nvim bindings hang off.
|
||
|
||
### Getting the branch (Phase 2)
|
||
|
||
- **git repo**: `gh pr checkout <n>` in a terminal (`<c-/>` toggles one at repo root).
|
||
- **jj-colocated repo**: stay jj-native — `jj git fetch`, then `jj new <head-branch>@origin`. Your working copy `@` is now an empty child of the PR head, which makes "diff against trunk" mean exactly "the PR."
|
||
|
||
### The map (Phase 3–4)
|
||
|
||
`<leader>gR` is the review map: diffview against the resolved base (`origin/HEAD`, falling back to `origin/main`) in git repos, `Jdiff trunk()` in jj repos. Keep the distinction deliberate: `<leader>gd` stays your *local working-tree* diff — mid-review it shows nothing on a clean checkout, and that's correct, not broken. `<leader>gD` gives the same PR-vs-base content as a grouped hunks picker (it now shares `gR`'s base resolution), useful when you want to sample hunks rather than walk files.
|
||
|
||
### Reading path, blast radius (Phase 4–5)
|
||
|
||
`<leader>ga` seeds the arglist with the PR's changed files (both VCSs); prune and reorder with `:args` into the briefing's dependency order, then walk with `:n` / `:prev`. For blast radius, `gr` (references) populates the quickfix list — `]q` / `[q` walk it **everywhere now, including inside octo review tabs**, reopen with `<leader>xq`. Any snacks picker sends results to quickfix with `<c-q>`. Project grep is `<leader>sg` (root) / `<leader>sG` (cwd); "is the old pattern really gone" sweeps go through `<leader>sr` (grug-far). History and blame dispatch too: `<leader>gf` / `<leader>gF` (file / repo history), `<leader>gl` / `<leader>gL` (log — `J log` in jj), `<leader>gb` (blame line — jj annotate in jj repos).
|
||
|
||
### The GitHub layer — octo.nvim (Phase 8)
|
||
|
||
Octo now uses the snacks picker, and `<localleader>` = `\`. `:Octo pr edit <n>` opens the PR buffer (description, threads — Phase 1 orientation without leaving nvim). `\vs` starts the review, `\vr` resumes a pending one. In the review diff: `\ca` adds a comment (visual mode gives a range comment), `\sa` a suggestion, `]t` / `[t` jump threads, `]u` / `[u` jump unviewed files, `\e` focuses the file panel, `\b` toggles it. Changed-file navigation is `]o` / `[o` (`]O` / `[O` for last/first) — remapped across review diff, thread view, and file panel so the quickfix keys stay yours. `\vs` again opens the submit window: `<C-a>` approve, `<C-m>` comment, `<C-r>` request changes; `<C-c>` closes the review tab.
|
||
|
||
### The briefing pane
|
||
|
||
`<leader>aR` prompts for a PR number and launches your right-side `claude` terminal with `/review-pr <n>` already running; `<leader>at` toggles the same pane bare. Keep it open as the map and walk the reading path in the main window — `<leader>ga` then `:args` reorder takes the briefing's file list into the walk.
|
||
|
||
### Cheat sheet
|
||
|
||
| Task | Git repo | jj-colocated repo |
|
||
|---|---|---|
|
||
| Orient (desc + threads) | `:Octo pr edit <n>` | same |
|
||
| Get the branch | `gh pr checkout <n>` | `jj git fetch` → `jj new <branch>@origin` |
|
||
| PR-vs-base map | `<leader>gR` | same (dispatched → `Jdiff trunk()`) |
|
||
| PR hunks picker | `<leader>gD` | same (dispatched) |
|
||
| Local working-tree diff | `<leader>gd` | same (dispatched) |
|
||
| Changed files → arglist | `<leader>ga` | same (dispatched) |
|
||
| Reading path | `:args` reorder → `:n` / `:prev` | same |
|
||
| Status | `<leader>gs` | same (dispatched) |
|
||
| File / repo history | `<leader>gf` / `<leader>gF` | same (dispatched) |
|
||
| Log / blame line | `<leader>gl`, `<leader>gL` / `<leader>gb` | same (→ `J log` / jj annotate) |
|
||
| Definition / references | `gd` / `gr` (→ quickfix) | same |
|
||
| Implementation / type / hover | `gI` / `gy` / `K` | same |
|
||
| Walk quickfix | `]q` / `[q` · list: `<leader>xq` | same |
|
||
| Picker → quickfix | `<c-q>` in any snacks picker | same |
|
||
| Project grep | `<leader>sg` (root) / `<leader>sG` (cwd) | same |
|
||
| Old-pattern sweep | `<leader>sr` (grug-far) | same |
|
||
| Start / resume review | `\vs` / `\vr` in PR buffer | same |
|
||
| Comment / suggestion at line | `\ca` / `\sa` (visual = range) | same |
|
||
| Jump threads / unviewed files | `]t` `[t` / `]u` `[u` | same |
|
||
| Changed-file nav (octo) | `]o` `[o` · `]O` `[O` last/first | same |
|
||
| Submit | `\vs` → `<C-a>` / `<C-m>` / `<C-r>` | same |
|
||
| Close review tab | `<C-c>` | same |
|
||
| Launch review briefing (PR #) | `<leader>aR` | same |
|
||
| Briefing pane (toggle) | `<leader>at` | same |
|
||
|
||
*(`\` = your localleader. Octo rows apply inside octo buffers; `]q`/`[q` are quickfix everywhere, including review tabs.)*
|