mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-05 19:18:25 +00:00
Johnathan Corgan
6a564e26ac
Everything the release needs except the version number, which stays at 0.5.0-dev until the tag. The changelog entry covers only the work that is new on this line. The point release's forty-six entries arrived under their own heading with the forward merge and are left alone; the twenty that remained are regrouped by topic and eight more added for changes no entry covered. Three of those eight matter to someone upgrading. Five root modules and four re-exports left the public library surface and Node::connections narrowed, none of it recorded anywhere; the entry names what to use instead and distinguishes the removed connection-phase enum from the Noise type of the same name, which is a different type that still exists. Tracing targets moved, so an existing RUST_LOG filter stops matching rather than erroring. And the handshake resend interval key no longer governs the first resend, which is now a constant, though it still governs later ones. Seven more entries cover the work that landed after the first content pass was written: the experimental native datagram API, the fipsctl probe diagnostic, per-instance transport addressing, the app-owned UDP socket seam, and the connect, disconnect and path-MTU fixes. The four bug fixes among them all reach the deployed line, so the release notes no longer claim this release carries exactly one fix for a shipped bug; it carries four. There is no security section, because after the split every security entry belongs to the point release. The release notes say so plainly rather than leaving a reader upgrading across both releases to conclude this one carries no security work. The notes are organized by audience, since the release spans OpenWrt routers, embedders, FreeBSD, and the existing platforms, and a single list serves none of them. The native datagram API is given a section of its own rather than folded into the embedding seam: it is a client-facing API rather than a way to host a node, and its one rule with no Berkeley-socket counterpart, that the v1 wire carries no half-close, needs to be somewhere a client author will read it. FreeBSD is advertised as supported on x86_64 only, stated wherever the platform appears. Android is advertised as an embedding seam and not as a supported platform: a compile-gated library surface with no artifact and no host application guide. The configuration table rename is carried through every shipped file that taught the old spelling: nine documentation files, the OpenWrt sample config and a test generator, twenty-two sites in all. Guides written this same cycle were among them, which is how the omission was found. The documentation that arrived with the native API was checked for the same omission and was already clean. The compatibility tests keep the old spelling deliberately, since they exist to test the fold. The changelog section is the fold of master's [Unreleased], not a snapshot of it. An earlier version of this commit took a copy that then drifted, so each section ended up holding a bullet the other did not and re-folding them would have picked a winner silently. Both causes were fixed on master instead — the NixOS module had never been recorded there, and the pre-release batch of fixes was new — so [Unreleased] is a strict superset and this is a copy rather than a merge. [0.5.0] carries all forty-six bullets byte for byte, [Unreleased] is empty, and [0.4.2] is untouched, checked by hashing it against master's copy. The BLE work landed after the content pass and gets one summary entry in the changelog and one section in the release notes rather than nine bullets: the ble_available gate replacing target_os = "linux", packet-boundary recovery for stream-oriented backends, peer recognition by node identity instead of a rotating link address, the L2CAP PSM moving into the backend seam and onto the advertisement, the embedder-supplied Android radio, bounded probe retry, and inbound handshakes moved off the accept loop. The two release-notes copies no longer share their link paths. Relative links resolve from one directory only, so the seven written for docs/releases/ all 404ed from the root copy. The root copy now uses paths from the repository root and the versioned copy keeps the ../ form; both sets were resolved against the tree. The same two links are broken the same way in the v0.4.0 through v0.4.2 notes, left as shipped history. The contributor tallies are re-derived against maint..HEAD rather than adjusted: twenty commits from outside the project and 171 from me, with Arjen at fifteen and fr34aky at two. An earlier count of twelve and 138 was carried from a measurement taken three days before this content was written, and the BLE branch widened the gap after it. Arjen's NixOS flake module, the UDP sin6_scope_id fix and most of the BLE rework were uncredited, as was fr34aky's L2CAP PSM seam. They want one last re-derive at tag time if anything lands before the tag. A sweep of all 99 tracked markdown files against the tree corrected fifty-three of them. Four told the reader to run a build.sh that does not exist; the only harness builder is testing/scripts/build.sh. The BLE build prerequisites were described as optional on the strength of a probe that build.rs does not perform, and bluez was named a build prerequisite when libdbus-sys asks only for libdbus-1-dev and pkg-config and bluez is the runtime daemon. Link cost is the primary sort key in next-hop ranking, not reserved for future use; Ethernet runs on macOS as well as Linux; the BLE MTU is the L2CAP CoC MTU rather than a negotiated ATT_MTU; effective Ethernet MTU is 1497; the LAN discovery subsystem is src/mdns and eight citations still named a src/discovery that never existed here. The connectivity states in three tutorials were invented, and their jq filters matched nothing including healthy peers. One command filtered on a literal fd97: address prefix, which only the first byte of fixes, so it returned empty for all but one reader in 256 and every later step using the variable failed silently. transports.tor.advertise_on_nostr was undocumented despite being validated against node.rendezvous.nostr.enabled. The transport design document gains the BLE section it never had, written from the source: the backend cascade and its compile_error tripwire, the platform gate, the PSM advertisement wire layout and the byte budget that forces a 16-bit service-data key, and the probe and admission bounds. Three source files carried the same class of staleness and are corrected with the documentation: the OpenWrt ipk usage line and Makefile error text both named a packaging/openwrt that does not exist, and chaos.sh parsed --subnet without listing it. Folded in with the content commit, having been prepared alongside it: The three GitHub Action pins that had gone stale. Every third-party action is pinned to a commit SHA, nothing reports that a pin has aged, and re-resolving all ten against their tags found dorny/test-reporter@v2, taiki-e/install-action@v2 and vmactions/freebsd-vm@v1 had moved. The three install-action@nextest references stay unpinned, since that action reads the tool to install from the ref name. check-action-pins.sh passes at 75 references and all nine workflow files parse. The lockfile refresh, which is the mutating half of the dependency sweep. Thirty-six packages move to their latest semver-compatible versions and every one is transitive; nothing declared in Cargo.toml changes version. No advisory forces any of them. It was taken before the validation battery, because a gate run against a lockfile that later moves proves nothing about what ships. The sha2 0.10 to 0.11, hkdf 0.12 to 0.13 and bech32 0.11 to 0.12 majors, three of the four deferred at v0.4.0 for change surface rather than security. All three land with no source change. sha2 and hkdf must move together, since both depend on digest 0.11, and neither changes an algorithm. That matters because the chaining-key KDF in the Noise handshake is built on Hkdf::<Sha256>, where an output change would be a wire break rather than a compile error; no known-answer vectors exist for that path, so the wire-compatibility gate is what covers it. secp256k1 0.31 is deliberately absent, since nostr's own requirement would leave two copies of the ECC library in the tree. The README support matrix, rebuilt as one feature table broken out by Linux variety. A single Linux column hid that Debian, Ubuntu, Arch and NixOS are one glibc build differing in packaging, that OpenWrt is musl and drops BLE, and that Android is not a daemon platform. Transport rows sort by how many platforms carry them. A Native API row reads its platform set from the cfg gates. The installer row becomes a package format row naming the artifact, and only the .deb is exercised per release. Four changelog and release-note gaps the BLE re-walk found: a Bluetooth LE bullet stranded inside the released 0.4.2 section, a missing Fixed entry for the scan and probe loop counting a pool-refused connection as an established link, the unnamed embedder call that installs an application-owned radio, and the fact that stopping the transport now stops scanning as well as advertising. Three release-document gaps found walking the unsurveyed commits: the UDP reuse-flag fix stated in the direction opposite to the one it was made, with the silent second-daemon bind it prevents left unsaid; the corrected native-API socket paragraph carried into both release-note copies, which still named SOCK_SEQPACKET on FreeBSD and two kernels where three are handled; and the coordinate-cache hardening, which shipped with no text anywhere despite adding four operator-visible status fields. That last entry states plainly that the checks are mitigations and not a closure, since the coordinate is still not authenticated. Also folded in, the documentation pass that followed the content commit: A stage-pipeline diagram for the probe, embedded in the fipsctl reference under the five-stage list. It draws the five stages left to right with each stage's failure reasons below it, and the bypass that skips both lookup stages when the coordinates are cached or the target is a direct peer. Its branches come from the probe state machine rather than from the report, so the path stage is drawn as the one failure that does not stop the probe. A rewrite of the README's "What FIPS does" section. It now opens with what a machine running FIPS gets, rather than with the two deployment modes, and gives the self-organizing and permissionless property its own paragraph since it holds for both modes. A regrouping of the README's feature list into the mesh, getting traffic onto it, and running a node, with a bullet added for the native datagram API, which had none despite sitting in the support matrix. The Quick start now leads with the released packages rather than a source build. It also fixes a real defect: the package enables fips.service and fips-dns.service and starts neither on a fresh install, so .fips name resolution was silently dead until the next reboot and neither page said to start the service. A rewrite of the release notes. They opened with seven subsections of upgrade caveats and reached the first feature two hundred lines in; they now open with a summary of the release and elaborate below it in the same order. Android is stated as supported through an embedded crate rather than as a standalone daemon, consistently across all three documents. The OpenWrt pair is corrected: it is 802.11s between routers with FIPS supplying encryption, authentication and routing, plus a convention of an open !FIPS SSID a client joins over WiFi, not meshing over a router's own radios. The probe's path output is described as the least-common-ancestor walk, which is the worst-case fallback route rather than the route a packet takes. Detail that did not change what a reader does was cut from the notes and kept in the changelog.
200 lines
8.7 KiB
Markdown
200 lines
8.7 KiB
Markdown
# PR Review Checklist
|
|
|
|
<!-- markdownlint-disable MD013 -->
|
|
|
|
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 <num> --json title,body,author,headRefName,baseRefName,headRefOid,baseRefOid,mergeable,statusCheckRollup,commits
|
|
```
|
|
|
|
2. The diff.
|
|
|
|
```bash
|
|
gh pr diff <num>
|
|
```
|
|
|
|
3. Base-branch freshness. How many commits have landed on the PR's
|
|
base since the PR forked from it.
|
|
4. Project guidance. Read [CONTRIBUTING.md](CONTRIBUTING.md) and
|
|
[docs/branching.md](docs/branching.md). 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 `CONTRIBUTING.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/<full-40-char-sha>/<path>#L<start>-L<end>
|
|
```
|
|
|
|
For multi-line ranges include at least one line of context before
|
|
and after the line(s) being discussed. After `gh pr checkout <num>`,
|
|
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.
|