Skip to content

fix(node): JSON-escape plain-text input in read_json_bytes_as_arrow - #3080

Open
phil-opp wants to merge 1 commit into
mainfrom
claude/dreamy-bardeen-v852wi-json-string-escape
Open

fix(node): JSON-escape plain-text input in read_json_bytes_as_arrow#3080
phil-opp wants to merge 1 commit into
mainfrom
claude/dreamy-bardeen-v852wi-json-string-escape

Conversation

@phil-opp

@phil-opp phil-opp commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Issue

read_json_bytes_as_arrow (apis/rust/node/src/daemon_connection/json_to_arrow.rs) first tries to parse the input as JSON, and on failure retries by treating it as a plain string. The retry (wrapped_quoted) wrapped the raw bytes in " quotes:

fn wrapped_quoted(data: impl BufRead) -> impl BufRead {
    let quoted = "\"".as_bytes().chain(data).chain("\"".as_bytes());
    wrapped(quoted)
}

If the input itself contains a ", a backslash, or a raw control character (newline/tab), the wrapped result is invalid JSON, so both the JSON attempt and the string-fallback attempt fail and the whole call errors with failed to infer JSON schema instead of yielding a string array.

This is reachable from dora run interactive mode, whose Data prompt advertises "String/JSON …" (apis/rust/node/src/daemon_connection/interactive.rs:140). Concretely, these all failed before this change:

  • typing say "hi" → produced { "inner":"say "hi""} (invalid JSON)
  • entering a Windows path C:\Users\node
  • pasting a multi-line value

Fix

JSON-escape the bytes via serde_json before wrapping, so any plain-text input becomes a valid JSON string literal. wrapped_quoted is replaced by a small json_quote helper. Behavior for bare JSON values (numbers, objects, arrays) is unchanged — they still parse natively on the first attempt.

Validation

  • Added regression tests in json_to_arrow.rs covering double quotes, backslashes, and control characters round-tripping to the expected string, plus a guard that a bare JSON number still parses as a non-string array.
  • cargo test -p dora-node-api --lib json_to_arrow — 4 passed.
  • cargo fmt --all -- --check and cargo clippy -p dora-node-api -- -D warnings clean.

🤖 This is a machine-generated pull request opened by Claude Code as part of an automated code-review pass. Please review carefully before merging.

🤖 Generated with Claude Code


Generated by Claude Code

When the JSON schema inference fails, `read_json_bytes_as_arrow` retries
by treating the input as a plain string. It did so by wrapping the raw
bytes in `"` quotes (`wrapped_quoted`), which produces invalid JSON
whenever the input itself contains a `"`, a backslash, or a raw control
character (newline/tab). Both inference attempts then fail and the input
is rejected with "failed to infer JSON schema" instead of becoming a
string array.

This is reachable from `dora run` interactive mode, whose Data prompt
advertises "String/JSON": typing `say "hi"`, pasting a multi-line value,
or entering a Windows path like `C:\Users\node` all failed.

Fix by JSON-escaping the bytes (via `serde_json`) before wrapping, so any
plain-text input is encoded as a valid JSON string literal. Adds
regression tests for double quotes, backslashes, and control characters,
plus a guard that bare JSON values still parse natively.

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

trunk-io Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

⏳ Waiting to start tests on this pull request - details.

phil-opp commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Automated code-review pass — summary

This is the tracking comment for a machine-generated code-review pass over the repository (branches suffixed off freshly-fetched origin/main). Each finding was verified against current origin/main and checked against open PRs from prior passes to avoid duplicates.

PRs opened, by category

Correctness

Simplification / Optimization

Documentation

Rejected findings (with one-line reasons)

  • OperatorId skips the validation NodeId/DataId enforce — latent only (the parser never builds an invalid id), and adding validation risks rejecting operator ids in existing dataflows (breaking change).
  • InputMapping::Logs Display drops node_filter when min_level is None — unreachable; FromStr never produces that combination.
  • mavlink wake_udp_recv keeps a non-loopback bind host — low confidence; the READER_SHUTDOWN_GRACE detach path already bounds the worst case.
  • local_listener _ arm neither replies nor closes on an unexpected request — low reachability (socket only expects NodeConfig), and a behavior change to a protocol/concurrency path is hard to cover with a regression test.
  • PendingNodes::handle_dataflow_stop discards DataflowStatus without a rationale comment — not a demonstrated defect; maintainability-only.
  • arrow-convert missing TryFrom<&ArrowData> for Vec<String> — API-symmetry/ergonomics gap (a feature), not a defect.
  • decode_arrow_ipc decodes only the first batch — single-batch is the internal producer contract; no demonstrated harm.
  • node_inputs per-node Vec alloc / json_quote cold-path alloc — cold or pre-existing; negligible.

Validation

  • Per branch: cargo fmt --all --check + cargo clippy -- -D warnings + cargo test for the affected crate(s) — all green.
  • Combined diff: full workspace suite (cargo test --all, Python/examples excluded) green except the two apis/rust/node/tests/zenoh_schema_cache.rs Zenoh loopback integration tests, which fail only in this network-restricted sandbox (they need a loopback peer link) and are unrelated to any change here — none of the diff touches Zenoh code.
  • /review + /simplify gates were run once over the combined diff; their findings (case-sensitive level match in fix(cli): parse JSONL log lines that omit or capitalize the level field #3082; pruning pre-existing rotated files in fix(daemon): honor max_rotated_files == 0 in rotate_log_files #3081) were folded into the respective PRs.

⚠️ These pull requests are machine-generated by Claude Code. Please review each carefully before merging.


Generated by Claude Code

phil-opp commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude — fully automated, no human in the loop. Findings are suggestions to verify.

Reviewed the diff against the surrounding code. The fix is correct: json_quote builds the string literal via serde_json::Value::String(...).to_string(), which escapes ", \, and control characters, so the subsequent wrapped(...) produces valid JSON for any plain-text input. Behavior for bare JSON values is unchanged — they still satisfy infer_json_schema on the first wrapped(data) attempt and never reach the fallback.

The added tests are meaningful and would fail against the old wrapped_quoted logic:

  • plain_text_with_double_quotes_round_trips / _with_backslash_ / _with_control_characters_ each exercise exactly the inputs the old raw-quote path turned into invalid JSON, and assert the round-tripped string value.
  • bare_json_values_still_parse_natively guards the no-regression case (a bare number stays a non-string array).

No issues found. (Minor, non-blocking: String::from_utf8_lossy means non-UTF-8 input is replaced lossily rather than errored — which is reasonable and matches the documented intent.)


Generated by Claude Code

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants