fix(ci): run doctests and lint the ffi feature - #328
Open
YuanYuYuan wants to merge 2 commits into
Open
Conversation
`cargo test -p hiroz --doc` fails on main: 55 passed, 7 failed, 32 ignored, rc=101. All seven are action doc examples. Three call `.build()` without `use hiroz::Builder` in scope. Four name `ZActionClient`, `GoalHandle` or `goal_state`, which `hiroz::action` does not re-export -- only `ClientGoalHandle` and the server type-state markers. No job in this repository has ever compiled a `///` example. `run-tests` drives `cargo nextest`, which does not execute doctests, and nothing else passes `--doc`. `check-rustdoc-links` runs `cargo doc`, which resolves intra-doc links and never builds an example. That is why seven broken examples sat on a green pipeline. The fix is the missing imports, in the hidden preamble so the rendered examples do not change. The step that would have caught them is added alongside. `generate_json5`'s example becomes `no_run` in the same change. Doctests run with the crate directory as their working directory, so executing it wrote `crates/hiroz/router_config.json5` into the source tree -- harmless while nothing ran doctests, and a file left behind on every CI run once something did. Measured on the worker, which came back with that path untracked.
`cargo clippy -p hiroz --features ffi --all-targets -- -D warnings` fails on main with 23 errors: 22 `missing_safety_doc` across ffi/action.rs (13), ffi/service.rs (6) and ffi/serialize.rs (3), plus one `implicit_slice_from_raw_parts` in hiroz_free_bytes. `clippy-workspace` runs `--all-targets` with default features, and `ffi` is not one, so no job ever compiled crates/hiroz/src/ffi/. `--all-targets` reaches every target of the features it was given; it does not reach a feature nobody named. ffi/context.rs and ffi/graph.rs already carry `# Safety` sections, so the convention existed -- nothing enforced it. Each contract states which constructor produced the pointer, which spans must be valid for reads or writes, and which function frees what. hiroz_free_bytes gets the strictest: passing a pointer this crate did not return, or a mismatched length, is undefined behaviour. hiroz_free_bytes also stops taking a `&mut [u8]` on the way to `Box::from_raw`. That reborrow asserts a unique reference the caller has not promised. `ptr::slice_from_raw_parts_mut` builds the fat pointer without it.
There was a problem hiding this comment.
Pull request overview
Adds CI coverage for doctests and FFI linting while fixing the failures those checks expose.
Changes:
- Adds dedicated doctest and FFI Clippy CI steps.
- Repairs action doctest imports and prevents a file-writing example from executing.
- Documents FFI safety contracts and adjusts raw-slice reconstruction.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ci.yml |
Runs the new CI checks. |
scripts/test-pure-rust.nu |
Defines doctest and FFI Clippy commands. |
crates/hiroz/src/action/client.rs |
Fixes hidden doctest imports. |
crates/hiroz/src/action/server.rs |
Imports the builder trait in a doctest. |
crates/hiroz/src/config.rs |
Marks the file-writing example no_run. |
crates/hiroz/src/ffi/action.rs |
Adds action FFI safety documentation. |
crates/hiroz/src/ffi/service.rs |
Adds service FFI safety documentation. |
crates/hiroz/src/ffi/serialize.rs |
Documents serialization FFI and updates buffer reconstruction. |
Suppressed comments (2)
crates/hiroz/src/ffi/action.rs:350
- These new safety contracts are absent from the checked-in C API header (
crates/hiroz-go/hiroz/hiroz_ffi.h), even thoughbuild.rsdefines that file as cbindgen output and existing FFI safety sections appear there. Please regenerate and commit the header so C/Go consumers—the audience for these contracts—actually receive the updated documentation.
/// # Safety
/// `node` must be a pointer returned by `hiroz_node_create`, or null.
/// Every string argument must be a valid null-terminated C string, or null.
crates/hiroz/src/ffi/action.rs:354
- The execute callback's output allocation is also part of this unsafe contract: every non-null pointer it writes is eventually passed to C
free(including on an error return), and successful non-empty results are read first. State that such pointers must befree-compatible and valid for reads of the reported length; otherwise conforming-looking C callers can cause undefined behavior.
/// `user_data` is passed back to them unchanged and is not dereferenced here.
/// The returned pointer must be freed with `hiroz_action_server_destroy`.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+334
to
+337
| /// `callback` must remain callable for the lifetime of the server; a background | ||
| /// thread invokes it from a thread the caller does not own. `user_data` is passed | ||
| /// back to it unchanged and is not dereferenced here. | ||
| /// The returned pointer must be freed with `hiroz_service_server_destroy`. |
Comment on lines
+351
to
+353
| /// `goal_callback` and `execute_callback` must remain callable for the lifetime of | ||
| /// the server; a background thread invokes them from a thread the caller does not own. | ||
| /// `user_data` is passed back to them unchanged and is not dereferenced here. |
Comment on lines
+239
to
+242
| /// `goal_handle` must be a pointer written by `hiroz_action_client_send_goal`, or null. | ||
| /// `result_data` and `result_len` must each be valid for writes. | ||
| /// On success, the buffer written to `*result_data` must be freed with | ||
| /// `hiroz_free_bytes(*result_data, *result_len)`. |
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
Two checks have never run in this repository. Each one, run for the first time, fails on
main.main(1fe6b3d619a7a92e4455e864318a8039583f4865)cargo test -p hiroz --docrc=101— 7 failed, 55 passed, 32 ignoredrun-testsdrivescargo nextest, which does not execute doctests. No workflow passes--doccargo clippy -p hiroz --features ffi --all-targets -- -D warningsrc=101— 23 errorsclippy-workspaceruns--all-targetswith default features.ffiis not oneBoth failures are old, both are invisible, and a fully green pipeline says nothing about either. This PR fixes the code and adds the step that would have caught it, one commit per pair.
The two gaps are the same shape
--all-targetsreaches every target of the features it was given. It does not reach a feature nobody named.cargo nextestruns every test binary. It does not run doctests, which are not binaries.In both cases the flag reads as exhaustive and is not. That is why neither gap was noticed: the commands look like they already cover the ground.
Commit 1 — doctests
Seven doc examples in
crates/hiroz/src/action/do not compile, in two groups:ZActionClientBuilder,ZActionClient(simple send/receive),ZActionServerBuilder.build()with nouse hiroz::Builderin scopeZActionClient(feedback),ZActionClient(cancellation),send_goal,GoalHandleZActionClient,GoalHandleorgoal_state, none of whichhiroz::actionre-exports — it exportsClientGoalHandleand the server type-state markers onlyThe second group is the more interesting one.
hiroz::action's own docs sayClientGoalHandleexists "to avoid the name collision withserver::GoalHandle", so the glob deliberately does not carry the client types. Four examples assumed it did, and nothing ever told them otherwise.Every fix goes in the hidden
#preamble, so no rendered example changes.generate_json5's example also becomesno_run. Doctests run with the crate directory as their working directory, so executing it writescrates/hiroz/router_config.json5into the source tree. Harmless while nothing ran doctests; a file left behind on every CI run and every developer'scargo testonce something did. Found by reading the worker'sgit statusafter the first green run, not by reasoning about it.check-rustdoc-linksdoes not overlap with the new step:cargo docresolves intra-doc links and never builds an example.Commit 2 — the
ffifeature23 errors: 22
missing_safety_docand oneimplicit_slice_from_raw_parts.missing_safety_docffi/action.rsffi/service.rsffi/serialize.rsffi/context.rsandffi/graph.rsalready carry# Safetysections, so the convention existed — nothing enforced it, and the three files added later never picked it up.Each contract names which constructor produced the pointer, which spans must be valid for reads or writes, and which function frees what.
hiroz_free_bytesgets the strictest, because it is the one a caller can most easily get wrong:hiroz_free_bytesalso stops going throughslice::from_raw_parts_muton its way toBox::from_raw. That reborrow asserts a unique&mut [u8]the caller has not promised;ptr::slice_from_raw_parts_mutbuilds the fat pointer without it.Before and after
cargo test -p hiroz --docrc=101, 7 failedrc=0, 62 passedcargo clippy -p hiroz --features ffi ... -D warningsrc=101, 23 errorsrc=0cargo clippy --all-targets -- -D warningsrc=0rc=0— unchanged///example that stops compilingRun doctestsunsafe extern "C" fnClippy (hiroz, ffi feature)cargo test --docin a clean treecrates/hiroz/router_config.json5no-ros-test/no-ros-checksWhat fails without this
Both directions measured on the same worker, in
.#pureRust-ci, at the head of this branch:run-doctestsclippy-ffirc=0rc=01fe6b3d619a7a92e4455e864318a8039583f4865rc=1—FAILED. 55 passed; 7 failed; 32 ignoredrc=1— 22missing a '# Safety' sectionThe revert is per-half:
crates/hiroz/src/action/alone for the first column,crates/hiroz/src/ffi/alone for the second. Each gate was shown to go red for its own defect and no other.cargo clippy --all-targets -- -D warningsstayedrc=0throughout, so neither commit regresses the existing lint surface.Cost
Two extra steps on jobs that already build the crate, so both reuse a warm
target/. The doctest run itself took 14 s on the worker; thefficlippy run is a single-crate lint.Out of scope
Cargo.lockis stale onmain:protobuf_demois still pinned at0.1.0after the0.2.0release, so any cargo invocation rewrites one line and dirties the tree. Observed onmainbefore this branch existed, on three separate jobs. Not touched here — it belongs in its own commit, and now has one: #330.Breaking changes
None. No public signature changes, no rendered doc example changes, no behaviour changes. The
# Safetysections state the contract that C callers were already required to honour; they do not narrow it.