Skip to content

Interpolate every value as one literal argument, like "$var" in sh - #201

Merged
konard merged 7 commits into
mainfrom
issue-41-448ca60fbc42
Sep 6, 2026
Merged

Interpolate every value as one literal argument, like "$var" in sh#201
konard merged 7 commits into
mainfrom
issue-41-448ca60fbc42

Conversation

@konard

@konard konard commented Sep 6, 2026

Copy link
Copy Markdown
Member

Summary

cat ${filePath} with a path that contains spaces already produced a single
argument, so the snippet in the issue works. Investigating the whole family of
cases around it (experiments in experiments/issue-41-*.mjs) found the real
divergence one layer down, in quote(): two "already quoted" shortcuts spliced
a value into the command as shell syntax instead of quoting it.

That behavior is not what sh — or any competitor — does, and it had two hard
failures:

value before after
'/My Documents/report.txt' (caller pre-quoted) quotes stripped, spliced in as syntax one literal argument, quotes kept
"it's" '"it's"'/bin/sh: Syntax error: Unterminated quoted string '"it'\''s"', runs
' ; touch /tmp/pwned ; ' spliced in verbatim — the injected touch ran quoted, no second command

Fixes #41.

Behavior

An interpolated value is now always exactly one literal argument, spaces and
quote characters included — the same guarantee as "$path" in a shell script:

const file = '/Users/john/My Documents/report.txt';
await $`cat ${file}`; // one argument: /Users/john/My Documents/report.txt

Measured against the competitors before choosing: Bun's $, zx and execa all
treat an interpolated value literally, quote characters included. This matches
the maintainer's direction in the issue — sh-like by default, configurable.

Configurable opt-out (mirrors the existing COMMAND_STREAM_QUOTE_CONTEXT
switch from #49): shell.preQuotedPassthrough(true),
setPreQuotedPassthroughEnabled(true), or
COMMAND_STREAM_PREQUOTED_PASSTHROUGH=1. Even then only balanced values are
passed through, so the injection above cannot be re-enabled.

How to reproduce

node experiments/issue-41-injection.mjs   # injected `touch` ran before the fix
node experiments/issue-41-diff-sh.mjs     # 17-value differential vs /bin/sh: 0 failures now
node js/examples/paths-with-spaces.mjs    # real file operations in a dir with spaces

Tests

  • js/tests/paths-with-spaces.test.mjs (new, 185 tests): quote() semantics;
    an argv-fidelity table of 15 tricky path values × unquoted/double/single
    contexts, asserted with a fixture that prints ARG[...] per argv entry; a
    real-binary case with virtual commands disabled; a /bin/sh differential
    parity table
    (8 scripts × 15 values) where the reference always uses
    "$V"; real file operations (cat, ls, cp/mv/rm, mkdir -p, redirection,
    pipeline, cd && pwd, test -f, .sync()) inside a temp dir named
    my documents …; an injection regression using a touch marker; and the
    legacy passthrough switch.
  • rust/tests/paths_with_spaces.rs (new) mirrors it, including the /bin/sh
    differential table and the injection regression. 3 of its 5 tests fail
    against the pre-fix quote(), confirming they reproduce the bug.
  • Unit tests updated in js/tests/$.test.mjs,
    js/tests/path-interpolation.test.mjs, js/tests/readme-examples.test.mjs,
    rust/src/quote.rs, rust/tests/utils.rs, where they asserted the removed
    passthrough.
  • Full suites: bun test js/tests/ — no new failures against the recorded
    baseline (the remaining failures need jq, which is not installed here);
    cargo test green; eslint, prettier, jscpd, cargo fmt, cargo clippy -D warnings all clean.

The issue links deep-assistant/hive-mind/command-stream-issues/issue-05-paths-with-spaces.mjs,
which now 404s; the coverage above was reconstructed from the issue text plus
the competitor comparison.

Parity & release

Mirrored in the Rust crate (quote, is_pre_quoted_passthrough_enabled,
exported from the crate root). Docs updated in js/README.md,
js/BEST-PRACTICES.md, rust/BEST-PRACTICES.md. Release triggers included:
js/.changeset/issue-41-paths-with-spaces.md (minor) and
rust/changelog.d/20260906_120000_paths_with_spaces.md (minor).

Follow-up fix: EPIPE in the streaming pipeline

The new /bin/sh parity table surfaced a pre-existing bug: on bun +
ubuntu CI, one case (printf '%s\n' <path> | cat) failed with
EPIPE: broken pipe, write at js/src/$.process-runner-pipeline.mjs:351
(await writer.close()). When a pipeline stage exits before the stage feeding
it has finished, closing its stdin raises EPIPE. The pump in
pipeStreamToProcess guarded its writes but not that close, and its promise
was never caught, so the rejection escaped as an unhandled error and failed an
otherwise successful command. The close is now guarded the same way the writes
are, and the pump promise is returned so the failure mode is testable
(js/tests/pipeline-epipe.test.mjs, 2 of its 3 tests fail without the guards).
The Rust implementation already discarded these errors
(let _ = stdin.write_all(...) / let _ = stdin.shutdown()), so this brings
JavaScript in line with it.

Adding .gitkeep for PR creation (default mode).
This file will be removed when the task is complete.

Issue: #41
@konard konard self-assigned this Sep 6, 2026
konard added 2 commits September 6, 2026 21:55
Interpolation had two "already quoted" shortcuts: a value wrapped in
matching quotes was spliced into the command as shell syntax instead of
being quoted. So a path the caller had quoted lost its quotes, and a
value like "it's" produced '"it's"' - an unterminated string the shell
refuses to run. Worse, the shortcut also accepted unbalanced values, so
"' ; touch /tmp/pwned ; '" was spliced in verbatim and the injected
command executed.

Values are now always quoted as literal text, so an interpolated path
reaches the command as exactly one argument, spaces and quote characters
included - the same guarantee as "$path" in sh, and the behavior of
Bun's $, zx and execa (issue #41).

The previous behavior stays available via shell.preQuotedPassthrough(),
setPreQuotedPassthroughEnabled(true), or
COMMAND_STREAM_PREQUOTED_PASSTHROUGH=1, and even then only balanced
values are passed through, so the injection can no longer happen.

Mirrored in the Rust crate (quote, is_pre_quoted_passthrough_enabled).
@konard konard changed the title [WIP] File paths with spaces need proper quoting Interpolate every value as one literal argument, like "$var" in sh Sep 6, 2026
konard and others added 4 commits September 6, 2026 21:59
A pipeline stage can exit before the stage feeding it has finished
writing (`source | grep -m1 ...`, or simply a race at the end of the
pipe). Closing that stdin then raises EPIPE. The pump in
pipeStreamToProcess guarded its writes but not the close in its
`finally` block, and the pump promise itself was never awaited or
caught, so the rejection escaped as an unhandled error and could fail an
otherwise successful command - as it did on CI for a `printf ... | cat`
parity case in tests/paths-with-spaces.test.mjs.

Guard the close the same way the writes are guarded, and return the pump
promise so the failure mode is testable. The Rust implementation already
discards these errors (`let _ = stdin.write_all(...)` /
`let _ = stdin.shutdown()`), so this brings JavaScript in line with it.
The changeset validator requires exactly one changeset per pull request.
@konard
konard marked this pull request as ready for review September 6, 2026 22:19
@konard

konard commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

Working session summary

Done. PR #201 is ready for review: #201

Final state of this session's work:

Root cause of the remaining CI failure — it was not a quoting mismatch and not a flake in my test's assertions. pipeStreamToProcess in js/src/$.process-runner-pipeline.mjs guarded its writer.write() calls against EPIPE but not the await writer.close() in the finally block, and the pump's promise was never awaited or caught. When a downstream stage exits before its stdin is closed (a normal pipeline race), the EPIPE escaped as an unhandled rejection and failed an otherwise successful command — which is what happened for the printf '%s\n' <tab path> | cat parity case on bun/ubuntu.

Fix — the close is now guarded the same way the writes are, via StreamUtils.handleStreamError(..., false), and the pump promise is returned (and caught) so the failure mode is testable. The Rust side already discarded these errors (let _ = stdin.write_all(...) / let _ = stdin.shutdown()), so this brings JS in line with it.

Regression testjs/tests/pipeline-epipe.test.mjs, driving pipeStreamToProcess with a fake process whose stdin raises EPIPE on close (and on write). I verified non-vacuity: with only the guards removed, 2 of its 3 tests fail; with the fix, all pass. I first tried an end-to-end reproduction (virtual source | grep -m1), but that race is not deterministic locally, so I used the injectable form instead.

Also this session: folded the EPIPE note into the single changeset after Check for JavaScript changesets rejected two changesets in one PR, updated the PR description with a "Follow-up fix" section, and re-checked issue and PR comments (no new feedback beyond konard's original design comment).

Verification: eslint, prettier --check, jscpd clean locally; all five workflows (JavaScript, Rust, Language parity, Repository quality, Security) green on the final commit 6e08ce4; working tree clean; origin/main is an ancestor of HEAD, so no merge was needed.


This summary was automatically extracted from the AI working session output.

@konard

konard commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

🤖 Solution Draft Log

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost: $12.319192

📊 Context and tokens usage:

Claude Opus 5: (3 sub-sessions)

  1. 115.1K / 1M (12%) input tokens, 36.1K / 128K (28%) output tokens
  2. 116.4K / 1M (12%) input tokens, 34.8K / 128K (27%) output tokens
  3. 63.6K / 1M (6%) input tokens, 17.6K / 128K (14%) output tokens

Total: (8.8K new + 264.0K cache writes + 14.0M cache reads) input tokens, 105.7K output tokens, $12.319193 cost

🤖 Models used:

  • Tool: Anthropic Claude Code
  • Requested: opus (claude-opus-5)
  • Thinking level: medium (~15999 tokens)
  • Model: Claude Opus 5 (claude-opus-5)

📎 Log file uploaded as Gist (5059KB)


Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard
konard merged commit aa68510 into main Sep 6, 2026
34 checks passed
@konard

konard commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

🎉 Auto-merged

This pull request has been automatically merged by hive-mind.

  • All CI checks have passed

Auto-merged by hive-mind with --auto-merge flag

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.

File paths with spaces need proper quoting

1 participant