fix(topic): validate absolute names and reject empty components - #275
Open
YuanYuYuan wants to merge 3 commits into
Open
fix(topic): validate absolute names and reject empty components#275YuanYuYuan wants to merge 3 commits into
YuanYuYuan wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Validates ROS 2 names earlier to prevent malformed topics from reaching Zenoh.
Changes:
- Adds shared topic-component validation.
- Rejects empty components in topics and namespaces.
- Adds validation tests and updates API documentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/hiroz/src/topic_name.rs |
Implements validation and tests. |
crates/hiroz/src/node.rs |
Updates entity-creation documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
qualify_topic_name validated components on the relative and ~private
branches only. Absolute names -- anything starting with '/' -- were
returned unchecked, so `create_client("/bad name", ..)` was accepted at
construction and failed later inside zenoh's key-expression parser, with
an error citing a dependency path and never naming the topic. Every entity
type routes through this function.
Empty components were also skipped rather than rejected on all three
branches, so `//a//b` passed ROS validation for the same late, opaque
failure.
Both now go through one helper. Note it strips exactly one leading slash:
trimming all of them would turn `//a` into `a` and accept the form this is
meant to reject.
Closes #264
Adversarial review found the headline claim was false. Empty components were rejected on the three *topic* branches but not in validate_namespace, which still skipped them -- and the namespace is concatenated verbatim into the qualified name. So `//ns` validated and produced `//ns/chatter`, the exact form the topic branch rejects, reaching zenoh's key-expression parser with the same opaque error this change exists to prevent. Namespaces are user-supplied, so the path is reachable. Also: the private branch did not strip a trailing slash, so `~/a/` alone was rejected while `/a/` and `a/` were accepted and normalized. No reason for the asymmetry. Docs corrected in the same pass: `qualify_topic_name` and six public rustdoc blocks on the node factories still said absolute names are "used as-is", which this change makes false. Tests added for what review found uncovered: the namespace cases, the trailing slash on all three branches, and the "just slashes" family.
Copilot caught a hole my own trailing-slash fix opened. `~//` strips to `//`, then one leading slash to `/`, then the new strip_suffix to `""` -- which is the "no suffix" case, so validation was skipped and the name silently qualified to `/ns/node`, aliasing `~`. `~` and `~/` legitimately mean the node itself. `~//` does not. Record whether a suffix was present before stripping and reject the case where stripping empties it.
YuanYuYuan
force-pushed
the
fix/topic-name-validation
branch
from
August 14, 2026 18:23
e99805b to
8ecbab8
Compare
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
qualify_topic_namevalidated name components on the relative and~privatebranches only. The absolute branch — anything starting with/— returned the name unchecked. Every entity type routes through this function, so publishers, subscribers, services and actions were all affected.Closes #264.
Defect
Examples use
namespace = "/ns"andnode_name = "node"unless the row shows otherwise.mainmaincreate_client("/bad name", ..)accepted; the name passed through unchangedvalidate_namespaceskipped empty componentsqualify_topic_name("chatter", "//ns", "node")returnedOk("//ns/chatter")a//bresolved to/ns/a//b;~//resolved to/ns/node//~/a/resolved to/ns/node/a/, while/a/resolved to/aanda/to/ns/aD1 is the reported defect; see #264 for the reproduction and the downstream failure it produces. D2 is reachable because namespaces are user-supplied, and hiroz concatenates the namespace verbatim into the qualified name.
//ns/chatteris the exact form the topic branch rejects.What this PR does
is_valid_topic_componentrule the other two branches already appliedvalidate_namespacevalidate_topic_componentscreate_pub,create_sub,create_service,create_client,create_dyn_sub_auto,create_dyn_sub) plus thequalify_topic_namedoc comment, all of which said absolute names are "used as-is"Two subtleties are load-bearing:
trim_start_matches('/')would turn//aintoaand then accept it — the form this exists to reject. Seevalidate_topic_components.~and~/legitimately mean the node itself, so they leave an empty suffix. Stripping the trailing slash of~//also empties it, which would alias it to~. A guard rejects that case before the emptiness test: private-branch guard.Evidence
Eight new tests in
crates/hiroz/src/topic_name.rstake the module from 11 to 19 tests. They assert 37 name forms across the boundary set.absolute_names_reject_invalid_componentsabsolute_names_still_accept_valid_componentsempty_components_are_rejected_on_every_branchnamespaces_reject_empty_componentstrailing_slash_is_normalized_on_every_branchprivate_slash_only_suffixes_are_rejectedslash_only_names_are_rejectedservice_names_reject_invalid_absolute_componentsThe service test exists because
qualify_service_namedelegates toqualify_topic_name, and the issue's reproduction is a client rather than a topic.All 28 GitHub checks are green on
e99805b578ba794ed7b06c8459b8f48478c55900, plus the CLA status.Breaking changes
mainaccepted names that this change rejects at construction, and one resolved name changes./bad name,/1abc,/a-b,/a.b,/a/2b//a,/a//b,///,a//b,~/a//b,~////ns,/ns//sub~/a/now resolves to/ns/node/ainstead of/ns/node/a/Not affected:
/ns,ns,/and the empty string remain valid namespaces;~and~/still qualify to/<ns>/<node>;/a/anda/normalize as before. The topic names/and//, and the namespace/ns/, were already rejected onmain, so they are not part of BC1–BC3.Note
Every form in BC1–BC3 was already an invalid ROS 2 name. It failed later and less legibly, or not at all. BC4 produced a name with a trailing slash, which the issue report describes as producing the same opaque key-expression error. A working peer match on the old form is therefore unlikely.
Coverage this does not have
None of these block the fix. They bound what green CI proves.
crates/,docs/and repo config for name literals the new rules reject. It found no call site that the new rules break; the only matches are the negative tests added hereImportant
The release note for BC1–BC4 is not written yet. The changelog is generated from the commit messages, and every commit on this branch is a plain
fix(topic)with no breaking marker.