refactor: RemotePayload::wrap could not tell argv from a shell script - #596
Merged
Conversation
Two callers meant opposite things by the same `Option<String>`. `dl`'s `-- <command>` 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<String>) | ::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<NonEmpty<String>>`. An empty argv is not a smaller answer but a different one -- `build_dl_args` would emit `[<spec>, "--"]`, 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 <ws> -- echo <NUL>hi` is reported as `echo '<NUL>hi'` rather than `echo <NUL>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 -- <cmd>` 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
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThe PR replaces ambiguous remote command strings with a typed argv-or-script model, centralizes shell-line construction in core, and propagates that distinction through launch, agent detection, dotfiles, and public attach APIs. It also makes empty agent commands unrepresentable and updates error semantics and regression tests to reflect the new composition boundary. Sequence diagram for typed command wrappingsequenceDiagram
participant DL
participant Launch
participant RemoteCommand
participant RemotePayload
participant RemoteShell
alt argv from dl
DL->>Launch: Attach(RemoteCommand::Argv(words))
Launch->>RemoteCommand: line()
RemoteCommand-->>Launch: shell::join(words)
else composed dotfiles script
Launch->>RemoteCommand: Script(script)
RemoteCommand-->>Launch: line()
end
Launch->>RemotePayload: wrap(command, zellij)
RemotePayload->>RemoteShell: bash -lc quoted(line)
RemoteShell-->>RemotePayload: execute command
Flow diagram for exact agent detectionflowchart LR
Command["RemoteCommand"] --> Kind{Command shape}
Kind -->|Argv| Assign["Skip assignment prefixes"]
Assign --> Named["agent_named(first program word)"]
Kind -->|Script| Split["agent_in(script)"]
Split --> Guess["Whitespace-based program guess"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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<String>` to `Option<RemoteCommand>` 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
…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
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The escalation from #591's review, now that it has its own PR.
The shape, not the instance
Two callers meant opposite things by the same
Option<String>:dl's-- <command>handed it words whose quoting the host's shell had already stripped.dotfiles_commandhanded it a script it composed itself,&&and$(...)included.The type could not tell them apart. So the join that turns the first into a command line lived up in
dl, and the second survived only by nobody joining it. #591 fixed one instance of that; this removes the shape that allowed it.RemoteCommand::line()is now the one place either sense becomes a command line, sodlno 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 a reading to pick.Two things fall out of holding words instead of a line
Agent detection gets exact.
RemoteCommand::agenttakes the first word that is not an assignment prefix.herdr::agent_inhad to split on whitespace, which would have taken'myout of a quoted program name. The script arm still uses the whitespace guess, because a script is the caller's own syntax and there is nothing better to do with it.The reachable half of the finding closes.
LaunchVerb::Attachispub, so an external caller could hand itsOption<String>a pre-composed line and get the old dangerous reading. It now takes aRemoteCommand.aid's half of the same finding
build_agent_commandreturnsOption<NonEmpty<String>>. An empty argv is not a smaller answer but a different one:build_dl_argswould emit[<spec>, "--"], anddlreads a separator with nothing after it as a plain interactive attach — an agent was asked for and a shell arrives.dlre-exportsNonEmptyfor it, since aid may name nothing butdl; dl carries no public-api snapshot, so that re-export moves none.One behaviour change, in an error rather than a payload
UnquotableCommandnow carries the composed line, so a NUL indl <ws> -- echo <NUL>hiis reported asecho '<NUL>hi'rather thanecho <NUL>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::joinquotes the offending word rather than rejecting it, so the NUL is still there whenposix_quotelooks at the line.Every payload assertion is unchanged
The core tests that modelled
dl -- <cmd>buildArgvnow, and their expectations did not move, because both words were already shell-safe. The exception is the one that spelled the bug —claude 'fix the bug'— which was never one argument and now is.public-api
This deliberately moves the
devlaunch-coresnapshots. I could not regenerate them here:cargo-public-api0.52.0 wants to buildopenssl-sys, and the devcontainer has no OpenSSL headers. Thepublic-apijob prints the exact diff, so the snapshots in this PR are whatever CI says they should be rather than my guess at them.Verification
2406 rust tests pass,
cargo fmt --check,cargo clippy --locked --all-targets -- -D warningsclean, 761 in the Python guards,prekclean.🤖 Generated with Claude Code
Summary by Sourcery
Represent remote commands by their intended shape so argument vectors and shell scripts are serialized safely and consistently.
Bug Fixes:
Enhancements:
LaunchVerb::AttachAPI distinguish argument vectors from shell scripts.dlfor agent command construction.CI:
Documentation:
Tests:
Chores: