diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d6682dd..f4418ffa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.36.0] - 2026-09-09 + ### Fixed - **Editing prose under `.devcontainer/` no longer throws away the prebuilt @@ -41,6 +43,36 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 context, so this commit moves the tag once; launches build locally until `devcontainer-prebuild.yml` republishes on `main`. +- **`dl -- ` no longer re-splits a quoted argument, and no longer + runs one as shell.** The words after `--` were rejoined with plain spaces and + handed to `bash -lc` as a command line, so every space the host's shell had + already consumed became a separator again. `dl -- claude 'fix the bug'` + arrived as four arguments where one was meant. Each word is quoted now + (`shell::join`), so the remote argv is the argv that was typed. + + Two consequences beyond the splitting, both of them silent. A word holding `#` + commented out the rest of the line: a supervisor sending + `claude 'Address the open review on PR #10848 (...)'` reached the agent as the + single word `Address`, and everything after the `#` -- the whole prompt, + including every rule it carried -- was discarded by the remote shell before + `claude` ran. And a word holding `$(...)` or a backtick was *executed*, in a + workspace that has the forwarded `GH_TOKEN`, which made any text flowing into + a `dl --` command line (a PR title, a review body) shell code. + + `aid` composed its own line and passed it as one word, which survived only + because the rejoin was an identity on a single argument. It hands dl argv now + and quotes nothing itself. The composed payload is byte for byte what it was: + a bare `NAME=value` needs no quoting, so the assignment-prefix spelling the + README documents still reaches the shell as one. + + A shell snippet is still asked for by naming a shell -- `dl -- bash -lc + 'a && b'` -- and that spelling was broken before this too, running `bash -lc a` + and then `b`. Passing a snippet as a single word no longer works, because a + single word is now a program name: `dl -- 'exit 7'` looks for a program + called `exit 7` and exits 127 where it used to exit 7. The e2e probes were the + only callers in the tree spelling it that way, and they name `bash -lc` now. + `dl -- ""` went from exit 0 to 127 for the same reason. + ## [0.35.0] - 2026-09-09 ### Added diff --git a/README.md b/README.md index a932efb6..fe5268ff 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ one argument instead of a clone, a config file and a build command. [![GitHub pull-requests merged](https://badgen.net/github/merged-prs/blooop/devlaunch)](https://github.com/blooop/devlaunch/pulls?q=is%3Amerged) [![GitHub release](https://img.shields.io/github/release/blooop/devlaunch.svg)](https://GitHub.com/blooop/devlaunch/releases/) [![PyPI](https://img.shields.io/pypi/v/devlaunch)](https://pypi.org/project/devlaunch/) -[![Conda](https://img.shields.io/badge/conda-v0.35.0-brightgreen?logo=anaconda)](https://prefix.dev/channels/blooop/packages/devlaunch) +[![Conda](https://img.shields.io/badge/conda-v0.36.0-brightgreen?logo=anaconda)](https://prefix.dev/channels/blooop/packages/devlaunch) [![License](https://img.shields.io/github/license/blooop/devlaunch)](https://opensource.org/license/mit/) [![Platform](https://img.shields.io/badge/platform-linux--64-blue)](https://github.com/blooop/devlaunch/releases) [![Pixi Badge](https://img.shields.io/endpoint?url=https://raw.githubusercontent.com/prefix-dev/pixi/main/assets/badge/v0.json)](https://pixi.sh) @@ -276,7 +276,7 @@ clone, and [docs/cleanup.md](docs/cleanup.md) says what it carries one past and ```bash $ dl --version -dl 0.35.0 +dl 0.36.0 ``` `--devcontainer ` picks a non-default `devcontainer.json`. A bare name means diff --git a/docs/cli.md b/docs/cli.md index bd73dc68..fb7b45fe 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -100,6 +100,37 @@ that clone last fetched, the attach says how far behind before it hands over the shell. [How fresh a launch is](workspaces.md#how-fresh-a-launch-is) is the whole of the freshness rules, and the section under it names which verb moves what. +## What `--` takes + +The command and its arguments, one word each. `dl` quotes every word on the way +into the remote payload, so a quoted argument stays one argument: `dl -- +claude 'fix the bug'` runs `claude` with a single argument, and a word holding a +space, a `#`, a `$(...)` or a backtick is that word rather than shell syntax. + +It has to be built that way because the payload is one `bash -lc ` for both +transports. The words used to be rejoined with plain spaces, which gave the remote +shell back every separator your own shell had already consumed: quoted arguments +were re-split, a `#` commented out the rest of the line, and a `$(...)` ran. + +One exception, and it is deliberate. The quoting leaves a word alone when it needs +none, and `=` counts as needing none, so a leading `NAME=value` still reaches the +shell as an assignment prefix and sets that variable for that command only. That +is what makes `dl -- IS_SANDBOX=1 claude ...` work, which is the spelling +`aid` uses and the one the README shows. + +The exception ends where the quoting begins, and it ends abruptly. It is the +whole word that has to need no quoting, value included, so `FOO=bar` is an +assignment and `FOO='a b'` is not: the value's space makes the word +`'FOO=a b'`, and a shell reads a quoted word as a program name, so the command +exits 127 with the variable never set. Values made of `[A-Za-z0-9_@%+=:,./-]` +are the ones that survive. For anything else, name the shell and write the +assignment inside it: `dl -- bash -lc 'FOO="a b" cmd'`. + +A shell snippet is a command like any other, so name the shell: `dl -- bash +-lc 'a && b'`. Redirections and pipes typed on your own command line belong to +your own shell and never reach `dl`, which is what makes `dl -- ls > +files.txt` write the file here. + ## Commands that need a terminal `dl -- ` gives the command a terminal whenever `dl` itself has one, diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 00c90b5c..1067898d 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -13,7 +13,7 @@ dependencies = [ [[package]] name = "aid" -version = "0.35.0" +version = "0.36.0" dependencies = [ "devlaunch-test-support", "dl", @@ -437,7 +437,7 @@ dependencies = [ [[package]] name = "devlaunch-core" -version = "0.35.0" +version = "0.36.0" dependencies = [ "devlaunch-runner", "devlaunch-test-support", @@ -455,7 +455,7 @@ dependencies = [ [[package]] name = "devlaunch-runner" -version = "0.35.0" +version = "0.36.0" dependencies = [ "libc", "portable-pty", @@ -464,7 +464,7 @@ dependencies = [ [[package]] name = "devlaunch-test-support" -version = "0.35.0" +version = "0.36.0" dependencies = [ "devlaunch-runner", "serde", @@ -506,7 +506,7 @@ dependencies = [ [[package]] name = "dl" -version = "0.35.0" +version = "0.36.0" dependencies = [ "clap", "devlaunch-core", diff --git a/rust/Cargo.toml b/rust/Cargo.toml index d40c9439..5659fcf4 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -11,7 +11,7 @@ members = [ # The single source of the version (docs/rust-rewrite-plan.md: cutover ships # 0.1.0, version read from Cargo.toml). [workspace.package] -version = "0.35.0" +version = "0.36.0" edition = "2024" license = "MIT" repository = "https://github.com/blooop/devlaunch" diff --git a/rust/aid/src/rewrite.rs b/rust/aid/src/rewrite.rs index 2b580ff5..06656474 100644 --- a/rust/aid/src/rewrite.rs +++ b/rust/aid/src/rewrite.rs @@ -6,8 +6,6 @@ //! containers itself is an `aid` that builds one `dl` would have reused, which is //! the drift `aid.py` was rewritten to end. -use dl::shell; - /// How one coding agent is started inside the workspace. /// /// Split three ways because not every part of the line belongs everywhere: `env` @@ -712,11 +710,18 @@ fn agent_flag(word: &str) -> Option<&'static str> { .find(|name| *name == named) } -/// The shell command that starts the agent inside the workspace. +/// The argv that starts the agent inside the workspace. +/// +/// Words, not a shell string, because dl's `-- ` form is argv: it quotes +/// what it is given (`shell::join`) on the way into the remote payload, so a caller +/// that pre-composed a line would have that line quoted as one command name. It used +/// to be one string, back when dl rejoined the tail with plain spaces and a +/// pre-quoted line survived by accident — the same accident that re-split everybody +/// else's quoted argument. /// -/// One shell string, because that is what dl's `-- ` form takes. The prompt -/// is quoted here rather than reassembled by the caller, so the words the user typed -/// reach the agent as the single argument they meant. +/// The composed payload is unchanged by all of that: every word this returns is +/// either already shell-safe or gets the same quoting aid used to apply itself, so +/// what reaches the remote shell is byte for byte what it was. /// /// `None` is an agent this build has no entry for, which only a caller inventing a /// name can produce — [`parse_aid_args`] answers with a name from the table. @@ -732,7 +737,7 @@ pub(crate) fn build_agent_command( agent: &str, prompt: &str, remote_control: Option<&str>, -) -> Option { +) -> Option> { let (_, started) = AGENTS.iter().find(|(name, _)| *name == agent)?; // No prompt to be interactive about: start the agent's plain session, without // the flags that only make sense alongside one. @@ -750,21 +755,25 @@ pub(crate) fn build_agent_command( } // Assignments prefixing a command set the variables for that command only, so // the agent is the one process that sees them and nothing in the login shell dl - // runs this under is changed. + // runs this under is changed. Still spelled as an assignment rather than as + // `env NAME=value`: `=` is in dl's shell-safe set, so a bare `NAME=value` word + // reaches the remote shell unquoted and keeps that meaning, and the payload + // stays the one README documents. let mut line: Vec = started .env .iter() - .map(|(name, value)| format!("{name}={}", shell::quote(value))) + .map(|(name, value)| format!("{name}={value}")) .collect(); - line.push(shell::join(words)); - Some(line.join(" ")) + line.extend(words.into_iter().map(str::to_owned)); + Some(line) } /// The dl command line that does the work. /// -/// `[
…, , "--", ]` — the shape `dl` reads back by -/// joining everything after `--` with spaces, which is why the agent command is one -/// argument and its quoting lives inside it. +/// `[
…, , "--", …]` — the agent's command and each of +/// its arguments as their own word, which is the shape `dl` reads: it quotes the tail +/// itself on the way into the remote payload, so the prompt stays one argument +/// without aid quoting anything. /// /// `--rm` lands between the spec and the `--`, so the agent still gets its prompt and /// dl still gets the flag: `[…, , "--rm", "--", ]`. @@ -793,7 +802,7 @@ pub(crate) fn build_dl_args(parsed: &AidArgs) -> Option> { RemoteControl::On => Some(parsed.spec.as_str()), RemoteControl::Off => None, }; - args.push(build_agent_command(agent, prompt, session)?); + args.extend(build_agent_command(agent, prompt, session)?); } Task::Retired => {} } @@ -1027,6 +1036,43 @@ mod tests { // --------------------------------------------- the agent's command + #[test] + fn every_agent_in_the_table_composes_into_argv_dl_can_carry() { + // Two invariants the table has to hold that its type does not, both of them + // silent at the point an entry is edited rather than here. + // + // A value is spliced in bare (`NAME=value`) and stays an assignment prefix + // only while dl's quoting leaves the whole word alone. Give one a space or a + // `$` and dl quotes it to `'NAME=a b'`, which bash reads as a program name: + // the launch dies at 127 with the variable never set. Quoting it here first + // does not help, because dl would then quote the quotes. + // + // An empty `command` would make `build_agent_command` answer `Some(vec![])`, + // and `build_dl_args` would emit `[, "--"]` -- a separator with nothing + // after it, which dl reads as a plain interactive attach. An agent was asked + // for and a shell would arrive. + for (agent, started) in AGENTS { + assert!( + !started.command.is_empty(), + "{agent} has no command, so its `--` tail would be empty" + ); + for (name, value) in started.env { + let word = format!("{name}={value}"); + assert_eq!( + dl::shell::quote(&word), + word, + "{agent}'s {name} needs quoting, so it would reach the remote \ + shell as a command name rather than an assignment" + ); + } + } + } + + /// The agent's argv, for a name the table has. + fn agent_argv(agent: &str, prompt: &str, remote_control: Option<&str>) -> Vec { + build_agent_command(agent, prompt, remote_control).expect("a known agent") + } + #[test] fn claude_is_started_sandboxed_with_and_without_a_prompt() { // The flag alone is not enough, and the gap is silent: claude exits 1 with @@ -1034,39 +1080,44 @@ mod tests { // --dangerously-skip-permissions under uid 0, and a devcontainer running as // root is ordinary. assert_eq!( - build_agent_command("claude", "fix the bug", None).as_deref(), - Some( - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions 'fix the bug'" - ) + agent_argv("claude", "fix the bug", None), + [ + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "fix the bug", + ] ); assert_eq!( - build_agent_command("claude", "", None).as_deref(), - Some( - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions" - ) + agent_argv("claude", "", None), + [ + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + ] ); } #[test] fn an_agent_that_needs_no_variable_gets_none() { for agent in ["codex", "gemini"] { - let command = build_agent_command(agent, "hi", None).expect("a known agent"); - assert!(!command.contains("IS_SANDBOX"), "{command}"); + let command = agent_argv(agent, "hi", None); + assert!( + !command.iter().any(|word| word.contains("IS_SANDBOX")), + "{command:?}" + ); } } #[test] fn gemini_gets_its_interactive_flag_only_beside_a_prompt() { assert_eq!( - build_agent_command("gemini", "hi", None).as_deref(), - Some("gemini --yolo --prompt-interactive hi") - ); - assert_eq!( - build_agent_command("gemini", "", None).as_deref(), - Some("gemini --yolo") + agent_argv("gemini", "hi", None), + ["gemini", "--yolo", "--prompt-interactive", "hi"] ); + assert_eq!(agent_argv("gemini", "", None), ["gemini", "--yolo"]); } #[test] @@ -1104,12 +1155,13 @@ mod tests { // `aid/tests/rewrite.rs` records: every truncation of a flag is a // substring of it, and so is every flag that merely starts with one, so // a table entry reading `--yolo-dry-run` satisfied a `contains("--yolo")` - // while asking gemini for the opposite of full auto. + // while asking gemini for the opposite of full auto. The words are argv + // now, so this is a comparison per word rather than a split of a line. for prompt in ["hi", ""] { - let command = build_agent_command(agent, prompt, None).expect("a known agent"); + let command = agent_argv(agent, prompt, None); assert!( - command.split_whitespace().any(|word| word == flag), - "{agent} is not in full auto (prompt {prompt:?}): {command}" + command.iter().any(|word| word == flag), + "{agent} is not in full auto (prompt {prompt:?}): {command:?}" ); } } @@ -1117,21 +1169,29 @@ mod tests { #[test] fn a_prompt_is_one_argument_however_it_is_spelled() { - // Python's `shlex.quote` spelling, byte for byte: the payload travels in - // argv, and a second command cannot be smuggled into it. + // The prompt is one word here and stays one word through dl, which quotes + // every word it is handed. Nothing is quoted at this layer any more, so what + // this asserts is that no layer *splits* it: a second command cannot be + // smuggled in, because `;` never reaches a shell as syntax. assert_eq!( - build_agent_command("claude", "don't break \"this\"", None).as_deref(), - Some( - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions 'don'\"'\"'t break \"this\"'" - ) + agent_argv("claude", "don't break \"this\"", None), + [ + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "don't break \"this\"", + ] ); assert_eq!( - build_agent_command("claude", "hi; rm -rf /", None).as_deref(), - Some( - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions 'hi; rm -rf /'" - ) + agent_argv("claude", "hi; rm -rf /", None), + [ + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "hi; rm -rf /", + ] ); } @@ -1149,8 +1209,12 @@ mod tests { [ "owner/repo@branch", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo@branch 'fix it'", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo@branch", + "fix it", ] ); assert_eq!( @@ -1161,17 +1225,23 @@ mod tests { "robot", "owner/repo", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo", ] ); } #[test] - fn dl_reads_the_command_back_whole() { - // The prompt survives dl's own parsing of `-- `: dl joins - // everything after `--` with spaces, so the quoting aid applies has to live - // inside a single argument rather than be spread across several. + fn the_tail_after_the_separator_is_argv_and_the_prompt_is_one_word_of_it() { + // What this used to assert was the bug: dl rejoined the tail with plain + // spaces, so aid kept the quoting inside one argument and everybody else's + // quoted argument was re-split. The tail is argv now, and the + // property worth holding is that the prompt is *one* word of it however + // many words were typed -- dl quotes each one on the way to the remote + // shell, so a word that survives here survives all the way. let args = build_dl_args(&parsed(&["owner/repo", "fix", "the", "flaky", "test"])) .expect("a known agent"); let after = args @@ -1180,10 +1250,15 @@ mod tests { .expect("the -- separator"); assert_eq!( - args[after + 1..].join(" "), - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo \ - 'fix the flaky test'" + args[after + 1..], + [ + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo", + "fix the flaky test", + ] ); } @@ -1229,8 +1304,12 @@ mod tests { "owner/repo", "--rm", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo 'fix it'" + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo", + "fix it", ] ); } @@ -1451,9 +1530,12 @@ mod tests { [ "owner/repo@branch", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo@branch \ - 'fix the flaky test'", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo@branch", + "fix the flaky test", ] ); // And with no prompt, which is the launch this most often is: the flag is @@ -1463,8 +1545,11 @@ mod tests { [ "owner/repo@branch", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo@branch", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo@branch", ] ); } @@ -1484,12 +1569,21 @@ mod tests { [ "owner/repo", "--", - "codex --dangerously-bypass-approvals-and-sandbox hi" + "codex", + "--dangerously-bypass-approvals-and-sandbox", + "hi", ] ); assert_eq!( build_dl_args(&parsed(&["--gemini", "owner/repo", "hi"])).expect("a known agent"), - ["owner/repo", "--", "gemini --yolo --prompt-interactive hi"] + [ + "owner/repo", + "--", + "gemini", + "--yolo", + "--prompt-interactive", + "hi", + ] ); // And the same through the variable, which is how somebody who set it once // launches every line. @@ -1561,8 +1655,11 @@ mod tests { [ "owner/repo@fix/x", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions 'fix it'", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "fix it", ], "{flag}" ); @@ -1650,22 +1747,29 @@ mod tests { #[test] fn a_session_name_that_needs_quoting_is_still_one_word() { - // The name travels through the same `shlex.quote` the prompt does, and the - // whole `--flag=` is what gets quoted — a name broken across two words - // would leave claude reading the rest of the line as its own arguments. + // The whole `--flag=` is one word, which is what keeps a name holding + // a space from leaving claude reading the rest of the line as its own + // arguments. dl quotes it from here. assert_eq!( - build_agent_command("claude", "", Some("./my project")).as_deref(), - Some( - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions '--remote-control=./my project'" - ) + agent_argv("claude", "", Some("./my project")), + [ + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=./my project", + ] ); assert_eq!( - build_agent_command("claude", "hi", Some("owner/repo@it's-mine")).as_deref(), - Some( - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions '--remote-control=owner/repo@it'\"'\"'s-mine' hi" - ) + agent_argv("claude", "hi", Some("owner/repo@it's-mine")), + [ + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo@it's-mine", + "hi", + ] ); } @@ -1874,8 +1978,12 @@ mod tests { "owner/repo", "--rm", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo 'fix the bug'" + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo", + "fix the bug", ] ); } @@ -1891,7 +1999,10 @@ mod tests { [ "owner/repo", "--", - "gemini --yolo --prompt-interactive 'explain this'" + "gemini", + "--yolo", + "--prompt-interactive", + "explain this", ] ); } @@ -1910,8 +2021,12 @@ mod tests { [ "owner/repo@fix/x", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo@fix/x 'fix the bug'", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo@fix/x", + "fix the bug", ] ); @@ -1924,8 +2039,11 @@ mod tests { [ "owner/repo@fix/x", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions 'fix it'", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "fix it", ] ); } @@ -1939,8 +2057,11 @@ mod tests { [ "owner/repo", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo" + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo", ] ); } diff --git a/rust/aid/tests/rewrite.rs b/rust/aid/tests/rewrite.rs index 406cc8c5..ae90911f 100644 --- a/rust/aid/tests/rewrite.rs +++ b/rust/aid/tests/rewrite.rs @@ -273,6 +273,10 @@ fn a_prompt_reaches_the_agent_as_one_argument_through_dls_own_launch() { // dl's own launch of the workspace, and one `devpod ssh --command` carrying the // agent. Byte for byte Python's, quoting included — the payload travels in argv. // + // The echoed `aid -> dl` line shows the tail as the words it is, because that is + // what aid now hands dl; the `--command` below is unchanged, since dl puts back + // the same quoting aid used to apply itself. + // // Remote Control rides along with nothing typed, which is what it is now: the // default. The name is the spec, which here is the workspace the line named. let world = World::with(&["--warm"]); @@ -281,9 +285,9 @@ fn a_prompt_reaches_the_agent_as_one_argument_through_dls_own_launch() { assert_eq!( run.err.lines().collect::>(), [ - "aid -> dl devlaunch-main-3j1t -- 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 \ + "aid -> dl devlaunch-main-3j1t -- CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 \ IS_SANDBOX=1 claude --dangerously-skip-permissions \ - --remote-control=devlaunch-main-3j1t '\"'\"'fix the bug'\"'\"''", + --remote-control=devlaunch-main-3j1t 'fix the bug'", "Workspace devlaunch-main-3j1t is already running, attaching...", "SSH command: devpod ssh devlaunch-main-3j1t --command bash -lc \ 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ diff --git a/rust/dl/src/launch.rs b/rust/dl/src/launch.rs index d1e74ac2..a172aef8 100644 --- a/rust/dl/src/launch.rs +++ b/rust/dl/src/launch.rs @@ -42,6 +42,7 @@ use devlaunch_core::flows::launch::{ }; use devlaunch_core::flows::lifecycle::Refresh; use devlaunch_core::flows::listing::CommandContext; +use devlaunch_core::shell; use crate::cli::{RmOnExit, Verb}; use crate::commands::Ending; @@ -101,19 +102,23 @@ pub(crate) fn family(verb: &Verb) -> Family { // ending rather than to this pass over one workspace. Verb::Remove { force, after: _ } => return Family::Remove { force: *force }, Verb::Attach { rm } => (LaunchVerb::Attach { command: None }, *rm), - // Python's `" ".join(args[2:])`: the words are rejoined with single spaces - // and the result is one shell command, quoted whole into the remote - // payload. A word that needed quoting to survive the *host's* shell has - // already been unquoted by it, so the join is what the user typed. + // Re-quoted, not just rejoined. [`RemotePayload::wrap`] quotes this + // string whole into `bash -lc ''`, so what is built here is a + // command line the *remote* shell parses -- and the words arriving here + // have already had their quoting removed by the *host's* shell. Joining + // them on spaces gave the remote shell every one of those separators + // back. The three failure modes that produced -- a re-split argument, a + // truncating `#`, an executed `$(...)` -- are a test each below, beside + // the plain command that must stay unquoted and the shell snippet that is + // now spelled by naming a shell. + // + // A bare `NAME=value` survives `shell::join` unquoted, because `=` is in + // the shell-safe set. That is not an oversight to tidy: it is what keeps + // `dl -- IS_SANDBOX=1 claude ...` setting a variable, which is the + // spelling `aid` builds and the README documents. Verb::Run(words, rm) => ( LaunchVerb::Attach { - command: Some( - words - .iter() - .map(String::as_str) - .collect::>() - .join(" "), - ), + command: Some(shell::join(words.iter().map(String::as_str))), }, *rm, ), @@ -323,3 +328,81 @@ fn ran(outcome: Result, cache: &Path) -> Ran { } } } + +#[cfg(test)] +mod tests { + use devlaunch_core::domain::workspace_state::NonEmpty; + use devlaunch_core::flows::launch::LaunchVerb; + + use super::{Family, family}; + use crate::cli::{RmOnExit, Verb}; + + /// The command `dl -- ` would hand the remote shell. + fn run_command(words: &[&str]) -> String { + let verb = Verb::Run( + NonEmpty::of(words.iter().map(|word| (*word).to_owned())).expect("a command"), + RmOnExit::No, + ); + match family(&verb) { + Family::Launch { + verb: + LaunchVerb::Attach { + command: Some(command), + }, + .. + } => command, + _ => panic!("`-- ` is a launch that attaches with a command"), + } + } + + #[test] + fn a_plain_command_is_unchanged() { + // The common case has nothing to quote, and quoting it anyway would put + // `'make' 'test'` in front of every reader of a `--command` line. + assert_eq!(run_command(&["make", "test"]), "make test"); + } + + #[test] + fn a_word_with_spaces_stays_one_word() { + // The host's shell had already removed the quotes from `dl -- + // claude 'fix the bug'`, so rejoining on spaces handed the remote shell + // four words and the agent was prompted with `fix`. + assert_eq!( + run_command(&["claude", "fix the bug"]), + r#"claude 'fix the bug'"# + ); + } + + #[test] + fn a_word_holding_a_comment_does_not_swallow_the_rest() { + // The failure this was found through: `#10848` began a comment, so everything + // after it -- including every hard rule the prompt carried -- was + // discarded by the remote shell before the command ran. + assert_eq!( + run_command(&["claude", "review PR #10848 now"]), + r#"claude 'review PR #10848 now'"# + ); + } + + #[test] + fn a_word_holding_a_substitution_is_not_executed() { + // Command substitution in a prompt is not hypothetical: PR titles and + // review bodies reach `dl -- claude ` from a supervisor, and the + // remote shell has the forwarded token. + assert_eq!( + run_command(&["echo", "uid=$(id -u)"]), + r#"echo 'uid=$(id -u)'"# + ); + } + + #[test] + fn a_shell_snippet_is_still_writable_the_documented_way() { + // `-- ` is argv, so a snippet is asked for by naming a shell. + // That spelling was broken before the quoting too: `bash -lc a && b` + // ran `bash -lc a` and then `b`. + assert_eq!( + run_command(&["bash", "-lc", "a && b"]), + r#"bash -lc 'a && b'"# + ); + } +} diff --git a/rust/dl/tests/launch.rs b/rust/dl/tests/launch.rs index 05fae6ca..766f2725 100644 --- a/rust/dl/tests/launch.rs +++ b/rust/dl/tests/launch.rs @@ -532,13 +532,20 @@ fn a_quoted_prompt_reaches_the_agent_intact() { // always single-quotes and escapes each `'` as `'"'"'`, where the `shlex` crate // would switch to double quotes for the same word. Both are the same word to a // POSIX shell and only one of them is the same bytes. + // + // What this asserted until the quoting fix was the opposite of its own name: the + // read `bash -lc 'claude it'"'"'s here'`, which is `claude it's here` to the + // remote shell -- two arguments, and the prompt was never intact at all. The + // apostrophe made it look right, because escaping it is the visible half of + // quoting and keeping the word whole is the half that was missing. let world = World::with(&["--warm"]); let run = world.dl(&[MAIN, "--", "claude", "it's here"]); run.exited(0); assert_eq!( world.calls().exact(&world.root).last(), Some(&format!( - "devpod ssh {MAIN} --command bash -lc 'claude it'\"'\"'s here'" + "devpod ssh {MAIN} --command bash -lc \ + 'claude '\"'\"'it'\"'\"'\"'\"'\"'\"'\"'\"'s here'\"'\"''" )) ); } @@ -557,7 +564,7 @@ fn the_zellij_opt_in_puts_a_session_beside_the_command() { run.stderr_lines()[1], format!( "SSH command: devpod ssh {MAIN} --command bash -lc 'zellij attach -b devlaunch \ - >/dev/null 2>&1 || true; claude fix it'" + >/dev/null 2>&1 || true; claude '\"'\"'fix it'\"'\"''" ) ); } diff --git a/test/e2e/test_interactive_session.py b/test/e2e/test_interactive_session.py index 81091a85..5b9c8ddd 100644 --- a/test/e2e/test_interactive_session.py +++ b/test/e2e/test_interactive_session.py @@ -58,6 +58,13 @@ # test's assertions depend on across the suite. WORKSPACE_ID = "e2e-test-interactive" +# Every probe below is a shell script, so each is run as one: `dl -- ` +# is the command and its arguments, one word each, and dl quotes every word on +# the way to the workspace. Passing a script as a single word used to work by +# accident, back when the tail was rejoined with plain spaces and handed to a +# shell whole; it now names a program with a space in it. `bash -lc