review

name	review
description	Run a deep code review across the manifold-csg crates
user-invocable	true

Review

Run a thorough code review of the manifold-csg workspace. Check all crates/ source files.

Arguments

• No arguments: review the entire codebase
• A file path or glob: review only matching files (e.g., crates/manifold-csg/src/manifold.rs)
• since release: review only files changed since the last release tag (use git diff --name-only $(git describe --tags --abbrev=0)..HEAD -- '*.rs' to get the list)
• staged: review only staged files (git diff --cached --name-only -- '*.rs')
• branch: review only files changed on the current branch vs main (git diff --name-only main...HEAD -- '*.rs')

Review Categories

Work through each category in order. For each finding, cite the file and line number.

1. Idiomatic Rust

Review as the pickiest expert Rust reviewer. Look for:

• Anti-patterns and non-idiomatic code
• Misuse of language features (lifetimes, traits, enums, error handling)
• Places where Result should replace bool/Option, enums should replace strings, named structs should replace tuples
• Missing standard trait implementations (Display, From, Default, etc.)
• Unnecessary clones, allocations, or copies
• Iterator chains that could replace manual loops

2. Code Smells

• Dead code (unused functions, imports, fields, variants)
• Duplicate code — check across crates, not just within files
• Functions that are too long or do too many things
• Refactoring opportunities (extract function, simplify conditionals)
• Magic numbers that should be named constants
• Stale comments — references to upstream PR/issue numbers or transient implementation details that will go stale. These belong in CLAUDE.md or API_COVERAGE.md, not code comments.
• Stale docs — hardcoded counts (test count, function count, version numbers) in README or API_COVERAGE.md that will go stale on the next change. Prefer prose over numbers, or make them easy to regenerate.

3. FFI Safety & Resource Management

This is the highest-risk area for a bindings crate. Verify:

• Every C allocation (manifold_alloc_*) is paired with deallocation (manifold_delete_*) on ALL paths including error returns and panics
• Drop implementations null-check before freeing
• No double-free paths — verify ownership transfer semantics in batch operations, decompose, etc.
• unsafe impl Send — justification still valid? Check upstream C++ for any new thread-local state or shared mutable state
• unsafe impl Sync — verify the justification holds. Check that whatever synchronization mechanism upstream uses (carry-patch or merged) is actually present in the pinned commit or applied via patch.
• No panic-across-FFI risk — if a Rust panic unwinds through C frames, it's UB. Check that all FFI callback trampolines use catch_unwind
• Integer casts at the FFI boundary (u32↔u64, usize→c_int) — check for truncation/overflow
• Pointer validity — every raw pointer dereference must have a preceding validity argument (allocation or invariant)
• Buffer size correctness — when copying data out via length+copy accessor pairs, verify buffer sizes match what the C API expects
• ManifoldManifoldPair — verify both returned pointers are always consumed (no leak if caller ignores one half of a split)
• Mutable FFI methods (those taking *mut) — verify the safe wrapper requires &mut self to prevent aliasing

4. Numerical Precision

Precision is our key differentiator (f64/MeshGL64). Verify:

• f64 precision is used by default everywhere — no accidental f32 narrowing
• f32 code paths are clearly documented as lossy
• No unnecessary f64→f32→f64 round-trips in internal code paths
• Operations that bridge CrossSection↔Manifold don't silently lose precision through intermediate representations
• Tolerance/epsilon values are documented and appropriate

5. API Completeness

We bind the manifold3d C API (pinned to a specific upstream commit in build.rs). The review focus is on maintaining completeness.

Sys crate vs upstream header:

• Find the built C header (manifoldc.h) under target/*/build/manifold-csg-sys-*/out/ and diff against manifold-csg-sys/src/lib.rs
• Check for newly added C API functions that need binding
• Verify all function signatures still match the header — ABI drift from upstream changes

Safe wrapper coverage:

• For every function group bound in the sys crate, verify there is a corresponding safe method. Flag sys-level functions that have no safe wrapper yet. Prioritize by usefulness.
• Are all callback-based APIs wrapped safely with catch_unwind?
• Are all MeshGL/MeshGL64 accessors exposed through the safe types?
• Are the quality globals exposed and documented as affecting global state?

Feature flag coverage:

• Does the nalgebra feature cover all methods that take/return geometric types?
• Are there other popular geometry crates that should have optional integration?

6. API Ergonomics

Review the safe API from a user's perspective:

• Are method signatures intuitive? Would a first-time user understand the parameter order?
• Are there missing convenience methods?
• Should any methods accept impl Into<T> for flexibility?
• Are builder patterns appropriate anywhere?
• Error types — are they specific enough to be actionable?
• Does the re-export structure in lib.rs give users a clean import experience?
• Are operator overloads discoverable and documented?

C/C++ API parity (many users will transition from the C/C++ library):

• Compare parameter order and types against the C header. Flag reordering, renamed parameters, or hidden defaults that would surprise a C/C++ user.
• Check that optional/defaulted parameters match C API defaults. If a wrapper omits a C parameter, verify there's a sensible default or _with_options variant.
• Check consistency: do similar functions handle the same parameter the same way?
• Where Rust packs multiple C params into an array (e.g., normal: [f64; 3] instead of nx, ny, nz), verify this is documented

7. Test Coverage & Quality

Assess the test suite:

• Untested public functions — every public method should have at least one test
• Edge cases — empty inputs, zero-size primitives, very large/small values, degenerate geometry
• Negative testing — do error paths get exercised?
• Thread safety — is Send tested with actual thread spawning? Is Sync tested with concurrent reads?
• Round-trip fidelity — are mesh export→import round-trips tested for volume/vertex preservation?
• Test isolation — do tests depend on execution order or shared mutable state?
• Assertion quality — are tests checking meaningful properties or just "doesn't panic"?

8. Examples & Documentation Artifacts

Examples:

• Do all examples compile and run? (cargo run -p manifold-csg --example <name>)
• Do examples cover the main entry points?
• Are examples free of unexplained unwrap()?

API_COVERAGE.md:

• Does it account for every function in the sys crate?
• Are safe wrapper links accurate?
• Does the summary count match the detailed tables?

README:

• Are feature descriptions accurate? (e.g., Send/Sync, f64 default, test count, upstream version)
• Do quick-start examples compile?
• Are links valid?
• MSRV, platform support, and build requirements documented?
• Badges present and pointing to the right crate/repo?

Docstrings:

• Do all main types (Manifold, CrossSection, MeshGL, MeshGL64) link to the upstream manifold3d API docs so users can understand parameter semantics?
• Does the crate-level doc (lib.rs) link to upstream?
• Run cargo doc --no-deps — any warnings?
• Are methods with non-obvious parameters (e.g., normal_idx, min_sharp_angle, column-major matrices) documented well enough that a user doesn't have to read C++ headers?

9. Packaging & Publishing Correctness

Versioning (see CLAUDE.md for the scheme):

• Verify sys crate version matches the upstream pin
• Verify safe crate dependency pins the correct sys version
• Sys patch bumps must be semver-compatible (additions only). Flag any violations.

Cargo.toml correctness:

• description, readme, keywords, categories — present and accurate
• package.exclude / package.include — no test fixtures or build artifacts in published crate
• links key in sys crate — correctly set

Publishing readiness:

• cargo publish --dry-run for both crates — does it succeed?
• Crate size — reasonably small? (no vendored C++ source)
• License files present and referenced

API stability audit (pre-publish):

• Flag any public API that feels provisional or likely to change — hard to undo after publishing
• Are there methods that should be pub(crate) instead of pub?
• Are return types locked in appropriately?

Dependency hygiene:

• cargo update --dry-run — anything to bump?
• Dev-dependencies correctly scoped?
• Feature flags complete?

Documentation:

• Crate-level //! docs present with examples
• Doc-tests — can any rust,ignore tests be made runnable?

10. Upstream Compatibility & Carry-Patches

This crate tracks manifold3d upstream, pinning to a specific commit SHA.

Pinned commit:

• Is the pinned SHA still on upstream's master branch (not reverted)?
• What's the latest upstream release tag? How far ahead/behind are we?
• Have any upstream changes since our pin affected the C API?

Carry-patches (crates/manifold-csg-sys/patches/):

• List all current patches. For each:
  • Has the upstream PR been merged? If so, is it included in our pinned commit? (Can be removed if yes.)
  • Does the patch still apply cleanly?
  • Is any safety justification in our code (e.g., unsafe impl Sync) dependent on this patch?
• Are there upstream fixes we need that aren't in our pin or patches?
• Patch scope: restricted to src/ (and bindings/ if relevant) — no test/ since we build with MANIFOLD_TEST=OFF, no docs/JS/python.
• Patch size roughly matches the upstream PR's stated diff. A patch that's much larger than the linked PR is a red flag — it usually means whoever generated it used git diff master..pr-branch instead of git diff $(git merge-base master pr-branch)..pr-branch, and silently picked up the reverse of master's intervening commits.
• Patch filename has a numeric prefix (e.g., 0001-) that encodes apply order, and references the upstream PR number.

Release PR descriptions and commit messages:

• Cross-repo PR refs use elalish/manifold#NNNN (no backticks) — bare #NNNN resolves to our own repo, and backticks suppress GitHub's auto-link entirely.
• Same-repo PR refs (#NNNN) are correct and auto-link as-is.
• When the release window includes a contributor PR (e.g., from a non-maintainer), credit them by @username in the release PR body.

Known upstream bugs:

• Check manifold3d issues for bugs affecting functions we bind
• Note any upstream error status propagation issues that affect our safe wrappers

11. Dependency Audit

• Cargo dependencies at latest? (cargo update --dry-run)
• C++ dependencies — check upstream releases for newer versions. Manifold pulls Clipper2 and TBB via CMake FetchContent.
• License concerns — warn on any copyleft/viral license
• Unnecessary or redundant dependencies

12. Cross-Platform & CI

• Does the build work on all platforms? Check CI results.
• Are there platform-specific code paths in build.rs?
• Does CI test with and without the parallel feature?
• Does CI test the declared MSRV?
• Are there CI jobs that should exist but don't?

Output Format

Group findings by category. For each finding:

**[Category] file.rs:123** — Short description of the issue.
Suggested fix or explanation.

Assign each finding a severity:

• error: Correctness bug, unsoundness, undefined behavior, memory safety violation
• warning: Likely bug, missing safety check, precision loss, incomplete API
• note: Style, ergonomics, documentation, nice-to-have improvement

At the end, provide a summary: total findings per category, severity breakdown (error/warning/note), and recommended priority order for fixes.

If a category has zero findings, say so explicitly — don't skip it silently.