fix: quote automation commands for the host shell - #700
Draft
NickJosevski wants to merge 1 commit into
Draft
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>
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
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.
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