fix: quote automation commands for the host shell

[NickJosevski]

Fixes #72

The problem

flag.GenerateAutomationCmd wrapped 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/shell package with a Shell type and per-shell quoting:

• bash (covers sh/bash/zsh/ksh/dash) — single quotes, ' escaped as '\''
• powershell (covers pwsh) — single quotes, ' escaped by doubling
• cmd — double quotes, plus the two-layer escaping cmd needs (see below)

Values made only of characters with no meaning to the target shell are emitted bare, so --environment Dev and --version 0.0.3 now 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: OCTOPUS_SHELL env var → Shell config key (octopus config set Shell cmd) → detection. Detection asks the parent process first (it names the shell the command was actually typed into), then $SHELL on unix, defaulting to bash; on Windows the parent process is the only reliable signal and the default is 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.

A generated command that cannot be pasted safely carries a yellow warning, the same shape as the existing sensitive-variable warning: %, ! and line breaks for cmd, and the three values Windows PowerShell 5.1 corrupts on its way to a native command (a trailing backslash, an embedded double quote, an empty value).

Only flag.GenerateAutomationCmd did 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:

• a literal " 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.
• runs of \ 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 deploy with project Soft Drinks, version 0.0.3, environment Dev, tenant tag Regions/us-east:

	generated command
before (all shells)	octopus release deploy --project 'Soft Drinks' --version '0.0.3' --environment 'Dev' --tenant-tag 'Regions/us-east' --no-prompt
after, bash	octopus release deploy --project 'Soft Drinks' --version 0.0.3 --environment Dev --tenant-tag Regions/us-east --no-prompt
after, powershell	octopus release deploy --project 'Soft Drinks' --version 0.0.3 --environment Dev --tenant-tag Regions/us-east --no-prompt
after, cmd	octopus release deploy --project "Soft Drinks" --version 0.0.3 --environment Dev --tenant-tag Regions/us-east --no-prompt

Awkward values:

value	bash	powershell	cmd
Dev	Dev	Dev	Dev
Soft 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"
(empty)	''	''	""

Test evidence

pkg/util/shell/quote_test.go is 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 string
• TestQuoteCmd_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-identical
• TestQuotePosix_RoundTrip — the same values through a real /bin/sh, asserting printf '%s' prints exactly the input
• TestQuotePowerShell_RoundTrip — same, through a real pwsh if one is installed; skips otherwise (it skips on CI and it skipped locally)
• TestParse / TestValidate / TestDetect, and pkg/util/flag/flag_test.go covering the assembled command per shell (strings, string slices, bools, ints, secure flags)

Results from the worktree:

go build ./...                     -> clean
GOOS=windows GOARCH=amd64 go build ./...  -> clean
go test ./pkg/...                  -> all packages pass
go vet ./pkg/...                   -> only pre-existing "unreachable code" warnings in unrelated files

The one existing assertion that pinned the old output (TestDeployCreate_GenerationOfAutomationCommand_MasksSensitiveVariables) was updated for the now-unquoted --version 2.0 --environment dev, and pins OCTOPUS_SHELL so it doesn't depend on where the tests run.

Decisions — settled on existing convention

• Shell-specific quoting, not one lowest-common-denominator format. A single double-quoted format works for cmd + PowerShell + bash on simple values but breaks on $ (expanded by bash and PowerShell inside double quotes) and ` (PowerShell escape), both legal in Octopus entity names. The repo has already chosen correctness over simplicity on exactly this trade-off: release create --package carries a documented backslash-escape syntax rather than banning colons in package IDs.
• cmd is the Windows fallback when detection fails. The degradation is asymmetric: cmd's double-quoted output is also valid in PowerShell for almost every value, while PowerShell's single-quoted output is broken in cmd — the same reason the bug in Better support for quoted string values in CLI input flags when generating automation commands #72 exists. PowerShell users who want single quotes can set octopus config set Shell powershell.
• Config key Shell + OCTOPUS_SHELL, no --shell flag. Editor and ShowOctopus are config keys with no flag, and the persistent flag set in NewCmdRoot is a deliberately tiny curated list — every entry in it appears in every command's help and in the generated docs. pkg/config binds ConfigShell to OCTOPUS_SHELL through viper exactly as ConfigEditor binds to VISUAL/EDITOR, so precedence holds without a flag. An invalid OCTOPUS_SHELL warns rather than fails: it is exported once from a profile and applies to every command afterwards, so rejecting it would lock the user out of the config set Shell needed to fix it.
• Warn when the generated command cannot be used verbatim. release deploy and runbook run already print a yellow warning when the automation command is not directly runnable (sensitive values replaced with placeholders). PasteWarning gives the shells' own limitations the same treatment rather than emitting text that is silently mangled.

Verified on a real Windows box

The three caveats this PR opened with have all been closed:

• cmd escaping is round-tripped through a real cmd.exe prompt and handed to a native command from both PowerShells, asserting the value arrives byte for byte. The simulator-based test stays because it runs everywhere. Non-ASCII values are skipped in the cmd round trip only — the harness reaches cmd through a pipe and cmd decodes redirected stdin with an OEM codepage; a real prompt is handed UTF-16 and doesn't have the problem, so those values are covered by the round trips that pass arguments as UTF-16.
• parent_windows.go (Toolhelp32 snapshot of the parent process) had never been executed; it now has a smoke test. A snapshot that returns nothing is indistinguishable from an unrecognised shell, since both fall through to the cmd default. PSModulePath was deliberately not used as a signal — it is machine-level and cmd.exe inherits it too.
• Windows PowerShell 5.1 turned out to corrupt a third shape of value beyond the two already documented: an empty argument is dropped from the command line entirely, so the flag before it swallows whatever came next. All three are warned about rather than silently emitted.

A windows-latest job in pr-validation.yml runs ./pkg/util/shell/... ./pkg/util/flag/...; the posix round trips moved to a !windows file so they don't hard-fail there looking for sh and zsh. OCTOPUS_TEST_ARGVECHO takes a pre-built helper so the suite can be cross-compiled and run on a Windows box with no Go toolchain.

🤖 Generated with Claude Code

[NickJosevski]

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.

[NickJosevski]

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 sees 100" + 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.

[NickJosevski]

Actioned in 102520c (docs) and ad69548 (the warning).

quoteCmd's "can't be fixed" list now has three entries and % is one of them, scoped the way you describe: the caret doesn't escape % in either mode, percent expansion being an earlier parsing phase than caret processing; the interactive prompt survives only because it leaves an unmatched % and an undefined %var% alone, and a batch file drops the former and deletes the latter. The comment also records that %% is the batch escape and doesn't collapse at the prompt, so no single encoding satisfies both and the interactive one is the one worth having.

simulateCmd's doc comment now says outright that it models the interactive prompt and that a value containing % doesn't survive a batch file, so the round trip test no longer reads as if it covered both.

On the warning: pkg/util/shell/warn.go holds a per-shell table of characters a quoted value can't reliably carry, and pkg/util/flag/flag.go appends the result to the generated command. % is in it with batch-specific wording — "is expanded before any escaping is applied, so it only survives at the interactive prompt; a .bat or .cmd script drops an unmatched % and replaces %var%" — alongside ! and a line break. Covered by TestPasteWarning in pkg/util/shell/warn_test.go: cmd warns about a percent (100% Cotton), cmd warns about an environment variable (%PATH%), and bash is always fine / powershell is always fine to pin that the warning is cmd-only.

Unverified by execution: there's no cmd.exe or Windows host available here, so both the prompt and the batch behaviours are reasoned from the documented parsing phases rather than run. TestQuoteCmd_RoundTrip exercises the model, not a real cmd — which is the limitation the comment now admits to rather than hides.

[NickJosevski]

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.

[NickJosevski]

Actioned in 8a88e5f.

All four variants are now doubled alongside the ASCII quote, via a strings.NewReplacer rather than the single ReplaceAll:

var powerShellQuoteEscaper = strings.NewReplacer(
	`'`, `''`,
	"‘", "‘‘", // left single quotation mark
	"’", "’’", // right single quotation mark
	"‚", "‚‚", // single low-9 quotation mark
	"‛", "‛‛", // single high-reversed-9 quotation mark
)

Covered by two cases in TestQuote: curly apostrophe (Bob’s Project → 'Bob’’s Project') and other smart single quotes (a‘b‚c‛d → 'a‘‘b‚‚c‛‛d'), each asserting bash and cmd leave them alone so the PowerShell-only scoping is pinned too. Bob’s Project is also in roundTripValues, so it goes through TestQuotePowerShell_RoundTrip and TestQuotePowerShell_RoundTripToNativeCommand on CI.

Two things worth recording:

Unverified by execution on my side — there is no pwsh or powershell on this machine (command -v finds neither), so the PowerShell round trip tests skipped locally and I'm taking your pwsh 7 result plus the tokenizer's documented character classes rather than reproducing it. about_Quoting_Rules lists exactly these four as single quotes, and the tokenizer's terminator check is on the character class rather than a match against the opening quote, which is why an ASCII-opened string is closed by a curly one; the doubling works because the escape rule is "the next character is also a single quote", not "the same character again". The pwsh round trip does run on CI (pwsh is on the ubuntu image, and lookShell fails rather than skips when CI is set), so the assertion is checked against a real parser there.

The smart double quotes — U+201C, U+201D, U+201E — are deliberately not in the replacer. They're in the same documented list, but the output is always a single quoted string and nothing terminates that except a single quote, so they're literal. isBare already forces quoting for them as non-ASCII.

[NickJosevski]

= 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).

[NickJosevski]

Actioned in 1491852.

= stays in posixSafeChars — mid-word it is harmless, and taking it out would quote every --variable Name=Value needlessly. The leading-position case is handled where ~ is, in quotePosix:

// = is harmless in the middle of a word but a word which starts with one is subject
// to zsh's =cmd expansion, so `=foo` aborts the whole command with "foo not found".
if !strings.HasPrefix(value, "=") && isBare(value, posixSafeChars) {
	return value
}

Reproduced here on zsh 5.9 (macOS /bin/zsh --no-rcs -c), and the silent case is worse than the aborting one:

zsh  printf '%s' =foo   → "zsh:1: foo not found", exit 1
sh   printf '%s' =foo   → "=foo", exit 0
zsh  printf '%s' '=foo' → "=foo", exit 0
zsh  printf '%s' a=b    → "a=b", exit 0
zsh  printf '%s' =ls    → "/bin/ls", exit 0

=foo fails loudly, but a value that happens to name a real command — =ls — is substituted with its path and the command runs with the wrong argument, no error. That's the one that argues for quoting rather than documenting.

Covered by leading equals (=foo → '=foo', unquoted for PowerShell and cmd) and embedded equals (a=b stays bare everywhere) in TestQuote, and =foo is in roundTripValues. TestQuoteZsh_RoundTrip was added in the same commit and does run here — 32 subtests pass, including "=foo" and "a=b"; zsh is installed on the CI runner by pr-validation.yml so it runs there too.

[NickJosevski]

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.

[NickJosevski]

Actioned in e89ef0d, then softened for the environment variable in 30e63a0. Doc comment corrected in 07cfce7.

The finding holds — Parse failing meant Current fell through to detection, so --shell powershel silently quoted for the detected shell. Validation now lives in root's PersistentPreRunE (which became PersistentPreRunE for it), but the two inputs are treated differently:

• --shell is a hard error: --shell: the provided value fish is not a valid shell, please use one of bash, powershell, cmd.
• OCTOPUS_SHELL warns on stderr and falls back to detection.

The split is the part I'd like you to confirm, because your comment asked for an error "or at least a warning" and I've landed on one of each. The reason for not failing on the environment variable: it's exported once, usually from a shell profile, and applies to every subsequent command, so rejecting it locks the user out of the whole CLI — including the octopus config set Shell they'd need to fix it. --shell doesn't have that problem: it was typed for one command and retyping it is the fix. The first version (e89ef0d) did fail on both; 30e63a0 walked the env var back for exactly that reason.

Covered by pkg/cmd/root/root_test.go: TestRoot_InvalidShellFlagIsRejected, TestRoot_ValidShellFlagIsAccepted, TestRoot_InvalidShellEnvIsWarnedAboutbutNotFatal, TestRoot_ValidShellEnvIsSilent, and TestRoot_ShellFlagSupersedesInvalidEnv (a good flag alongside a bad env var is silent, since the flag is the more specific signal). All pass.

Two residuals, both deliberate but say if you disagree:

1. The config file value is not checked in PersistentPreRunE at all, and Current ignores a bad one silently. config set Shell validates on the way in, so a bad value there can only come from hand editing the file. 07cfce7 fixes the code comment, which wrongly claimed the file value was warned about too.
2. The warning goes to stderr unconditionally, so a scripted --no-prompt run with a stale OCTOPUS_SHELL exported will emit it on every invocation. That seemed better than silence, but it is noise if anyone is in that state.

Open question: is one-error-one-warning the split you want, or would you rather OCTOPUS_SHELL also be fatal and accept the lockout (recoverable with env -u OCTOPUS_SHELL octopus ...)?

[NickJosevski]

Two small gaps in the config surface:

1. 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.
2. config get's interactive promptMissing key list (pkg/cmd/config/get/get.go) wasn't given ConfigShell, so the new key appears in set's picker but not get's. (config get Shell by name still works, since IsValidKey goes through viper.AllKeys().)

[NickJosevski]

Both actioned in 144f46b, with tests added in 24e693c.

1. Empty now clears the setting rather than failing validation, so there's a way back to auto-detection without hand editing the file:

case strings.ToLower(constants.ConfigShell):
	// empty is the documented default and clears the setting, putting the CLI back
	// to detecting the shell it is running under
	if value != "" {
		if err := shell.Validate(value); err != nil {
			return err
		}
	}
	localViper.Set(key, value)

It writes "shell": "" rather than removing the key — viper has no unset — but that's the same state as never having set it, since Current calls Parse(""), gets false, and falls through to Detect.

2. constants.ConfigShell is now in get's promptMissing list too, so the key appears in both pickers.

There were no tests over any of this — pkg/cmd/config/{get,list,set} had no test files at all — so 24e693c adds pkg/cmd/config/set/set_test.go. TestSetRun_Shell is a table over the Shell case: a name is stored, an alias is stored as typed, empty clears the setting asserts the written file has shell: "", and powershel / fish are rejected with the shell package's message. TestSetRun_OtherKeys covers the NoPrompt and default branches either side of the new case plus the invalid-key path, so reshaping the switch can't quietly drop them. The test points HOME (and AppData) at a temp dir and sets the global viper up against it, since IsValidKey deliberately reaches out to the global instance.

One thing I didn't do: get's and set's key lists are still two hand-maintained copies, which is how the omission you found happened in the first place — set gained ConfigShell and get didn't. A shared constants.ConfigKeys slice would make the next key a one-line change, but it touches both commands beyond what this PR needs, so I've left it. Want it in here or as a follow-up?