fix(exec): resolve executables on Windows — PATHEXT, mode bits, and a swallowed failure

go/exec/exec.go

@@ -219,7 +219,11 @@ func (c *Cmd) prepare() core.Result {
 		return core.Fail(core.E("Cmd.prepare", "exec: command context is required", ErrCommandContextRequired))
 	}
 
-	c.cmd = commandContext(c.ctx, c.name, c.args...)
+	resolved := commandContext(c.ctx, c.name, c.args...)
+	if !resolved.OK {
+		return resolved
+	}
+	c.cmd = resolved.Value.(*core.Cmd)
 
 	c.cmd.Dir = c.opts.Dir
 	if len(c.opts.Env) > 0 {
@@ -285,26 +289,51 @@ func (c *Cmd) logError(msg string, failure core.Result) {
 	c.getLogger().Error(msg, "cmd", c.name, "args", core.Join(" ", c.args...), "err", failure.Error())
 }
 
-func commandContext(ctx context.Context, name string, arg ...string) *core.Cmd {
-	path := name
-	if result := lookPath(name); result.OK {
-		path = result.Value.(string)
+// commandContext resolves name to a concrete executable and builds the handle
+// for it.
+//
+// Resolution failure is returned, never swallowed. Handing the bare name to
+// os/exec instead is not a harmless fallback on Windows: when Dir is set,
+// Cmd.Start resolves a separator-free Path relative to Dir, so an unresolved
+// "git" is reported as `exec: "C:\...\some work dir\git"` — a confusing error
+// naming a path nobody asked for, in place of the honest "not found on PATH".
+func commandContext(ctx context.Context, name string, arg ...string) core.Result {
+	resolved := lookPath(name)
+	if !resolved.OK {
+		return resolved
 	}
-
-	cmd := &core.Cmd{
-		Path: path,
+	return core.Ok(&core.Cmd{
+		Path: resolved.Value.(string),
 		Args: append([]string{name}, arg...),
-	}
-	return cmd
+	})
 }
 
+// defaultPathExt is the extension list Windows itself assumes when %PATHEXT%
+// is unset.
+const defaultPathExt = ".COM;.EXE;.BAT;.CMD"
+
+// lookPath resolves file to a runnable path, searching PATH when file carries
+// no directory component.
+//
+// On Windows a command is named without its extension — "git", not "git.exe" —
+// so every candidate is also tried with each %PATHEXT% suffix. Without that,
+// no Windows executable is ever found by its bare name.
 func lookPath(file string) core.Result {
+	return lookPathWith(file, executableExtensions())
+}
+
+// lookPathWith is lookPath with the extension list supplied rather than read
+// from the environment. Taking it as an argument is what lets the Windows
+// resolution rules be pinned on a POSIX runner — the tests drive it with a
+// fixture PATH and a fake %PATHEXT%, so no Windows box is needed to prove the
+// logic and the CI lane is left to prove only the wiring.
+func lookPathWith(file string, extensions []string) core.Result {
 	if file == "" {
 		return core.Fail(core.E("lookPath", "executable file not found in PATH", nil))
 	}
-	if core.Contains(file, string(core.PathSeparator)) {
-		if isExecutable(file) {
-			return core.Ok(file)
+	if containsSeparator(file) {
+		if path, ok := firstExecutable(file, extensions); ok {
+			return core.Ok(path)
 		}
 		return core.Fail(core.E("lookPath", core.Sprintf("executable file %q not found", file), nil))
 	}
@@ -313,15 +342,111 @@ func lookPath(file string) core.Result {
 		if dir == "" {
 			dir = "."
 		}
-		path := core.PathJoin(dir, file)
-		if isExecutable(path) {
+		if path, ok := firstExecutable(core.PathJoin(dir, file), extensions); ok {
 			return core.Ok(path)
 		}
 	}
 	return core.Fail(core.E("lookPath", core.Sprintf("executable file %q not found in PATH", file), nil))
 }
 
+// firstExecutable returns the first of base's candidate spellings that names a
+// runnable file.
+func firstExecutable(base string, extensions []string) (string, bool) {
+	for _, candidate := range executableCandidates(base, extensions) {
+		if isExecutableWith(candidate, extensions) {
+			return candidate, true
+		}
+	}
+	return "", false
+}
+
+// executableCandidates returns the spellings of base to try, in order. With no
+// extensions in play — every POSIX case — base stands alone. On Windows a base
+// that already ends in a listed extension also stands alone; anything else is
+// tried once per extension, so "git" becomes "git.com", "git.exe" and so on.
+func executableCandidates(base string, extensions []string) []string {
+	if len(extensions) == 0 || hasExecutableExtension(base, extensions) {
+		return []string{base}
+	}
+	candidates := make([]string, 0, len(extensions))
+	for _, extension := range extensions {
+		candidates = append(candidates, base+extension)
+	}
+	return candidates
+}
+
+// hasExecutableExtension reports whether base already ends in one of the
+// listed extensions. Windows filenames are case-insensitive, so the comparison
+// is too.
+func hasExecutableExtension(base string, extensions []string) bool {
+	lowered := core.Lower(base)
+	for _, extension := range extensions {
+		if core.HasSuffix(lowered, extension) {
+			return true
+		}
+	}
+	return false
+}
+
+// executableExtensions returns the %PATHEXT% list, or nil off Windows where a
+// command name is used exactly as written.
+func executableExtensions() []string {
+	if string(core.PathSeparator) != `\` {
+		return nil
+	}
+	return parsePathExt(core.Getenv("PATHEXT"))
+}
+
+// parsePathExt normalises a %PATHEXT% value into lower-cased, dot-prefixed
+// extensions, dropping blanks and duplicates. An unset or unusable value falls
+// back to the set Windows assumes, so a stripped environment still resolves
+// the common executables.
+func parsePathExt(value string) []string {
+	extensions := make([]string, 0, 8)
+	seen := make(map[string]bool, 8)
+	for _, field := range core.Split(value, ";") {
+		extension := core.Lower(core.Trim(field))
+		if extension == "" || extension == "." {
+			continue
+		}
+		if !core.HasPrefix(extension, ".") {
+			extension = "." + extension
+		}
+		if seen[extension] {
+			continue
+		}
+		seen[extension] = true
+		extensions = append(extensions, extension)
+	}
+	if len(extensions) == 0 && value != defaultPathExt {
+		return parsePathExt(defaultPathExt)
+	}
+	return extensions
+}
+
+// containsSeparator reports whether file carries a directory component under
+// either convention. Windows accepts '/' as well as '\', so a name spelled
+// "bin/tool" there is a path to check directly, not a name to hunt on PATH.
+func containsSeparator(file string) bool {
+	if core.Contains(file, "/") {
+		return true
+	}
+	separator := string(core.PathSeparator)
+	return separator != "/" && core.Contains(file, separator)
+}
+
 func isExecutable(path string) bool {
+	return isExecutableWith(path, executableExtensions())
+}
+
+// isExecutableWith applies the platform's own rule for "this can be run".
+//
+// POSIX asks the mode bits. Windows has no execute bit — os.Stat synthesises
+// 0666, or 0444 for a read-only file — so mode&0111 is never set there and a
+// mode test rejects every file, git.exe included. Under a non-empty extension
+// list the question becomes whether the suffix is one %PATHEXT% names, which
+// is what Windows itself keys on.
+func isExecutableWith(path string, extensions []string) bool {
 	stat := core.Stat(path)
 	if !stat.OK {
 		return false
@@ -330,5 +455,8 @@ func isExecutable(path string) bool {
 	if !ok || info.IsDir() {
 		return false
 	}
+	if len(extensions) > 0 {
+		return hasExecutableExtension(path, extensions)
+	}
 	return info.Mode()&0111 != 0
 }