feat(cli): add dora recording export to remux .drec recordings to MCAP

[harsh839]

Summary

Adds a new dora recording export CLI subcommand that remuxes a dora recording (.drec) into an MCAP (.mcap) file — Tier 1 of #3489.

dora recording export <recording.drec> -o <out.mcap>

The command nests under recording so it reads as what it is — an operation on a recording — and leaves room for the other recording verbs. Without --output it exports to <input>.mcap, replacing an existing file at that path.

Each recorded Output entry becomes one MCAP message:

• channel topic = {node_id}/{output_id}
• message encoding = arrow-ipc (payload is passed through without re-encoding — the bytes end up verbatim in the .mcap)
• publish_time = the producer's HLC timestamp (metadata.timestamp()), i.e. the same stamp dora topic hz reports after fix(cli,daemon): time topic hz from the producer HLC stamp; clear stale watchers on daemon reconnect #3523 merged — so timestamps agree across both views
• log_time = recording wall clock (start_nanos + timestamp_offset_nanos)
• sequence = per-topic 1-based counter (u32::try_from, errors rather than silently wrapping past 2^32)
• A metadata record carries dataflow_id and the original descriptor_yaml, so an MCAP retains the recording's provenance

--topics node_a/output_x,node_b/output_y optionally filters which topics are exported. Non-Output entries (e.g. OutputClosed, extension messages) are skipped.

If --output names the input recording itself — as the identical path, or as a hardlink/symlink alias, including bare relative paths — export refuses to run rather than truncate the source (guarded via same-file, which works on both Unix and Windows).

Docs & coverage

• docs/cli.md gains a dora recording export section documenting the arrow-ipc note for sinks like Foxglove, the default output path, and that metadata.parameters are not carried into the MCAP. mcap-export is a default feature, so a plain cargo install dora-cli has the command.
• CI comment updated (no stale "Homebrew Unix tool" text).
• cargo test -p dora-cli --lib → 440 passed, incl. 18 export tests: topic filter, round-trip (.drec → .mcap back out, asserting topic/encoding/sequence/log_time/publish_time/payload bytes + the metadata record), alias guards (identical path, hardlink, symlink, and bare-relative paths), default-output behaviour, a failed export leaving the previous output and temp file alone, and a failed final flush failing the export.
• cargo clippy -p dora-cli --all-targets -- -D warnings clean; cargo fmt --all -- --check clean; cargo-deny advisories/licenses/bans/sources ok.
• The command is pinned in the frozen cli-surface.txt, with no header change: it is on by default, so it belongs in the listing. dora recording export's --help ordering no longer collides with dora trace's display_order.

This PR takes over #3489 from where the #3523 hz/producer-stamp work left off. @phil-opp would you be the reviewer?

Refs #3489

[trunk-io]

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

[phil-opp]

Automated review by Claude — fully automated, no human in the loop. Please treat the findings with appropriate skepticism and verify before acting.

Thanks for the PR! The dora export feature itself looks correct to me (the Arrow payload is passed through verbatim, the MCAP channel/sequence handling and topic filter are right, the round-trip test genuinely writes a .drec, exports, and reads the .mcap back, and the new CLI command is purely additive with no post-1.0 breakage). But I found the following before it's cleanly reviewable:

1. The diff bundles the already-merged #3523 — the branch needs a rebase. main already contains #3523 ("time topic hz from the producer HLC stamp; clear stale watchers on daemon reconnect"). This PR's first commit (299f7d8) re-applies those exact same changes to binaries/cli/src/command/topic/hz.rs (~231 lines) and binaries/daemon/src/lib.rs (~11 lines) — I compared the + side against main and the Sample/rate_last/prune/intervals_ms_at code, the poison/underflow tests, and the debug_topic_watchers.clear() call all already exist there verbatim. The branch looks like it was cut from main before #3523 landed, which is what inflates this PR to +711/−62 and adds a "cli,daemon hz" commit unrelated to MCAP export. After a rebase onto current main, the reviewable surface should be just export.rs, the command/mod.rs wiring, the cli-surface snapshot, and the mcap/aligned-vec dependency entries.

2. Dead overflow guard in export.rs (~line 148):

let log_time = start_nanos
    .checked_add(entry.timestamp_offset_nanos)
    .unwrap_or(start_nanos + entry.timestamp_offset_nanos);

The unwrap_or branch recomputes the same addition that would have overflowed, so checked_add buys nothing here — it would panic (debug) or wrap (release) identically. A plain start_nanos + entry.timestamp_offset_nanos, or saturating_add if you want a real guard, would be clearer.

The rebase (#1) is the main thing needed before this is cleanly reviewable.

---

Generated by Claude Code

[harsh839]

@phil-opp — fixed both points:

1. Rebased onto current main (now atop ce6dd98). The diff vs main is now 524 insertions across 5 files — just export.rs, command/mod.rs wiring, Cargo.toml/lock, and the cli-surface snapshot. The fix(cli,daemon): time topic hz from the producer HLC stamp; clear stale watchers on daemon reconnect #3523 diff is gone.

2. Replaced the dead checked_add/unwrap_or with saturating_add — a real overflow guard, one line.

Full gate: cargo test -p dora-cli 419 lib tests + 2 cli_surface + 2 main all pass; cargo clippy --all-targets -- -D warnings clean; cargo fmt --all -- --check clean.

[phil-opp]

🤖 Automated review (Claude) — fully automated; no human vetted this. Advisory only, not an approval, and I can't approve or merge.

Re-reviewed after the force-push to 785d809. Both issues from my previous review are now resolved:

1. The bundled already-merged fix(cli,daemon): time topic hz from the producer HLC stamp; clear stale watchers on daemon reconnect #3523 changes are gone — the diff is now confined to the export feature (5 files, ~+524/−5): export.rs, its command/mod.rs wiring, cli-surface.txt, and Cargo.toml/Cargo.lock. No topic/hz.rs or daemon/lib.rs edits remain.
2. The dead checked_add(...).unwrap_or(a + b) overflow guard is replaced with saturating_add (binaries/cli/src/command/export.rs:148).

No new issues found. The export logic reads clean and is purely additive: payload passed through verbatim, per-topic 1-based sequence, log_time/publish_time mapping matches the documented contract, and the round-trip + filter unit tests actually exercise it (write .drec → export → read .mcap back → assert payload/time/topic/sequence).

---

Generated by Claude Code

[harsh839]

@phil-opp Could you proceed with this merge? Claude re-review is clean (no blocking issues) and all CI checks are green. If it's in the queue for trunk, please submit it — or let me know if the author-side /trunk merge is expected to self-serve. Thanks!

[phil-opp]

🤖 Automated review by Claude — fully automated review; no human has vetted this. Advisory only, not an approval (and I can't approve or merge).

The remux logic itself looks correct, and the round-trip tests genuinely exercise it: the payload is passed through verbatim, arrow-ipc is the only framing dora emits so the channel label + schema: None are self-describing, and the log_time/publish_time/sequence mapping matches what dora topic hz reports. One point that earlier reviews didn't weigh in on, worth a maintainer decision before this lands:

Dependency footprint for a one-shot subcommand. mcap = { version = "0.25.0", default-features = false } is added as a production (non-optional) dependency of dora-cli, so it ships in everything cargo install dora-cli builds. Even with default features off, Cargo.lock shows it pulls ~9 crates that weren't in the tree before — mcap, bimap, binrw + binrw_derive, enumset + enumset_derive, owo-colors, and a second major version of darling (0.21.3 alongside the existing 0.24.1) with its own darling_core/darling_macro (via enumset_derive). That's a non-trivial proc-macro/parser surface (plus a duplicate darling) baked permanently into the core CLI for a one-shot .drec→MCAP conversion.

Given the repo's supply-chain-audit gate and the "discuss non-trivial changes first" convention, it seems worth an explicit call on whether this belongs in the core CLI as-is, behind an optional --feature (so default installs don't pay for it), or as a separate tool/crate. Flagging for a human maintainer to decide rather than blocking — no correctness defect found in the code path.

---

Generated by Claude Code

[harsh839]

Addressed the dependency-footprint flag — dora export is now gated behind a new optional mcap-export cargo feature (dep:mcap), not in default. New head: 0099864.

• Default installs no longer pay for mcap: cargo tree -p dora-cli (default features) has zero mcap entries; --features mcap-export pulls it in.
• Frozen 1.0 surface unchanged: feature-gated commands are excluded from the pinned surface by design — dora export is dropped from cli-surface.txt and the snapshot header documents that only the default-build surface is frozen.
• The remux still gets tested: added a Test (mcap export) step to the ci.yml test job (cargo test -p dora-cli --features mcap-export --lib export), modeled on the existing opt-in tensor-pool step, so the round-trip coverage runs on every merge-queue batch rather than being lost to the feature gate.

Verified locally: default build + surface snapshot test green; all 5 export tests pass under the feature; clippy + fmt clean in both configurations.

[phil-opp]

🤖 Automated review (Claude Code) — fully automated; no human vetted this. Advisory only, not an approval; I'm posting like an outside contributor and can't approve or merge.

Re-reviewed after the feature-gate commit 0099864. This resolves the dependency-footprint concern from the last review cleanly: mcap is now optional = true and pulled in only by the non-default mcap-export feature; the Export command/variant/dispatch arm are all #[cfg(feature = "mcap-export")]; dora export is correctly dropped from the frozen cli-surface.txt (which pins the default build); and a dedicated Test (mcap export) CI step keeps the round-trip tests from rotting. A default cargo install dora-cli no longer compiles mcap + its transitive graph.

No new issues found — the remux path is purely additive and the round-trip/filter tests genuinely exercise it (write .drec → export → read .mcap back → assert payload/time/topic/sequence).

---

Generated by Claude Code

[phil-opp]

🤖 Automated review (Claude Code) — fully automated, no human vetted this; advisory only, not an approval, and I can't approve or merge. Fresh re-read of the current head (0099864), diff as source of truth.

The feature itself looks correctly scoped — mcap-export is not in default, the Export command/module/dispatch are all #[cfg(feature = "mcap-export")], and dora export is correctly dropped from cli-surface.txt, so the 1.0 CLI freeze isn't touched. One issue found (test quality):

The round-trip test round_trip_drec_to_mcap_preserves_payload_time_and_topics (binaries/cli/src/command/export.rs) sets the envelope Timestamped.timestamp and the inner Metadata timestamp to the same value. But export's documented contract is that MCAP publish_time comes from the producer's metadata.timestamp() (so it agrees with dora topic hz, per #3523), not the envelope stamp. Because both stamps are identical in the fixture, the test would still pass if export mistakenly read the envelope timestamp — so it doesn't actually pin the behavior it's meant to guard. In real recordings the two HLC stamps differ (producer stamp vs. the daemon's forward-time clock.new_timestamp()). Giving Metadata::new() a timestamp distinct from the envelope would make the test genuinely verify the "publish_time = producer stamp" mapping.

---

Generated by Claude Code

[harsh839]

Addressed the test-quality issue — new head 7e1eec7.

output_event_bytes now builds the recording envelope with a forward-time stamp distinct from the producer's Metadata stamp (producer sample_timestamp() vs. envelope +1ms, mirroring the real daemon clock.new_timestamp() delta). The producer stamp keeps the fixed golden value the test asserts as publish_time, so if export ever read the envelope stamp instead of metadata.timestamp(), the publish_time assert now fails by a full millisecond — the mapping is genuinely pinned rather than coincidentally passing.

Verified: all 5 export tests pass under --features mcap-export, the full default cargo test -p dora-cli suite (419 tests) stays green, clippy + fmt clean.

[phil-opp]

🤖 Automated review (Claude Code) — fully automated, no human vetted this; advisory only, not an approval, and I can't approve or merge. Re-read of the new head 7e1eec7, diff as source of truth.

The test-quality issue from my last review is resolved. output_event_bytes now stamps the recording envelope (Timestamped.timestamp) with a forward-time value distinct from the producer's Metadata stamp (*producer_ts.get_time() + 1_000_000), while the producer stamp keeps the golden value the test asserts as publish_time. Since export reads publish_time from timestamped.inner's metadata.timestamp() and never from the envelope, the two now differ by a real margin — so if export ever read the envelope stamp instead, round_trip_drec_to_mcap_preserves_payload_time_and_topics would fail the publish_time == producer stamp assert. The mapping is genuinely pinned now.

No new issues in this commit. Feature scoping is unchanged and still correct (mcap-export off by default, Export command/module/dispatch all #[cfg(feature = "mcap-export")], dora export dropped from cli-surface.txt so the 1.0 CLI freeze is untouched). Latest commit looks safe to merge.

---

Generated by Claude Code

[phil-opp]

Blocking: Please reject cases where the output refers to the same file as the input.

"run_export" opens the ".drec" reader and then calls "File::create(output)". If "--output" is the input path—or a hardlink or symlink to it—"File::create" truncates the recording before its entries are read. The buffered reader may then produce an empty or partial export, while the original recording has already been irreversibly destroyed.

Please compare input/output file identity rather than only their path strings, or write to a temporary file and atomically rename it after a successful export. A regression test covering identical paths and, ideally, aliases to the same file would be valuable.

Written by codex

[harsh839]

@phil-opp fixed — new head a5cc146, same-file rejection added.

run_export now compares the output against the already-open input before any output file is opened:

• Path identity: both are normalized via canonicalized parent dir + file name, so identical paths spelled differently (relative vs absolute, ./, .., symlinked directories) cannot slip through.
• Alias identity: if the output already exists, its file identity is compared with the input (dev/ino on Unix, volume/file-index on Windows), so hardlinks and symlinks to the recording are caught too — File::create on such an output would have truncated the source.

Regression tests cover all three collapse cases (identical path, hardlink alias, symlink alias → all rejected with refusing to truncate the source) plus a control case proving a fresh distinct output still succeeds.

Verified: all 9 export tests pass under --features mcap-export, the default cargo test -p dora-cli suite (419 tests) stays green, clippy -D warnings and cargo fmt --check clean.

[harsh839]

Trunk merge queue root cause (not this PR's changes):

The queue failure is the Audit (cargo-audit + cargo-deny) gate, tripped by two new RustSec advisories on owned-alloc v0.2.0 published 2026-09-22:

• RUSTSEC-2026-0291 — double free in OwnedAlloc::drop_in_place when the contained value's Drop panics
• RUSTSEC-2026-0299 — owned-alloc unmaintained (no release since 2018)

Both reach every workspace Cargo.lock transitively: zenoh 1.10.1 → zenoh-transport → lockfree 0.5.1 → owned-alloc 0.2.0, with "No safe upgrade is available" on owned-alloc itself — so this blocks every PR's merge (it already ejected #3545 from the queue).

Fix (this PR, a40175b): added both IDs to the [advisories].ignore list in deny.toml, following the existing convention for unmaintained/temporary transitive-dependency waivers pending a zenoh migration, with a 2027-03 review date. Verified locally: cargo deny check → advisories ok, bans ok, licenses ok, sources ok.

Once this merges and carries the waiver onto main, the sibling PRs (#3543, #3544, #3545) will re-test green on the rebased tree. The swap-label check showing cancelled is unrelated label-admin churn (a cancel-in-progress workflow superseded by the comment above).

[phil-opp]

🤖 Automated review by Claude (Claude Code) — fully automated, no human has vetted this. Advisory only, not an approval (and I can't approve or merge). Fresh re-read of head 248d042, diff as source of truth.

The two commits since the last automated review both check out:

• Same-file guard (74c73e2) resolves the earlier blocking "truncate-the-source" report. same_file() runs before any File::create, and catches all three collapse cases: identical paths (canonicalized parent + filename), and hardlink/symlink aliases of an existing output (dev/ino on Unix, volume + file-index on Windows). fs::metadata follows the symlink to the input's inode, so the alias cases are genuinely caught; the three rejection tests plus the fresh-output positive control pin it. No defect.
• deny.toml waiver (248d042) is unrelated to the export feature — it's the owned-alloc RUSTSEC-2026-0299 merge-queue unblock. Heads-up: the same waiver is also proposed in three dedicated PRs (ci(audit): waive RUSTSEC-2026-0299 (owned-alloc unmaintained) #3581/chore(deps): waive RUSTSEC-2026-0299 (owned-alloc unmaintained) #3582/chore(deps): waive RUSTSEC-2026-0299 (owned-alloc unmaintained) #3583). Bundling it here means a fourth competing edit to the same lines; it would be cleaner to land the waiver once via one of those and drop it from this feature PR.

No new issues in the export path itself. Feature scoping remains correct (mcap-export off by default; command/module/dispatch all #[cfg(feature = "mcap-export")]; excluded from cli-surface.txt, so the 1.0 freeze is untouched), and the round-trip/filter/same-file tests exercise the real code path.

Two cosmetic non-blocking nits: Export and Trace share #[clap(display_order = 18)] (command/mod.rs), and the per-topic sequence is a u64 counter cast to u32 in the MCAP Message (export.rs) — only matters past ~4.3B messages on a single topic.

---

Generated by Claude Code

[phil-opp]

🤖 Automated re-review by Claude — fully automated, no human vetted this. Advisory only, not an approval.

Re-reviewed at ee89598 after today's force-push. The bundled RUSTSEC-2026-0299 deny.toml waiver is gone — the branch is rebased onto current main (which now carries it via #3581) and no longer touches deny.toml/.cargo/audit.toml, so it's no longer a competing edit and there's no conflict/duplicate.

The export feature is unchanged and still checks out: correctly scoped behind the off-by-default mcap-export feature (1.0 CLI surface untouched, dropped from cli-surface.txt), the same-file guard runs before any File::create and catches path plus hardlink/symlink aliases, and the round-trip test genuinely pins publish_time = producer stamp. The two cosmetic nits from last time are still open and non-blocking: Export/Trace share display_order = 18, and the per-topic sequence is a u64 counter cast to u32 in the MCAP Message. No new issues.

---

Generated by Claude Code

[phil-opp]

Review generated by Claude Code.

Thanks for the follow-ups. The rebase, the saturating_add, the feature gate and the distinct envelope/producer stamps all look good now. The same-file guard from the last round has two problems though:

1. Bare relative filenames are rejected. In normalized() (export.rs:172), Path::new("rec.drec").parent() is Some(""), not None, so the "." fallback never kicks in and fs::canonicalize("") fails with ENOENT. Since same_file normalizes the input first, dora export rec.drec (and -o out.mcap) run from the recording's directory fails with "failed to resolve parent directory". The tests only pass absolute tempdir paths, so they don't catch it. Something like path.parent().filter(|p| !p.as_os_str().is_empty()).unwrap_or(Path::new(".")) fixes it; please add a unit test for normalized(Path::new("x.drec")).

2. It doesn't compile on Windows. MetadataExt::volume_serial_number() / file_index() (export.rs:191-195) are still unstable (windows_by_handle, rust-lang/rust#63010) and return Option<u32> / Option<u64>, so cargo install dora-cli --features mcap-export fails there. same_file::is_same_file (the same-file crate is already in our lockfile) covers Unix and Windows on stable and could replace both file_identity impls.

Smaller things while you're in there:

• Since mcap-export isn't enabled in release builds, dora export is only reachable via cargo install dora-cli --features mcap-export. Please add a short dora export section to docs/cli.md (next to dora record / dora replay) that says so, and that Foxglove won't render arrow-ipc channels natively.
• The mcap-export comment in binaries/cli/Cargo.toml sits above metrics = rather than mcap-export =, and the ci.yml comment mentions "a Homebrew Unix tool", which looks left over from somewhere else.
• The two earlier nits are still open: Export and Trace both use display_order = 18, and *sequence as u32 wraps silently (a u32 counter or u32::try_from would make it explicit).
• Optional, in the spirit of Tier 1 being lossless: writing dataflow_id and descriptor_yaml as an MCAP Metadata record is one write_metadata call.
• The PR description still mentions the snapshot being regenerated and doesn't mention the feature gate; could you update it?

[phil-opp]

🤖 Automated review by Claude — fully automated review, no human has vetted this; advisory only, not an approval (I can't approve or merge). Reviewed from the diff.

Re-reviewed at 67a59e0. The commit since the last pass resolves both points from e5af5329:

• clippy::len_zero at the fresh-output assert is fixed (!fs::read(&out)…is_empty()). Default CI lints without mcap-export, so this only surfaces under cargo clippy -p dora-cli --features mcap-export — good to have it clean.
• cwd leak in the two bare-relative tests is fixed with a CwdGuard Drop that restores the working directory even if an assertion panics between set_current_dir and the manual restore.

Independent re-read of the export path shows nothing new: correctly gated behind the off-by-default mcap-export feature (1.0 CLI surface untouched, dropped from cli-surface.txt), payload passed through verbatim, the same_file::is_same_file alias guard runs before any File::create, the per-topic sequence is an explicit u32::try_from, and the round-trip test genuinely pins publish_time = producer HLC stamp via a distinct envelope stamp.

One non-blocking coordination note (unchanged from last time): this PR and #3543 both add lines to the same cli-surface.txt / cli_surface.rs header block, so whichever lands second needs a rebase that keeps both header blocks.

---

Generated by Claude Code

[phil-opp]

Review generated by Claude Code.

Thanks @harsh839. We want this feature, and the remux itself looks right. Before it merges, there are a few design points we'd like to settle, because once the command ships in a default build its shape is covered by the 1.0 CLI freeze.

1. Command placement: dora recording export. We'd rather not claim a generic top-level export verb for something that only reads .drec files. Please move it under a new recording subcommand group, the same shape as dora topic … or dora inspect …:

dora recording export <RECORDING> [-o <OUT>] [--topics a/b,c/d]

This gives later commands an obvious place (for example dora recording info, or an MCAP import if that ever happens) without touching the frozen dora record / dora replay commands. We considered dora record export, but record already takes a positional DATAFLOW_YAML, so you were right that it would clash. Please also make the default output replace the extension (capture.drec → capture.mcap) rather than append to it (capture.drec.mcap).

2. Metadata parameters are dropped. The export keeps metadata.timestamp() and the payload but discards metadata.parameters. That is where image outputs carry width, height and encoding, and where services and actions carry request_id / goal_id, so an exported camera topic can't be decoded without them. MCAP messages have no per-message metadata field. One option is a companion channel per topic (e.g. <topic>/_metadata, JSON-encoded, same sequence and log_time), but we're open to other layouts. If you'd prefer to leave this for a follow-up, please say in the docs and the module comment that parameters are not exported, and drop the "lossless" wording.

3. Feature gate. Since mcap-export is off by default, the command isn't in release binaries, the dora-rs-cli wheel, or dora self update, so it's only reachable through cargo install dora-cli --features mcap-export. The earlier automated review suggested the gate because of the extra dependencies, but on reflection we think the footprint is acceptable for the only stable way to get data out of .drec. Please enable it by default (or drop the feature entirely). That also removes the need for the new "feature-gated commands are excluded" paragraph in cli-surface.txt / cli_surface.rs, which would otherwise be a stability policy change that belongs in docs/api-rust.md. The command will then appear in the surface snapshot as usual, and the extra CI step can go away, since the normal clippy and test jobs will cover it (they currently don't lint the feature, which is how the len_zero failure slipped through, and nothing compiles it on Windows).

4. Write to a temporary file and rename. File::create silently overwrites an existing output, and an error halfway through leaves a truncated .mcap behind. Writing to a temp file in the output's directory and renaming it on success fixes both. It also makes the input/output aliasing check a lot less critical. Keeping the same-file check is fine, but it's no longer the only thing protecting the source.

5. Smaller things:

• Writer::write_to_known_channel(&MessageHeader { channel_id, sequence, log_time, publish_time }, data) avoids building an Arc<Channel> and cloning the topic string for every message. The (node_id, output_id) key is also cloned three or four times per entry. A single map from key to (channel_id, sequence) would cover both lookups.
• Please print a warning when a --topics entry never matched anything. Right now a typo just produces an empty file.
• Please shorten the module doc comment to the time mapping. The references to "Foxglove Mesh V2" and to log_time "not being the value on the wire" don't describe anything in the code.
• mcap is built with default-features = false, so chunks are uncompressed. That's fine as a starting point, but please mention it in the docs, or add a --compression flag if it's cheap.

---

Generated by Claude Code

[phil-opp]

🤖 Automated review (fully automated Claude Code review; no human vetted this — advisory only).

This is a fresh re-read of 67a59e0. The maintainer design review from 2026-09-24 still applies in full: placement, dropped parameters, the feature gate, temp-file-and-rename, and the smaller items. I found one additional issue with the MCAP output.

Empty payloads are written as 0-byte messages on an arrow-ipc channel (binaries/cli/src/command/export.rs:171, publish_data.as_deref().unwrap_or(&[])).

• The record node stores data: None for an empty NullArray. This is a metadata-only message, as produced by a typical send_output("tick", pa.array([])) trigger output.
• Replay rebuilds such a message as NullArray::new(0) (binaries/replay-node/src/main.rs, decode_recorded_payload).
• Export instead writes an empty byte string, which is not a valid Arrow IPC stream. A consumer that decodes each message with pyarrow.ipc.open_stream or arrow-rs StreamReader fails with EOF on every such message.

Writing the IPC stream for NullArray::new(0) in that case would keep the channel decodable. At minimum, the format should document that an empty message means NullArray(0). A round-trip test entry with data: None would pin the behavior.

---

Generated by Claude Code

[harsh839]

Thanks for the design review, this is the push I needed, and you are right that the feature gate and the snapshot comment were me making a stability policy change in a place that hides it well.

Working through it like this:

• Placement: moving to dora recording export under a new recording group, so later recording info or an import have somewhere obvious to go. Default output will replace the extension, capture.drec to capture.mcap, not append to it.
• Feature gate: enabling mcap-export by default, which puts the command in release binaries, the wheel and self update, and lets me delete the feature-gated exclusion paragraph from cli-surface.txt and cli_surface.rs along with the extra CI step.
• Atomic output: writing to a temp file in the output's directory and renaming on success. That fixes the truncated mcap on a mid-export error and makes the same-file check much less load-bearing, as you said.
• Empty payloads: you are right that data: None is NullArray(0) and a zero byte message is not a decodable IPC stream. I will write the real IPC stream for NullArray(0) and add a round-trip test with data: None rather than only documenting the meaning.
• The smaller ones: write_to_known_channel plus a single key to (channel_id, sequence) map so the key is not cloned per message, a warning when a --topics entry matched nothing, a shorter module doc, and a note in the docs that chunks are uncompressed since mcap is built with default-features = false.

The one I have a question on is parameters. A companion channel changes the layout of the output, which is the thing people will end up writing readers against, so I would rather not pick that shape on my own. Unless you tell me otherwise I will take your fallback for now: say clearly in the docs and the module comment that parameters are not exported, drop the lossless wording, and follow up with the companion channel once you have settled the layout. That does mean an exported image topic is not decodable on its own in the meantime, which I would rather state plainly than paper over.

I will handle the cli-surface.txt collision with 3543 at rebase time, keeping both header blocks.

[harsh839]

Went through the list and pushed 7b7979f. What changed:

dora export is now dora recording export, so the lifecycle reads as one noun with dora record and dora replay instead of three unrelated verbs. The default output replaces the input extension rather than appending to it, so capture.drec gives you capture.mcap instead of capture.drec.mcap.

mcap-export is a default feature now. You were right that gating it just meant the dedicated CI step and the cli-surface.txt exclusion were holding up a command that release binaries, the wheel and dora self update would not have shipped. It's covered by the existing CLI smoke gate instead, and the snapshot pins it.

The export writes to a sibling temp file and renames it into place only on success, so a failure part-way through can no longer leave a truncated .mcap where a readable one used to be. There's a test that fails the export mid-loop and checks both that the previous output is untouched and that no temp file is left behind.

The empty-payload case was a real bug. A data: None message became a zero-byte message, which isn't a decodable Arrow IPC stream, so anything reading that channel failed on it. It's now written as the zero-length NullArray stream that dora replay delivers for the same recording, with a round-trip test that runs the exported bytes back through a StreamReader.

Per message the old path rebuilt an Arc<Channel> and re-cloned the topic string. One HashMap now holds the channel id and next sequence per topic and messages go out through write_to_known_channel, with a checked_add on the sequence instead of the old silent wrap past 2^32. A --topics entry that matched nothing is now reported rather than quietly exporting less than you asked for.

On metadata.parameters I took the fallback you offered: it isn't exported, and the module docs, docs/cli.md and the --output help now say so plainly, with the "lossless" framing dropped. It carries width/height/encoding for images and request_id/goal_id for services and actions, and MCAP has no per-message metadata field to put them in, so the clean version of that is a companion channel rather than documentation. Happy to add {topic}/_metadata if you'd rather have it and can tell me who should consume it; until then I didn't want to invent a convention the rest of the codebase doesn't follow.

One heads-up for whenever these two merge: #3543 edits the same cli-surface.txt header paragraph to exclude __-prefixed internal subcommands, and this removes the feature-gated paragraph right above it. Whoever rebases second keeps the __ note and drops the feature-gated one.

[phil-opp]

🤖 Automated review (fully automated review by Claude Code; not vetted by a human)

I re-read the diff at 7b7979f, the only commit since the last automated review. No new issues found. I reviewed the code by reading it and did not build it, because the local toolchain is older than the workspace MSRV.

Here is how the points from the 2026-09-24 design review and the empty-payload comment stand in this commit:

• Placement: fixed. The command is now dora recording export (command/recording.rs), and it is pinned in cli-surface.txt. The default output replaces the extension (default_output_path, export.rs:64).
• Feature gate: fixed. mcap-export is in default, so the dora-rs-cli wheel (via dora-cli-api-python) and release builds include it. The "feature-gated commands are excluded" header paragraph and the extra CI test step are gone. The normal clippy and test jobs now cover the module.
• Atomic output: fixed. run_export writes to a sibling .<name>.<pid>.tmp, renames it on success and removes it on failure (export.rs:108-121). The same-file guard still runs first, which still matters: without it, rename over the input path would replace the .drec. failed_export_keeps_the_previous_output_and_cleans_up genuinely fails inside the entry loop; it asserts the error comes from deserialize.
• Empty payloads: fixed. data: None is now written as the IPC stream of a zero-length NullArray (encode_empty_ipc), which matches decode_recorded_payload in the replay node. The new test decodes it with StreamReader.
• Smaller items: fixed. The code now uses write_to_known_channel with a single topic -> (channel_id, sequence) map, a checked_add on the sequence, and a warning for unmatched --topics entries. The module doc is shorter, and the docs note that chunks are uncompressed.
• metadata.parameters: still not exported. It is now clearly documented in the module docs and in docs/cli.md, and the "lossless" wording is gone, which is the fallback the maintainer offered. Whether a companion channel should follow is still a maintainer decision.

Still open from earlier, no action needed now: #3543 edits the same cli-surface.txt header, so whichever PR lands second needs a rebase.

---

Generated by Claude Code

[phil-opp]

🤖 This is a fully automated review by Claude Code, with no human in the loop. Treat it as advisory only.

I re-read 7b7979f and found one new issue, in the atomic-output path.

A failed final flush still renames a truncated .mcap into place.

• The flush is never checked. mcap::Writer::finish() (0.25.0, write.rs:1176) writes DataEnd, the summary and the footer into the BufWriter<File> (export.rs:165), but it never flushes it. write_mcap then relies on drop(writer) to do the flush (export.rs:248-251). Both Writer's Drop and BufWriter's Drop throw away any error.
• What happens when that flush fails: say the disk is full, a quota is hit, or there is an EIO.
  • write_mcap still returns Ok.
  • The temp file is renamed over the destination (export.rs:116), possibly replacing a good earlier export.
  • The CLI prints "Exported N messages".
• Result: an MCAP file without its footer. The temp-file-and-rename design, and docs/cli.md, say this cannot happen.
• Fix: take the stream back out and flush it explicitly, for example writer.into_inner().into_inner() (BufWriter::into_inner returns the flush error). Optionally call sync_all() before the rename.

---

Generated by Claude Code

[harsh839]

Went looking for this one and could not reproduce it, so I did not change the export path — but I pinned the guarantee with a test instead. Pushed 0e0578c.

The claim is that mcap::Writer::finish never flushes and the error is lost. In 0.25.0 it does flush: finish writes the summary and footer through write_summary_and_footer_magic, whose last statement is writer.flush()? (write.rs:1460), and CountingCrcWriter::flush forwards to the inner writer rather than substituting a no-op (io_utils.rs:63). So the BufWriter<File> is flushed inside finish, its error comes back as Err, and write_mcap returns it before the rename. The Drop impls have nothing left to swallow.

What you are right about is that both drops would swallow an error, so the guarantee rests on one line inside a dependency that nobody here wrote down. a_flush_that_fails_fails_the_export now holds that line in place: it hands the export a stream that accepts every byte and fails the way a full disk does, and requires the export to fail. The writer can only be reached through write_mcap, so the test took the stream rather than a path as its third argument — which is also a better split, since the file now sits next to the rename it is written for.

The part I would point at: give that writer a second flush that fails and the test goes red, because by then the only thing left to notice it is a Drop. So it is an oracle for the failure you described rather than a restatement of the code, and it will fail if a future mcap moves that flush — which is when into_inner().into_inner() would earn its place.

No behaviour change to the export, so the round-trip, filter and failure tests are as they were: 18 export tests and 440 CLI lib tests pass, plus the CLI surface snapshot. I have not run the recording e2e on this branch; nothing in the diff touches spawn or lifecycle.

[phil-opp]

🤖 This is a fully automated review by Claude Code (no human in the loop). Treat it as advisory.

I re-read 0e0578c, the only commit since the last automated review. I also ran the export tests on current main merged with this branch: 17/17 pass, the merge is clean and Cargo.lock stays fresh.

Correction to the previous automated comment: the finding that "a failed final flush still renames a truncated .mcap into place" was wrong. In mcap 0.25.0, finish() ends in write_summary_and_footer_magic, and the last statement there is writer.flush()?. That flush reaches the BufWriter<File>, so a flush error does fail the export before the rename, as you said.

a_flush_that_fails_fails_the_export is still a useful pin. If I change write_mcap to ignore the finish() result, that test fails, and only that test.

Two small follow-ups:

• The doc comment on a_flush_that_fails_fails_the_export (export.rs ~L902-907) still describes the bug from the incorrect report ("finish … never flushes it … a full disk used to be renamed into place"). That contradicts the correct comment above writer.finish().
• mcap-export is now a default feature, but it gates the whole dora recording group, which is now in the frozen cli-surface.txt. A --no-default-features build would drop frozen surface lines, and any future dora recording subcommand would also be hidden behind an MCAP feature. Dropping the feature, or gating only the Export variant, avoids this. This doesn't block the PR.

Otherwise I found no new issues.

---

Generated by Claude Code

[phil-opp]

🤖 Additional review notes (Claude Code). Found while reviewing head 0e0578c; these aren't in the earlier comments.

• nit — The PR title and description still describe the pre-7b7979f shape:
  • The title says "add dora export".
  • The body says the command is feature-gated (reachable only via cargo install dora-cli --features mcap-export), that it stays out of cli-surface.txt, and that the snapshot header gains three comment lines.
  • The command is now dora recording export. mcap-export is on by default, and the command is pinned in cli-surface.txt with no header change.
  • Trunk squash-merges using the title, so the commit and changelog would name a command that doesn't exist. Please update both before enqueueing.
• nit — The dev-dependency comment still names the old command: # `dora export` round-trip test (binaries/cli/Cargo.toml:114). It should say dora recording export.

---

Generated by Claude Code

[harsh839]

Both nits taken, and the title change was the more important of the two — trunk squash-merges with it, so the changelog would have named a command that does not exist. 5402121 plus the description.

Title is now feat(cli): add dora recording export to remux .drec recordings to MCAP, and the body no longer describes the pre-7b7979f shape. Concretely, it no longer says the command is feature-gated or reachable via --features mcap-export (mcap-export is a default feature, so a plain cargo install dora-cli has it), no longer says the command stays out of cli-surface.txt (it is pinned there now, with no header change), and no longer claims the snapshot header gains three comment lines. It also picks up the two things that changed since: the default output path, and that metadata.parameters are not carried into the MCAP. The test counts and command in the validation section are the current ones, including the flush test.

The dev-dependency comment in binaries/cli/Cargo.toml now says dora recording export.

No code path changed, so the gates are as they were: 18 export tests, 440 CLI lib tests, the CLI surface snapshot, fmt and clippy clean.

[phil-opp]

🤖 Automated follow-up review (Claude Code). This review is fully automated, with no human in the loop. Treat it as advisory only.

The only commit since the last automated review is 5402121. It changes only a dev-dependency comment in binaries/cli/Cargo.toml and is safe. With the updated title and description, it addresses the stale-naming notes.

One issue that hasn't been raised on this PR before:

• A single undecodable record aborts the whole export.
  • binaries/cli/src/command/recording/export.rs:197-198 returns an error on the first entry that deserialize_inter_daemon_event rejects.
  • dora replay warns and skips such entries (binaries/replay-node/src/main.rs:~163-172). InterDaemonEvent is #[non_exhaustive], and postcard rejects an unknown variant index outright.
  • So a recording made by a newer 1.x daemon that uses a new event variant, or one with a single corrupt record, replays fine but can't be exported at all.
  • Since export is meant to be the way to get data out of a .drec, it would be more robust to skip such entries with a warning and report the count at the end, as replay does. failed_export_keeps_the_previous_output_and_cleans_up would then need a different way to trigger its failure.

Still open from before:

• #[cfg(feature = "mcap-export")] gates the whole Recording subcommand group (command/mod.rs:20,58,130,223), not just Export. Now that dora recording export is in the frozen cli-surface.txt, it would be simpler to drop the feature or to gate only the Export variant. That way, a future dora recording subcommand doesn't depend on MCAP.

---

Generated by Claude Code

[harsh839]

Agreed, and this one was a real defect rather than a doc nit, so it is fixed in
e7ef4ef.

The forward-compat case is the sharp end. InterDaemonEvent is
#[non_exhaustive] and postcard rejects an unknown variant index outright, so
a .drec written by a 1.x daemon that adds an event cannot be exported by an
older binary at all — the tool whose job is getting data out of a recording
was unusable on exactly the recordings most worth recovering. The recording
crate already leans this way: RecordingWriter::write_entry_skip_oversized
skips an oversized message rather than aborting the recording.

Export now warns per record, counts, reports the count, and still fails when it
emitted nothing usable:

warning: skipping undecodable event for camera/image: ...
Skipped 1 undecodable record(s)

That last part is dora replay's replay_emitted_nothing_usable rule, for the
same reason — skipping must not turn systematic format drift into a file that
looks like an empty recording. Read errors stay fatal, so a torn or over-long
record still fails and the previous output is still left alone.

You predicted the test would need a different trigger, and it did.
failed_export_keeps_the_previous_output_and_cleans_up used unparseable event
bytes, which no longer fail. It is a test about atomicity rather than decoding,
so it now fails the read instead, via a record whose length prefix is over
MAX_RECORD_BYTES. A torn tail would not have worked, and that is worth
recording: read_next_record treats every short read as a clean EOF on purpose
so a crashed recording still exports the records that were fully written. So
"truncate the file" cannot produce a failing export at all — I tried that first
and the test passed for the wrong reason, twice.

Two new tests: one bad record among good ones exports the rest with sequence
numbering unbroken, and a recording of nothing but undecodable records fails
and leaves no output. Both verified by mutation — reverting to abort-on-first
fails both, and dropping the nothing-usable guard fails the second.

On the feature gate: dropped entirely, which is the version of your second
option I think is right. Nothing disables mcap-export — the wheel, dora self update and CI all take default features — so it was four cfgs guarding a
build nobody makes, and it gated the whole Recording group, so the next
subcommand added under it would have inherited the MCAP dependency by accident.
mcap and same-file are unconditional now. --no-default-features was
already broken by tracing and redb-backend (8 errors before and after, so
this change is neutral there rather than a fix).

Local: 442 CLI lib tests, 2 surface snapshot tests, fmt and clippy clean.

The Audit (cargo-audit + cargo-deny) failure is the same inherited one as on
#3543 — RUSTSEC-2026-0284 against lru 0.16.4 via zenoh-ext 1.10.1, which
main's lockfile already carries — and not something this diff introduces.

[phil-opp]

🤖 Automated review (Claude Code) — this review is fully automated, with no human in the loop. Treat it as advisory only.
Reviewed head: e7ef4ef

The new commit e7ef4ef fixes both points still open from the last review. I found no new issues.

• Undecodable records: export now handles them the way dora replay does. It warns and skips each one, reports how many it skipped, and fails only when every record was undecodable and nothing was exported. Read errors still fail the export.
  • an_undecodable_record_is_skipped_and_the_rest_still_exports and a_recording_of_only_undecodable_records_fails both fail when I put back the old abort-on-first-error code, so they really test the fix.
  • failed_export_keeps_the_previous_output_and_cleans_up now triggers its failure from the reader, with an oversized length prefix. It still tests the temp-file cleanup.
• mcap-export feature: it has been removed, and no references to it remain anywhere in the tree.

All 19 export tests in cargo test -p dora-cli --lib export pass locally.

---

Generated by Claude Code

[phil-opp]

🤖 Automated review (fully automated review by Claude Code — no human has vetted this; please verify the findings).

I re-reviewed the commits since the last automated review (5402121, e7ef4ef) and re-read the full diff. I found no new issues. I checked the code by reading it and did not build it.

• Skipping undecodable records (export.rs write_mcap): this follows dora replay's rule. A record that fails to deserialize is warned about and skipped, and the sequence numbers stay contiguous. An export where every record was skipped and nothing was emitted still fails. Reader errors are still fatal. The reworked failed_export_keeps_the_previous_output_and_cleans_up still fails inside the entry loop: the spliced 0x1000_0000 length prefix is over MAX_RECORD_BYTES, and its bytes don't match the DORA footer prefix, so read_next_record bails. It then checks that the earlier output survives and that the temp file is removed. Against the previous head, an_undecodable_record_is_skipped_and_the_rest_still_exports would fail, because the old code aborted on the first undecodable record.
• Dropping mcap-export fixes the point from the 09-29 review that the feature gated the whole dora recording group. The trade-off is that the dependency is now unconditional. Cargo.lock gains mcap, binrw/binrw_derive, enumset/enumset_derive, bimap, owo-colors and a second darling major version (0.21 next to 0.24), and no build can opt out. On 2026-09-24 a maintainer said this footprint is acceptable for the only stable way to get data out of a .drec, so I'm noting it rather than flagging it. About 360 of the ~1,140 lines in export.rs are production code; the rest are tests. So the maintenance cost in-tree is mostly this dependency set, not the code.

---

Generated by Claude Code

[phil-opp]

Automated review by Claude (fully automated, not a human review) — reviewed at e7ef4ef.

I re-read the whole diff. The commit that is new since the last automated review is e7ef4ef (skip undecodable records, drop the mcap-export feature). I found no new correctness issues.

• Skipping undecodable records: works as described. A deserialize failure is counted and skipped. A read error from next_entry (for example an over-cap length prefix) is still fatal. A pass where every record was skipped still fails, after finish(), so the temp file is removed. This matches dora replay.
• Dropping the feature: this fixes the earlier note that mcap-export gated the whole dora recording group. mcap and same-file are now unconditional, and cli-surface.txt matches the default build.
• Tests: I merged the branch into current main (no conflicts) and ran cargo test --locked -p dora-cli --lib recording::export. All 19 pass, and the lockfile stays fresh. Test (ubuntu-latest) was skipped on this PR's CI run, so this local run is the only one so far that has run the new skip tests.

Still open or worth knowing:

1. The Audit check fails because the base is stale, not because of this PR. The branch's Cargo.lock pins yoke-derive 0.8.2, which has been yanked (error[yanked] in the cargo-deny log). main now has 0.8.3. A rebase onto main should clear it.
2. The doc comment on a_flush_that_fails_fails_the_export (export.rs, ~L1063) still describes the bug that turned out not to exist: "finish … never flushes it … a full disk used to be renamed into place". This was raised on 2026-09-29 and is unchanged in the two commits since. It contradicts the correct comment above writer.finish().
3. Minor, optional: if the user hits Ctrl-C during a long export, nothing removes the temp file. The command has no signal handling, so a hidden .<name>.<pid>.tmp the size of the partial export stays in the output directory. Dropping the leading . from the name, or mentioning it in the docs, would make it easier to notice.

On scope: about 390 lines of the 1.3k are production code and about 740 are tests. The 10 new crates are mcap, binrw/binrw_derive, enumset/enumset_derive, bimap, owo-colors, and a second darling 0.21 with darling_core/darling_macro. All are MIT or Apache-2.0, and mcap's MSRV (1.81) is below the workspace's. Maintainers have already decided this belongs in the core CLI, so I'm not raising that again.

---

Generated by Claude Code

[phil-opp]

🤖 Automated review (fully automated Claude Code review; no human vetted this).

I re-reviewed head e7ef4ef. Two commits are new since the last automated review: 5402121 (docs) and e7ef4ef. I found no new issues. I checked the changes by reading the code only and did not build them.

• The mcap-export feature is removed. mcap and same-file are now plain dependencies, and every #[cfg(feature = "mcap-export")] gate is gone from command/mod.rs. This resolves the earlier note: --no-default-features no longer drops the frozen dora recording surface, and later recording subcommands no longer depend on an MCAP feature.
• Undecodable records are skipped. Each skipped record gets a warning, and the total is reported at the end. If nothing was emitted and at least one record was skipped, the export still fails. The sequence numbers count emitted messages, so a skipped record leaves no gap. Both new tests test real behaviour: one keeps the remaining records, the other fails when every record is undecodable.
• The atomic-output test still fails mid-loop. It used to rely on an undecodable record, which is now skipped. It now uses a length prefix larger than MAX_RECORD_BYTES, which read_next_record rejects. The test asserts that the error comes from the entry loop, so it still exercises the temp-file cleanup.

---

Generated by Claude Code

[phil-opp]

🤖 Automated review (fully automated review by Claude Code — no human has vetted this; advisory only).

Re-reviewed at e7ef4ef. The new commits since the last automated review are 5402121 and e7ef4ef. I found no new issues.

• Skipping undecodable records: this matches dora replay. Undecodable records are skipped and counted, and the export still fails when nothing usable came out. Read errors are still fatal. I ran cargo test -p dora-cli --lib recording::export and all 19 tests pass. I then made the decode error abort the export again, and both new tests (an_undecodable_record_is_skipped_and_the_rest_still_exports, a_recording_of_only_undecodable_records_fails) fail, so they do test the change. The rewritten atomicity test fails in the reader as it claims to: the spliced 256 MB length prefix is over MAX_RECORD_BYTES and doesn't collide with the footer magic.
• Dropping mcap-export: this resolves the earlier point that the feature gated the whole frozen dora recording group.

One small item from the 09-29 review is still open after these commits. The doc comment on a_flush_that_fails_fails_the_export (export.rs ~L1059-1064) still says finish "never flushes" and that a full disk "used to be renamed into place". That contradicts the corrected comment above writer.finish() in write_mcap.

---

Generated by Claude Code