# Pr Review > 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 - Skill: `tomevault-io/pr-review-11` (Agent Skill, multi-file: 2 files) - Install (CLI): `npx skillmds@latest add tomevault-io/pr-review-11` - Raw SKILL.md: https://api.skillmd.com/api/skills/tomevault-io/pr-review-11/raw - Safety review: pending (external: skill-scanner PASS, skillspector PASS) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: tomevault-io (https://skillmd.com/u/tomevault-io) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/tomevault-io/pr-review-11 --- # 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 --json title,body,files,additions,deletions` and `gh pr diff `. - 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. ### 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 ### Blocking - file:line — issue — why it matters downstream — suggested fix ### Should-fix - ... ### Consider - ... ### Verification run - cargo test: - cargo test --features tokio / futures: - cargo clippy / cargo doc: - (any perf or invariant checks performed) ### Semver impact - — reason ### Recommendation - — one-line rationale for the human maintainer ``` --- > Source: [nlfiedler/fastcdc-rs](https://github.com/nlfiedler/fastcdc-rs) — distributed by [TomeVault](https://tomevault.io).