fix: quote automation commands for the host shell - #700
Conversation
Generated automation commands always used single quotes, which cmd.exe passes through verbatim, so the command fails to find the entity. Values are now quoted using the rules of the shell the CLI is running under, and values needing no quoting are emitted bare. The shell can be forced with `octopus config set Shell cmd`, OCTOPUS_SHELL, or --shell. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
NickJosevski
left a comment
There was a problem hiding this comment.
Reviewed with a focus on shell-quoting edge cases. The core design (per-shell quoting, cmd two-pass escaping, round-trip tests against real parsers) holds up well; the comments below are the cases that survived verification — two were reproduced against real shells (pwsh 7, zsh 5.9), the cmd/batch one analytically against cmd's documented phase rules.
| sb.WriteString(strings.Repeat(`\`, backslashes*2)) | ||
| backslashes = 0 | ||
| sb.WriteString(`"\^""`) | ||
| case '%': |
There was a problem hiding this comment.
The "^%" escape only holds on the interactive cmd prompt. Inside a batch file (.bat/.cmd) — arguably the main automation-command destination — percent expansion is an earlier phase with different rules: a lone unmatched % is stripped, and an undefined %...% construct is deleted outright. So:
100% Done→"100"^%" Done"→ batch reduces it to"100"^" Done"→ argv sees100"+Done%PATH%→""^%"PATH"^%""→ batch deletes the whole%"PATH"^%run
The batch escape is %%, but that doesn't collapse on the interactive line, so no single encoding satisfies both. In neither mode does ^ actually escape % (percent expansion runs before caret processing); interactive survives only because undefined-variable references are left untouched there — which is also the assumption simulateCmd bakes in, so the round-trip test can't see this. Worth promoting % into the "can't be fixed" list above (scoped to batch files), and it's another candidate for the warning you float in open question 5.
|
|
||
| // quotePowerShell quotes for PowerShell. Single quoted strings are literal, and a single | ||
| // quote is escaped by doubling it. | ||
| func quotePowerShell(value string) string { |
There was a problem hiding this comment.
PowerShell's tokenizer treats the Unicode single-quote variants — U+2018 ‘, U+2019 ’, U+201A ‚, U+201B ‛ — as string delimiters equivalent to '. Only ASCII ' is doubled here, so a value containing a curly apostrophe (easy to acquire from a web UI or Word, e.g. Bob’s Project) produces 'Bob’s Project', which pwsh rejects as a parse error.
Verified on pwsh 7: /bin/echo 'Bob’s Project' exits 1 with a parse error, while 'Bob’’s Project' round-trips byte-identically — so doubling all four variants alongside ' fixes it. (isBare already forces quoting for these since they're non-ASCII; the gap is only in the escaping.) Bash and cmd don't treat smart quotes specially, so this is PowerShell-only.
| // Characters which carry no special meaning to the shell and so never need quoting. | ||
| // Letters and digits are always safe and aren't repeated here. | ||
| const ( | ||
| posixSafeChars = `@%+=:,./-_` |
There was a problem hiding this comment.
= in the bare set breaks zsh, which this mode explicitly covers: a word starting with = undergoes zsh's =cmd expansion, so a bare =foo makes the whole command abort — verified with zsh 5.9: echo =foo → zsh: foo not found, exit 1. Mid-word = (a=b) is fine; it only needs the leading-position treatment, i.e. quote when the value begins with = (same category of hazard as ~, which you handled by exclusion).
|
|
||
| // Current returns the shell to generate automation commands for; the explicitly | ||
| // configured shell if there is one, otherwise the detected host shell. | ||
| func Current() Shell { |
There was a problem hiding this comment.
An unrecognized --shell / OCTOPUS_SHELL value is silently swallowed: Parse fails, so Current falls through to detection and --shell powershel (typo) quietly emits detected-shell quoting — while config set Shell rejects the same value with an error. Since the user explicitly asked for a shell, consider validating the flag/env value (e.g. in root's PersistentPreRun) so an invalid choice errors, or at least warns, instead of being ignored.
| localViper.Set(key, boolValue) | ||
| } else { | ||
| case strings.ToLower(constants.ConfigShell): | ||
| if err := shell.Validate(value); err != nil { |
There was a problem hiding this comment.
Two small gaps in the config surface:
- There's no way back to auto-detect:
octopus config set Shell ""fails validation, so once set, the only reset is hand-editing the config file. Allowing empty (the documented default) to clear the key would fix that. config get's interactivepromptMissingkey list (pkg/cmd/config/get/get.go) wasn't givenConfigShell, so the new key appears inset's picker but notget's. (config get Shellby name still works, sinceIsValidKeygoes throughviper.AllKeys().)
zsh's EQUALS option, on by default, expands a word beginning with = to the path of the named command, so a bare `=foo` aborts the whole command with "foo not found" rather than passing the value through. The bash quoting covers zsh, so a leading = now forces quoting the same way ~ already does. Adds a zsh round trip test alongside the sh one; zsh is the stricter of the two so it catches this class of expansion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PowerShell's tokenizer accepts U+2018, U+2019, U+201A and U+201B as single quotes, so any of them closes a single quoted string in the same way ' does. A value carrying a curly apostrophe, which is easy to pick up from a web UI or Word, produced 'Bob’s Project' and PowerShell rejected it as a parse error. All four are now doubled alongside the ascii quote; doubling the same character is how PowerShell escapes it, so the value round trips unchanged. bash and cmd don't treat these characters specially, so this is PowerShell only. isBare already forced quoting for them since they aren't ascii; the gap was in the escaping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The caret doesn't escape %; percent expansion is an earlier parsing phase than caret processing. "^%" works at the prompt only because an unmatched % and an undefined %var% are left alone there. A batch file drops the first and deletes the second, so `100% Done` and `%PATH%` are both mangled when the command is pasted into a .bat or .cmd file. The batch escape is %%, which in turn doesn't collapse at the prompt, so no single encoding suits both and % joins newlines and delayed expansion in the can't-be-fixed list. The round trip test simulates the interactive prompt, which is noted on the simulator so it isn't read as evidence for batch files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Current falls back to detection when the configured value doesn't parse, so `--shell powershel` quietly produced detected-shell quoting while `config set Shell powershel` rejected the same value. The flag and the environment variable are now validated in the root PersistentPreRun, which becomes PersistentPreRunE so it can fail. The config file value is deliberately not validated there: a bad value hand-edited into the file would otherwise fail every command including the config commands needed to correct it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…fig get Two gaps in the config surface for the new key: - an empty value was rejected by validation, so once Shell was set the only way back to auto detection was hand editing the config file. Empty is the documented default, so it now clears the key. - config get's interactive key picker never listed Shell, though set's did. Getting it by name already worked, since IsValidKey goes through viper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes #72
The problem
flag.GenerateAutomationCmdwrapped every string value in single quotes. cmd.exe doesn't treat single quotes as quoting, so it hands them to the CLI verbatim and the server can't find'Soft Drinks'.Approach
New
pkg/util/shellpackage with aShelltype and per-shell quoting:'escaped as'\'''escaped by doublingValues made only of characters with no meaning to the target shell are emitted bare, so
--environment Devand--version 0.0.3now have no quotes at all (goal 1 in the issue). The safe set differs per shell —%is safe in bash but not in cmd,,is safe in bash but not in PowerShell.Shell selection, highest precedence first:
--shellflag →OCTOPUS_SHELLenv var →Shellconfig key (octopus config set Shell cmd) → detection. Detection uses$SHELLon unix (default bash) and the parent process name on Windows (default cmd, because double-quoted output also works in PowerShell whereas single-quoted output is broken in cmd — so the wrong guess degrades gracefully in only one direction).Only
flag.GenerateAutomationCmddid any quoting, so there was exactly one call site to change; the ~30 commands that call it are untouched.cmd.exe escaping
cmd is irregular enough to be worth spelling out. The generated text has to survive cmd's parsing and then the argv parsing Go does at startup:
"is emitted as"\^""— the surrounding quotes are closed around it so cmd's quote counting stays balanced, the quote is caret-escaped so cmd doesn't toggle on it, and argv sees\"which is a literal quote that keeps argv inside its quoted run. Plain""doubling is wrong here: Go's argv parser emits the quote but also leaves quoted mode, so a later space would split the argument.\are doubled when they hit a"(including the closing one), per the usual Windows argv rules%is expanded by cmd even inside double quotes and cannot be caret-escaped there, so it's emitted outside the quotes as"^%"Before / after
Issue example,
octopus release deploywith projectSoft Drinks, version0.0.3, environmentDev, tenant tagRegions/us-east:octopus release deploy --project 'Soft Drinks' --version '0.0.3' --environment 'Dev' --tenant-tag 'Regions/us-east' --no-promptoctopus release deploy --project 'Soft Drinks' --version 0.0.3 --environment Dev --tenant-tag Regions/us-east --no-promptoctopus release deploy --project 'Soft Drinks' --version 0.0.3 --environment Dev --tenant-tag Regions/us-east --no-promptoctopus release deploy --project "Soft Drinks" --version 0.0.3 --environment Dev --tenant-tag Regions/us-east --no-promptAwkward values:
DevDevDevDevSoft Drinks'Soft Drinks''Soft Drinks'"Soft Drinks"Bob's Project'Bob'\''s Project''Bob''s Project'"Bob's Project"Say "hi"'Say "hi"''Say "hi"'"Say "\^""hi"\^"""100% Done'100% Done''100% Done'"100"^%" Done"$PATH'$PATH''$PATH'"$PATH"C:\Program Files\App'C:\Program Files\App''C:\Program Files\App'"C:\Program Files\App"''''""Test evidence
pkg/util/shell/quote_test.gois the heart of the change:TestQuote— table-driven, every shell against plain values, spaces, single quotes, double quotes, backticks,$VAR,%VAR%, newlines, tildes, commas, non-ASCII, Windows paths, backslash-before-quote, and the empty stringTestQuoteCmd_RoundTrip— 29 awkward values pushed through a cmd.exe simulator (carets, quote-state tracking, and a hard failure on any unescaped%or metacharacter left outside quotes) and then through an implementation of the Windows argv rules, asserting the value comes back byte-identicalTestQuotePosix_RoundTrip— the same values through a real/bin/sh, assertingprintf '%s'prints exactly the inputTestQuotePowerShell_RoundTrip— same, through a realpwshif one is installed; skips otherwise (it skips on CI and it skipped locally)TestParse/TestValidate/TestDetect, andpkg/util/flag/flag_test.gocovering the assembled command per shell (strings, string slices, bools, ints, secure flags)Results from the worktree:
The one existing assertion that pinned the old output (
TestDeployCreate_GenerationOfAutomationCommand_MasksSensitiveVariables) was updated for the now-unquoted--version 2.0 --environment dev, and pinsOCTOPUS_SHELLso it doesn't depend on where the tests run.Open questions / options
Everything below is a real decision, not a rhetorical one.
Lowest-common-denominator vs shell-specific. I went shell-specific. A single double-quoted format looks tempting and does work for cmd + PowerShell + bash on simple values, but it breaks the moment a value contains
$(bash and PowerShell expand it inside double quotes) or`(PowerShell escape), and those are legal in Octopus entity names. The cost is that we now have three code paths and a detection problem. Happy to collapse to one format if the team prefers fewer moving parts over correctness on odd names.What detection should do when it can't tell. On Windows I default to
cmdwhen the parent process can't be identified. Rationale: cmd's double-quoted output is also valid in PowerShell for almost every value, whereas the reverse is not true at all. The counter-argument is that most Windows developers are in PowerShell and would see slightly less idiomatic output all the time to protect the minority of cmd users. The other option is defaulting topowershelland telling cmd users to set the config value.Config key name. I used
Shell(octopus config set Shell cmd), matching the existingEditor/OutputFormatstyle, withOCTOPUS_SHELLand--shell. Alternatives:AutomationShell/OCTOPUS_AUTOMATION_SHELL, which is more precise about what it affects but wordier. Also worth deciding whether--shelldeserves to be a global flag — it appears in every command's help and therefore in the generated docs. Config + env var alone would keep help output unchanged.What I could not verify without a real Windows box. I have no Windows machine here, so:
--project "Bob's & Co",100% DoneandSay "hi"would be worth doing before merge.parent_windows.go(Toolhelp32 snapshot of the parent process) cross-compiles cleanly but has never been executed. If the parent turns out to be something like Windows Terminal or a wrapper rather than the shell, detection falls through to thecmddefault. Suggestions welcome for a better signal — I deliberately did not usePSModulePath, since it's a machine-level variable that cmd.exe inherits too and so is a false positive generator."regardless of how the string literal is written (this is the known pre-7.3 argument-passing behaviour). Values with embedded double quotes may still not survive on Windows PowerShell; that is a PowerShell limitation rather than something this quoting can fix.Known unfixable in cmd. A newline in a value cannot be represented in a cmd command line at all — it's emitted literally and the command will break.
!is expanded when delayed expansion is enabled. Both are noted in the code comments. Do we want to warn the user when the generated command contains one of these, similar to the existing sensitive-variable warning?🤖 Generated with Claude Code