fix: quote automation commands for the host shell

.github/workflows/integration-test.yml

@@ -62,6 +62,11 @@ jobs:
       - name: Setup gotestsum
         run: go install gotest.tools/gotestsum@latest
 
+      # the unit tests below include the shell quoting round trips, which need a real
+      # zsh; it isn't on the runner image
+      - name: Install zsh
+        run: sudo apt-get update && sudo apt-get install -y zsh
+
       # we don't technically need to run the unit tests but they're fast so why not
       - name: Unit Tests
         run: gotestsum --format testname --junitfile ../unit-tests.xml

.github/workflows/pr-validation.yml

@@ -28,6 +28,12 @@ jobs:
       - name: Setup gotestsum
         run: go install gotest.tools/gotestsum@latest
 
+      # the shell quoting round trip tests run the generated commands through the real
+      # shells; pwsh is already on the runner image but zsh isn't, and zsh is the strict
+      # one because it expands a leading = and ~ where the other posix shells don't
+      - name: Install zsh
+        run: sudo apt-get update && sudo apt-get install -y zsh
+
       - name: Unit Tests
         run: gotestsum --format testname --junitfile ../unit-tests.xml
         working-directory: ./pkg
@@ -39,3 +45,21 @@ jobs:
           name: Test Results
           path: '*-tests.xml'
           reporter: java-junit
+
+  # the quoting this repo generates for cmd.exe and for Windows PowerShell can only be
+  # checked on Windows, and the parent process detection it relies on doesn't compile
+  # anywhere else. Only the two packages that have Windows specific behaviour are run;
+  # the rest are already covered by the job above and don't need a second runner.
+  test-windows:
+    runs-on: windows-latest
+
+    steps:
+      - uses: actions/checkout@v7
+
+      - name: Set up Go
+        uses: actions/setup-go@v7
+        with:
+          go-version-file: go.mod
+
+      - name: Shell Quoting Tests
+        run: go test ./pkg/util/shell/... ./pkg/util/flag/...

pkg/cmd/config/get/get.go

@@ -65,6 +65,7 @@ func promptMissing(ask question.Asker) (string, error) {
 		constants.ConfigOutputFormat,
 		constants.ConfigShowOctopus,
 		constants.ConfigEditor,
+		constants.ConfigShell,
 		// 	constants.ConfigProxyUrl,
 	}
 

pkg/cmd/config/list/list.go

@@ -49,6 +49,7 @@ func listRun(cmd *cobra.Command) error {
 		Host         string `json:"host"`
 		NoPrompt     string `json:"noprompt"`
 		OutputFormat string `json:"outputformat"`
+		Shell        string `json:"shell"`
 		Space        string `json:"space"`
 	}
 
@@ -74,6 +75,8 @@ func listRun(cmd *cobra.Command) error {
 				configData.Space = configFile.GetString(key)
 			case strings.ToLower(constants.ConfigOutputFormat):
 				configData.OutputFormat = configFile.GetString(key)
+			case strings.ToLower(constants.ConfigShell):
+				configData.Shell = configFile.GetString(key)
 			default:
 				return fmt.Errorf("the key '%s' is not a supported config option", key)
 			}

pkg/cmd/config/set/set.go

@@ -10,6 +10,7 @@ import (
 	"github.com/OctopusDeploy/cli/pkg/constants"
 	"github.com/OctopusDeploy/cli/pkg/factory"
 	"github.com/OctopusDeploy/cli/pkg/question"
+	"github.com/OctopusDeploy/cli/pkg/util/shell"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
 )
@@ -67,13 +68,23 @@ func setRun(isPromptEnabled bool, ask question.Asker, key string, value string)
 		key = k
 	}
 	key = strings.ToLower(key)
-	if key == strings.ToLower(constants.ConfigNoPrompt) {
+	switch key {
+	case strings.ToLower(constants.ConfigNoPrompt):
 		boolValue, err := strconv.ParseBool(value)
 		if err != nil {
 			return fmt.Errorf("the provided value %s is not valid for NoPrompt, please use true of false", value)
 		}
 		localViper.Set(key, boolValue)
-	} else {
+	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)
+	default:
 		localViper.Set(key, value)
 	}
 	if err := localViper.WriteConfig(); err != nil {
@@ -91,6 +102,7 @@ func promptMissing(ask question.Asker, key string) (string, string, error) {
 		constants.ConfigOutputFormat,
 		constants.ConfigShowOctopus,
 		constants.ConfigEditor,
+		constants.ConfigShell,
 		// constants.ConfigProxyUrl,
 	}
 

pkg/cmd/config/set/set_test.go

@@ -0,0 +1,109 @@
+package set
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/OctopusDeploy/cli/pkg/config"
+	"github.com/spf13/viper"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// isolateConfig points the config path at a temporary directory so the test writes
+// there rather than to the config file of whoever is running it, and sets the global
+// viper up so IsValidKey knows about the keys.
+func isolateConfig(t *testing.T) string {
+	t.Helper()
+
+	dir := t.TempDir()
+	t.Setenv("HOME", dir)    // unix
+	t.Setenv("AppData", dir) // windows
+
+	// IsValidKey deliberately reaches out to the global viper, so it has to be set up
+	// against the temporary directory rather than the real config file
+	viper.Reset()
+	require.NoError(t, config.Setup(viper.GetViper()))
+	t.Cleanup(viper.Reset)
+
+	path, err := config.EnsureConfigPath()
+	require.NoError(t, err)
+	return filepath.Join(path, "cli_config.json")
+}
+
+func readConfig(t *testing.T, file string) map[string]any {
+	t.Helper()
+
+	contents, err := os.ReadFile(file)
+	require.NoError(t, err)
+
+	var settings map[string]any
+	require.NoError(t, json.Unmarshal(contents, &settings))
+	return settings
+}
+
+func TestSetRun_Shell(t *testing.T) {
+	tests := []struct {
+		name    string
+		value   string
+		wantErr string
+		want    string
+	}{
+		{name: "a shell name is stored", value: "powershell", want: "powershell"},
+		{name: "an alias is stored as typed", value: "pwsh", want: "pwsh"},
+		// empty is the documented default; it has to be accepted so there is a way back
+		// to auto detection without hand editing the config file
+		{name: "empty clears the setting", value: "", want: ""},
+		{name: "a misspelling is rejected", value: "powershel", wantErr: "the provided value powershel is not a valid shell, please use one of bash, powershell, cmd"},
+		{name: "an unsupported shell is rejected", value: "fish", wantErr: "the provided value fish is not a valid shell, please use one of bash, powershell, cmd"},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			file := isolateConfig(t)
+
+			err := setRun(false, nil, "Shell", test.value)
+
+			if test.wantErr != "" {
+				assert.EqualError(t, err, test.wantErr)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, test.want, readConfig(t, file)["shell"])
+		})
+	}
+}
+
+// the switch in setRun grew a Shell case; this covers the two branches either side of
+// it so that reshaping it can't quietly drop them
+func TestSetRun_OtherKeys(t *testing.T) {
+	t.Run("NoPrompt parses a bool", func(t *testing.T) {
+		file := isolateConfig(t)
+
+		require.NoError(t, setRun(false, nil, "NoPrompt", "true"))
+		assert.Equal(t, true, readConfig(t, file)["noprompt"])
+	})
+
+	t.Run("NoPrompt rejects a non bool", func(t *testing.T) {
+		isolateConfig(t)
+
+		err := setRun(false, nil, "NoPrompt", "yes please")
+		assert.EqualError(t, err, "the provided value yes please is not valid for NoPrompt, please use true of false")
+	})
+
+	t.Run("any other key is stored as given", func(t *testing.T) {
+		file := isolateConfig(t)
+
+		require.NoError(t, setRun(false, nil, "Space", "Default"))
+		assert.Equal(t, "Default", readConfig(t, file)["space"])
+	})
+
+	t.Run("an unknown key is rejected", func(t *testing.T) {
+		isolateConfig(t)
+
+		err := setRun(false, nil, "NotAKey", "x")
+		assert.EqualError(t, err, "the key 'NotAKey' is not a valid")
+	})
+}

pkg/cmd/release/deploy/deploy_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/OctopusDeploy/cli/pkg/executor"
 	"github.com/OctopusDeploy/cli/pkg/question"
 	"github.com/OctopusDeploy/cli/pkg/surveyext"
+	"github.com/OctopusDeploy/cli/pkg/util/shell"
 	"github.com/OctopusDeploy/cli/test/fixtures"
 	"github.com/OctopusDeploy/cli/test/testutil"
 	"github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/channels"
@@ -2312,6 +2313,9 @@ func TestDeployCreate_AutomationMode(t *testing.T) {
 
 // this happens outside the scope of the normal AskQuestions flow so warrants its own integration-style test
 func TestDeployCreate_GenerationOfAutomationCommand_MasksSensitiveVariables(t *testing.T) {
+	// pin the shell so the expected command doesn't depend on where the tests are run
+	t.Setenv(constants.EnvOctopusShell, string(shell.Bash))
+
 	const spaceID = "Spaces-1"
 	const fireProjectID = "Projects-22"
 
@@ -2492,7 +2496,7 @@ func TestDeployCreate_GenerationOfAutomationCommand_MasksSensitiveVariables(t *t
 		  Deployment Targets: All included
 		  Target Tags: All included
 
-		Automation Command: octopus release deploy --space 'Default Space' --project 'Fire Project' --version '2.0' --environment 'dev' --priority 'true' --variable 'Boring Variable:BORING' --variable 'Nuclear Launch Codes:*****' --variable 'Secret Password:*****' --no-prompt
+		Automation Command: octopus release deploy --space 'Default Space' --project 'Fire Project' --version 2.0 --environment dev --priority true --variable 'Boring Variable:BORING' --variable 'Nuclear Launch Codes:*****' --variable 'Secret Password:*****' --no-prompt
 		Warning: Command includes some sensitive variable values which have been replaced with placeholders.
 		Successfully started 2 deployment(s)
 

pkg/cmd/root/root.go

@@ -1,6 +1,9 @@
 package root
 
 import (
+	"fmt"
+	"os"
+
 	"github.com/OctopusDeploy/cli/pkg/apiclient"
 	accountCmd "github.com/OctopusDeploy/cli/pkg/cmd/account"
 	apiCmd "github.com/OctopusDeploy/cli/pkg/cmd/api"
@@ -26,7 +29,9 @@ import (
 	workerPoolCmd "github.com/OctopusDeploy/cli/pkg/cmd/workerpool"
 	"github.com/OctopusDeploy/cli/pkg/constants"
 	"github.com/OctopusDeploy/cli/pkg/factory"
+	"github.com/OctopusDeploy/cli/pkg/output"
 	"github.com/OctopusDeploy/cli/pkg/question"
+	"github.com/OctopusDeploy/cli/pkg/util/shell"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
 )
@@ -116,7 +121,21 @@ func NewCmdRoot(f factory.Factory, clientFactory apiclient.ClientFactory, askPro
 	// if we attempt to check the flags before Execute is called, cobra hasn't parsed anything yet,
 	// so we'll get bad values. PersistentPreRun is a convenient callback for setting up our
 	// environment after parsing but before execution.
-	cmd.PersistentPreRun = func(_ *cobra.Command, _ []string) {
+	cmd.PersistentPreRunE = func(cmd *cobra.Command, _ []string) error {
+		// OCTOPUS_SHELL is warned about rather than rejected: it is exported once, usually
+		// from a shell profile, and applies to every command afterwards, so failing would
+		// lock the user out of the whole CLI, including the `config set Shell` needed to
+		// fix it. Current falls back to detecting the host shell in that case.
+		//
+		// The config file value isn't checked here at all. `config set Shell` validates
+		// on the way in, so a bad value there can only come from hand editing the file,
+		// and Current ignores it the same way.
+		if v := os.Getenv(constants.EnvOctopusShell); v != "" {
+			if err := shell.Validate(v); err != nil {
+				fmt.Fprintf(cmd.ErrOrStderr(), "%s\n", output.Yellow(fmt.Sprintf("Warning: ignoring %s: %s", constants.EnvOctopusShell, err)))
+			}
+		}
+
 		// map flag alias values
 		for k, v := range flagAliases {
 			for _, aliasName := range v {
@@ -138,6 +157,8 @@ func NewCmdRoot(f factory.Factory, clientFactory apiclient.ClientFactory, askPro
 		if spaceNameOrId := viper.GetString(constants.ConfigSpace); spaceNameOrId != "" {
 			clientFactory.SetSpaceNameOrId(spaceNameOrId)
 		}
+
+		return nil
 	}
 
 	cmd.RunE = func(cmd *cobra.Command, args []string) error {

pkg/cmd/root/root_test.go

@@ -0,0 +1,57 @@
+package root_test
+
+import (
+	"bytes"
+	"testing"
+
+	cmdRoot "github.com/OctopusDeploy/cli/pkg/cmd/root"
+	"github.com/OctopusDeploy/cli/pkg/constants"
+	"github.com/OctopusDeploy/cli/test/testutil"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// runVersion executes a command that needs neither a server nor prompting, so the only
+// thing under test is the shell validation in PersistentPreRunE.
+func runVersion(t *testing.T, args ...string) (string, error) {
+	t.Helper()
+
+	stdout := &bytes.Buffer{}
+	stderr := &bytes.Buffer{}
+
+	rootCmd := cmdRoot.NewCmdRoot(testutil.NewMockFactory(testutil.NewMockHttpServer()), nil, nil)
+	rootCmd.SetOut(stdout)
+	rootCmd.SetErr(stderr)
+	rootCmd.SetArgs(append([]string{"version"}, args...))
+
+	err := rootCmd.Execute()
+	return stderr.String(), err
+}
+
+// the shell is a config setting and an env var, never a flag; Editor and ShowOctopus are
+// the same shape. The persistent flag set is deliberately small, and every entry in it
+// shows up in each command's help and in the generated docs.
+func TestRoot_ThereIsNoShellFlag(t *testing.T) {
+	_, err := runVersion(t, "--shell", "bash")
+	require.Error(t, err)
+	assert.EqualError(t, err, "unknown flag: --shell")
+}
+
+// an invalid OCTOPUS_SHELL is warned about rather than rejected; it is set once and
+// applies to every command afterwards, so failing would lock the user out of the CLI
+// entirely, including the config command needed to fix it.
+func TestRoot_InvalidShellEnvIsWarnedAboutbutNotFatal(t *testing.T) {
+	t.Setenv(constants.EnvOctopusShell, "fish")
+
+	stderr, err := runVersion(t)
+	assert.NoError(t, err)
+	assert.Contains(t, stderr, "Warning: ignoring OCTOPUS_SHELL: the provided value fish is not a valid shell")
+}
+
+func TestRoot_ValidShellEnvIsSilent(t *testing.T) {
+	t.Setenv(constants.EnvOctopusShell, "pwsh")
+
+	stderr, err := runVersion(t)
+	assert.NoError(t, err)
+	assert.Equal(t, "", stderr)
+}

pkg/config/config.go

@@ -30,6 +30,7 @@ func setDefaults(v *viper.Viper) {
 	//	v.SetDefault(constants.ConfigProxyUrl, "")
 	v.SetDefault(constants.ConfigShowOctopus, true)
 	v.SetDefault(constants.ConfigOutputFormat, "table")
+	v.SetDefault(constants.ConfigShell, "")
 
 	if runtime.GOOS == "windows" {
 		v.SetDefault(constants.ConfigEditor, "notepad")
@@ -58,6 +59,9 @@ func bindEnvironment(v *viper.Viper) error {
 	if err := v.BindEnv(constants.ConfigNoPrompt, constants.EnvCI); err != nil {
 		return err
 	}
+	if err := v.BindEnv(constants.ConfigShell, constants.EnvOctopusShell); err != nil {
+		return err
+	}
 	return nil
 }
 

pkg/constants/constants.go

@@ -38,13 +38,15 @@ const (
 	ConfigEditor       = "Editor"
 	ConfigShowOctopus  = "ShowOctopus"
 	ConfigOutputFormat = "OutputFormat"
+	ConfigShell        = "Shell"
 )
 
 const (
 	EnvOctopusUrl         = "OCTOPUS_URL"
 	EnvOctopusApiKey      = "OCTOPUS_API_KEY"
 	EnvOctopusAccessToken = "OCTOPUS_ACCESS_TOKEN"
 	EnvOctopusSpace       = "OCTOPUS_SPACE"
+	EnvOctopusShell       = "OCTOPUS_SHELL"
 	EnvEditor             = "EDITOR"
 	EnvVisual             = "VISUAL"
 	EnvCI                 = "CI"