Skip to content

fix(ros2-bridge): reject a null sequence, array or nested-message row - #3678

Open
phil-opp wants to merge 2 commits into
mainfrom
claude/laughing-brown-y6wvr6-ros2-null-sequence
Open

phil-opp wants to merge 2 commits into
mainfrom
claude/laughing-brown-y6wvr6-ros2-null-sequence

Conversation

@phil-opp

@phil-opp phil-opp commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Problem

When the ros2-bridge serializes a message:

ListArray::value(0), BinaryArray::value(0) and a struct's child columns all ignore the row's validity bit. So a producer that sends None for such a field, e.g. pyarrow pa.array([None], pa.list_(pa.int32())), had it published as whatever the null row's buffers held. Arrow allows a null list slot whose offsets still span values, so a null int32[] row over [1, 2, 3] went out to ROS2 as [1, 2, 3]. A null Binary row went out as its bytes, and a null nested message went out as its masked child values.

Fix

Every ROS2 message member must have a value. Check once in TypedValue::serialize's per-member loop, before dispatching on the member type: a column whose row 0 is null is rejected with field <Msg>.<field> is null, but ROS2 messages cannot represent null. This covers every member kind, including nested messages and kinds added later, without repeating the check in each column-type arm.

Validation

Class: B (behaviour change: a null row that used to be published is now an error, matching the existing scalar/string/element behaviour)

New tests, all RED without the check and GREEN with it:

Test Field type Column
sequence::tests::null_sequence_row_is_rejected int32[] List, null row over [1,2,3]
sequence::tests::null_binary_sequence_row_is_rejected uint8[] Binary
array::tests::null_fixed_array_row_is_rejected uint8[3] List and Binary
tests::null_nested_message_field_is_rejected nested message Struct

Each test also asserts that the same column without the null still serializes.

  • cargo test -p dora-ros2-bridge-arrow: ✅ 35 passed. cargo clippy -p dora-ros2-bridge-arrow --all-targets -D warnings: ✅. cargo fmt --check: ✅.

🤖 Machine-generated. This PR was written by Claude (Claude Code) during an automated codebase review. The finding was checked by hand against current main, but please review it as you would any external contribution.

🤖 Generated with Claude Code

https://claude.ai/code/session_01R8CSx6yQ4JbrB3dqSiU2ME


Generated by Claude Code

Scalar and string fields reject a null row (`check_not_null`) and
sequence elements reject null elements (#3271), but the row of a
sequence (`T[]`, `T[<=N]`), fixed array (`T[N]`) or nested-message
column was never checked: `ListArray::value(0)`, `BinaryArray::value(0)`
and the struct's child columns ignore the row's validity bit. A producer
sending `None` for such a field had it go out as whatever the null row's
buffers held: an empty sequence, or masked values the producer never
meant to send.

Every ROS2 message member has a value, so check once in
`TypedValue::serialize`'s per-member loop, before dispatching on the
member type, instead of in each column-type arm.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R8CSx6yQ4JbrB3dqSiU2ME
@trunk-io

trunk-io Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

`cargo deny check advisories` fails every PR's Audit job with
`error[yanked]: detected yanked crate` for yoke-derive 0.8.3 (via
url -> idna -> icu). `cargo update -p yoke-derive` moves it to 0.8.4;
no other lock entry changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R8CSx6yQ4JbrB3dqSiU2ME
(cherry picked from commit 2132289)

phil-opp commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

The Audit job fails on every PR right now: yoke-derive 0.8.3, pinned in main's Cargo.lock, was yanked from crates.io. That isn't caused by this PR. I've cherry-picked the lockfile-only fix from #3682 (→ 0.8.4) here as the last commit. It changes nothing once #3682 lands.


Generated by Claude Code

phil-opp commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Fully automated review by Claude Code. No human checked it.

I found one gap in the fix.

A null element inside a sequence or array of nested messages is still published. The new check in serialize/mod.rs (if !column.is_empty() && column.is_null(0)) only covers the field's own column. Elements of an Inner[] / Inner[N] field go through sequence.rs and array.rs instead. Those slice each struct row with array.slice(i, 1) and serialize it as a TypedValue. That checks the row's child columns but never the row's own validity bit.

Example: a Python node sends {"items": [{"x": 1}, None]} for an Inner[] field. pyarrow fills the null struct slot's child with a valid 0. The message then goes out as [{x: 1}, {x: 0}], which is the same invented value this PR rejects at the field level. A probe test against this head returned Ok([2,0,0,0, 1,0,0,0, 0,0,0,0]).

Suggestion: reject input.is_null(0) at the top of TypedValue::serialize, guarded for empty input. That one check covers nested-message elements of sequences and arrays and the top-level message. The per-field check is still needed for list and binary columns. A regression test like the probe would pin this down: a two-element Inner[] list whose second struct row is null.

A compatibility note, not a blocker: with pyarrow, a None sequence field becomes a null row with an empty span. On main that went out as [], and a None nested message went out as an all-zero message rather than the ROS default. After this PR both are errors. 1.0 already rejects None for scalars, strings and elements, so I see this as a consistent bug fix. It is still a visible change for Python nodes that use None to mean "empty", so it deserves a Changelog entry telling users to omit the field to get its default.

Tests: with the new check in mod.rs reverted, all four new tests fail. They pass with it.


Generated by Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants