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]

User-visible CLI break filed under Fixed, not Breaking

This entry's own bold sentence concedes it "removes an expansion that some invocations relied on", yet it sits under ### Fixed. Meanwhile the ### Breaking section in this same ## [Unreleased] block (line 44) holds only a change the text itself scopes to "consumers of the avocado_cli lib target".

Failure path: a maintainer bumps the pinned avocado CLI version in CI and reads only ### Breaking for upgrade impact. It says lib-target consumers only, so they ship. A job running avocado sdk run -- '$BUILD_DIR/x' now passes a literal $BUILD_DIR instead of the expansion - no compile error, no warning, and the pre-change behaviour was already wrong-but-plausible output rather than a hard failure.

The asymmetry is what makes this worth changing: the entry that is filed under Breaking would have failed loudly as a compile error, while this one fails silently at runtime. Swapping which section each sits in would match the actual blast radius.