| name | pr-review |
| description | Review a pull request (or the current diff) against fastcdc-rs project standards. Use when asked to review a PR, vet changes before merging to master, or check a branch for the kinds of issues that have slipped through before. Distills reviewer feedback (notably ciehanski on PR |
| metadata | {"author":"nlfiedler"} |
fastcdc-rs PR review
This skill reviews a pull request against the standards this project's maintainers and
active downstream users expect. The criteria below were derived from real reviewer
feedback (ciehanski on PR #44 and #45) plus the determinism guarantees in CLAUDE.md.
fastcdc is a published crate (currently 4.0.1) with real downstream consumers, so
the bar is "would a careful human depending on this crate be comfortable merging it into
master?" — not "does it compile and pass the existing tests?"
How to run the review
- Determine the scope of changes:
- A specific PR:
gh pr view <N> --json title,body,files,additions,deletions and
gh pr diff <N>.
- The current branch:
git diff master...HEAD (or git diff for uncommitted work).
- Read the changed files in full, not just the diff hunks — context outside the hunk
often determines whether a change is correct (e.g. constructor invariants that make an
edge case reachable or not).
- Walk every criterion below against the change. For each finding, cite
file:line and say why it matters to a downstream user, not just what it is.
- Verify, don't assume: run
cargo test, cargo test --features tokio,
cargo test --features futures, cargo clippy, and cargo doc --features futures.
Report what you actually ran and its result. If a claim in the PR body is testable
(e.g. "every byte is emitted", "no perf regression"), test it or say you couldn't.
- Summarize findings grouped as Blocking, Should-fix, and Consider, then give
an explicit merge recommendation.
Review criteria
1. No silent data loss or silent failures
The chunking code's whole job is to account for every input byte. A break, return,
early-exit, or swallowed error that can drop or skip data is a blocking issue.
- Trace every loop exit and error path: can any input byte fail to be emitted?
- An edge case that is "currently unreachable through the public API" is still a latent
bug. It must
debug_assert! (or otherwise fail loudly) and degrade safely (emit the
leftover) rather than silently dropping data. (This is exactly the silent-break
tail-drop ciehanski flagged on #44.)
- Confirm the chunker produces contiguous, gapless, exact coverage of the input.
2. Per-iteration / per-chunk waste
Hot-path work that could be hoisted out of the loop is a real defect in a perf-critical
crate, not a nitpick.
- Look for conversions, allocations,
try_into, copies, or table rebuilds happening
per chunk or per byte that could be done once (the per-chunk try_into on the GEAR
tables ciehanski flagged on #44).
- The core chunking loop is latency-bound on the gear-hash recurrence; be skeptical of
changes that add work to it, and ask for A/B benchmark evidence (see criterion 6).
3. Public API surface and semver
This is a published crate; API changes ripple to every downstream user.
- For any new
pub item (struct, fn, field, trait, enum variant): is it intended to be
public, or should it be pub(crate) / private? Default to the smallest surface that
works. (ciehanski: "A new public Chunker struct is introduced — should this struct
be public to the consuming user?")
- Does the change alter, remove, or rename any existing public API, or change behavior of
existing public API? If so it's a breaking change requiring a major version bump
per semver. Flag the required version change explicitly against the current
Cargo.toml
version.
- Adding a public item is at least a minor bump; document the expected version change.
- New public items need rustdoc and ideally a doctest/example.
4. Determinism is a contract
Identical cut points across versions are a core guarantee (see CLAUDE.md).
- Any change that alters cut points / hashes / chunk boundaries must be intentional,
called out loudly in the PR, and is itself a breaking change.
- The hardcoded fixture hashes/lengths in tests are the guardrail — they should only
change deliberately. A PR that edits expected fixture values needs strong justification.
- Cross-check
v2016 and v2020 produce the same cut points where the docs claim they do.
5. Test rigor — tests must actually catch regressions
- New behavior needs tests that assert the invariant that matters (e.g. every byte
emitted, in order, no gaps), not just that the happy path returns something.
- A test is only credible if it fails when the code is broken. Prefer changes where the
author demonstrably verified this (deliberately break it, watch it fail). When reviewing,
if a test's failure mode is unclear, mentally (or actually) inject the bug and check the
test would catch it.
- Be honest about coverage gaps: if a branch can't be reached through the public API and
therefore isn't covered by a test, say so plainly rather than implying full coverage.
- Cover edge cases: empty input, sub-minimum-size input, all-identical bytes (worst case
for finding a cut), and the real fixture.
- Run the feature-gated test matrix: default,
tokio, futures (the two async features
are mutually exclusive — check the cfg guards still hold).
6. Performance claims need reproducible evidence
- Any "faster" / "no regression" claim should be backed by an interleaved old-vs-new A/B
measurement that asserts identical output before timing, ideally on more than one
architecture (e.g. ARM + x86_64). Don't accept single-run wall-clock numbers.
- New perf-sensitive changes are a good prompt to ask whether CI should gain a
cross-platform test / perf-regression workflow (ciehanski's suggestion), so future PRs
have data before merge.
7. Human accountability and provenance
This was ciehanski's central concern: PRs and comments written end-to-end by an AI agent
with no evident human review, landing in a depended-on crate.
- The review's job is to be the human-quality gate. Surface issues directly and concretely
so a human can make the merge decision — never imply "looks good, merge it" as a
substitute for maintainer judgment.
- Flag anything that looks auto-generated and unreviewed: plausible-but-wrong assertions,
comments/PR text that overstate what was verified, tests that look like coverage but
don't exercise the claimed invariant.
- Encourage a clear paper trail: PR description states what changed, why, what was tested,
and the semver impact. Do not vouch for code paths you did not actually exercise.
Output format
## Review of <PR # / branch>
### Blocking
- file:line — issue — why it matters downstream — suggested fix
### Should-fix
- ...
### Consider
- ...
### Verification run
- cargo test: <result>
- cargo test --features tokio / futures: <result>
- cargo clippy / cargo doc: <result>
- (any perf or invariant checks performed)
### Semver impact
- <none / patch / minor / major> — reason
### Recommendation
- <merge / merge after fixes / needs work> — one-line rationale for the human maintainer
Source: nlfiedler/fastcdc-rs — distributed by TomeVault.
