fix(ffi): regenerate the drifted cbindgen header and gate it in CI - #277
Open
YuanYuYuan wants to merge 2 commits into
Open
fix(ffi): regenerate the drifted cbindgen header and gate it in CI#277YuanYuYuan wants to merge 2 commits into
YuanYuYuan wants to merge 2 commits into
Conversation
The committed header lacked the namespace_ field that CContextConfig has carried since it was added. cgo sizes its struct from this file, so Go allocated 80 bytes where the Rust side reads 88 and dereferenced cfg.namespace past the end of the allocation on every advanced-config ContextBuilder.Build(). Regenerated with the cbindgen the flake pins (0.29.3). Also restores 11 parameter-type constants, KEEP_ALL_CACHE_DEPTH, and corrected doc text. Refs #270
Nothing regenerated the header in CI, so its drift from the Rust structs was invisible. cbindgen was absent from every dev shell, and build.rs degrades its absence to a non-fatal cargo:warning=, so enabling the ffi feature alone would not have caught this. Adds rust-cbindgen to commonBuildInputs and a check-ffi-header check that deletes the header, regenerates it, and fails on any diff -- mirroring check-python-stubs, which gates the generated Python stubs the same way. The deletion is what makes a build that generates nothing fail instead of reporting on the committed copy; cbindgen's absence is checked explicitly so that path cannot pass vacuously either. Runs as its own nix-based job because go-tests has no nix environment and cbindgen must come from the pinned dev shell -- 0.29.4 emits a constant 0.29.3 does not, so an unpinned version would itself read as drift. Closes item 3 of #270
There was a problem hiding this comment.
Pull request overview
Regenerates the committed FFI header and adds CI drift detection.
Changes:
- Updates the C header with the missing context namespace field and constants.
- Adds cbindgen and a header freshness script.
- Adds a dedicated CI freshness job.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
scripts/test-pure-rust.nu |
Adds the regeneration check. |
flake.nix |
Adds cbindgen to build inputs. |
crates/hiroz-go/hiroz/hiroz_ffi.h |
Regenerates the C FFI surface. |
.github/workflows/ci.yml |
Runs the freshness check in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
crates/hiroz-go/hiroz/hiroz_ffi.his generated by cbindgen fromcrates/hiroz/src/ffi/and committed, because cgo needs it at build time. Nothing in CI regenerated it, so it drifted. The drift was not confined to documentation.This PR regenerates the header and adds a CI gate that fails when it drifts again.
The defect
The committed header was missing the
namespace_field thatCContextConfighas carried since it was added (crates/hiroz/src/ffi/context.rs:36-38; cbindgen appends the trailing underscore becausenamespaceis a C++ keyword):crates/hiroz-go/hiroz/context.go:133declaresvar cfg C.hiroz_context_config_tcrates/hiroz/src/ffi/context.rsCContextConfigis 88 bytes;namespacesits at offset 80crates/hiroz/src/ffi/context.rs:84readscfg.namespacecstr_to_strdereferences it as a C stringReached by any Go caller that sets an advanced-config option, which is what selects the
hiroz_context_create_with_configpath incontext.go.The 80/88 figures are from compiling a
sizeofprobe against each version of the header. They also follow from the x86-64 layout:enable_loggingends at offset 73, tail padding to 80, and the appended pointer occupies 80-87.What this PR does
crates/hiroz-go/hiroz/hiroz_ffi.h(52 insertions, 4 deletions)rust-cbindgentocommonBuildInputsflake.nixcheck-ffi-headercommand and wire it into the pipelinescripts/test-pure-rust.nuffi-header-freshjob, mirroringpython-stubs-fresh.github/workflows/ci.ymlThe regenerated header adds, beyond
namespace_: theKEEP_ALL_CACHE_DEPTHconstant,DEPTH_RECURSIVE, ten parameter-type constants (NOT_SETthroughSTRING_ARRAY), and corrected doc text forDEFAULT_HISTORY_DEPTHandhiroz_service_client_wait_for_service. None of these had ever reached the header.rust-cbindgenwas in no dev shell. That is the root cause: CI could not have regenerated the header even if something had asked it to. It is placed besidego, since that header is what cgo compiles against.Two details in the check are load-bearing rather than incidental:
Dentry.crates/hiroz/build.rs:55-60degrades a missing cbindgen to a non-fatalcargo:warning=. Without the assertion the build would succeed, write nothing, and the check would report on whatever was already in the tree.The check also
touchescrates/hiroz/build.rs, so a warm target directory cannot turn it into a passing no-op.It runs as its own Nix-based job because
go-testshas no Nix environment, and cbindgen must come from the pinned dev shell. cbindgen 0.29.4 emits aCDR_HEADER_LEconstant that 0.29.3 does not, so an unpinned cbindgen would itself read as drift. The version used is printed in the log, so that case is diagnosable rather than mysterious.Evidence
The header on
mainis stale. Running the new check against it fails:Three inputs, so the detector is known to detect rather than merely never to fire:
mainas-is)e20f37a9)PATHrmBreaking changes
sizeof(hiroz_context_config_t)Not affected: the Rust API, and the offsets of every pre-existing member — the field is appended, so source compiled against the new header is source-compatible.
namespace_at offset 80 — the out-of-bounds read described above. The doc comment inherited from the Rust source says "to preserve ABI compatibility"; that is too strong. The accurate statement is offset compatibility plus a mandatory rebuild. The comment is left as-is in this PR.cgo rebuilds automatically for the in-tree Go bindings. Any out-of-tree C consumer holding a copy of the old header must regenerate it.
Coverage this does not have
--features ffiunder-D warnings) is untouched, as are item 1 (the 22missing_safety_doccontracts), item 2 (theserialize.rs:81cast) and item 5 (theRawPublisher::publish_bytesregression test). This PR addresses only the two header-drift follow-ups recorded in FFI module is never compiled, linted or tested; the "Go FFI tests" CI step silently skips #270's second comment, and it does so with a dedicated Nix job rather than by installing cbindgen intogo-tests.check-python-stubs.cfg.namespace_stays null from in-tree Go callers. What this PR fixes is the allocation size, not missing functionality.git checkout -- crates/hiroz-go/hiroz/hiroz_ffi.hrestores it. This is stated in a comment in the check rather than handled automatically.Relationship to #273
Both PRs edit
.github/workflows/ci.yml. They touch different jobs: #273 changesgo-testsandscripts/test-go.nu; this PR adds a newffi-header-freshjob and does not modifygo-tests. Neither depends on the other.Notes
flake.nixhas twoextraShellHook = ''''occurrences (lines 396 and 511) that anixfmt --checkrun flags. Both are pre-existing onmainand outside this diff; left alone.