From 145e6b4ad51e84da4859f82207b4a74457ec976f Mon Sep 17 00:00:00 2001 From: blooop Date: Wed, 9 Sep 2026 10:18:09 +0100 Subject: [PATCH 1/9] fix: dl -- re-splits every quoted argument it is given 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 -- 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. --- CHANGELOG.md | 29 ++++ docs/cli.md | 19 +++ rust/aid/src/rewrite.rs | 310 +++++++++++++++++++++++----------- rust/aid/tests/interactive.rs | 14 +- rust/aid/tests/rewrite.rs | 30 +--- rust/dl/src/launch.rs | 102 +++++++++-- rust/dl/tests/launch.rs | 11 +- 7 files changed, 370 insertions(+), 145 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d750973..74137a5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,35 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **`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 its variables are set with `env(1)` rather than the shell's + assignment-prefix syntax, so the payload is spelled the same way it is meant: + as arguments. The visible difference is one word in the `--command` devpod is + given, `bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 ...'`. + + 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`. + ## [0.34.0] - 2026-09-09 ### Fixed diff --git a/docs/cli.md b/docs/cli.md index bd73dc68..e8e3dd6d 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -100,6 +100,25 @@ 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 + +Argv. The words after `--` are the command and its arguments, one word each, and +`dl` quotes every one of them 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 reads that way because the payload is one `bash -lc ` for both +transports, and the line has to be built rather than assumed. Before 0.34.1 the +words were rejoined with plain spaces, which gave the remote shell back every +separator the host's shell had already consumed. Quoted arguments were re-split, +a `#` commented out the rest of the line, and a `$(...)` ran. + +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/aid/src/rewrite.rs b/rust/aid/src/rewrite.rs index 2b580ff5..0146f285 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,19 @@ 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 (#588). /// -/// 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 agent's variables are set with `env(1)` rather than the shell's +/// assignment-prefix syntax, which is the same "this process only" guarantee spelled +/// in argv instead of in shell. Nothing in the login shell dl runs this under is +/// changed either way. /// /// `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 +738,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. @@ -748,23 +754,30 @@ pub(crate) fn build_agent_command( words.extend(started.prompt_flags.iter().copied()); words.push(prompt); } - // 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. - let mut line: Vec = started - .env - .iter() - .map(|(name, value)| format!("{name}={}", shell::quote(value))) - .collect(); - line.push(shell::join(words)); - Some(line.join(" ")) + // `env NAME=value … cmd` sets the variables for that command only, so the agent + // is the one process that sees them. Spelled as argv rather than as the shell's + // assignment prefix because this is argv now: the values need no quoting here, + // since dl quotes every word it is handed. + let mut line: Vec = Vec::new(); + if !started.env.is_empty() { + line.push("env".to_owned()); + line.extend( + started + .env + .iter() + .map(|(name, value)| format!("{name}={value}")), + ); + } + 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 +806,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 +1040,11 @@ mod tests { // --------------------------------------------- the agent's command + /// 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 +1052,46 @@ 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), + [ + "env", + "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), + [ + "env", + "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 +1129,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 +1143,31 @@ 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), + [ + "env", + "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), + [ + "env", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "hi; rm -rf /", + ] ); } @@ -1149,8 +1185,13 @@ 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'", + "env", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo@branch", + "fix it", ] ); assert_eq!( @@ -1161,17 +1202,24 @@ mod tests { "robot", "owner/repo", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo", + "env", + "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 (#588). 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 +1228,16 @@ 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..], + [ + "env", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo", + "fix the flaky test", + ] ); } @@ -1229,8 +1283,13 @@ mod tests { "owner/repo", "--rm", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo 'fix it'" + "env", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo", + "fix it", ] ); } @@ -1451,9 +1510,13 @@ 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'", + "env", + "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 +1526,12 @@ mod tests { [ "owner/repo@branch", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo@branch", + "env", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo@branch", ] ); } @@ -1484,12 +1551,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 +1637,12 @@ mod tests { [ "owner/repo@fix/x", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions 'fix it'", + "env", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "fix it", ], "{flag}" ); @@ -1650,22 +1730,31 @@ 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")), + [ + "env", + "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")), + [ + "env", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo@it's-mine", + "hi", + ] ); } @@ -1874,8 +1963,13 @@ 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'" + "env", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo", + "fix the bug", ] ); } @@ -1891,7 +1985,10 @@ mod tests { [ "owner/repo", "--", - "gemini --yolo --prompt-interactive 'explain this'" + "gemini", + "--yolo", + "--prompt-interactive", + "explain this", ] ); } @@ -1910,8 +2007,13 @@ 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'", + "env", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo@fix/x", + "fix the bug", ] ); @@ -1924,8 +2026,12 @@ mod tests { [ "owner/repo@fix/x", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions 'fix it'", + "env", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "fix it", ] ); } @@ -1939,8 +2045,12 @@ mod tests { [ "owner/repo", "--", - "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control=owner/repo" + "env", + "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", + "IS_SANDBOX=1", + "claude", + "--dangerously-skip-permissions", + "--remote-control=owner/repo", ] ); } diff --git a/rust/aid/tests/interactive.rs b/rust/aid/tests/interactive.rs index f9bc1bfc..ca3d38b0 100644 --- a/rust/aid/tests/interactive.rs +++ b/rust/aid/tests/interactive.rs @@ -325,9 +325,7 @@ fn a_typed_prompt_reaches_the_agent_with_no_shell_in_the_way() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control={MAIN} \ - '\"'\"'fix the \"flaky\" test'\"'\"''" + "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN} '\"'\"'fix the \"flaky\" test'\"'\"''" ) ); } @@ -350,9 +348,7 @@ fn a_pasted_multi_line_prompt_arrives_whole_rather_than_leaking() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control={MAIN} \ - '\"'\"'fix this\nand then that'\"'\"''" + "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN} '\"'\"'fix this\nand then that'\"'\"''" ) ); } @@ -367,8 +363,7 @@ fn an_empty_enter_is_the_plain_session_it_always_was() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control={MAIN}'" + "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN}'" ) ); } @@ -396,8 +391,7 @@ fn the_boot_runs_while_the_prompt_is_still_being_typed() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control={MAIN} go'" + "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN} go'" ) ); } diff --git a/rust/aid/tests/rewrite.rs b/rust/aid/tests/rewrite.rs index 406cc8c5..f5d757b8 100644 --- a/rust/aid/tests/rewrite.rs +++ b/rust/aid/tests/rewrite.rs @@ -281,14 +281,11 @@ 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 -- env 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 \ - --dangerously-skip-permissions --remote-control=devlaunch-main-3j1t \ - '\"'\"'fix the bug'\"'\"''", + "SSH command: devpod ssh devlaunch-main-3j1t --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control=devlaunch-main-3j1t '\"'\"'fix the bug'\"'\"''", ] ); assert_eq!( @@ -296,10 +293,7 @@ fn a_prompt_reaches_the_agent_as_one_argument_through_dls_own_launch() { [ format!("devpod status {MAIN} --output json"), format!( - "devpod ssh {MAIN} --command bash -lc \ - 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control={MAIN} \ - '\"'\"'fix the bug'\"'\"''" + "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN} '\"'\"'fix the bug'\"'\"''" ), ] ); @@ -317,9 +311,7 @@ fn no_remote_control_is_the_one_way_back_to_a_purely_local_session() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc \ - 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions hi'" + "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions hi'" ), "{flag}" ); @@ -357,9 +349,7 @@ fn an_appended_off_switch_is_observed_from_outside_to_turn_it_off() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc \ - 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions '\"'\"'fix the bug'\"'\"''" + "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions '\"'\"'fix the bug'\"'\"''" ) ); } @@ -397,9 +387,7 @@ fn no_prompt_starts_the_agents_plain_session() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc \ - 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control={MAIN}'" + "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN}'" ) ); // `Command::output()` gives aid no terminal, and off a terminal the promptless @@ -475,9 +463,7 @@ fn remote_control_reaches_claude_as_one_named_flag_and_dl_never_sees_it() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc \ - 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ - --dangerously-skip-permissions --remote-control={MAIN} '\"'\"'fix the bug'\"'\"''" + "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN} '\"'\"'fix the bug'\"'\"''" ) ); } diff --git a/rust/dl/src/launch.rs b/rust/dl/src/launch.rs index d1e74ac2..8480ba81 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,20 @@ 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, and the difference is the whole of + // #588. [`RemotePayload::wrap`] quotes this string whole into `bash -lc + // ''`, so whatever is built here is a command line the *remote* + // shell parses. A plain `words.join(" ")` handed that shell every space + // the host's shell had already eaten, and it re-split on all of them: + // `dl -- claude 'fix the bug'` arrived as four arguments where one + // was meant, a word holding `#` commented out the rest of the line, and + // a word holding `$(...)` was executed. `shell::join` puts back exactly + // the quoting the host's shell removed, which is what makes the remote + // argv the argv that was typed. Guarded by + // `a_word_with_spaces_stays_one_word` and the three beside it. 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 +325,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() { + // #588: 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 that named #588: `#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..fb29d160 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 #588 was the opposite of its own name: the payload + // 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'\"'\"''" ) ); } From 2d367b252d796cc7d289e52735168ad827b4e5bc Mon Sep 17 00:00:00 2001 From: blooop Date: Wed, 9 Sep 2026 10:26:38 +0100 Subject: [PATCH 2/9] fix: the env(1) prefix made aid's command line unreadable to agent detection `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 ... is exactly dl -- IS_SANDBOX=1 ...` equivalence true, which the env spelling had quietly broken. --- CHANGELOG.md | 9 +++--- docs/cli.md | 26 +++++++++-------- rust/aid/src/rewrite.rs | 53 +++++++++++------------------------ rust/aid/tests/interactive.rs | 14 ++++++--- rust/aid/tests/rewrite.rs | 32 ++++++++++++++++----- rust/dl/src/launch.rs | 29 ++++++++++--------- rust/dl/tests/launch.rs | 2 +- 7 files changed, 87 insertions(+), 78 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74137a5e..aac39d91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,11 +26,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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 its variables are set with `env(1)` rather than the shell's - assignment-prefix syntax, so the payload is spelled the same way it is meant: - as arguments. The visible difference is one word in the `--command` devpod is - given, `bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 ...'`. + 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` diff --git a/docs/cli.md b/docs/cli.md index e8e3dd6d..9b631316 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -102,17 +102,21 @@ of the freshness rules, and the section under it names which verb moves what. ## What `--` takes -Argv. The words after `--` are the command and its arguments, one word each, and -`dl` quotes every one of them 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 reads that way because the payload is one `bash -lc ` for both -transports, and the line has to be built rather than assumed. Before 0.34.1 the -words were rejoined with plain spaces, which gave the remote shell back every -separator the host's shell had already consumed. Quoted arguments were re-split, -a `#` commented out the rest of the line, and a `$(...)` ran. +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. 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 diff --git a/rust/aid/src/rewrite.rs b/rust/aid/src/rewrite.rs index 0146f285..f4e983f1 100644 --- a/rust/aid/src/rewrite.rs +++ b/rust/aid/src/rewrite.rs @@ -717,12 +717,11 @@ fn agent_flag(word: &str) -> Option<&'static str> { /// 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 (#588). +/// else's quoted argument. /// -/// The agent's variables are set with `env(1)` rather than the shell's -/// assignment-prefix syntax, which is the same "this process only" guarantee spelled -/// in argv instead of in shell. Nothing in the login shell dl runs this under is -/// changed either way. +/// 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. @@ -754,20 +753,17 @@ pub(crate) fn build_agent_command( words.extend(started.prompt_flags.iter().copied()); words.push(prompt); } - // `env NAME=value … cmd` sets the variables for that command only, so the agent - // is the one process that sees them. Spelled as argv rather than as the shell's - // assignment prefix because this is argv now: the values need no quoting here, - // since dl quotes every word it is handed. - let mut line: Vec = Vec::new(); - if !started.env.is_empty() { - line.push("env".to_owned()); - line.extend( - started - .env - .iter() - .map(|(name, value)| format!("{name}={value}")), - ); - } + // 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. 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}={value}")) + .collect(); line.extend(words.into_iter().map(str::to_owned)); Some(line) } @@ -1054,7 +1050,6 @@ mod tests { assert_eq!( agent_argv("claude", "fix the bug", None), [ - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1065,7 +1060,6 @@ mod tests { assert_eq!( agent_argv("claude", "", None), [ - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1150,7 +1144,6 @@ mod tests { assert_eq!( agent_argv("claude", "don't break \"this\"", None), [ - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1161,7 +1154,6 @@ mod tests { assert_eq!( agent_argv("claude", "hi; rm -rf /", None), [ - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1185,7 +1177,6 @@ mod tests { [ "owner/repo@branch", "--", - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1202,7 +1193,6 @@ mod tests { "robot", "owner/repo", "--", - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1216,7 +1206,7 @@ mod tests { 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 (#588). The tail is argv now, and the + // 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. @@ -1230,7 +1220,6 @@ mod tests { assert_eq!( args[after + 1..], [ - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1283,7 +1272,6 @@ mod tests { "owner/repo", "--rm", "--", - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1510,7 +1498,6 @@ mod tests { [ "owner/repo@branch", "--", - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1526,7 +1513,6 @@ mod tests { [ "owner/repo@branch", "--", - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1637,7 +1623,6 @@ mod tests { [ "owner/repo@fix/x", "--", - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1736,7 +1721,6 @@ mod tests { assert_eq!( agent_argv("claude", "", Some("./my project")), [ - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1747,7 +1731,6 @@ mod tests { assert_eq!( agent_argv("claude", "hi", Some("owner/repo@it's-mine")), [ - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -1963,7 +1946,6 @@ mod tests { "owner/repo", "--rm", "--", - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -2007,7 +1989,6 @@ mod tests { [ "owner/repo@fix/x", "--", - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -2026,7 +2007,6 @@ mod tests { [ "owner/repo@fix/x", "--", - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", @@ -2045,7 +2025,6 @@ mod tests { [ "owner/repo", "--", - "env", "CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1", "IS_SANDBOX=1", "claude", diff --git a/rust/aid/tests/interactive.rs b/rust/aid/tests/interactive.rs index ca3d38b0..f9bc1bfc 100644 --- a/rust/aid/tests/interactive.rs +++ b/rust/aid/tests/interactive.rs @@ -325,7 +325,9 @@ fn a_typed_prompt_reaches_the_agent_with_no_shell_in_the_way() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN} '\"'\"'fix the \"flaky\" test'\"'\"''" + "devpod ssh {MAIN} --command bash -lc 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ + --dangerously-skip-permissions --remote-control={MAIN} \ + '\"'\"'fix the \"flaky\" test'\"'\"''" ) ); } @@ -348,7 +350,9 @@ fn a_pasted_multi_line_prompt_arrives_whole_rather_than_leaking() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN} '\"'\"'fix this\nand then that'\"'\"''" + "devpod ssh {MAIN} --command bash -lc 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ + --dangerously-skip-permissions --remote-control={MAIN} \ + '\"'\"'fix this\nand then that'\"'\"''" ) ); } @@ -363,7 +367,8 @@ fn an_empty_enter_is_the_plain_session_it_always_was() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN}'" + "devpod ssh {MAIN} --command bash -lc 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ + --dangerously-skip-permissions --remote-control={MAIN}'" ) ); } @@ -391,7 +396,8 @@ fn the_boot_runs_while_the_prompt_is_still_being_typed() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN} go'" + "devpod ssh {MAIN} --command bash -lc 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ + --dangerously-skip-permissions --remote-control={MAIN} go'" ) ); } diff --git a/rust/aid/tests/rewrite.rs b/rust/aid/tests/rewrite.rs index f5d757b8..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,11 +285,14 @@ fn a_prompt_reaches_the_agent_as_one_argument_through_dls_own_launch() { assert_eq!( run.err.lines().collect::>(), [ - "aid -> dl devlaunch-main-3j1t -- env 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'", "Workspace devlaunch-main-3j1t is already running, attaching...", - "SSH command: devpod ssh devlaunch-main-3j1t --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control=devlaunch-main-3j1t '\"'\"'fix the bug'\"'\"''", + "SSH command: devpod ssh devlaunch-main-3j1t --command bash -lc \ + 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ + --dangerously-skip-permissions --remote-control=devlaunch-main-3j1t \ + '\"'\"'fix the bug'\"'\"''", ] ); assert_eq!( @@ -293,7 +300,10 @@ fn a_prompt_reaches_the_agent_as_one_argument_through_dls_own_launch() { [ format!("devpod status {MAIN} --output json"), format!( - "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN} '\"'\"'fix the bug'\"'\"''" + "devpod ssh {MAIN} --command bash -lc \ + 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ + --dangerously-skip-permissions --remote-control={MAIN} \ + '\"'\"'fix the bug'\"'\"''" ), ] ); @@ -311,7 +321,9 @@ fn no_remote_control_is_the_one_way_back_to_a_purely_local_session() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions hi'" + "devpod ssh {MAIN} --command bash -lc \ + 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ + --dangerously-skip-permissions hi'" ), "{flag}" ); @@ -349,7 +361,9 @@ fn an_appended_off_switch_is_observed_from_outside_to_turn_it_off() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions '\"'\"'fix the bug'\"'\"''" + "devpod ssh {MAIN} --command bash -lc \ + 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ + --dangerously-skip-permissions '\"'\"'fix the bug'\"'\"''" ) ); } @@ -387,7 +401,9 @@ fn no_prompt_starts_the_agents_plain_session() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN}'" + "devpod ssh {MAIN} --command bash -lc \ + 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ + --dangerously-skip-permissions --remote-control={MAIN}'" ) ); // `Command::output()` gives aid no terminal, and off a terminal the promptless @@ -463,7 +479,9 @@ fn remote_control_reaches_claude_as_one_named_flag_and_dl_never_sees_it() { assert_eq!( world.devpod_calls().last().expect("a session"), &format!( - "devpod ssh {MAIN} --command bash -lc 'env CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions --remote-control={MAIN} '\"'\"'fix the bug'\"'\"''" + "devpod ssh {MAIN} --command bash -lc \ + 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude \ + --dangerously-skip-permissions --remote-control={MAIN} '\"'\"'fix the bug'\"'\"''" ) ); } diff --git a/rust/dl/src/launch.rs b/rust/dl/src/launch.rs index 8480ba81..292a601c 100644 --- a/rust/dl/src/launch.rs +++ b/rust/dl/src/launch.rs @@ -102,17 +102,20 @@ 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), - // Re-quoted, not just rejoined, and the difference is the whole of - // #588. [`RemotePayload::wrap`] quotes this string whole into `bash -lc - // ''`, so whatever is built here is a command line the *remote* - // shell parses. A plain `words.join(" ")` handed that shell every space - // the host's shell had already eaten, and it re-split on all of them: - // `dl -- claude 'fix the bug'` arrived as four arguments where one - // was meant, a word holding `#` commented out the rest of the line, and - // a word holding `$(...)` was executed. `shell::join` puts back exactly - // the quoting the host's shell removed, which is what makes the remote - // argv the argv that was typed. Guarded by - // `a_word_with_spaces_stays_one_word` and the three beside it. + // 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: a plain `words.join(" ")` + // handed that shell every space the host's shell had already eaten, and + // it re-split on all of them. `dl -- claude 'fix the bug'` arrived + // as four arguments where one was meant, a word holding `#` commented + // out the rest of the line, and a word holding `$(...)` was executed. + // + // `shell::join` is the same quoter the rest of the tree already uses, + // and this was the one call site not using it. It puts back exactly the + // quoting the host's shell removed, which is what makes the remote argv + // the argv that was typed. A bare `NAME=value` word survives it + // unquoted, since `=` is shell-safe, so the assignment-prefix spelling + // the README documents keeps working. Verb::Run(words, rm) => ( LaunchVerb::Attach { command: Some(shell::join(words.iter().map(String::as_str))), @@ -361,7 +364,7 @@ mod tests { #[test] fn a_word_with_spaces_stays_one_word() { - // #588: the host's shell had already removed the quotes from `dl -- + // 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!( @@ -372,7 +375,7 @@ mod tests { #[test] fn a_word_holding_a_comment_does_not_swallow_the_rest() { - // The failure that named #588: `#10848` began a comment, so everything + // 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!( diff --git a/rust/dl/tests/launch.rs b/rust/dl/tests/launch.rs index fb29d160..766f2725 100644 --- a/rust/dl/tests/launch.rs +++ b/rust/dl/tests/launch.rs @@ -533,7 +533,7 @@ fn a_quoted_prompt_reaches_the_agent_intact() { // 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 #588 was the opposite of its own name: the payload + // 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 From a573ea396060b79e8f987e8ccd81d7758ab23d69 Mon Sep 17 00:00:00 2001 From: blooop Date: Wed, 9 Sep 2026 14:16:54 +0100 Subject: [PATCH 3/9] fix: the e2e probes passed a shell script as one argv word Thirteen call sites in test/e2e/test_interactive_session.py handed `dl --` 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. --- test/e2e/test_interactive_session.py | 33 +++++++++++++++++----------- 1 file changed, 20 insertions(+), 13 deletions(-) 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