diff --git a/CHANGELOG.md b/CHANGELOG.md index 8df2ac7..84a653d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- [`PR-REVIEW.md`](PR-REVIEW.md) — the 13-criteria PR review checklist + the maintainer runs against every incoming PR, published at the + repo root so contributors can run the same pass on their own change + (directly or by handing the document to a coding agent) before + opening. Linked from `CONTRIBUTING.md` under "Submitting pull + requests" and "Further reading". Running the checklist before + opening surfaces problems that would otherwise come back as review + comments, saving a round trip. + ### Changed - Sidecar example (`examples/sidecar-nostr-relay`): `udp.mtu` is now diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a616897..2583bfc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -148,6 +148,20 @@ that touches your code path. This is the same matrix that runs on GitHub Actions. Catching a regression locally is much cheaper than catching it in CI. +### Self-review against the project review checklist + +The 13-criteria checklist the maintainer runs on every incoming PR is +published at [PR-REVIEW.md](PR-REVIEW.md). Run your own change through +it before opening — or hand the document to your coding agent with +"review my branch against this checklist" and let it do the pass. The +checklist covers PR hygiene (body, commit shape, base freshness), diff +content (does the change do what the description says, does it fit the +codebase as a natural extension), and cross-cutting concerns (tests, +docs, dependencies, security, contributor-conventional Rust patterns). + +This is the first thing the maintainer does on any submission, so +running it yourself saves a review round trip. + ### Additional requirements for feature PRs - **New CI coverage.** Features added without a test that exercises @@ -233,6 +247,9 @@ home yet, file a GitHub issue with the `design` label. ## Further reading +- [PR-REVIEW.md](PR-REVIEW.md) — the 13-criteria PR review checklist + the maintainer runs on every incoming PR; run it yourself before + opening to save a round trip. - [docs/design/README.md](docs/design/README.md) — protocol design tree. - [docs/branching.md](docs/branching.md) — full release workflow and merge-direction rationale. diff --git a/PR-REVIEW.md b/PR-REVIEW.md new file mode 100644 index 0000000..a6f6d1c --- /dev/null +++ b/PR-REVIEW.md @@ -0,0 +1,199 @@ +# PR Review Checklist + + + +This is the 13-criteria checklist the maintainer runs against every +incoming PR. The first pass on any submission is exactly this list, +so executing it yourself before opening — or after pushing a fresh +revision — saves a review round trip and surfaces problems faster. + +The document is also written so you can hand it to a coding agent +(Claude Code, Copilot, Cursor, Aider, etc.) with "review my branch +against this checklist" and get a structured pass. The agent gets +better results than a free-form "review my PR" because every concern +the maintainer cares about is enumerated below. + +## Step 1 — Should this even be reviewed? + +Skip the review (and say so) if the PR is: + +- closed, merged, or marked draft +- automated (bot author, dependabot, etc.) and trivially OK +- so small and obviously correct (typo fix, single-line doc tweak) + that a thirteen-point pass is overkill — a one-paragraph informal + review is better in that case + +## Step 2 — Gather context + +Read these *before* analyzing the diff so the review is grounded: + +1. PR metadata. Title, body, author, head ref, base ref, head SHA, + base SHA, mergeable status, CI rollup, commit list. + + ```bash + gh pr view --json title,body,author,headRefName,baseRefName,headRefOid,baseRefOid,mergeable,statusCheckRollup,commits + ``` + +2. The diff. + + ```bash + gh pr diff + ``` + +3. Base-branch freshness. How many commits have landed on the PR's + base since the PR forked from it. +4. Project guidance. Read [CLAUDE.md](CLAUDE.md) at the repo root and + any nested `CLAUDE.md` in directories the diff touches. These + describe project-specific conventions and constraints not visible + from the diff alone. +5. Related work on GitHub. Skim the [open issues](https://github.com/jmcorgan/fips/issues) + and other [open PRs](https://github.com/jmcorgan/fips/pulls) for + work that overlaps, duplicates, partially addresses, or is unblocked + by this PR. +6. For "this looks wrong" observations later: `git blame` the modified + lines and read recent commit history on the same files for context + before flagging something as a problem. What looks like a bug at + first glance is often a deliberate workaround documented in a prior + commit message. + +## Step 3 — The 13 criteria + +The review must address all 13 criteria below at some point. They +group naturally into PR hygiene, diff content, and cross-cutting +concerns — but the report itself is *not* organized this way; see +Step 4. + +### Group A — PR hygiene (structural review) + +1. **PR body and issue cross-reference**. Does the body accurately + describe the change (feature added or bug fixed) and match what + the diff actually does? Is there an associated issue that + should be referenced via `Closes #N` / `Fixes #N`? +2. **Commit hygiene and base freshness**. Is the PR a clean set of + commits (or a single commit) representing appropriately chunked + development items, or are there intermediate "WIP" / "fix typo" / + "address review" commits that should have been squashed? Is the + branch based off a recent `maint` / `master` / `next`, or has the + base diverged far enough that rebase work is needed? +3. **Commit message quality**. Are the commit messages well-structured + (subject + body where the change warrants), accurately referencing + everything actually in each commit, and free of extraneous footers + — particularly coding-assistant attribution (`Generated with + Claude Code`, `Co-Authored-By: Claude`, similar from other AI + tools)? + +### Group B — Diff content + +4. **Does it do what it says it does**. Walk each claimed behavior + from the PR body against the actual diff lines. +5. **Coherent whole**. Are all parts of the diff in service of the + stated goal, or are there drive-by formatting changes, unrelated + touch-ups, or scope creep? +6. **Fits the codebase as a natural extension**. Does the new code + use existing idioms, helpers, error types, and patterns, or does + it introduce new ones where existing ones would have served? + +### Group C — Cross-cutting concerns + +7. **New dependency surface**. Any new crates, system deps, + build-time requirements, or external-service dependencies? +8. **New test coverage**. Are the new code paths covered, are the + tests scoped correctly (unit / integration / end-to-end), and + are there obvious test gaps? Don't reflag anything CI already + enforces (formatting, lint, type errors, unit-test pass/fail). +9. **Documentation impact**. Does this need a CHANGELOG entry, + rustdoc updates, design-doc changes + ([docs/design/](docs/design/)), README adjustments, or operator + doc updates in [docs/](docs/)? +10. **Security vulnerabilities**. Any new attack surface, + untrusted-input parsing, `unsafe` blocks, panic-on-untrusted + paths, secret-handling concerns, or side-channel exposure? +11. **Rust and OSS best practices**. Idiomatic error handling, no + silently-swallowed errors, no `unwrap` / `expect` on untrusted + input, no `#[allow]` without justification, appropriate + visibility (`pub` vs `pub(crate)` vs private), naming, and + module shape. +12. **Overlap with existing work**. Cross-check open issues and + other open PRs (and recently closed/merged ones) for related + work that overlaps, duplicates, partially addresses, or is + unblocked by this PR. +13. **Other concerns**. Anything not captured above — wire-format + implications, branch-flow questions (`maint` vs `master` vs + `next`; see [docs/branching.md](docs/branching.md)), + deployment / packaging impact, contributor coordination needs, + fragility notes for future maintainers. + +## Step 4 — Compose the review + +The review report is **not** a Q&A walk through the 13 criteria. +Write it as natural prose in a coherent, integrated narrative that +reads start-to-finish. All 13 criteria must be addressed at some +point in the body, but ordering, grouping, and emphasis follow the +actual shape of THIS PR — lead with what matters most for this PR, +not a fixed template. + +A typical shape that often falls out naturally: + +- **Opening paragraph**: what the PR does and the headline + observations (subsumes criteria 1 and 4). +- **Substantive body**: diff analysis, design fit, cross-cutting + concerns, surprises, fragilities, missing coverage, + cross-PR/issue overlap, anything unusual. Don't reference + criterion numbers in the prose. +- **Closing**: short summary and a proposed disposition — *land*, + *land-with-followups* (list them), *request-changes* (with the + blocking items called out), or *hold-for-thematic-batch*. + +Short subheadings are fine where they aid scanning. Bullets are fine +for enumerable items (test names, file paths, follow-up actions). +Avoid bullets that just enumerate criterion responses. + +## Step 5 — Filter aggressively + +Quality over quantity. Do not flag: + +- Pre-existing issues on lines the PR did not modify +- Issues that linter, type-checker, formatter, or CI would catch +- Pedantic style nitpicks a senior engineer would not call out +- Likely intentional changes related to the broader goal +- Things explicitly silenced by an `#[allow]` with justification +- Stylistic preferences not anchored in `CLAUDE.md` or the + surrounding codebase's idioms + +When in doubt about whether something is worth surfacing: would a +senior maintainer skim past it, or would they want it raised? +Skim-past items don't belong in the report. + +For every issue you *do* surface, include a concrete fix suggestion +inline ("rename X to Y", "extract this into the existing helper at +`foo.rs:42`", "add a test exercising the `Err` branch") so the +author can act without a round-trip. + +## Step 6 — Citation discipline + +When the review references a specific code location, use full-SHA +GitHub permalinks so the link survives future history rewrites: + +```text +https://github.com/jmcorgan/fips/blob//#L-L +``` + +For multi-line ranges include at least one line of context before +and after the line(s) being discussed. After `gh pr checkout `, +use `git rev-parse HEAD` to grab the full SHA — never partial SHAs +in permalinks. + +## Notes + +- The review is one human's read of the PR. Confidence calibration + matters: distinguish "this is a blocker" from "this is worth asking + about" from "this is a fragility note for future maintainers." The + closing disposition makes the action explicit. +- If a re-review is triggered after the author pushes new commits, + lead with the delta from the prior review rather than re-walking + the whole PR. +- This checklist exists to surface problems, not to assign blame. + If you're running it as the author or via an agent, treat each + finding as "would the maintainer ask about this?" — and either fix + it before opening, or pre-empt it in the PR body so the maintainer + doesn't have to ask.