Repository navigation
cmux-app-ffi: generate cmux_motion_ffi.h with cbindgen, fail on drift - #35307
Conversation
…r drift The Mac app imports include/cmux_motion_ffi.h (CCmuxMotionFFI); it was hand-written, so a header that disagreed with the Rust extern "C" signatures or #[repr(C)] layouts was silent UB no Swift test sees. - cbindgen.toml generates the header from src/motion.rs, src/hover.rs and cmux-motion's hover::c structs; the Rust doc comments become the C comments. - tests/c_header.rs regenerates it and fails with a unified diff when the committed header differs; CMUX_APP_FFI_HEADER_BLESS=1 rewrites it. rust-workspaces.yml already runs cargo test --locked on this workspace when it or a path dependency changes, so no new CI job. - cbindgen is a dev-dependency pinned =0.29.2 in Cargo.lock; no build.rs, so no build rewrites a tracked file. The regenerated header declares the same 45 types and functions as the hand-written one (compared after preprocessing, order-insensitive) and yields the same 29 exported symbols build-app-ffi.sh greps from it. Only order, comments and line wrapping changed. No signature or ABI_VERSION change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-2941a46bf10b2e0f35b7
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
cbindgen copied HoverRect::ZERO and ::NULL as CmuxHoverRect_* macros, and NULL's f64_INFINITY is not C. Mark both cbindgen:ignore (doc comments only) and fail the header test on any macro but the include guard. The header again has exactly the old one's declarations and compiles with clang -std=c11 -Wall -Wextra. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-2941a46bf10b2e0f35b7
teamleaderleo
left a comment
There was a problem hiding this comment.
Review (claude-opus-5-5): SHIP at 5ce2a76
|
Found 11 test failures on Blacksmith runners: Failures
...and 1 more test failures. View all on Blacksmith
|
|
Non-green checks at 5ce2a76, classified before landing:
|
|
Review (codex, router-review): SHIP at 5ce2a76 Reviewer: codex gpt-6.1-sol xhigh, fast tier. SHIP at 5ce2a76 Visual verdict: no images; Leo tries the build Clang confirms identical types and field order across all 45 declarations, with the same 29 export matches; the header passes C11 with warnings as errors. Reviewed drift-test logic and CI wiring; Cargo tests were unavailable because this box lacks Rust tooling. Follow-ups: none |
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
include/cmux_motion_ffi.h, the header the Mac app imports asCCmuxMotionFFI, is now generated by cbindgen from the Rust C ABI. A test fails, and prints the diff, when the committed header no longer matches what cbindgen generates.Until now the header was hand-written next to
src/motion.rsandsrc/hover.rs. If it disagreed with a Rustextern "C"signature or a#[repr(C)]layout, the result was undefined behavior that no Swift test catches. This comes from an ecosystem sweep that looked for mature outside tools to borrow where it's cheap, not to rewrite working code ("We always want to consider the option, consider the alternative."). The hot C-ABI paths stay on C, and iOS keeps UniFFI.What changed
cmux-tui/crates/cmux-app-ffi/cbindgen.tomlgenerates the header fromsrc/motion.rs,src/hover.rsand cmux-motion'shover::cstructs. The Rust doc comments become the C comments.tests/c_header.rsregenerates the header and fails with a unified diff when the committed file differs.CMUX_APP_FFI_HEADER_BLESS=1 cargo test --test c_headerrewrites it.rust-workspaces.ymlalready runscargo test --lockedon this workspace whenever the crate or a path dependency (such as cmux-motion) changes, so this adds no CI job.=0.29.2in the committedCargo.lock. There is nobuild.rs, so a build never rewrites a tracked file. The staticlib's own dependency versions are unchanged; the lock diff only adds cbindgen's tree and disambiguatessyn.ABI_VERSIONchanged.HoverRect::ZERO/::NULLin cmux-motion getcbindgen:ignore(doc comments only). Without it, cbindgen copies them asCmuxHoverRect_*macros, andNULL'sf64_INFINITYis not valid C. The test also fails on any macro other than the include guard, since this ABI has no constants.Same ABI
const double from[4]becomingconst double *from(the same parameter type in C).build-app-ffi.shbuilds its exported-symbols list by greppingcmux_*(from this header. That grep gives the same 29 symbols for both headers.build-app-ffi.sh(headers_in) still copiescmux-app-ffi/include/cmux_motion_ffi.hintoCCmuxMotionFFI.Other FFI headers (not in this PR)
I tried the same recipe on the other three crates. None reproduce cleanly yet, so each would be its own follow-up:
cmux-rd-ffi: cbindgen emits anonymous typedef structs where the hand-written header has tagged ones, and stops on two conflictingNONEconstants.cmux-layout-reducer-ffi: its opaque handle types (Focus,Scroll,Mirror,Faces) would need renames in the config.cmux-terminal-client:Option<extern fn>callback typedefs need explicit config.Testing
Observed
cargo test --locked --test c_header: passes on the head.cmux_motion_ease'st: f64tof32insrc/motion.rsmade the test fail with the one-line-double … t/+float … tdiff. After reverting, it passes again.cbindgen:ignorelines fails the test onunexpected macros in the header. Restoring them passes.clang -fsyntax-only -std=c11 -Wall -Wextra.cargo clippy --locked --all-targets -- -D warningsandcargo fmt --checkincmux-app-ffiare clean.prepush-gate.pyreports no new violations.Not run
cmux-app-ffi.Changelog
none
🤖 Generated with Claude Code
Agent session: agent session