fix(shell): keep spaced command paths intact and explain Windows launch errors

hack/ci/suites.yaml

@@ -80,6 +80,14 @@ unit:
       run: '^(TestInvocationSuite|TestProviderDaemonOverrides.*|TestStdioTransportSuite|TestLaunch.*|TestEnvBlock.*)$'
       args: [-race, -count=1, -v, -timeout=5m]
 
+    - name: windows-provider-launch
+      description: native Windows provider executable launch and launch failure reporting
+      os: [windows]
+      packages: [./pkg/shell, ./pkg/client/clientimplementation]
+      run: '^(TestExecutablePathTestSuite|TestProviderCommandErrorTestSuite)$'
+      require_tests: [TestExecutablePathTestSuite, TestProviderCommandErrorTestSuite]
+      args: [-race, -count=1, -v, -timeout=5m]
+
     - name: ci-tooling-windows
       description: Go CI helpers that also run on Windows
       os: [windows]

pkg/client/clientimplementation/process_other.go

@@ -0,0 +1,7 @@
+//go:build !windows
+
+package clientimplementation
+
+func isInvalidProcessParameter(error) bool {
+	return false
+}

pkg/client/clientimplementation/process_windows.go

@@ -0,0 +1,19 @@
+//go:build windows
+
+package clientimplementation
+
+import (
+	"errors"
+	"io/fs"
+
+	"golang.org/x/sys/windows"
+)
+
+// isInvalidProcessParameter reports only process-creation failures, which os/exec wraps as
+// "fork/exec"; the same errno from an output writer after the process started is not one.
+func isInvalidProcessParameter(err error) bool {
+	var pathErr *fs.PathError
+	return errors.As(err, &pathErr) &&
+		pathErr.Op == "fork/exec" &&
+		errors.Is(pathErr.Err, windows.ERROR_INVALID_PARAMETER)
+}

pkg/client/clientimplementation/provider_command_error_test.go

@@ -0,0 +1,45 @@
+package clientimplementation
+
+import (
+	"errors"
+	"io/fs"
+	"testing"
+
+	"github.com/devsy-org/devsy/pkg/provider"
+	"github.com/stretchr/testify/suite"
+)
+
+type ProviderCommandErrorTestSuite struct {
+	suite.Suite
+}
+
+func TestProviderCommandErrorTestSuite(t *testing.T) {
+	suite.Run(t, new(ProviderCommandErrorTestSuite))
+}
+
+func (s *ProviderCommandErrorTestSuite) TestOmitsEmptyAction() {
+	cause := errors.New("exit status 255")
+
+	err := providerCommandError("${SSH_PROVIDER} init", nil, cause)
+
+	s.Equal(`run provider command "${SSH_PROVIDER} init": exit status 255`, err.Error())
+	s.ErrorIs(err, cause)
+}
+
+func (s *ProviderCommandErrorTestSuite) TestIncludesAction() {
+	cause := errors.New("exit status 1")
+	environ := []string{"UNRELATED=value", provider.CommandEnv + "=ls"}
+
+	err := providerCommandError("${SSH_PROVIDER} command", environ, cause)
+
+	s.Equal(`run provider command "${SSH_PROVIDER} command" (ls): exit status 1`, err.Error())
+}
+
+func (s *ProviderCommandErrorTestSuite) TestPlainStartFailureHasNoWindowsHint() {
+	cause := &fs.PathError{Op: "fork/exec", Path: "provider", Err: fs.ErrPermission}
+
+	err := providerCommandError("provider", nil, cause)
+
+	s.NotContains(err.Error(), "ERROR_INVALID_PARAMETER")
+	s.ErrorIs(err, fs.ErrPermission)
+}

pkg/client/clientimplementation/provider_command_error_windows_test.go

@@ -0,0 +1,40 @@
+//go:build windows
+
+package clientimplementation
+
+import (
+	"io/fs"
+	"os"
+	"os/exec"
+	"syscall"
+
+	"golang.org/x/sys/windows"
+)
+
+// Combining DETACHED_PROCESS with CREATE_NEW_CONSOLE makes the real CreateProcess
+// call fail with ERROR_INVALID_PARAMETER, the error reported in issue 1638.
+func (s *ProviderCommandErrorTestSuite) TestRealInvalidParameterStartFailureGetsHint() {
+	self, err := os.Executable()
+	s.Require().NoError(err)
+	cmd := exec.Command(self)
+	cmd.SysProcAttr = &syscall.SysProcAttr{
+		CreationFlags: windows.DETACHED_PROCESS | windows.CREATE_NEW_CONSOLE,
+	}
+	startErr := cmd.Start()
+	s.Require().ErrorIs(startErr, windows.ERROR_INVALID_PARAMETER)
+
+	err = providerCommandError(self, nil, startErr)
+
+	s.ErrorIs(err, windows.ERROR_INVALID_PARAMETER)
+	s.Contains(err.Error(), invalidProcessParameterHint)
+	s.Contains(err.Error(), "The parameter is incorrect")
+}
+
+func (s *ProviderCommandErrorTestSuite) TestInvalidParameterFromOutputWriterGetsNoHint() {
+	writeErr := &fs.PathError{Op: "write", Path: "stdout", Err: windows.ERROR_INVALID_PARAMETER}
+
+	err := providerCommandError("provider", nil, writeErr)
+
+	s.ErrorIs(err, windows.ERROR_INVALID_PARAMETER)
+	s.NotContains(err.Error(), invalidProcessParameterHint)
+}

pkg/client/clientimplementation/workspace_client.go

@@ -864,12 +864,7 @@ func RunCommand(ctx context.Context, opts RunCommandOptions) (retErr error) {
 			Stderr:  stderr,
 			Environ: opts.Environ,
 		}); err != nil {
-			return fmt.Errorf(
-				"run provider command %q (%s): %w",
-				opts.Command[0],
-				providerAction(opts.Environ),
-				err,
-			)
+			return providerCommandError(opts.Command[0], opts.Environ, err)
 		}
 		return nil
 	}
@@ -880,10 +875,7 @@ func RunCommand(ctx context.Context, opts RunCommandOptions) (retErr error) {
 	cmd.Stderr = stderr
 	cmd.Env = opts.Environ
 	if err := cmd.Run(); err != nil {
-		return fmt.Errorf(
-			"run provider command %q (%s): %w",
-			strings.Join(opts.Command, " "), providerAction(opts.Environ), err,
-		)
+		return providerCommandError(strings.Join(opts.Command, " "), opts.Environ, err)
 	}
 	return nil
 }
@@ -902,6 +894,25 @@ func flushCommandOutput(stdout, stderr *subprocess.StreamingRedactingWriter) err
 	return errors.Join(stdoutErr, stderrErr)
 }
 
+const invalidProcessParameterHint = "Windows rejected starting the provider executable " +
+	"(ERROR_INVALID_PARAMETER) before it ran, so this is a local process-launch failure " +
+	"rather than a remote host error; the cause is unconfirmed, so please report the Windows " +
+	"version and whether running the executable directly from a terminal works"
+
+func providerCommandError(command string, environ []string, err error) error {
+	action := ""
+	if name := providerAction(environ); name != "" {
+		action = fmt.Sprintf(" (%s)", name)
+	}
+	if isInvalidProcessParameter(err) {
+		return fmt.Errorf(
+			"run provider command %q%s: %w; %s",
+			command, action, err, invalidProcessParameterHint,
+		)
+	}
+	return fmt.Errorf("run provider command %q%s: %w", command, action, err)
+}
+
 // providerAction extracts the provider action from the environment variables.
 func providerAction(environ []string) string {
 	for _, kv := range environ {

pkg/shell/executable_path_test.go

@@ -0,0 +1,197 @@
+package shell
+
+import (
+	"bytes"
+	"context"
+	"io"
+	"os"
+	"path/filepath"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/suite"
+)
+
+const helperProcessEnv = "DEVSY_SHELL_TEST_HELPER_PROCESS"
+
+func TestMain(m *testing.M) {
+	if os.Getenv(helperProcessEnv) == "1" {
+		_, _ = os.Stdout.WriteString("helper ran with " + os.Args[len(os.Args)-1])
+		os.Exit(0)
+	}
+	os.Exit(m.Run())
+}
+
+type ExecutablePathTestSuite struct {
+	suite.Suite
+	executable string
+}
+
+func TestExecutablePathTestSuite(t *testing.T) {
+	suite.Run(t, new(ExecutablePathTestSuite))
+}
+
+// SetupTest installs a copy of the test binary under a directory containing a
+// space, like the binaries directory of a Windows profile such as "Jane Doe".
+func (s *ExecutablePathTestSuite) SetupTest() {
+	dir := filepath.Join(s.T().TempDir(), "Jane Doe", "binaries")
+	s.Require().NoError(os.MkdirAll(dir, 0o750))
+
+	name := "provider"
+	if runtime.GOOS == "windows" {
+		name += ".exe"
+	}
+	s.executable = filepath.Join(dir, name)
+
+	s.copyTestBinary(s.executable)
+}
+
+func (s *ExecutablePathTestSuite) TestCommandVariableWithSpacesInPathIsNotSplit() {
+	output, err := s.run(
+		"${SSH_PROVIDER} init",
+		"SSH_PROVIDER="+s.executable,
+		helperProcessEnv+"=1",
+	)
+
+	s.Require().NoError(err)
+	s.Equal("helper ran with init", output)
+}
+
+func (s *ExecutablePathTestSuite) TestShortVariableFormWithSpacesInPathIsNotSplit() {
+	output, err := s.run(
+		"$SSH_PROVIDER init",
+		"SSH_PROVIDER="+s.executable,
+		helperProcessEnv+"=1",
+	)
+
+	s.Require().NoError(err)
+	s.Equal("helper ran with init", output)
+}
+
+func (s *ExecutablePathTestSuite) TestCommandVariableWithArgumentsStillSplits() {
+	output, err := s.run("${CMD} world", "CMD=echo hello")
+
+	s.Require().NoError(err)
+	s.Equal("hello world\n", output)
+}
+
+func (s *ExecutablePathTestSuite) TestReassignedCommandVariableStillSplits() {
+	output, err := s.run("CMD='echo hello'; $CMD world", "CMD="+s.executable)
+
+	s.Require().NoError(err)
+	s.Equal("hello world\n", output)
+}
+
+func (s *ExecutablePathTestSuite) TestCommandVariableAssignedInScriptIsNotSplit() {
+	output, err := s.run(
+		"PROVIDER='"+s.executable+"'; ${PROVIDER} init",
+		helperProcessEnv+"=1",
+	)
+
+	s.Require().NoError(err)
+	s.Equal("helper ran with init", output)
+}
+
+func (s *ExecutablePathTestSuite) TestLiteralCommandIsNotJoinedWithUnrelatedVariable() {
+	dir := s.T().TempDir()
+	ext := filepath.Ext(s.executable)
+	tool := filepath.Join(dir, "tool"+ext)
+	spacedTool := filepath.Join(dir, "tool status"+ext)
+	s.copyTestBinary(tool)
+	s.copyTestBinary(spacedTool)
+
+	output, err := s.run(
+		"'"+tool+"' status; $CMD",
+		"CMD="+spacedTool,
+		helperProcessEnv+"=1",
+	)
+
+	s.Require().NoError(err)
+	s.Equal("helper ran with status"+"helper ran with "+spacedTool, output)
+}
+
+func (s *ExecutablePathTestSuite) TestRelativePathIsResolvedAgainstShellDirectory() {
+	shellDir := s.T().TempDir()
+	relative := filepath.Join("spaced dir", filepath.Base(s.executable))
+	s.Require().NoError(os.MkdirAll(filepath.Join(shellDir, "spaced dir"), 0o750))
+	s.copyTestBinary(filepath.Join(shellDir, relative))
+
+	output, err := s.run(
+		"cd '"+shellDir+"'; ${PROVIDER} init",
+		"PROVIDER="+relative,
+		helperProcessEnv+"=1",
+	)
+
+	s.Require().NoError(err)
+	s.Equal("helper ran with init", output)
+}
+
+func (s *ExecutablePathTestSuite) TestRelativePathExistingOnlyInProcessDirectoryIsNotJoined() {
+	processDir := s.T().TempDir()
+	shellDir := s.T().TempDir()
+	name := "spaced" + filepath.Ext(s.executable)
+	relative := "." + string(filepath.Separator) + name + " dir" +
+		string(filepath.Separator) + "provider"
+	s.Require().NoError(os.MkdirAll(filepath.Join(processDir, name+" dir"), 0o750))
+	s.copyTestBinary(filepath.Join(processDir, name+" dir", "provider"))
+	s.copyTestBinary(filepath.Join(shellDir, name))
+	s.T().Chdir(processDir)
+
+	output, err := s.run(
+		"cd '"+shellDir+"'; ${PROVIDER} init",
+		"PROVIDER="+relative,
+		helperProcessEnv+"=1",
+	)
+
+	s.Require().NoError(err)
+	s.Equal("helper ran with init", output)
+}
+
+func (s *ExecutablePathTestSuite) TestNonBreakingSpaceStaysInsideTheJoinedPath() {
+	dir := filepath.Join(s.T().TempDir(), "Jane\u00a0Doe and co")
+	s.Require().NoError(os.MkdirAll(dir, 0o750))
+	executable := filepath.Join(dir, filepath.Base(s.executable))
+	s.copyTestBinary(executable)
+
+	output, err := s.run(
+		"${PROVIDER} init",
+		"PROVIDER="+executable,
+		helperProcessEnv+"=1",
+	)
+
+	s.Require().NoError(err)
+	s.Equal("helper ran with init", output)
+}
+
+func (s *ExecutablePathTestSuite) TestVariableArgumentWithSpacesStillSplits() {
+	output, err := s.run("echo ${ARGS}", "ARGS=a b")
+
+	s.Require().NoError(err)
+	s.Equal("a b\n", output)
+}
+
+func (s *ExecutablePathTestSuite) run(command string, environ ...string) (string, error) {
+	var stdout, stderr bytes.Buffer
+	err := RunEmulatedShell(context.Background(), &CommandRunner{
+		Command: command,
+		Stdout:  &stdout,
+		Stderr:  &stderr,
+		Environ: append(os.Environ(), environ...),
+	})
+	return stdout.String(), err
+}
+
+func (s *ExecutablePathTestSuite) copyTestBinary(target string) {
+	self, err := os.Executable()
+	s.Require().NoError(err)
+	source, err := os.Open(self) //nolint:gosec // copies this test binary
+	s.Require().NoError(err)
+	defer func() { _ = source.Close() }()
+	file, err := os.OpenFile( //nolint:gosec // the copy must be executable
+		target, os.O_CREATE|os.O_WRONLY, 0o700,
+	)
+	s.Require().NoError(err)
+	_, err = io.Copy(file, source)
+	s.Require().NoError(err)
+	s.Require().NoError(file.Close())
+}