Skip to content

fix: quote automation commands for the host shell - #700

Draft
NickJosevski wants to merge 1 commit into
mainfrom
nj/issue-72
Draft

fix: quote automation commands for the host shell#700
NickJosevski wants to merge 1 commit into
mainfrom
nj/issue-72

Conversation

@NickJosevski

Copy link
Copy Markdown
Contributor

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: --shell flag → OCTOPUS_SHELL env var → Shell config key (octopus config set Shell cmd) → detection. Detection uses $SHELL on 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.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.

Open questions / options

Everything below is a real decision, not a rhetorical one.

  1. 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.

  2. What detection should do when it can't tell. On Windows I default to cmd when 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 to powershell and telling cmd users to set the config value.

  3. Config key name. I used Shell (octopus config set Shell cmd), matching the existing Editor / OutputFormat style, with OCTOPUS_SHELL and --shell. Alternatives: AutomationShell / OCTOPUS_AUTOMATION_SHELL, which is more precise about what it affects but wordier. Also worth deciding whether --shell deserves 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.

  4. What I could not verify without a real Windows box. I have no Windows machine here, so:

    • the cmd escaping is verified against a simulator of cmd's rules plus an implementation of the Windows argv rules, not against real cmd.exe. The rules I implemented are the well-documented ones, but a five-minute check on a real box against --project "Bob's & Co", 100% Done and Say "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 the cmd default. Suggestions welcome for a better signal — I deliberately did not use PSModulePath, since it's a machine-level variable that cmd.exe inherits too and so is a false positive generator.
    • PowerShell 5.1 mangles native-command arguments containing " 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.
  5. 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

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better support for quoted string values in CLI input flags when generating automation commands

1 participant