fix: dl -- <cmd> re-splits every quoted argument it is given by blooop · Pull Request #591 · blooop/devlaunch

blooop · 2026-09-09T13:20:05Z

What was wrong

The words after -- were rejoined with plain spaces and handed to bash -lc as one command line, so the remote shell got back every separator the host's shell had already consumed.

$ dl <ws> -- printf '[%s]\n' 'a b' c
SSH command: devpod ssh <ws> --command bash -lc 'printf [%s]\n a b c'
[a]n[b]n[c]n            # expected [a b] and [c]

Two more consequences, both silent:

$ dl <ws> -- echo 'before #10848 after'
before                  # `#` began a comment; the rest of the line was discarded

$ dl <ws> -- echo 'uid=$(id -u)'
uid=1000                # executed, in a workspace holding the forwarded GH_TOKEN

That last one makes any text flowing into a dl -- command line (a PR title, a review body) shell code.

The # case is how this was found. A PR supervisor sent claude 'Address the open review on PR #10848 (…)'; the agent was prompted with the single word Address and asked what to address, a question the discarded half of the prompt forbade. Every hard rule it carried was gone before claude started.

The fix

shell::join instead of words.join(" "), one line in rust/dl/src/launch.rs. That quoter already existed and is already what the rest of the tree uses; this call site was the one place not using it.

The same three commands now:

[a b] / [c]  ·  before #10848 after  ·  uid=$(id -u)

aid had to move with it. It composed a whole shell line and passed it as a single word, which survived only because joining one element is an identity — the same accident that re-split everyone else's quoted argument. It hands dl argv now and quotes nothing itself. The composed payload is byte for byte what it was, which rust/aid/tests/ proves: every devpod ssh --command bash -lc '…' assertion in that suite is unmodified from main and passes. The one line that changed is the echoed aid -> dl command, which now shows the tail as the words aid hands over.

A bare NAME=value still reaches the shell as an assignment prefix, because = is in the shell-safe set. That is load-bearing rather than incidental: it is what aid builds and what README documents, so docs/cli.md now writes it down as the exception it is.

Behaviour change

Passing a shell snippet as a single word no longer works, because a single word is now a program name. dl <ws> -- 'exit 7' looks for a program called exit 7 and exits 127 where it used to exit 7; dl <ws> -- "" went from exit 0 to 127. Name a shell instead: dl <ws> -- bash -lc 'a && b' — which was itself broken before this, running bash -lc a and then b.

The 13 e2e probes were the only callers in the tree spelling it the old way. They name bash -lc now.

Review

Four commits, /review-self over three axes. What it changed:

The first version prefixed aid's tail with env(1). That broke herdr::agent_in, which steps over NAME=value words and takes the first remaining word as the program: env is in no agent table, so an aid-launched workspace classified as None. Dropped, which also restored README's aid … is exactly dl … -- IS_SANDBOX=1 … equivalence and made the payload byte-identical.
The e2e breakage above. Found by the Defects axis, not by me.
Six citations of an issue number that does not exist, and a docs/cli.md reference to version 0.34.1, which never shipped.

Escalated rather than fixed: RemotePayload::wrap now has two callers meaning opposite things — dl's -- hands it argv-joined text, dotfiles_command hands it a live shell script with && and $(…). Both are Option<String> and the type cannot tell them apart, so the old dangerous reading is still constructible through pub LaunchVerb::Attach. The shape that fixes it is RemoteCommand::Argv(NonEmpty<String>) | ::Script(String), joined inside core; that is a public API change with a public-api snapshot behind it, so it belongs in its own PR.

Verification

cargo test --workspace, cargo clippy --locked --all-targets -- -D warnings, cargo fmt --check, pre-commit, and 740 passed / 7 skipped in the Python guards. Live-validated against a running workspace with a release build.

Not run locally: test-e2e. It builds containers, so CI proves that part rather than me. It is also the part this change touches most.

🤖 Generated with Claude Code

Summary by Sourcery

Preserve dl -- argument boundaries and prevent remote shell injection while retaining explicit shell-script execution.

Bug Fixes:

Preserve quoted arguments passed after dl <workspace> -- so spaces, comments, and shell substitutions remain literal instead of being re-split or executed remotely.
Update agent launches to pass command arguments directly, preserving existing payloads while preventing prompts and other text from becoming shell syntax.

Enhancements:

Clarify that dl -- accepts an argument vector and that shell snippets must explicitly invoke a shell, while documenting the intentional NAME=value assignment behavior.

Build:

Bump the project version and release metadata to 0.36.0.

Documentation:

Document dl -- argument handling, assignment-prefix behavior, shell snippets, and local-shell redirections.

Tests:

Add coverage for argument preservation, comment and command-substitution safety, shell snippets, and agent argv construction.
Update end-to-end command probes to invoke shell scripts explicitly.

The words after `--` were rejoined with plain spaces and handed to `bash -lc` as one command line, so the remote shell got back every separator the host's shell had already consumed. `dl <ws> -- claude 'fix the bug'` arrived as four arguments where one was meant. Two silent consequences beyond the splitting. A word holding `#` commented out the rest of the line: a supervisor sending a prompt that named `PR #10848` reached the agent as the single word `Address`, and every rule the prompt carried was discarded before claude ran. And a word holding `$(...)` was executed, in a workspace holding the forwarded GH_TOKEN, which made any text flowing into a `dl --` line (a PR title, a review body) shell code. `shell::join` was already here and already the right spelling; the call site was the one place not using it. aid composed its own line and passed it as a single word, which survived only because the rejoin was an identity on one argument. It hands dl argv now, with its variables set by env(1) rather than the shell's assignment-prefix syntax. Two dl tests asserted the old behaviour, one of them named `a_quoted_prompt_reaches_the_agent_intact` while asserting a payload that reached the agent as two arguments.

…tection `herdr::agent_in` steps over `NAME=value` words and takes the first remaining word as the program. Prefixing aid's tail with the literal word `env` made that first word `env`, which is in no agent table, so an aid-launched workspace classified as `None` where `clients/herdr.rs` asserts `Some("claude")`. Masked only because aid exports HERDR_AGENT itself, and nothing said so. `env` bought nothing anyway: `=` is in `shell::quote`'s safe set, so a bare `NAME=value` word reaches the remote shell unquoted and keeps its assignment-prefix meaning. Dropping it makes the composed payload byte for byte what it was before this branch, which is what `rust/aid/tests/` now proves -- every `devpod ssh --command bash -lc '...'` assertion in that suite is main's own, unmodified, and passes. The one line that did change is the echoed `aid -> dl` command, which shows the tail as the words aid now hands dl. It also keeps README's `aid <ws> ... is exactly dl <ws> -- IS_SANDBOX=1 ...` equivalence true, which the env spelling had quietly broken.

Thirteen call sites in test/e2e/test_interactive_session.py handed `dl <ws> --` a whole shell script as a single argument, which the rejoin used to hand straight to `bash -lc`. Quoting each word turns every one of them into a program name with spaces in it: `dl("--", "exit 7")` looks for a program called `exit 7` and exits 127, where `test_one_shot_command_propagates_failure` asserts 7. The tty, login-shell, cwd and claude probes all fail the same way, and `pixi run test-e2e` is a required job. They name the shell now, `dl("--", "bash", "-lc", PROBE)`, which is the spelling docs/cli.md documents for exactly this. Note a quoted word is not an assignment to bash either, so even the `D=PWD; ...` probes needed it. Not run locally: the suite builds containers. This is the one part of the change CI proves rather than me.

The comments, CHANGELOG and docs cited #588 six times for a ticket that does not exist: the highest issue is #584, and 588 is an unrelated open PR about sharing agent skills in the container feature. Every one of those citations pointed a reader at somebody else's work. Removed rather than renumbered, since there is no issue for this bug to cite. docs/cli.md said the old behaviour was "Before 0.34.1"; Cargo is 0.34.0 and the entry is under [Unreleased], so that version has never existed. It was also the only version reference in docs/. The docs claimed `--` is argv full stop, which overstates it: `=` is shell-safe, so a leading `NAME=value` still reaches the shell as an assignment prefix. That is load-bearing rather than incidental, since it is what `aid` builds and what README documents, so it is now written down as the exception it is. The launch.rs comment also narrated all three failure modes that the CHANGELOG, the docs and four test names each already state, and miscounted the tests it cited.

# Conflicts: # CHANGELOG.md

sourcery-ai

Sorry @blooop, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 4 days and 23 hours by commenting @sourcery-ai review. Upgrade to get a review now.

sourcery-ai · 2026-09-09T13:20:23Z

Reviewer's Guide

The PR fixes remote command injection and argument re-splitting by treating everything after dl -- as argv and quoting each word exactly once when constructing the remote bash -lc payload. aid now forwards argv directly, documentation and changelog describe the new contract and shell-snippet exception, and unit/e2e coverage is updated accordingly.

Sequence diagram for safe dl argv forwarding

sequenceDiagram
    participant User
    participant HostShell
    participant dl
    participant RemoteShell
    participant Command

    User->>HostShell: dl workspace -- command quoted_arg
    HostShell->>dl: argv words after --
    dl->>dl: shell::join(words)
    dl->>RemoteShell: bash -lc quoted command line
    RemoteShell->>Command: parse preserved argv
    Command-->>User: quoted_arg remains one argument

Sequence diagram for aid forwarding agent argv

sequenceDiagram
    participant aid
    participant dl
    participant RemoteShell
    participant Agent

    aid->>aid: build_agent_command()
    aid->>dl: agent argv after --
    dl->>dl: shell::join(argv)
    dl->>RemoteShell: bash -lc quoted payload
    RemoteShell->>Agent: start command with prompt as one argument

Flow diagram for documented dl shell snippet behavior

flowchart LR
    A[dl workspace -- command argv] --> B{Need shell syntax?}
    B -->|No| C[shell::join each word]
    B -->|Yes| D[Name the shell: bash -lc snippet]
    C --> E[Remote bash -lc payload]
    D --> E
    E --> F[Remote command executes safely]

File-Level Changes

Change	Details	Files
Preserve `dl --` arguments as remote argv instead of reconstructing an unquoted shell command.	Replace plain-space joining with `shell::join` so spaces, comments, substitutions, and shell metacharacters remain argument data. Document the argv semantics, the deliberate `NAME=value` assignment exception, and the required `bash -lc` form for shell scripts. Add focused unit coverage for quoting and shell-snippet behavior.	`rust/dl/src/launch.rs` `docs/cli.md` `CHANGELOG.md` `rust/dl/tests/launch.rs`
Update `aid` to pass agent command arguments directly to `dl`.	Change agent command construction from a precomposed shell string to `Vec<String>` argv. Preserve assignment-prefix behavior without quoting values in `aid`, allowing `dl` to perform the sole payload quoting. Update argv-oriented unit and integration expectations while retaining byte-identical remote payload assertions.	`rust/aid/src/rewrite.rs` `rust/aid/tests/rewrite.rs`
Adapt end-to-end command probes to the explicit shell invocation contract.	Change script-shaped probes from a single command word to `bash -lc <script>`. Retain coverage for one-shot execution, exit propagation, login-shell behavior, TTYs, working directory, interactive sessions, and agent-shaped payloads.	`test/e2e/test_interactive_session.py`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

codecov · 2026-09-09T13:22:59Z

Codecov Report

❌ Patch coverage is 98.36066% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.86%. Comparing base (908a24e) to head (d318cad).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
rust/dl/src/launch.rs	96.55%	1 Missing ⚠️

Additional details and impacted files

Flag	Coverage Δ
python	`42.98% <ø> (ø)`
rust	`95.13% <98.36%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
shipped code (rust)	`95.13% <98.36%> (-0.02%)`	⬇️
harness and tooling (python)	`42.98% <ø> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

CI caught it: "A released section is not a place to file a new entry". main cut 0.35.0 while this branch was open, which renames the `[Unreleased]` heading the entry was anchored under, and my resolution of the merge conflict captured main's side down past the new `## [0.35.0]` heading -- so appending after it put the entry inside a release that never contained this fix. That is the exact hazard the guard was written for, walked into while resolving the conflict the release cut produced. Everything from `## [0.35.0]` down is now byte-identical to main again, and the entry sits in `[Unreleased]`.

The env value is spliced in bare now (`format!("{name}={value}")`), which is what keeps it an assignment prefix, and that is only true while dl's quoting leaves the whole word alone. A row given a value holding a space or a `$` gets quoted to `'NAME=a b'`, which bash reads as a program name: 127, variable never set. `shell::quote(value)` used to absorb that; nothing does now, and the enforcement lives in a different crate from the table. An empty `command` is the same class from the other side: `build_agent_command` would answer `Some(vec![])` and `build_dl_args` would emit `[<spec>, "--"]`, a separator with nothing after it, which dl reads as a plain interactive attach. An agent was asked for and a shell arrives. Neither is reachable from the table as it stands -- both values are `1` and every row has a command -- so the guard is proved by the construction it rejects rather than by a red run on this tree: `("IS_SANDBOX", "a b")` fails it on the quoting, `command: &[]` on the emptiness. Both were run. The typed fix is `Option<NonEmpty<String>>` and an `env` field kept out of the word list, but `NonEmpty` is not among dl's re-exports and aid may name nothing else, so that is a public API change and belongs with the `RemoteCommand::Argv | ::Script` follow-up this PR already escalates. Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW

"The four tests below are one failure mode each" sits above five tests, and only three of them are failure modes: a re-split argument, a truncating `#`, an executed `$(...)`. The other two are the plain command that must come through unquoted and the shell snippet spelled by naming a shell -- both properties of the fix rather than symptoms of the bug. Same class as 6797f4c, which exists because the comment it replaced miscounted the tests it cited. A count is the one thing a comment can get wrong without anything failing. Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW

The paragraph derives the exception correctly -- the quoting leaves a word alone when it needs none -- and then states the conclusion without the condition: "a leading `NAME=value` still reaches the shell as an assignment prefix". It is the whole word that has to need no quoting, so `dl <ws> -- FOO='a b' claude` becomes `'FOO=a b' claude`, and bash reads a quoted word as a program name: $ bash -lc "'FOO=a b' env" bash: line 1: FOO=a b: command not found 127, and the variable never set. Not a regression -- before the quoting fix that line set `FOO=a` and ran `b claude` -- but this diff is what writes the feature down as supported, so it is this diff's job to say where it stops. Names the surviving character set and gives the spelling that works for anything else. Verified against bash rather than reasoned about: quote removal happens after the assignment-prefix scan, so a quoted `NAME=` is never an assignment. Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW

The [Unreleased] section as it stands becomes the release: the `dl <ws> -- <command>` quoting fix, and the `.dockerignore` that stopped prose edits under `.devcontainer/` from throwing away the prebuilt container. **0.36.0 and not 0.35.1**, and the reason is one line of the changelog rather than the size of the diff. `dl <ws> -- <command>` is argv now, so a shell snippet passed as a single word is a program name with a space in it: `dl <ws> -- 'exit 7'` exits 127 where it used to exit 7, and `dl <ws> -- ""` went from 0 to 127. That is a behaviour change to a documented spelling, which the patch reading is not available for. No flag moves and `--help` is unchanged, so the surface is the same; what a caller passing a snippet gets back is not. The quoting fix is what the version is for. Words after `--` were rejoined with plain spaces into a `bash -lc` line, so the remote shell got back every separator the host's shell had consumed: quoted arguments re-split, a `#` truncated the line, and a `$(...)` ran in a workspace holding the forwarded GH_TOKEN. Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW

`changelog_frozen.py` compares head against the base branch, which works only while the bad entry is still on a branch. Once it merges, it *is* the base: every honest correction then changes a released section, the guard refuses all of them, and the false record becomes the only text CI will accept. That is the opposite of what the guard is for, and it is not hypothetical -- #591 re-cut a version #593 had already published, so its entry landed under the shipped `## [0.36.0]` heading from the one direction a base-versus-head comparison cannot see. A released section may now move in exactly one direction: back to `git show v<version>:CHANGELOG.md`, byte for byte. The tag is what makes that safe rather than a loophole -- it is written by the release, not by the branch asking to be let through, so agreement with it cannot be forged. Anything short of a confident answer from git (no tag, no git, an unparsable file at that tag) refuses, so the arm only ever permits a restoration it has positively proved. Three tests, and the first fails without the change: a restoration proved by its tag passes, an edit to anything else is still refused, and a restoration with no tag to prove it is refused rather than waved through. `ci.yml` fetches tags shallowly, since `actions/checkout` fetches none and the oracle is unreachable without them. Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW

`test_the_collision_that_happened_is_refused` and its neighbour replayed #591's real manifests by SHA, which fails everywhere but a full clone: `actions/checkout` fetches depth 1, so `git show 908a24e:rust/Cargo.toml` is `fatal: path exists on disk, but not in ...`. Reading the real commits was wrong for a second reason the failure did not show. The guard's oracle is `git rev-parse refs/tags/v<version>` in whatever directory it runs in, and those tests ran it in the checkout -- which has not fetched tags by the time pytest runs, since `pixi run ci` is the step before the one that fetches them. The collision test would have gone green by the guard finding no tag and permitting the bump, which is the opposite of what it asserts. Each test builds its own repository and tags it now. The numbers and the shape are #591's still; only the bytes are the test's own. The module docstring said the replay was the point, so it says the opposite now and why. Also stops `test_publish_decision.py` hardcoding `PATH` for the shell it runs: the script needs `git` and `sed`, and naming three directories is a test that passes here and fails on a runner that puts them elsewhere. Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW

0.36.0 was cut and published by blooop#593 while blooop#591 was in review, so blooop#591's own `release: 0.36.0` commit re-cut a version that already existed. The publish job did the right thing on merge -- `v0.36.0 is already tagged; nothing to publish` -- which left the `dl <ws> -- <command>` quoting fix merged to main and not in any released artifact. The v0.36.0 wheel, conda package and GitHub release are all built from 2f05c38 and do not contain it. What made that worse than a wasted version number is the changelog. blooop#591's entry merged in under the `## [0.36.0]` heading that blooop#593 had just created, with no conflict -- exactly the failure blooop#527 added `scripts/changelog_frozen.py` for, arriving from the one direction the guard cannot see, since the guard compares against a base that already has the entry misfiled. So main claimed 0.36.0 fixed the quoting and 0.36.0 does not. The entry moves into a new 0.37.0. The 0.36.0 section is now byte-identical to `git show v0.36.0:CHANGELOG.md`, which is the check that says this restores the record rather than editing it. **`changelog_frozen` is red on this branch, deliberately.** It sees 0.36.0 change and cannot tell a restoration from a corruption; its own message asks for this to be said out loud in the pull request rather than worked around. Nothing is skipped, loosened or ignored to get it green. Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW

blooop added 5 commits September 9, 2026 10:18

Merge remote-tracking branch 'origin/main' into fix/dl-run-argv-quoting

a6f095d

# Conflicts: # CHANGELOG.md

sourcery-ai Bot reviewed Sep 9, 2026

View reviewed changes

blooop and others added 5 commits September 9, 2026 14:23

blooop merged commit 84cfaf1 into main Sep 9, 2026
15 checks passed

blooop deleted the fix/dl-run-argv-quoting branch September 9, 2026 15:04

blooop mentioned this pull request Sep 9, 2026

release: 0.37.0, and move the argv entry out of the shipped 0.36.0 #594

Merged

blooop mentioned this pull request Sep 9, 2026

fix: a version collision published nothing and said nothing #595

Merged

blooop mentioned this pull request Sep 9, 2026

refactor: RemotePayload::wrap could not tell argv from a shell script #596

Merged

Navigation Menu

fix: dl -- <cmd> re-splits every quoted argument it is given - #591

fix: dl -- <cmd> re-splits every quoted argument it is given#591
blooop merged 10 commits into
mainfrom
fix/dl-run-argv-quoting

blooop commented Sep 9, 2026 •

edited by sourcery-ai Bot

Loading

Uh oh!

sourcery-ai Bot left a comment

Uh oh!

sourcery-ai Bot commented Sep 9, 2026

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

codecov Bot commented Sep 9, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

blooop commented Sep 9, 2026 • edited by sourcery-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was wrong

The fix

Behaviour change

Review

Verification

Summary by Sourcery

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot commented Sep 9, 2026

Reviewer's Guide

Sequence diagram for safe dl argv forwarding

Sequence diagram for aid forwarding agent argv

Flow diagram for documented dl shell snippet behavior

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

codecov Bot commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

blooop commented Sep 9, 2026 •

edited by sourcery-ai Bot

Loading

codecov Bot commented Sep 9, 2026 •

edited

Loading