Skip to content

fix(cli,record-node): pass the recorded descriptor base64-encoded, past env expansion - #3681

Open
phil-opp wants to merge 2 commits into
mainfrom
claude/laughing-brown-y6wvr6-record-descriptor-env
Open

phil-opp wants to merge 2 commits into
mainfrom
claude/laughing-brown-y6wvr6-record-descriptor-env

Conversation

@phil-opp

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

Copy link
Copy Markdown
Collaborator

Problem

dora record hands the original dataflow YAML to the record node as an env: value:

env_mapping.insert("DORA_RECORD_DESCRIPTOR", String::from_utf8_lossy(&yaml_bytes));

Every EnvValue is deserialized with with_expand_envs (shellexpand::env), and that happens each time the generated descriptor is parsed: once by the CLI, and again by the daemon. So the raw YAML text was $VAR-expanded:

  • Unset variable: a $NAME anywhere in the dataflow file, even in a comment such as # set $ROS_DOMAIN_ID first, made dora record fail with the opaque data did not match any variant of untagged enum EnvValue.
  • Set variable: its value was substituted, so the .drec header stored a descriptor different from the user's file. For example, env: {TOKEN: ${API_TOKEN}} put the real token, in plain text, into a recording that is meant to be shared.

Escaping $ as $$ does not help, because the second parse on the daemon would expand it again.

Fix

  • The CLI now passes the exact descriptor bytes base64-encoded as DORA_RECORD_DESCRIPTOR_BASE64. The base64 alphabet has no $, the value is 1.33× the size, and the bytes survive exactly instead of going through a lossy UTF-8 conversion.
  • The record node decodes it, and still accepts DORA_RECORD_DESCRIPTOR from an older CLI.
  • Adds a direct base64 = "0.22" dependency to dora-cli and dora-record-node. It was already in Cargo.lock, so nothing new is compiled.

Validation

Class: C (record/replay)

  • New regression test record::tests::descriptor_env_value_survives_env_expansion. It builds the record node's env: entry for a YAML containing $DORA_TEST_SURELY_UNSET_VAR (in a comment) and ${HOME}, deserializes it as EnvValue (the real expansion path), and asserts it decodes back to the exact original bytes.
    • RED with the old raw value: data did not match any variant of untagged enum EnvValue, the same error users see.
    • GREEN with the fix.
  • cargo test -p dora-cli -p dora-record-node: ✅. cargo clippy -p dora-cli -p dora-record-node --all-targets -D warnings: ✅. cargo fmt --check: ✅.
  • Contract tests: see the batch summary comment.

Not addressed

  • Older record node on PATH: one that predates this change reads only DORA_RECORD_DESCRIPTOR, so it writes an empty descriptor header when paired with this CLI. Setting both variables would bring the bug back.
  • Other env values: DORA_RECORD_FILE and DORA_RECORD_TOPICS are still raw env: values, so an output path containing $ has the same problem. That is rarer, and left as a follow-up.

🤖 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

…st env expansion

`dora record` handed the original dataflow YAML to the record node as
the `env:` value `DORA_RECORD_DESCRIPTOR`. Every `env:` value is
`$VAR`-expanded each time the descriptor is parsed (by the CLI, then
again by the daemon), so:

- a `$NAME` anywhere in the dataflow file, even in a comment, failed
  `dora record` with "data did not match any variant of untagged enum
  EnvValue" when `NAME` was unset;
- when set, its value was substituted, so the `.drec` header stored a
  descriptor that differed from the user's file, e.g. with the real
  value of an `${API_TOKEN}` reference in plain text.

Escaping `$` as `$$` does not survive the second parse, so pass the
bytes base64-encoded as `DORA_RECORD_DESCRIPTOR_BASE64` instead: no `$`
in the alphabet, 1.33x the size, and exact bytes rather than a lossy
UTF-8 conversion. The record node decodes it and still accepts
`DORA_RECORD_DESCRIPTOR` from an older CLI.

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.

The underlying bug is real. A raw descriptor containing $UNSET_VAR fails EnvValue parsing, and one containing ${HOME} gets substituted. I found the following issues with the fix:

1. A new CLI with an existing record-node binary silently writes recordings that can't be replayed.
node_binary::find (binaries/cli/src/command/node_binary.rs:9) reuses any dora-record-node it finds next to the executable, on PATH or in target/{debug,release}. It never checks the binary's version and doesn't rebuild it. Anyone who has run dora record on 1.0.x already has such a binary. After upgrading the CLI to this change, that old node reads only DORA_RECORD_DESCRIPTOR, which is no longer set, so it writes an empty descriptor into the .drec header without any error. The recording looks fine until dora replay fails with "descriptor has no nodes array", and at that point the descriptor can't be recovered.

The legacy fallback in descriptor_from_env only covers the other direction (old CLI, new node). Suggestion: also keep setting the legacy DORA_RECORD_DESCRIPTOR when the YAML contains no $ (record.rs, around the env_mapping.insert). An old node then keeps working in the common case, and only the $ case, which was already broken, depends on the new node. If you don't want that, at least add a Changelog note that dora-record-node must be rebuilt along with the CLI.

2. The new test doesn't exercise the changed code.
descriptor_env_value_survives_env_expansion builds its own env mapping with BASE64_STANDARD.encode and round-trips it, so it never touches what run_record actually emits. I reverted the production change in run_record back to the raw DORA_RECORD_DESCRIPTOR, and the test still passes. The record-node side has no test at all: neither descriptor_from_env, its legacy fallback, nor the invalid-base64 error. Suggestion: move the env_mapping construction into a helper that run_record calls, and test that helper's output. A regression back to raw YAML would then fail the test.

I ran the test with the run_record change reverted, and I confirmed the original bug with a temporary deserialization probe. For finding 1 I read the code (find, the old record-node main, replay.rs) but didn't run the skewed setup end to end.


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