sdk run: quote argv elements instead of joining them raw

[mobileoverlord]

What

sdk run splices the user's argv into a shell script inside the container using a bare cmd.join(" "), so the container shell re-splits it. This:

avocado sdk run -- bash -lc 'U=/opt/x; ls $U'

arrives in the container as:

bash -lc U=/opt/x; ls $U

The shell reads that as two commands — a throwaway bash -lc U=/opt/x, then ls $U where $U is expanded by the outer shell to nothing, i.e. plain ls.

It does not error. It prints plausible output for a different directory. I hit this while debugging an SDK sysroot and it produced two confidently wrong readings before I noticed — a debugging tool that lies is worse than one that fails.

Fix

Quote each argv element with the existing utils::runs_on::shell_escape (promoted to pub(crate) — no new helper). Extracted join_argv so this is unit-testable without spinning a container.

Behavior change

Argv is now treated as argv, matching docker run / kubectl exec convention. Anyone currently passing a single string of shell code:

avocado sdk run -- 'ls /foo && ls /bar'      # was: interpreted as shell

now gets that string as one command word, and should use the explicit form:

avocado sdk run -- bash -lc 'ls /foo && ls /bar'

which is what the flag combination already implies, and which only works correctly after this fix. Flagging it since it is user-visible.

Tests

Two unit tests in src/commands/sdk/run.rs — argv-boundary preservation (the exact bash -lc 'X=1; echo $X' case) and embedded-single-quote escaping. cargo test --bin avocado sdk::run — 11 passed. fmt and clippy clean.

Separate from #200; no overlap.

[Copilot]

Pull request overview

This PR fixes avocado sdk run incorrectly re-splitting user-provided argv inside the container by quoting each argv element before splicing it into the container’s bash -c script, aligning behavior with typical docker run / kubectl exec argv semantics.

Changes:

• Promotes utils::runs_on::shell_escape to pub(crate) so it can be reused for argv-safe quoting.
• Adds join_argv to shell-escape each argv element (instead of join(" ")) when building the container command string.
• Adds unit tests covering argv-boundary preservation and embedded single-quote escaping.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/utils/runs_on.rs	Makes shell_escape available within the crate to support safe argv quoting.
src/commands/sdk/run.rs	Uses per-argv-element shell escaping via join_argv and adds targeted unit tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jetm]

The fix is right and I verified the premise rather than taking it on faith: RunConfig.command really is spliced into bash -c at four sites in src/utils/container.rs (944/1153/1543/1841), and the --runs-on path does not double-escape, because build_docker_command already wraps the whole script in shell_escape. Both new tests assert the exact escaped strings rather than truthiness, and main.rs:3404 maps an empty argv to None, so join_argv never sees an empty slice. cargo fmt --all -- --check, cargo clippy --all-targets --all-features -- -D warnings and cargo test --lib join_argv all pass at e7806cb.

One thing I would add before merging.

This is a user-visible behavior change with no CHANGELOG entry. Anyone who was single-quoting a $VAR or a glob into sdk run and relying on the container shell to expand it loses that silently. I confirmed both halves in bash: bash -c "'bash' '-lc' 'X=1; echo $X'" now prints 1 (the fix), and bash -c "'echo' '\$HOME'" prints a literal $HOME (the loss). The repo keeps a populated Keep-a-Changelog Unreleased block, so there is a natural home for it — I would file it under Fixed with a line about the expansion change, since the people affected will not find it in the diff.

Three nits, all optional:

• src/commands/sdk/run.rs:275 — stale error text inside the function you touched: "You must either provide a --command (-c)". avocado sdk run --help shows no such flag; the command is a trailing positional and -c is unassigned (-C is --config). I checked with cargo run -- sdk run --help.
• src/commands/sdk/run.rs:22 — layering: a generic shell-quoting helper now comes from utils::runs_on, the remote-execution module, into a local non-remote path, and via an inline crate::utils::runs_on::shell_escape(..) rather than a use. utils::shell would be the honest home.
• The same untrusted-argv-into-bash -c splice is still raw in sdk/dnf.rs:115, ext/dnf.rs:358 and runtime/dnf.rs:303 (self.command.join(" ")). Out of scope here, but the class of bug is not closed by this PR.

[mobileoverlord]

All three taken, plus the fourth as a filed follow-up. Pushed in c785a69.

CHANGELOG. Added under Fixed, per your suggestion of where it belongs. I gave the loss its own paragraph rather than a clause, because your bash -c "'echo' '\$HOME'" check is exactly the thing nobody will connect to a sdk run release note six weeks from now — so the entry names both concrete forms that stop working ('ls /foo && ls /bar' as one arg, echo '$HOME') and the bash -lc form to move to.

Stale flag text. Fixed. Worth noting it was wrong in a second way beyond the flag not existing: the message told you to provide --command while the actual requirement is a trailing positional, so following it literally gets you error: unexpected argument '--command'. Now reads "You must either provide a command to run or use --interactive (-i)", and the test asserting on that string moved with it.

Layering. Agreed, and done — shell_escape now lives in src/utils/shell.rs with its four tests, and both consumers take it by use. Your framing is what convinced me: the direction of the dependency was backwards, a local non-remote path reaching into the remote-execution module for a generic helper. The follow-up below is the other half of the argument — three more non-remote callers want it, and each one reaching into runs_on would have been worse.

The dnf sites. Confirmed all three are the identical self.command.join(" ") shape, and filed as #207 rather than folded in here. Reasoning: the fix is now genuinely one line each, but it carries the same user-visible expansion loss this PR does, and making that call for sdk dnf / ext dnf / runtime dnf is a decision to take on its own terms rather than inherit from a sdk run fix. You are right that the class is not closed — #207 says so and names all three lines.

cargo fmt, clippy --all-targets --all-features -D warnings, 1392 lib tests and the 11 sdk::run tests are green at c785a69.

[jetm]

Re-reviewed the increment since e7806cb - the changelog/flag-text/shell_escape commit answering my last pass. The shell_escape move is byte-identical and every call site still resolves; the reworded flag text checks out against the actual flags. One blocking finding inline, on where the changelog files this change rather than on the code.

Six advisory notes were withheld rather than appended here, so the blocking one stays readable.

[jetm]

Re-reviewed the increment since c785a693 - one commit, 4d54dfa, CHANGELOG only. No source file moved, so the code carries over from the last pass.

Reclassifying this as Breaking is right, and the reason given is the one that matters: the old invocations do not fail, they run something else. A silent behaviour change is worse for an upgrader than a loud one, and Breaking is where they look.

Checked each claim in the new text against the code rather than against the old entry. The base had cmd.join(" ") at src/commands/sdk/run.rs:315; head routes through join_argv, which maps shell_escape over every element, and shell_escape wraps in single quotes with '\'' for embedded quotes. So a $HOME, a && chain and a quoted glob all arrive as one literal word, which is what the entry says. The bash -lc 'ls /foo && ls /bar' recommendation holds too - under the old join that became bash -lc ls /foo && ls /bar, so "only works correctly after this change" is accurate rather than promotional.

Nothing was lost moving it. Every example, the U=/opt/x walkthrough and the docker run / kubectl exec comparison all survive in the new shape, and the section order under Unreleased reads Added / Changed / Breaking / Fixed with no duplicated heading. CI is green on 4d54dfa including the test suite.

No findings.

[jetm]

Re-review after the head moved. Read the diff and every enclosing function, traced RunConfig.command through all four full_command sinks, ran the escape/sdk::run test suites and clippy, and reproduced the CHANGELOG's own worked example against both code paths. Two findings on the CHANGELOG entry itself, inline below - the code change is sound (escaping is complete POSIX single-quoting, verified against 16 payloads and 6 injection attempts, remote double-escape composes correctly).

[jetm]

Published HIL doc teaches the pattern this breaks. docs/src/docs-guides/hardware-in-the-loop.md:140-146 (peridio/docs) instructs avocado sdk run cd /opt/_avocado \&\& mkdir -p ./qemux86-64/extensions/my-app/usr \&\& echo "hello from host" \> ./qemux86-64/extensions/my-app/usr/hello.txt, followed by an "Escaping SDK run commands" callout stating that && and > are escaped precisely so they "reach the container's shell unchanged." That is exactly the contract this PR retires, and the vendor's own guide isn't in the audit list this entry asks users to check.

Replayed the documented argv against both code paths:

• Before: cd <dir> && mkdir -p ./ext/usr && echo hello from host > ./ext/usr/hello.txt -> exit 0, file created.
• After: each argv element quoted separately -> bash: line 1: cd: too many arguments, exit 2, nothing created.

The echo/redirect half in isolation is worse: it exits 0, prints hello from host > ./out.txt to stdout, and writes nothing - so someone following this guide sees a success and a missing file. Worth a companion docs fix landing with this change, not after.

[jetm]

"no error, no warning" doesn't hold for && chains - and the entry's own first example two sentences up falsifies it.

$ bash -c "'ls /foo && ls /bar'"
bash: line 1: ls /foo && ls /bar: No such file or directory

That's the exact avocado sdk run -- 'ls /foo && ls /bar' example from line 58 above, reproduced as what join_argv actually sends the container shell (a single element, shell_escaped into one quoted word since it's already one argv element). It exits 127 with a clear error, not silently. The cd recipe in the first comment on this PR exits 2, also loudly.

Only the expansion cases (echo '$HOME') are genuinely silent - the &&/> chain cases fail loudly. Recommend splitting the claim: expansion breaks silently, but a &&/|/>/; chain passed as one argument now hard-fails at runtime, which is actually the easier case to catch in CI, not a risk to bury under "no error, no warning."

Separately, the audit list ($VAR, a glob, a && chain) omits the assignment-prefix form, which bash only honors on a fully unquoted word: avocado sdk run -- CC=clang make now breaks silently (the one real case) while matching none of the three things this paragraph tells the reader to grep for.