deep-review - SKILL.md Agent Skill

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

Deep 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

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 on Manifold and CrossSection — justification still valid? Check upstream C++ for any new thread-local state or shared mutable state
Sync is deliberately NOT implemented — verify this is still correct (check for mutable shared_ptr<CsgNode> pNode_ in upstream)
No panic-across-FFI risk — if a Rust panic unwinds through C frames, it's UB. Check that no panic-capable code runs inside FFI call sequences
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 manifold_meshgl*_vert_properties / manifold_meshgl*_tri_verts, 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)

Build script (build.rs) correctness for cross-compile:

cfg!(target_os = ...) / cfg!(target_arch = ...) / cfg!(target_env = ...) in build.rs is a footgun when used to detect the target — these macros evaluate at the build-script-host's compile time, NOT the target's. Coincidentally correct as long as we never cross-compile; fails silently the moment we do (e.g. wasm). Use env::var("CARGO_CFG_TARGET_OS") / ..._ARCH / ..._ENV instead. (cfg! is fine when you genuinely want host-side detection — e.g., picking a shell command for the build script's own use.)
cargo:rustc-link-arg=FLAG from a sys crate's build.rs does NOT propagate to downstream link invocations — only rustc-link-lib and rustc-link-search do. The proper sys-crate idiom for forwarding link flags: emit cargo:KEY=VALUE (Cargo translates this into DEP_<UPPERCASE_LINKS>_<UPPERCASE_KEY> env var visible to dependents), and have the safe wrapper crate's build.rs read it and re-emit cargo:rustc-link-arg=.... End-user binaries then need a similar build.rs (or .cargo/config.toml). Flag any cargo:rustc-link-arg=... in a sys crate that isn't backed by this pattern.
The artifact-typed variants (cargo:rustc-link-arg-bins=, -tests=, -cdylib=) are only legal from crates that declare those targets — a library-only sys crate can't use them (cargo rejects with "package does not have a bin target"). Flag any of these in a crate that doesn't have the matching target.

CI cache key correctness (actions/cache + cmake builds):

cmake's CMakeCache.txt records absolute paths (toolchain file, source dir, -D options). If a cached target/ is restored on a run where any of those paths differ — including from a different OUT_DIR layout, a different emsdk install location, or a different CMakeLists.txt source dir — cmake refuses with "source does not match the source used to generate cache." We've been bitten by this twice: once on the Emscripten lane (emsdk path moved between runs), once on the wasm32-uu lane (source dir moved when adopting the shim's CMake helper).
The footgun is restore-keys being too permissive. restore-keys does prefix matching, so if the prefix is just ${runner.os}-cargo-<lane>-<cache-version>-, ANY old cache for that lane gets pulled in — even one built with an incompatible cmake source dir.
Fix pattern: split the cache key so layout-affecting inputs (env vars like EMSDK, files like build.rs and wasm32-uu/**) are part of the restore-keys PREFIX, while frequently-changing inputs (Cargo.lock, lower-level build.rs files) live in the suffix. That way changing a layout input shifts the prefix → no incompatible cache restored. Look for any cache stanza whose restore-keys doesn't include the same prefix-segments as the key for inputs that affect cmake's recorded paths.
The cache-version bump file is the manual escape hatch when caches go bad — it busts every lane's cache. Useful as a one-shot recovery, but not a substitute for fixing the key structure.

4. Numerical Precision

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

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

5. API Completeness

We bind the complete manifold3d v3.4.1 C API (256 functions in manifold-csg-sys). The review focus is on maintaining completeness and ensuring the safe layer covers everything useful.

Sys crate vs upstream header:

Read the manifold3d C header (manifoldc.h, built during compilation at target/*/build/manifold-csg-sys-*/out/build/_deps/manifold-src/bindings/c/include/manifold/manifoldc.h) and diff against manifold-csg-sys/src/lib.rs
If we've updated the pinned manifold version, check for newly added C API functions that need binding
Verify all function signatures still match the header (parameter types, return types) — ABI drift from upstream changes

Safe wrapper coverage:

For every function group bound in manifold-csg-sys, verify there is a corresponding safe method in manifold-csg. Flag sys-level functions that have no safe wrapper yet. Prioritize by usefulness.
Specifically check: are all callback-based APIs (warp, set_properties, level_set, write_obj) wrapped safely? These are the hardest to get right.
Are all MeshGL/MeshGL64 advanced accessors (merge, run_index, face_id, tangents) exposed through the safe MeshGL/MeshGL64 types?
Are the quality globals (set_min_circular_angle, set_circular_segments, etc.) exposed? If so, are they documented as affecting global state?

Feature flag coverage:

Does the nalgebra feature cover all methods that take/return geometric types (vectors, points, matrices)?
Are there other popular geometry crates that should have optional integration (e.g., glam, mint)?

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? (e.g., Manifold::translate_z(), CrossSection::offset_round())
Should any methods accept impl Into<T> for flexibility?
Are builder patterns appropriate anywhere? (e.g., extrude with optional twist/scale)
Error types — are they specific enough to be actionable? Can users match on error variants?
Does the re-export structure in lib.rs give users a clean import experience?
Are operator overloads (+, -, ^) discoverable and documented?
Should Manifold::extrude be an associated function or a method on CrossSection?

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

For every safe wrapper method, compare parameter order and types against the corresponding C function in manifoldc.h. Flag any reordering, renamed parameters, or hidden defaults that would surprise a C/C++ user.
Check that optional/defaulted parameters match C API defaults. If a Rust wrapper omits a C parameter (e.g., center, slices, twist), verify there's either a sensible default or a _with_options variant that exposes full control.
Check consistency: do similar functions handle the same parameter the same way? (e.g., cube, cylinder, and CrossSection::square should all handle center identically — not some hardcoded and some exposed)
Verify the layout/ordering of array parameters matches C conventions (e.g., transform's column-major [f64; 12] should document the mapping to C's 12 individual params)
Check that enum variant names are recognizable to C users (e.g., JoinType::Round maps obviously to MANIFOLD_JOIN_TYPE_ROUND)
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 in the safe API 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? (invalid meshes, empty polygons, etc.)
Thread safety — is Send tested with actual thread spawning, not just compile-time assertions?
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?
#[ignore] tests — are they still relevant or should they be deleted/fixed?
Assertion quality — are tests checking meaningful properties or just "doesn't panic"?
Multi-target gating: tests that depend on host-OS facilities (std::thread::spawn, filesystem, sockets, signals) should be gated with #[cfg_attr(target_os = "...", ignore = "explanation")] for targets that lack them. The ignore reason should explain why it's ignored on this target, not just that it is.
For cross-compiled targets without a native runner (wasm, embedded), check whether CARGO_TARGET_<TRIPLE>_RUNNER is configured (.cargo/config.toml, CI workflow env). Without a runner, "build clean" doesn't tell us tests pass.

8. Examples & Documentation Artifacts

Verify the documentation artifacts are consistent with the code:

Examples (crates/manifold-csg/examples/):

Do all examples compile and run without errors? (cargo run -p manifold-csg --example basics, etc.)
Do examples cover the main entry points: primitives, booleans, transforms, 2D cross-sections, extrusion, SDF, OBJ I/O, and threading?
Are examples free of unwrap() on fallible operations without explanation?
Do examples demonstrate idiomatic usage patterns that new users should follow?

API coverage table (API_COVERAGE.md):

Does the table account for every function declared in manifold-csg-sys/src/lib.rs?
Are the safe wrapper links accurate (correct file and line number)?
Are "Internal" and "Not wrapped" statuses correct?
Does the summary count match the detailed tables?
Has the table been updated after any API additions or removals?

README:

Are feature descriptions accurate and consistent with the code?
Do quick-start examples compile?
Are links to examples/, API_COVERAGE.md, and source files valid?

9. Packaging & Publishing Correctness

Review everything a crate maintainer needs for correct, user-friendly publishing:

Versioning scheme (see CLAUDE.md):

manifold-csg-sys version = {upstream_major}.{upstream_minor}.{100+our_release} (e.g., 3.4.100). Verify the version matches the manifold3d tag pinned in build.rs.
manifold-csg version = standard semver (0.x.y), independent of upstream. Its dependency on manifold-csg-sys must pin the correct sys version.
When the manifold3d pin is bumped, the sys crate version MUST be updated to match. Flag any mismatch.

Cargo.toml correctness:

package.description — present, concise, accurate for both crates
package.documentation — points to docs.rs (or will auto-generate)
package.readme — set if README exists
package.keywords and package.categories — set and relevant (max 5 keywords). Good keywords for discoverability: csg, geometry, mesh, manifold, 3d
package.exclude / package.include — exclude test fixtures, build artifacts, CI config from the published crate
links key in sys crate — correctly set to prevent duplicate linking
Edition — using latest stable edition? (currently 2024)

Dependency hygiene:

Are all dependencies at their latest compatible versions? (cargo update --dry-run)
Are dev-dependencies correctly scoped? (nothing test-only leaking into the main dependency tree)
build-dependencies — cmake version current?
Feature flags — are there any that should exist? (e.g., parallel for TBB, nalgebra for convenience conversions)

Publishing readiness:

cargo publish --dry-run for both crates — does it succeed?
Crate size — is the published crate reasonably small? (no vendored C++ source, no build artifacts)
License files — LICENSE-APACHE and LICENSE-MIT present and referenced in Cargo.toml
links = "manifold" — will this conflict with other -sys crates linking the same library? Document how users should handle this.

Ecosystem & Cargo evolution:

Is the resolver = "2" setting still the recommended default, or has a newer resolver landed?
Are there new Cargo features (e.g., artifact dependencies, public/private dependencies via dep:, lints table changes) that would improve the crate?
Are there upcoming Rust edition changes that affect this code? (e.g., new unsafe extern syntax was stabilized in 2024 edition — are we using it?)
Check rust-version (MSRV) — should we declare one? What's the minimum Rust version that compiles this?
Are there new crates.io policies (e.g., trusted publishing, provenance attestations) we should adopt?

Documentation:

Crate-level //! docs — present, has example code, links to upstream manifold3d
#[doc(hidden)] on internal items that leak through pub(crate)
Do doc-tests compile? (currently ignored with rust,ignore — can they be made runnable?)

10. Upstream Compatibility

This crate tracks manifold3d upstream. Verify:

Pinned version in build.rs — what version are we on? What's latest? What changed?
ABI compatibility — have any C API function signatures changed in newer manifold3d releases?
Deprecated functions — are we binding any C API functions that upstream has deprecated?
wasm32-unknown-unknown cfg-gates may need revisiting. Grep for target_os = "unknown" across the workspace. Each gated FFI declaration / safe wrapper / test is gated because that C API surface postdates the shim's tested manifold pin. When the shim's tested pin moves to (or past) our host pin, those gates can be removed. The OBJ I/O gates (manifold_*_obj) are gated for a different reason (iostream patches strip them) and stay regardless.
New capabilities — has upstream added significant new C API surface (new types, new operations) that we're missing?
Build system changes — has manifold3d changed its CMake structure, added/removed FetchContent deps, changed library names?
Known upstream bugs — check manifold3d issues for bugs affecting functions we bind (especially boolean operations, MeshGL64, CrossSection offset)

11. Dependency Audit

Check all Cargo dependencies are at their latest published version — run cargo update --dry-run and flag anything that can be bumped
Check C++ dependencies in build.rs — the manifold3d tag is pinned there. Check github.com/elalish/manifold/releases for newer releases. Manifold also pulls Clipper2 and TBB via CMake FetchContent (versions controlled by manifold's CMakeLists.txt)
Known issues in upstream crates or C++ libraries
License concerns — warn on any copyleft/viral license (GPL, LGPL, AGPL, SSPL). Note: manifold3d is Apache-2.0, Clipper2 is Boost, TBB is Apache-2.0 — all compatible
Unnecessary or redundant dependencies

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.