Read hook payload from fd 0, not /dev/stdin
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.
This commit is contained in:
@@ -0,0 +1,138 @@
|
||||
# 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.)*
|
||||
Reference in New Issue
Block a user