fastcdc-rs PR Review

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

1. 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).
2. 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).
3. 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.
4. 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.
5. 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.