fix: propagate node domain/format to built-in services, read ROS_DOMAIN_ID - #336
Merged
Merged
Conversation
…ices Fixes circle/hiroz#143 (D2). TypeDescriptionService::new and ParameterService::new each built their own NodeEntity with domain_id hardcoded to 0 and the default KeyExprFormat, instead of the owning node's actual domain and format. A node on a non-zero ROS_DOMAIN_ID had its built-in services (get_type_description, the six parameter services) always announce domain 0.
Crane review of circle/hiroz#150: ParameterService::new still hardcoded enclave to "" while this PR threaded domain_id and keyexpr_format through ParameterServiceConfig for exactly this reason -- its sibling fix, TypeDescriptionService::new_with_node, already documents avoiding silently resetting a node's domain, namespace, or enclave. A node built with a non-default enclave had it correctly on its own liveliness token and get_type_description, but all six parameter services and /parameter_events still reported enclave "".
The previous run's whippet job showed the real defect: the chosen enclave "/sros2/enclave" contains an internal slash, and the liveliness key expression places enclave as exactly one slash-delimited field (format/rmw_zenoh.rs) with no escaping for that -- a separate, pre-existing wire-format limitation this PR does not touch. Use a single-segment value instead.
format/rmw_zenoh.rs's liveliness encoder hardcodes the enclave segment to the empty placeholder for every entity type -- the decoder's own comment says "Enclave (not supported yet)". No entity's enclave round-trips today, not even a plain ZNode's own liveliness token, so the earlier commit's assertion was testing something the wire format cannot carry regardless of construction site. That's a separate, pre-existing gap, not something this PR touches. The production fix (threading the node's enclave into ParameterServiceConfig instead of hardcoding "") stays: it's still correct and consistent with TypeDescriptionService, and will matter the day the encoder is fixed.
Fixes circle/hiroz#151. ZContextBuilder derived its domain_id default from usize::default(), always 0 regardless of environment -- ROS_DOMAIN_ID was never read anywhere in the crate. rclcpp/rclpy read it at init time and use it unless the caller overrides it in code; a plain hiroz ZContext gave the env var no effect at all, so the normal ROS 2 deployment story (set the variable, don't touch source) silently did nothing. default_domain_id() now reads ROS_DOMAIN_ID, falling back to 0 when unset or unparseable. .with_domain_id() called after default() still overrides it, same precedence as every other ROS 2 client library.
Addresses the non-blocking Crane finding on circle/hiroz#150: the domain-propagation test exercised domain_id but not keyexpr_format, even though ParameterServiceConfig/TypeDescriptionService::new_with_node thread both through identically. A regression that silently re-hardcoded KeyExprFormat::default() for either service would have passed unnoticed. Uses a pass-through custom KeyExprFormatter with its own admin space, so the built-in services are only discoverable at all if they actually used the context's format.
There was a problem hiding this comment.
Pull request overview
Propagates node discovery settings to built-in services and adds ROS_DOMAIN_ID support.
Changes:
- Reuses node domain, enclave, and key-expression format for built-in services.
- Reads the default domain from
ROS_DOMAIN_ID. - Adds graph and environment-handling tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/hiroz/src/context.rs |
Reads and tests ROS_DOMAIN_ID. |
crates/hiroz/src/node.rs |
Passes node settings to built-in services. |
crates/hiroz/src/parameter/service.rs |
Applies propagated discovery settings. |
crates/hiroz/src/dynamic/type_description_service.rs |
Adds node-aware service construction. |
crates/hiroz/tests/graph.rs |
Tests domain and formatter inheritance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two real findings from Copilot's review of #336: - ROS_DOMAIN_ID validity now gates ZContextBuilder::build() with a real error, matching rcl_get_default_domain_id/rcl_init aborting on an invalid domain, instead of silently falling back to 0 with a warning. Modeled as a DomainId enum (Value/Invalid) rather than a usize plus a side-channel error field, so "pending an invalid ROS_DOMAIN_ID" is a state the type carries rather than an invariant call sites (and with_domain_id()) have to remember to check/clear. - The domain-id tests no longer mutate the real ROS_DOMAIN_ID env var at all. DomainId::parse() takes the value as a plain argument, so parsing is a pure unit test with no process-global mutation, no #[serial], and no race against the many other tests elsewhere in this crate that build a ZContextBuilder::default() on their own thread -- eliminating the concurrency hazard Copilot flagged rather than mitigating it. Also: crates/hiroz/tests/graph.rs duplicated message_type_info_derive.rs's TestRouter almost verbatim. Moved to tests/common/mod.rs and both files now share it.
Directly exercises the behavior Copilot's review asked for -- the existing tests only covered DomainId::parse and with_domain_id, not that build() itself turns an unresolved DomainId::Invalid into an Err rather than silently constructing a ZContext on domain 0.
Copilot's two findings were fixed with private unit tests in context.rs's mod tests, which is the wrong place per the workspace's test-folder convention -- new tests belong in tests/, exercising the public API, not private internals. Added ZContext::domain_id() as the public observation point, moved all five tests to tests/domain_id.rs against ZContextBuilder's public surface, and deleted the inline mod tests block. Each tests/*.rs file is its own process, so #[serial] here only guards this file's own tests -- no cross-file or cross-binary race.
CI clippy (--all-targets -D warnings) failed: graph.rs read the endpoint field directly instead of calling the endpoint() method, so within that test binary's own compilation the method was unused -- each tests/*.rs file is a separate crate, so dead-code is judged per binary, not per source file.
This was referenced Sep 1, 2026
0x53A
added a commit
to 0x53A/hiroz
that referenced
this pull request
Sep 1, 2026
Upstream extracted both defects from the draft PR ZettaScaleLabs#331 into ZettaScaleLabs#336 and ZettaScaleLabs#337 and merged them, so every native change on this branch is now superseded. Resolved by taking upstream verbatim everywhere the two sides overlapped: - hiroz-protocol/src/qos.rs: upstream's parser is ours plus a real correction -- an omitted history depth resolves to rmw_zenoh_cpp's wire default RMW_ZENOH_DEFAULT_HISTORY_DEPTH (42), not hiroz's own unrelated QosProfile default of 10. Ours misreported the depth of every peer that relied on the SYSTEM_DEFAULT omission. - parameter/service.rs and node.rs: upstream is a strict superset, additionally propagating the node's enclave, which we had dropped. - tests/graph.rs: upstream's version, with the shared TestRouter in tests/common/mod.rs replacing our inline DomainTestRouter. - Dropped our inline tests in qos.rs and format/rmw_zenoh.rs; upstream adopted the same cases into tests/qos.rs and tests/key_expr.rs. Ours would now fail anyway, having asserted the pre-ZettaScaleLabs#337 depth of 10. - Dropped docs/patches/, an artifact of the draft PR. Two wasm-only fixes were needed on top: - ZContextBuilder's new hand-written Default impl initialises shm_config, which does not exist on wasm32; cfg it out to match the field. - build_async() now has to resolve the DomainId enum ZettaScaleLabs#336 introduced. It rejects an unparseable ROS_DOMAIN_ID exactly as build() does, rather than silently falling back to domain 0 and landing the node on the wrong ROS graph. Verified: cargo test -p hiroz-protocol (51 tests), cargo test -p hiroz --test graph --test domain_id (28 tests), and cargo check -p hiroz --target wasm32-unknown-unknown --no-default-features --features rmw-zenoh,jazzy.
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.
A thorough solution to part of #331 — the domain-hardcoding half of it, plus one layer deeper.
Fixes #335.
#331 reported that built-in services silently assumed domain 0 in a few places. This PR fixes that at its actual source (
TypeDescriptionService/ParameterServiceconstruction), and then goes one step further:ZContextBuilderitself never readROS_DOMAIN_IDat all, so even a correctly-propagated domain was always0unless set in code. Both are fixed together, since the second only matters once the first is true.Before and after
TypeDescriptionService/ParameterServicebuild their ownNodeEntitywithdomain_idhardcoded to0NodeEntity, sodomain_id,keyexpr_formatandenclavematchget_type_descriptionand the six parameter services on domain 0, everything else on the real domainROS_DOMAIN_ID=Nset in the shellZContextBuilder's default domain was always0— the env var was never read anywhere in the crateZContextBuilder::default()readsROS_DOMAIN_ID, matchingrclcpp/rclpy; an explicit.with_domain_id(N)in code still overrides itROS_DOMAIN_IDset to something that isn't a valid non-negative integer.build()returns anErrnaming the invalid value, matchingrcl_init's abort-on-invalid-domain behavior, unless.with_domain_id()overrides it firstModeled the pending state as a
DomainIdenum (Value(usize)/Invalid(String)) internally, rather than ausizeplus a side-channel error flag, sobuild()cannot forget to check it.What the tests do
get_type_description/parameter-service entities reportdomain_id == 0crates/hiroz/tests/graph.rs; asserts all seven built-in service entities and the/parameter_eventspublisher report the node's actual domainkeyexpr_formatcrates/hiroz/tests/graph.rs, using a pass-through customKeyExprFormatterwith its own admin space, so the built-in services are only discoverable at all if they actually used the context's formatROS_DOMAIN_IDhandlingcrates/hiroz/tests/domain_id.rs, covering unset, valid, and invalid env values, and that an explicit.with_domain_id()still wins over an invalid one. Driven entirely throughZContextBuilder's public API and a newZContext::domain_id()getter — no reach into private internalscrates/hiroz/tests/graph.rsandcrates/hiroz/tests/message_type_info_derive.rsalso had their own independent copies of aTestRoutertest helper; consolidated intocrates/hiroz/tests/common/mod.rs, shared by both plusdomain_id.rs.Verified standalone:
cargo test -p hiroz-protocol -p hiroz --lib --tests— all green.cargo clippy --all-targets -- -D warnings— clean.Breaking changes
None.
TypeDescriptionService::newkeeps its existing signature;new_with_nodeis additive.ParameterServiceConfiggained three fields (domain_id,keyexpr_format,enclave), but it ispub(crate)with one caller (ZNodeBuilder::build), which this PR updates.ZContextBuilderswitches from#[derive(Default)]to a manualimpl Defaultto read the env var; behavior for callers who never relied onROS_DOMAIN_IDis unchanged (defaults to0exactly as before, when the env var is unset).ZContextgains one additive public method,domain_id().