Skip to content

fix(cli): reject an invalid output id in dora record --proxy instead of panicking - #3695

Open
phil-opp wants to merge 2 commits into
mainfrom
claude/laughing-brown-rpqunk-record-proxy-dataid
Open

phil-opp wants to merge 2 commits into
mainfrom
claude/laughing-brown-rpqunk-record-proxy-dataid

Conversation

@phil-opp

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

Copy link
Copy Markdown
Collaborator

🤖 Machine-generated PR. Opened by Claude Code as part of an automated codebase review. Please review carefully before merging.

Issue

dora record --proxy subscribes to the (node, output) pairs found by discover_descriptor_outputs. That function walks the untyped YAML and accepts any string. The typed parse is deliberately non-fatal.

The two ids were converted differently:

  • The node id went through the fallible parse::<NodeId>().
  • The output id was converted with .into(), i.e. DataId's From<String>, which panics on an invalid id (see its doc in libraries/message/src/id.rs).

A descriptor with outputs: ["bad id"], "a//b" or a leading / therefore crashed the recorder with invalid DataId 'bad id' instead of returning an error.

Fix

Parse both ids fallibly in a small helper, parse_ws_topics, and return invalid output ID in topic: …. Operator outputs such as op/out are still accepted.

Validation

Class: B

  • New test invalid_output_id_in_proxy_topics_is_an_error_not_a_panic: the three invalid forms return Err, and op/out parses.
  • cargo test -p dora-cli --lib record: ✅ (28 passed)
  • cargo clippy -p dora-cli --all-targets -- -D warnings: ✅, cargo fmt --check: ✅

🤖 Generated with Claude Code

https://claude.ai/code/session_016ZqAqLhDyZGTnpGHyENnh7


Generated by Claude Code

…d of panicking

The proxy recorder subscribes to the (node, output) pairs found by an
untyped YAML walk that accepts any string. The node id went through the
fallible `parse::<NodeId>()`, but the output id was converted with
`.into()` -- `DataId`'s `From<String>`, which panics on an invalid id
(e.g. `"bad id"`, `"a//b"`, a leading `/`). Parse both ids fallibly in a
small helper and add a regression test.

Machine-generated by an automated Claude Code review. Please review carefully.

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

trunk-io Bot commented Oct 4, 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 4, 2026

Copy link
Copy Markdown
Collaborator Author

CI note: Audit fails on main's yanked yoke-derive 0.8.3, not because of this PR. I've cherry-picked the fix from #3682 (Cargo.lock → 0.8.4); it becomes a no-op once that lands.


Generated by Claude Code

phil-opp commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review (fully automated Claude Code review; no human checked it)

No issues found.

  • The bug is real on main. o.clone().into() (binaries/cli/src/command/record.rs:564) goes through DataId::from(String), which panics on an invalid id. The untyped YAML walk can produce such an id, because the typed descriptor parse is non-fatal.
  • The fix is right. Switching to the fallible parse::<DataId>() fixes it. I found no other place in the CLI that converts a user string into a DataId with .into().
  • The test covers the important cases. It checks the panic cases ("bad id", "a//b", "/out") and a valid operator-qualified id. It can't compile on main because parse_ws_topics is new there, but the panic it guards against follows directly from the From<String> impl.

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