From 53c04ecfd91ca2faa52b807b2ca3f43ebe96522e Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Wed, 9 Sep 2026 16:16:05 +0000 Subject: [PATCH 1/4] refactor: RemotePayload::wrap could not tell argv from a shell script Two callers meant opposite things by the same `Option`. `dl`'s `-- ` handed it words that had already had their quoting stripped by the host's shell, and `dotfiles_command` handed it a script it had composed itself, `&&` and `$(...)` included. The type could not tell them apart, so the join that turns the first into a line lived in `dl` and the second had to be left alone by hoping nobody joined it. That is the shape #591 fixed one instance of; this removes the shape. `RemoteCommand::Argv(NonEmpty) | ::Script(String)` is the split, and the quoting moves in beside it: `line()` is the one place either sense becomes a command line, so `dl` no longer composes one at all -- it hands over the words it was given. A caller can no longer pick the wrong reading, because there is no longer one reading to pick. Two things fall out of having the words rather than a line. `RemoteCommand::agent` finds the program exactly for argv -- the first word that is not an assignment -- where `herdr::agent_in` had to split on whitespace and would have taken `'my` out of a quoted program name. And `LaunchVerb::Attach`'s field can no longer be handed a pre-composed line by an external caller, which was the reachable half of the finding: it is `pub`. aid's half of the same finding: `build_agent_command` returns `Option>`. An empty argv is not a smaller answer but a different one -- `build_dl_args` would emit `[, "--"]`, and dl reads a separator with nothing after it as a plain interactive attach, so an agent that was asked for arrives as a shell. `dl` re-exports `NonEmpty` for it, since aid may name nothing else; dl carries no public-api snapshot, so that re-export moves none. One behaviour change, in an error rather than in a payload. `UnquotableCommand` now carries the composed line, so a NUL in `dl -- echo hi` is reported as `echo 'hi'` rather than `echo hi`. There is no single string the caller gave any more, and the line is the thing that actually could not be made into a shell word. The refusal itself is unchanged and still tested: `shell::join` quotes the offending word rather than rejecting it, so the NUL is still there when `posix_quote` looks. Every existing payload assertion is unchanged. The core tests that modelled `dl -- ` now build `Argv`, and their expectations did not move because both words were already shell-safe -- except the one that spelled the bug, `claude 'fix the bug'`, which was never one argument and now is. Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW --- rust/aid/src/rewrite.rs | 20 +- rust/devlaunch-core/src/clients/herdr.rs | 12 +- rust/devlaunch-core/src/flows/launch.rs | 237 +++++++++++++++++------ rust/dl/src/launch.rs | 31 ++- rust/dl/src/lib.rs | 7 + 5 files changed, 222 insertions(+), 85 deletions(-) diff --git a/rust/aid/src/rewrite.rs b/rust/aid/src/rewrite.rs index 06656474..cd404480 100644 --- a/rust/aid/src/rewrite.rs +++ b/rust/aid/src/rewrite.rs @@ -6,6 +6,8 @@ //! containers itself is an `aid` that builds one `dl` would have reused, which is //! the drift `aid.py` was rewritten to end. +use dl::NonEmpty; + /// How one coding agent is started inside the workspace. /// /// Split three ways because not every part of the line belongs everywhere: `env` @@ -737,7 +739,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. @@ -765,7 +767,13 @@ pub(crate) fn build_agent_command( .map(|(name, value)| format!("{name}={value}")) .collect(); line.extend(words.into_iter().map(str::to_owned)); - Some(line) + // `NonEmpty` rather than the `Vec`, because an empty tail is not a smaller + // answer but a different one: `build_dl_args` would emit `[, "--"]` and + // dl reads a separator with nothing after it as a plain interactive attach, so + // an agent that was asked for would arrive as a shell. Unreachable while every + // row has a command -- which `every_agent_in_the_table_composes_into_argv_dl_can_carry` + // is what holds -- and now unrepresentable rather than merely untaken. + NonEmpty::of(line) } /// The dl command line that does the work. @@ -802,7 +810,7 @@ pub(crate) fn build_dl_args(parsed: &AidArgs) -> Option> { RemoteControl::On => Some(parsed.spec.as_str()), RemoteControl::Off => None, }; - args.extend(build_agent_command(agent, prompt, session)?); + args.extend(build_agent_command(agent, prompt, session)?.iter().cloned()); } Task::Retired => {} } @@ -1070,7 +1078,11 @@ mod tests { /// 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") + build_agent_command(agent, prompt, remote_control) + .expect("a known agent") + .iter() + .cloned() + .collect() } #[test] diff --git a/rust/devlaunch-core/src/clients/herdr.rs b/rust/devlaunch-core/src/clients/herdr.rs index 6d02bba7..ed89e5d9 100644 --- a/rust/devlaunch-core/src/clients/herdr.rs +++ b/rust/devlaunch-core/src/clients/herdr.rs @@ -66,6 +66,16 @@ pub(crate) fn agent_in(command: &str) -> Option<&'static str> { let program = command .split_whitespace() .find(|word| !is_assignment(word))?; + agent_named(program) +} + +/// The agent one already-split program word names, by its last path component. +/// +/// Split out from [`agent_in`] so a caller holding real argv can skip the +/// whitespace guess entirely: `RemoteCommand::Argv` knows where its words end, and +/// splitting a word that legitimately contains a space would be a worse answer +/// than the one it already has. +pub(crate) fn agent_named(program: &str) -> Option<&'static str> { let name = program.rsplit('/').next()?; AGENT_NAMES.iter().copied().find(|known| *known == name) } @@ -75,7 +85,7 @@ pub(crate) fn agent_in(command: &str) -> Option<&'static str> { /// `FOO=bar claude` runs claude. The name half must be non-empty for the same /// reason the shell requires it: `=x` is a program named `=x`, however unlikely, /// and not an assignment. -fn is_assignment(word: &str) -> bool { +pub(crate) fn is_assignment(word: &str) -> bool { match word.split_once('=') { Some((name, _)) => !name.is_empty(), None => false, diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index beeb2703..ae8aeb1f 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -57,6 +57,7 @@ //! *is* a string here is a remote payload — `bash -lc ` — because those //! bytes are a contract with a shell rather than prose for a person. +use std::borrow::Cow; use std::cell::{Cell, OnceCell, RefCell}; use std::collections::BTreeMap; use std::path::{Path, PathBuf}; @@ -75,6 +76,7 @@ use crate::domain::spec::{self, DevcontainerPath, SpecIdentity, WorkspaceSpec}; use crate::domain::workspace_id::{ NamePart, UnsafeName, WorkspaceId, identity_of, validate_ref_name, }; +use crate::domain::workspace_state::NonEmpty; use crate::flows::kept_copies::KeptCopies; use crate::flows::launch_locks::LaunchLocks; use crate::flows::lifecycle::{ @@ -1754,10 +1756,16 @@ pub(crate) struct RemotePayload(String); impl RemotePayload { /// Wrap `command` for the remote shell. - pub(crate) fn wrap(command: &str, zellij: ZellijWrap) -> Result { - let inner = with_zellij_session(command, zellij); + pub(crate) fn wrap( + command: &RemoteCommand, + zellij: ZellijWrap, + ) -> Result { + // `line()` is where argv becomes one command line, and the only place it + // does. A `Script` is already one and is passed through untouched. + let line = command.line(); + let inner = with_zellij_session(&line, zellij); let quoted = posix_quote(&inner).ok_or_else(|| UnquotableCommand { - command: command.to_owned(), + command: line.into_owned(), })?; Ok(Self(format!("bash -lc {quoted}"))) } @@ -2168,7 +2176,7 @@ impl<'a> SessionContext<'a> { pub(crate) fn workspace_ssh( session: &SessionContext<'_>, workspace_id: &str, - command: Option<&str>, + command: Option<&RemoteCommand>, workdir: Option<&str>, forward: &mut dyn FnMut(&str), notices: &mut dyn Notices, @@ -2183,7 +2191,7 @@ pub(crate) fn workspace_ssh( // Read off the command dl was handed, not the payload: the payload is already // wrapped in a `cd` and possibly in zellij, and the question is what program // the person asked for. - let agent = command.and_then(herdr::agent_in); + let agent = command.and_then(RemoteCommand::agent); // The already-cached options, never a fresh `devpod context options`: this is // on the warm path, where that round trip costs more than the pty it decides. let options = already_cached_options(session.host, SystemTime::now()); @@ -2608,7 +2616,10 @@ pub(crate) fn dotfiles_update( session.host.devpod_config().as_deref(), SystemTime::now(), ); - let command = dotfiles_command(options.dotfiles_url(), bound); + // A script, and named as one: it is composed here with `&&` and `$(...)` in + // it, so quoting it as argv would run a program whose name is the whole + // pipeline. + let command = RemoteCommand::Script(dotfiles_command(options.dotfiles_url(), bound)); workspace_ssh( session, workspace_id, @@ -2952,7 +2963,7 @@ pub(crate) fn attach_workspace( workspace_id: &str, title: TerminalTitle, herdr_tab: HerdrTabRename, - command: Option<&str>, + command: Option<&RemoteCommand>, forward: &mut dyn FnMut(&str), notices: &mut dyn Notices, ) -> Result { @@ -3639,6 +3650,70 @@ pub(crate) fn prepare( // the whole launch // =========================================================================== +/// What the workspace is asked to run, and in which of the two senses. +/// +/// One `bash -lc ` carries both, so by the time it reaches the transport the +/// difference is gone -- and it is exactly the difference that decides whether a +/// space in the text is a separator or a character. Splitting the type is what +/// stops the two from being handed to each other: `dl -- claude 'fix the bug'` +/// is [`Argv`](RemoteCommand::Argv) and must not be reparsed, while the dotfiles +/// pass composes a real script with `&&` and `$(...)` in it and must not be quoted. +/// +/// Both used to arrive as `Option`, and the join lived in `dl`. That is how +/// blooop/devlaunch#591 happened: the argv side was rejoined with plain spaces, so +/// the remote shell got back every separator the host's shell had already consumed +/// -- quoted arguments re-split, a `#` truncated the line, and a `$(...)` ran. The +/// quoting moved in here with the type, so there is one place that turns either +/// sense into a line and no caller that can pick the wrong one. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum RemoteCommand { + /// The command and its arguments, one word each, as the caller's own shell + /// left them. Every word is quoted on the way out, so a word is a word. + Argv(NonEmpty), + /// A shell script, run as written. The caller composed the syntax and means it. + Script(String), +} + +#[cfg(test)] +impl RemoteCommand { + /// Argv from words, for the tests that model `dl -- `. + /// + /// A panic on the empty slice rather than an `Option`: a test that asks for a + /// command with no words has asked the wrong question, and `NonEmpty` is the + /// production answer to that case. + pub(crate) fn argv(words: &[&str]) -> Self { + Self::Argv(NonEmpty::of(words.iter().map(|word| (*word).to_owned())).expect("a command")) + } +} + +impl RemoteCommand { + /// The one command line, in whichever sense this is. + pub fn line(&self) -> Cow<'_, str> { + match self { + Self::Argv(words) => Cow::Owned(shell::join(words.iter().map(String::as_str))), + Self::Script(script) => Cow::Borrowed(script), + } + } + + /// The program this starts, for a session manager that wants to name it. + /// + /// Exact for [`Argv`](RemoteCommand::Argv), where the words are already split + /// and the program is simply the first one that is not an assignment prefix. A + /// [`Script`](RemoteCommand::Script) has to be guessed at by + /// [`herdr::agent_in`], which splits on whitespace and is wrong for a quoted + /// program name -- but a script is the caller's own syntax and there is nothing + /// better to do with it. + pub(crate) fn agent(&self) -> Option<&'static str> { + match self { + Self::Argv(words) => words + .iter() + .find(|word| !herdr::is_assignment(word)) + .and_then(|program| herdr::agent_named(program)), + Self::Script(script) => herdr::agent_in(script), + } + } +} + /// What `dl ` asks for. /// /// One arm per shape `_run_cli` dispatches to on this path. `stop`, `rm` and the @@ -3648,7 +3723,7 @@ pub enum LaunchVerb { /// `dl ` and `dl -- `: bring it up if it is not up, then /// attach. The default, and the shape wayfinder hands dl for every agent /// launch. - Attach { command: Option }, + Attach { command: Option }, /// `dl up`: the warm half of a launch, for callers that want the /// container ready before a user arrives. Idempotent and quiet when already up. Up, @@ -3690,9 +3765,9 @@ impl LaunchVerb { } /// The command the session runs, if this verb ends in a session. - fn command(&self) -> Option<&str> { + fn command(&self) -> Option<&RemoteCommand> { match self { - Self::Attach { command } => command.as_deref(), + Self::Attach { command } => command.as_ref(), Self::Recreate | Self::Reset | Self::Restart => None, Self::Up | Self::Code | Self::Dotfiles => None, } @@ -4339,7 +4414,7 @@ impl<'a, 'r, 'l> Launch<'a, 'r, 'l> { fn attach( &mut self, placement: &Placement, - command: Option<&str>, + command: Option<&RemoteCommand>, ) -> Result { // A warm attach runs no pass, so nothing has observed this container's Claude // config directory during this launch. The host's own records stand in, and @@ -4916,7 +4991,7 @@ mod tests { scene: &Scene, token: &HostToken, workspace_id: &str, - command: Option<&str>, + command: Option<&RemoteCommand>, notices: &mut Vec, ) -> Result { let claude_seen = ClaudeSeen::new(); @@ -6115,7 +6190,8 @@ mod tests { fn a_command_is_wrapped_for_a_login_shell() { // devpod runs `--command` under a non-login, non-interactive `bash -c`, // which sources neither ~/.profile nor ~/.bashrc. - let payload = RemotePayload::wrap("echo hi", ZellijWrap::Off).expect("quotable"); + let payload = RemotePayload::wrap(&RemoteCommand::argv(&["echo", "hi"]), ZellijWrap::Off) + .expect("quotable"); assert_eq!(payload.as_str(), "bash -lc 'echo hi'"); } @@ -6123,8 +6199,11 @@ mod tests { #[test] fn a_quoted_prompt_reaches_the_agent_intact() { // `aid repo fix the bug` becomes one quoted argument; it must stay one. - let payload = - RemotePayload::wrap("claude 'fix the bug'", ZellijWrap::Off).expect("quotable"); + let payload = RemotePayload::wrap( + &RemoteCommand::argv(&["claude", "fix the bug"]), + ZellijWrap::Off, + ) + .expect("quotable"); assert_eq!( payload.as_str(), @@ -6134,7 +6213,9 @@ mod tests { #[test] fn the_zellij_wrap_ensures_a_session_beside_the_command() { - let payload = RemotePayload::wrap("echo hi", ZellijWrap::Beside).expect("quotable"); + let payload = + RemotePayload::wrap(&RemoteCommand::argv(&["echo", "hi"]), ZellijWrap::Beside) + .expect("quotable"); assert_eq!( payload.as_str(), @@ -6512,10 +6593,17 @@ mod tests { // A NUL cannot survive a shell word, so there is no payload to build. // Python has no such refusal — `shlex.quote` wraps it and the remote shell // mangles it. + // + // The refusal survives the argv split, which is the part worth asserting: + // `shell::join` quotes the offending word rather than rejecting it, so the + // NUL is still there when `posix_quote` looks at the whole line and still + // stops the launch. What the error carries is that composed line, which for + // argv is the quoted form -- there is no single string the caller gave, and + // the line is the thing that actually could not be made a word. assert_eq!( - RemotePayload::wrap("echo \0hi", ZellijWrap::Off), + RemotePayload::wrap(&RemoteCommand::argv(&["echo", "\0hi"]), ZellijWrap::Off), Err(UnquotableCommand { - command: "echo \0hi".to_owned() + command: "echo '\0hi'".to_owned() }) ); } @@ -6689,7 +6777,8 @@ mod tests { config: published.clone() } ); - let payload = RemotePayload::wrap("claude", ZellijWrap::Off).expect("quotable"); + let payload = RemotePayload::wrap(&RemoteCommand::argv(&["claude"]), ZellijWrap::Off) + .expect("quotable"); assert_eq!( route(Some(&payload), &terminal, "myws", &mut no_notices()), Route::Terminal { @@ -6738,7 +6827,8 @@ mod tests { config: config.clone() } ); - let payload = RemotePayload::wrap("claude", ZellijWrap::Off).expect("quotable"); + let payload = RemotePayload::wrap(&RemoteCommand::argv(&["claude"]), ZellijWrap::Off) + .expect("quotable"); assert_eq!( route(Some(&payload), &terminal, "myws", &mut notices), Route::DevpodCommand(&payload) @@ -6773,7 +6863,8 @@ mod tests { looked_in: never_written.clone() } ); - let payload = RemotePayload::wrap("claude", ZellijWrap::Off).expect("quotable"); + let payload = RemotePayload::wrap(&RemoteCommand::argv(&["claude"]), ZellijWrap::Off) + .expect("quotable"); assert_eq!( route(Some(&payload), &terminal, "myws", &mut notices), Route::DevpodCommand(&payload) @@ -6801,7 +6892,8 @@ mod tests { let terminal = terminal_here(&host, "myws"); assert_eq!(terminal, Terminal::ConfigUnlocatable); - let payload = RemotePayload::wrap("claude", ZellijWrap::Off).expect("quotable"); + let payload = RemotePayload::wrap(&RemoteCommand::argv(&["claude"]), ZellijWrap::Off) + .expect("quotable"); assert_eq!( route(Some(&payload), &terminal, "myws", &mut notices), Route::DevpodCommand(&payload) @@ -6857,7 +6949,7 @@ mod tests { /// [`a_session`], with the Claude config ownership the pass would have observed. fn a_session_seeing( scene: &Scene, - command: Option<&str>, + command: Option<&RemoteCommand>, seen: Option, ) -> Vec { let token = HostToken::new(); @@ -6978,7 +7070,7 @@ mod tests { fn a_session( scene: &Scene, - command: Option<&str>, + command: Option<&RemoteCommand>, ) -> ( Result, Vec, @@ -7009,7 +7101,7 @@ mod tests { /// nothing. fn a_session_on_our_own_claude( scene: &Scene, - command: Option<&str>, + command: Option<&RemoteCommand>, ) -> Result { let token = HostToken::new(); let mut notices = no_notices(); @@ -7038,7 +7130,7 @@ mod tests { let opened = workspace_ssh( &context, "myws", - Some("claude"), + Some(&RemoteCommand::argv(&["claude"])), None, &mut |_| {}, &mut notices, @@ -7132,7 +7224,7 @@ mod tests { .naming_a_claude_profile("work", true), ); - let _ = a_session_on_our_own_claude(&scene, Some("claude")); + let _ = a_session_on_our_own_claude(&scene, Some(&RemoteCommand::argv(&["claude"]))); let labelled: Vec> = scene .runner @@ -7164,7 +7256,7 @@ mod tests { .with_running("myws") .naming_a_claude_profile("work", false); - match a_session_on_our_own_claude(&scene, Some("claude")) { + match a_session_on_our_own_claude(&scene, Some(&RemoteCommand::argv(&["claude"]))) { Err(SessionRefused::ClaudeProfile { name, problem }) => { assert_eq!(name, "work"); match problem { @@ -7192,7 +7284,7 @@ mod tests { .with_running("myws") .naming_a_claude_profile("work", false); - let _ = a_session_on_our_own_claude(&scene, Some("claude")); + let _ = a_session_on_our_own_claude(&scene, Some(&RemoteCommand::argv(&["claude"]))); assert!(scene.runner.calls_to("ssh").is_empty(), "openssh was run"); assert!( @@ -7214,7 +7306,8 @@ mod tests { .with_running("myws") .naming_a_claude_profile("work", true); - a_session_on_our_own_claude(&scene, Some("claude")).expect("a session"); + a_session_on_our_own_claude(&scene, Some(&RemoteCommand::argv(&["claude"]))) + .expect("a session"); let calls = scene.runner.calls_to("ssh"); let call = calls.last().expect("an openssh session"); @@ -7236,7 +7329,8 @@ mod tests { // would fire on every launch of every host that does not use Claude. let scene = Scene::new().on_a_terminal(&["myws"]).with_running("myws"); - a_session_on_our_own_claude(&scene, Some("echo hi")).expect("a session"); + a_session_on_our_own_claude(&scene, Some(&RemoteCommand::argv(&["echo", "hi"]))) + .expect("a session"); let calls = scene.runner.calls_to("ssh"); let call = calls.last().expect("an openssh session"); @@ -7256,7 +7350,7 @@ mod tests { scene.host.claude_profiles_root = Some(scene.dir.path().join("claude-profiles")); scene.host.claude.profile = Some("../../etc".to_owned()); - match a_session_on_our_own_claude(&scene, Some("claude")) { + match a_session_on_our_own_claude(&scene, Some(&RemoteCommand::argv(&["claude"]))) { Err(SessionRefused::ClaudeProfile { name, problem }) => { assert_eq!(name, "../../etc"); // No directory, because the name was refused before one was built from @@ -7279,7 +7373,10 @@ mod tests { fn a_command_that_names_an_agent_names_it_to_the_ssh_child() { let scene = Scene::new().on_a_terminal(&["myws"]).with_running("myws"); - let _ = a_session(&scene, Some("claude 'fix the bug'")); + let _ = a_session( + &scene, + Some(&RemoteCommand::argv(&["claude", "fix the bug"])), + ); let calls = scene.runner.calls_to("ssh"); let call = calls.last().expect("an openssh session"); @@ -7311,7 +7408,10 @@ mod tests { fn a_command_that_names_an_agent_names_it_on_the_devpod_route_too() { let scene = Scene::new().with_running("myws"); - let _ = a_session(&scene, Some("claude 'fix the bug'")); + let _ = a_session( + &scene, + Some(&RemoteCommand::argv(&["claude", "fix the bug"])), + ); let calls = scene.runner.calls_to("devpod"); let call = calls.last().expect("a devpod session"); @@ -7338,7 +7438,7 @@ mod tests { fn an_ordinary_command_names_no_agent() { let scene = Scene::new().on_a_terminal(&["myws"]).with_running("myws"); - let _ = a_session(&scene, Some("make test")); + let _ = a_session(&scene, Some(&RemoteCommand::argv(&["make", "test"]))); let calls = scene.runner.calls_to("ssh"); let call = calls.last().expect("an openssh session"); @@ -7362,7 +7462,7 @@ mod tests { fn a_one_shot_command_travels_as_the_shlex_quoted_payload() { let scene = Scene::new().with_running("myws"); - let (session, _, _) = a_session(&scene, Some("echo hi")); + let (session, _, _) = a_session(&scene, Some(&RemoteCommand::argv(&["echo", "hi"]))); assert_eq!(session, Ok(Session::RemoteExit { status: 0 })); assert_eq!( @@ -7432,7 +7532,7 @@ mod tests { panic!("a scratch cache is short enough to multiplex through"); }; - let (session, _, _) = a_session(&scene, Some("claude")); + let (session, _, _) = a_session(&scene, Some(&RemoteCommand::argv(&["claude"]))); assert_eq!( session, @@ -7525,7 +7625,7 @@ mod tests { let scene = Scene::new().on_a_terminal(&["myws"]).with_running("myws"); scene.runner.script(["ssh"], Response::exited(code)); - let (session, _, _) = a_session(&scene, Some("false")); + let (session, _, _) = a_session(&scene, Some(&RemoteCommand::argv(&["false"]))); assert_eq!( session.expect("a session").exit_status(), @@ -7542,7 +7642,7 @@ mod tests { let scene = Scene::new().on_a_terminal(&["myws"]).with_running("myws"); scene.runner.script_missing("ssh"); - let (session, _, _) = a_session(&scene, Some("claude")); + let (session, _, _) = a_session(&scene, Some(&RemoteCommand::argv(&["claude"]))); assert_eq!(session, Err(SessionRefused::Ssh(ssh::NotRun::NotInstalled))); } @@ -7551,7 +7651,7 @@ mod tests { fn the_argv_of_the_session_is_reported_before_it_starts() { let scene = Scene::new().with_running("myws"); - let (_, notices, _) = a_session(&scene, Some("echo hi")); + let (_, notices, _) = a_session(&scene, Some(&RemoteCommand::argv(&["echo", "hi"]))); assert!(notices.contains(&LaunchNotice::SshCommand { argv: vec![ @@ -7686,7 +7786,7 @@ mod tests { fn the_openssh_transport_forwards_the_same_token_by_name() { let scene = logged_in(Scene::new().on_a_terminal(&["myws"]).with_running("myws")); - let _ = a_session(&scene, Some("claude")); + let _ = a_session(&scene, Some(&RemoteCommand::argv(&["claude"]))); let argv = scene .runner @@ -7711,8 +7811,8 @@ mod tests { let with_token = logged_in(Scene::new().on_a_terminal(&["myws"]).with_running("myws")); let without = Scene::new().on_a_terminal(&["myws"]).with_running("myws"); - let _ = a_session(&with_token, Some("claude")); - let _ = a_session(&without, Some("claude")); + let _ = a_session(&with_token, Some(&RemoteCommand::argv(&["claude"]))); + let _ = a_session(&without, Some(&RemoteCommand::argv(&["claude"]))); let keyed = |scene: &Scene| -> String { let argv = scene @@ -7747,7 +7847,7 @@ mod tests { // takes it away with everything else. let scene = Scene::new().on_a_terminal(&["myws"]).with_running("myws"); - let _ = a_session(&scene, Some("claude")); + let _ = a_session(&scene, Some(&RemoteCommand::argv(&["claude"]))); let argv = scene .runner @@ -8073,7 +8173,13 @@ mod tests { scene.host.dotfiles_on_attach = Some("1".to_owned()); let token = HostToken::new(); - let _ = attaching(&scene, &token, "myws", Some("echo hi"), &mut no_notices()); + let _ = attaching( + &scene, + &token, + "myws", + Some(&RemoteCommand::argv(&["echo", "hi"])), + &mut no_notices(), + ); assert_eq!( scene.devpod_commands(), @@ -8091,7 +8197,8 @@ mod tests { let scene = Scene::new().with_running("myws"); let token = HostToken::new(); - for command in [None, Some("echo hi")] { + let echo = RemoteCommand::argv(&["echo", "hi"]); + for command in [None, Some(&echo)] { scene.runner.forget_calls(); let _ = attaching(&scene, &token, "myws", command, &mut no_notices()); @@ -8542,7 +8649,7 @@ mod tests { let launched = launch.run( &format!("blooop/devlaunch@{COLLIDING_B}"), &LaunchVerb::Attach { - command: Some("true".to_owned()), + command: Some(RemoteCommand::argv(&["true"])), }, None, ); @@ -8603,7 +8710,7 @@ mod tests { let launched = launch.run( &format!("blooop/devlaunch@{COLLIDING_A}"), &LaunchVerb::Attach { - command: Some("true".to_owned()), + command: Some(RemoteCommand::argv(&["true"])), }, None, ); @@ -8678,7 +8785,7 @@ mod tests { let launched = launch.run( spec, &LaunchVerb::Attach { - command: Some("true".to_owned()), + command: Some(RemoteCommand::argv(&["true"])), }, None, ); @@ -8836,7 +8943,7 @@ mod tests { let launched = launch.run( workspace.value(), &LaunchVerb::Attach { - command: Some("true".to_owned()), + command: Some(RemoteCommand::argv(&["true"])), }, None, ); @@ -8924,7 +9031,7 @@ mod tests { let launched = launch.run( "NVIDIA/cuda-samples@main", &LaunchVerb::Attach { - command: Some("true".to_owned()), + command: Some(RemoteCommand::argv(&["true"])), }, None, ); @@ -8982,7 +9089,7 @@ mod tests { let launched = launch.run( "owner/repo@Main", &LaunchVerb::Attach { - command: Some("true".to_owned()), + command: Some(RemoteCommand::argv(&["true"])), }, None, ); @@ -9037,7 +9144,7 @@ mod tests { let launched = launch.run( "owner/repo@main", &LaunchVerb::Attach { - command: Some("true".to_owned()), + command: Some(RemoteCommand::argv(&["true"])), }, None, ); @@ -9081,7 +9188,7 @@ mod tests { let launched = launch.run( &format!("blooop/devlaunch@{COLLIDING_B}"), &LaunchVerb::Attach { - command: Some("true".to_owned()), + command: Some(RemoteCommand::argv(&["true"])), }, None, ); @@ -9199,7 +9306,7 @@ mod tests { let launched = launch.run( "octocat/Hello-World@master", &LaunchVerb::Attach { - command: Some("true".to_owned()), + command: Some(RemoteCommand::argv(&["true"])), }, None, ); @@ -9262,7 +9369,7 @@ mod tests { let _ = launch.run( "octocat/Hello-World@master", &LaunchVerb::Attach { - command: Some("true".to_owned()), + command: Some(RemoteCommand::argv(&["true"])), }, None, ); @@ -9313,7 +9420,7 @@ mod tests { let launched = launch.run( "blooop/devlaunch@wayfinder/devlaunch-7", &LaunchVerb::Attach { - command: Some("echo hi".to_owned()), + command: Some(RemoteCommand::argv(&["echo", "hi"])), }, None, ); @@ -9377,7 +9484,7 @@ mod tests { let launched = launch.run( "blooop/devlaunch@feature/auth", &LaunchVerb::Attach { - command: Some("echo hi".to_owned()), + command: Some(RemoteCommand::argv(&["echo", "hi"])), }, None, ); @@ -9500,7 +9607,7 @@ mod tests { let launched = launch.run( "myws", &LaunchVerb::Attach { - command: Some("echo hi".to_owned()), + command: Some(RemoteCommand::argv(&["echo", "hi"])), }, None, ); @@ -10456,7 +10563,7 @@ mod tests { let launched = launch.run( "owner/repo@feature/x", &LaunchVerb::Attach { - command: Some("echo hi".to_owned()), + command: Some(RemoteCommand::argv(&["echo", "hi"])), }, None, ); @@ -10502,7 +10609,7 @@ mod tests { let _ = launch.run( "owner/repo", &LaunchVerb::Attach { - command: Some("echo hi".to_owned()), + command: Some(RemoteCommand::argv(&["echo", "hi"])), }, None, ); @@ -10741,7 +10848,13 @@ mod tests { let token = HostToken::new(); let (_, document) = measured(timing::Seam::default(), || { - attaching(&scene, &token, "myws", Some("claude"), &mut no_notices()) + attaching( + &scene, + &token, + "myws", + Some(&RemoteCommand::argv(&["claude"])), + &mut no_notices(), + ) }); assert_eq!(span_labels(&document, "attach"), ["ssh"]); diff --git a/rust/dl/src/launch.rs b/rust/dl/src/launch.rs index a172aef8..3b41e8bd 100644 --- a/rust/dl/src/launch.rs +++ b/rust/dl/src/launch.rs @@ -36,13 +36,13 @@ use std::path::Path; use devlaunch_core::domain::spec::DevcontainerPath; use devlaunch_core::domain::workspace_id::WorkspaceId; use devlaunch_core::flows::completion_cache; +use devlaunch_core::flows::launch::RemoteCommand; use devlaunch_core::flows::launch::{ self, ColdPath, Host, Launch, LaunchAborted, LaunchRefusal, LaunchVerb, Launched, Plan, Session, ToolProvisioning, }; 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; @@ -102,23 +102,14 @@ 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. [`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. + // Handed over as the argv it is. The quoting lives in + // [`RemoteCommand::line`] now, so this no longer composes a command line + // at all -- which is the point: the words arriving here have had their + // quoting removed by the *host's* shell, and whoever turns them back into + // one line has to be the one that knows they are words. Verb::Run(words, rm) => ( LaunchVerb::Attach { - command: Some(shell::join(words.iter().map(String::as_str))), + command: Some(RemoteCommand::Argv(words.clone())), }, *rm, ), @@ -337,7 +328,11 @@ mod tests { use super::{Family, family}; use crate::cli::{RmOnExit, Verb}; - /// The command `dl -- ` would hand the remote shell. + /// The command line `dl -- ` would hand the remote shell. + /// + /// The join is [`RemoteCommand::line`]'s now rather than this crate's, so what + /// these tests pin is the seam: the words go over as words, and the quoting + /// that makes them survive happens once, in core. fn run_command(words: &[&str]) -> String { let verb = Verb::Run( NonEmpty::of(words.iter().map(|word| (*word).to_owned())).expect("a command"), @@ -350,7 +345,7 @@ mod tests { command: Some(command), }, .. - } => command, + } => command.line().into_owned(), _ => panic!("`-- ` is a launch that attaches with a command"), } } diff --git a/rust/dl/src/lib.rs b/rust/dl/src/lib.rs index 5316fa8f..41f43dbd 100644 --- a/rust/dl/src/lib.rs +++ b/rust/dl/src/lib.rs @@ -39,6 +39,13 @@ use devlaunch_core::flows::lifecycle::{Refresh, RefreshReason}; use devlaunch_core::runner::ProcessRunner; use devlaunch_core::timing; +/// Re-exported for `aid`, which may name nothing but `dl`. +/// +/// `build_agent_command` returns the agent's argv, and an empty one is not a +/// degenerate answer but a wrong one: `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. The type is what refuses it. +pub use devlaunch_core::domain::workspace_state::NonEmpty; /// `shlex.quote`, for the entry point that builds a `dl` command line out of its /// own: `aid` reaches it through here rather than through `devlaunch-core`, so the /// only thing it can see of devlaunch is `dl`'s command line and the quoting that From 37a2c7533e992af040611575e375b1895fbf7403 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Wed, 9 Sep 2026 16:23:43 +0000 Subject: [PATCH 2/4] refactor: the public-api snapshots this deliberately moves Regenerated by CI rather than here: `cargo-public-api` 0.52.0 builds `openssl-sys`, and the devcontainer carries no OpenSSL headers, so the job's own diff is the source of these two files. `public-api.api.txt` is a **promise change, and a break**: `LaunchVerb::Attach::command` goes from `Option` to `Option` at both paths it is rendered at. That is the point of the change rather than a side effect -- the `pub` field taking a pre-composed line was the reachable half of the finding, since an external caller could hand it a shell script where dl hands it argv and nothing in the type objected. `public-api.rest.txt` gains `RemoteCommand` and its derives. `line()` is the only inherent method on it; `agent()` stays `pub(crate)`, because naming the agent is dl's business with a session manager and not a promise to anyone outside. Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW --- rust/devlaunch-core/public-api.api.txt | 4 ++-- rust/devlaunch-core/public-api.rest.txt | 13 +++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/rust/devlaunch-core/public-api.api.txt b/rust/devlaunch-core/public-api.api.txt index 26a32174..97bc07c7 100644 --- a/rust/devlaunch-core/public-api.api.txt +++ b/rust/devlaunch-core/public-api.api.txt @@ -101,7 +101,7 @@ pub fn devlaunch_core::flows::launch::LaunchNotice::fmt(&self, &mut core::fmt::F impl core::marker::StructuralPartialEq for devlaunch_core::flows::launch::LaunchNotice pub enum devlaunch_core::api::LaunchVerb pub devlaunch_core::api::LaunchVerb::Attach -pub devlaunch_core::api::LaunchVerb::Attach::command: core::option::Option +pub devlaunch_core::api::LaunchVerb::Attach::command: core::option::Option pub devlaunch_core::api::LaunchVerb::Code pub devlaunch_core::api::LaunchVerb::Dotfiles pub devlaunch_core::api::LaunchVerb::Recreate @@ -593,7 +593,7 @@ pub fn devlaunch_core::flows::launch::LaunchNotice::fmt(&self, &mut core::fmt::F impl core::marker::StructuralPartialEq for devlaunch_core::flows::launch::LaunchNotice pub enum devlaunch_core::flows::launch::LaunchVerb pub devlaunch_core::flows::launch::LaunchVerb::Attach -pub devlaunch_core::flows::launch::LaunchVerb::Attach::command: core::option::Option +pub devlaunch_core::flows::launch::LaunchVerb::Attach::command: core::option::Option pub devlaunch_core::flows::launch::LaunchVerb::Code pub devlaunch_core::flows::launch::LaunchVerb::Dotfiles pub devlaunch_core::flows::launch::LaunchVerb::Recreate diff --git a/rust/devlaunch-core/public-api.rest.txt b/rust/devlaunch-core/public-api.rest.txt index 20e9357a..572a3739 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -1606,6 +1606,19 @@ pub fn devlaunch_core::flows::launch::Plan::eq(&self, &devlaunch_core::flows::la impl core::fmt::Debug for devlaunch_core::flows::launch::Plan pub fn devlaunch_core::flows::launch::Plan::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for devlaunch_core::flows::launch::Plan +pub enum devlaunch_core::flows::launch::RemoteCommand +pub devlaunch_core::flows::launch::RemoteCommand::Argv(devlaunch_core::domain::workspace_state::NonEmpty) +pub devlaunch_core::flows::launch::RemoteCommand::Script(alloc::string::String) +impl devlaunch_core::flows::launch::RemoteCommand +pub fn devlaunch_core::flows::launch::RemoteCommand::line(&self) -> alloc::borrow::Cow<'_, str> +impl core::clone::Clone for devlaunch_core::flows::launch::RemoteCommand +pub fn devlaunch_core::flows::launch::RemoteCommand::clone(&self) -> devlaunch_core::flows::launch::RemoteCommand +impl core::cmp::Eq for devlaunch_core::flows::launch::RemoteCommand +impl core::cmp::PartialEq for devlaunch_core::flows::launch::RemoteCommand +pub fn devlaunch_core::flows::launch::RemoteCommand::eq(&self, &devlaunch_core::flows::launch::RemoteCommand) -> bool +impl core::fmt::Debug for devlaunch_core::flows::launch::RemoteCommand +pub fn devlaunch_core::flows::launch::RemoteCommand::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::StructuralPartialEq for devlaunch_core::flows::launch::RemoteCommand pub enum devlaunch_core::flows::launch::Resolution pub devlaunch_core::flows::launch::Resolution::Cold pub devlaunch_core::flows::launch::Resolution::Cold::workspace: devlaunch_core::domain::workspace_id::WorkspaceId From cdafc951786e1e8d2ea1f7da3024646c1a840d9c Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Wed, 9 Sep 2026 16:25:18 +0000 Subject: [PATCH 3/4] fix: the residual grew to 38, and one of its two figures was already stale `RemoteCommand` is a type `api` never re-exports but a promised signature names -- `LaunchVerb::Attach::command` -- so it is contract surface classified as binary surface, and it lands in `public-api.rest.txt` with 13 rows. That is the documented limit of the classifier rather than a new hole, and the count of types in it goes 37 to 38. Three sites carry that count and `test_public_api_snapshots_doc.py` holds all three: `docs/development.md`, the `public-api-snapshots.sh` header, and `devlaunch-core/src/lib.rs`. A fourth figure was wrong before this change and the guard could not see it. "a row whose subject is one of the 39" is from e79e752, when the residual was 39; 3b7b2c7 and a2d8e7e later moved the count to 36 and then 37 and updated only the sentence the guard matches on, which is the "N types" phrasing. So the same paragraph said 37 in one clause and 39 in the next. Both now say 38, in all three files. Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW --- docs/development.md | 6 +++--- rust/devlaunch-core/src/lib.rs | 2 +- scripts/public-api-snapshots.sh | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/development.md b/docs/development.md index 91b8947b..b01c8cf9 100644 --- a/docs/development.md +++ b/docs/development.md @@ -69,7 +69,7 @@ once under the `api` section and once under the module that owns them. **What it still does not reach, and it is not one type.** A type `api` never re-exports but a promised signature hands back is reachable from outside and classified as binary surface. Counted -on the checked-in files rather than guessed at, that is **37 types owning close to six hundred rows** in +on the checked-in files rather than guessed at, that is **38 types owning close to six hundred rows** in `public-api.rest.txt`, and the command that lists them needs no toolchain: **A falling count is not automatically a win, and it is worth knowing which kind you are looking @@ -93,10 +93,10 @@ file that moves is the one this page calls freely regenerated. section used to give on its own, which made six hundred rows read as one. So a diff in `public-api.rest.txt` is routine for a row whose subject nothing promised names, and a -contract change for a row whose subject is one of the 39. `--print-residual` is how you tell the two +contract change for a row whose subject is one of the 38. `--print-residual` is how you tell the two apart, and `test/test_public_api_snapshots_doc.py` diffs the count of types in this paragraph against it, so the sentence goes red rather than stale. The row total is left round on purpose: it -moves whenever anything is added to any one of the 39 and says nothing about the scale of the +moves whenever anything is added to any one of the 38 and says nothing about the scale of the limit, where the count of types moves only when the residual really grows. The `-ss` flag also omits blanket and auto-trait impls from both files, diff --git a/rust/devlaunch-core/src/lib.rs b/rust/devlaunch-core/src/lib.rs index 2a8345ae..45a74ac3 100644 --- a/rust/devlaunch-core/src/lib.rs +++ b/rust/devlaunch-core/src/lib.rs @@ -66,7 +66,7 @@ //! **What it still does not reach, and it is not one type.** A type [`api`] //! never re-exports but a promised signature names is reachable from outside //! and classified as binary surface, so a break in it diffs -//! `public-api.rest.txt` alone. Counted rather than guessed at: **37 such types +//! `public-api.rest.txt` alone. Counted rather than guessed at: **38 such types //! own close to six hundred rows over there**, and `scripts/public-api-snapshots.sh //! --print-residual` lists them. //! diff --git a/scripts/public-api-snapshots.sh b/scripts/public-api-snapshots.sh index 952f24c0..a91ec9e5 100755 --- a/scripts/public-api-snapshots.sh +++ b/scripts/public-api-snapshots.sh @@ -37,7 +37,7 @@ # The limit that is left, and it is not one type. A type `api` never re-exports # but a promised signature names is reachable from outside and is classified as # binary surface, so a break in it diffs public-api.rest.txt alone. Measured on -# the checked-in files rather than guessed at: 37 such types own close to six +# the checked-in files rather than guessed at: 38 such types own close to six # hundred rows over there. `--print-residual` lists them, needs no toolchain, # and prints the exact row count; the type count above is the figure # `test/test_public_api_snapshots_doc.py` diffs against it, because that is the @@ -55,7 +55,7 @@ # # What that means for reading a rest-file diff: it is routine for a row whose # subject is nothing a promised signature names, and a contract change for a row -# whose subject is one of the 39. `--print-residual` is how you tell. +# whose subject is one of the 38. `--print-residual` is how you tell. # # Whether the tool does this already, since the obvious first question is why # any of it is hand-rolled (#352 asked it explicitly). It does not, in the pin From 37825907f26c7bd85072eab20711ce06ab841740 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Wed, 9 Sep 2026 16:31:21 +0000 Subject: [PATCH 4/4] docs: the changelog entries for both follow-ups Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW --- CHANGELOG.md | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b04868e3..7d598d76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,42 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **`LaunchVerb::Attach` carries a `RemoteCommand` rather than an + `Option`, and the change is a break for anyone driving + `devlaunch-core` directly.** The field meant two opposite things depending on + who filled it: `dl -- ` put words in it whose quoting the host's + shell had already stripped, and the dotfiles pass put a script in it that it + had composed itself, `&&` and `$(...)` included. Nothing in the type could tell + those apart, so the join that turns the first into a command line lived in `dl` + and the second survived only by nobody joining it. That is the shape 0.37.0 + fixed one instance of. + + `RemoteCommand::Argv(NonEmpty)` and `RemoteCommand::Script(String)` are + the two senses, and `RemoteCommand::line` is now the only place either becomes a + command line. A caller passing argv writes `Argv` and a caller passing a script + writes `Script`; there is no longer a reading to get wrong. + + Two things follow from holding words instead of a line. The program a session + manager is told about is now exact for argv, where it had to be guessed at by + splitting on whitespace. And `UnquotableCommand` carries the composed line, so + the refusal of a command holding a NUL names `echo 'hi'` where it used to + name `echo hi`: there is no single string the caller gave any more, and the + line is the thing that could not be made into a shell word. The refusal itself + is unchanged. + + Nothing about what reaches a workspace changes. Every payload assertion in the + suite is untouched. + +- **A version that is already published is refused before it can be merged.** + Two branches bumped to the same number and git merged it without a conflict, + because the same version line on both sides is one edit; the second merge then + published nothing and said so in the words an ordinary push gets. + `scripts/version_untaken.py` runs on the pull request, where the two versions + still differ, and `publish.yml` now separates a re-run over the commit it + published from a push moving the version onto somebody else's tag. + ## [0.37.0] - 2026-09-09 ### Fixed