Skip to content

Commit cd04129

Browse files
committed
feat(shell): auto-detect shell on Windows
Shell commands ($"...") hard-failed on a clean Windows install because rad's resolver only knew about the SHELL env var (a Unix convention) and /bin/sh (which doesn't exist on Windows). Closes #123. Resolution chain when SHELL is unset: - Windows: pwsh.exe -> powershell.exe -> cmd.exe - Other: /bin/sh PowerShell is preferred over cmd.exe because it ships with Unix-like aliases (ls, cat, pwd), so common rad shell idioms work without modification. Validated by the issue reporter. On Windows we also LookPath the SHELL value when it IS set, falling through to the candidate chain if it doesn't resolve. This rescues the common Git Bash / MSYS2 / Cygwin case where SHELL is set to /usr/bin/bash - a virtual path the native Win32 binary can't see. On Unix, SHELL is trusted as-is; explicit misconfiguration there should fail loudly. resolveShell is extracted as a pure function (deps injected) so the fallback logic is unit-testable without mutating the host environment. Also handles whitespace-only SHELL (treated as unset), case-insensitive cmd.exe detection, and Windows-style paths. shellExecFlag picks "-c" (POSIX shells, PowerShell) or "/c" (cmd.exe) based on the shell's basename, with separator-agnostic detection (handles both / and \\) so it doesn't depend on filepath.Base's GOOS-specific behavior. Cross-machine portability variance from auto-detect is real but no worse than the existing /bin/bash vs /bin/zsh variance on Unix; a follow-up @shell script-header macro will let scripts pin a shell explicitly. Filed as kan card a_1BidxHsvp. Removed the dead resolveCmd duplicate while we were in there.
1 parent c815ac9 commit cd04129

11 files changed

Lines changed: 353 additions & 21 deletions

File tree

.kan/boards/bugfixes/config.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
default_column = "todo-lo"
22
id = "b_sBGk8Gtk"
3-
kan_schema = "board/10"
3+
kan_schema = "board/11"
44
name = "bugfixes"
55

66
[card_display]

.kan/boards/main/cards/3d85TpNT.json

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,12 @@
22
"_v": 2,
33
"alias": "rad-9-transposing-tables",
44
"alias_explicit": false,
5-
"column": "low",
5+
"column": "in-progress",
66
"created_at_millis": 1767762148915,
77
"creator": "Alexander Terp",
88
"id": "3d85TpNT",
9-
"position": "0K",
9+
"position": "U",
1010
"title": "RAD-9: Transposing tables",
1111
"type": "feature",
12-
"updated_at_millis": 1767762148915
12+
"updated_at_millis": 1775785559110
1313
}
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
{
2+
"_v": 2,
3+
"alias": "windows-shell-macro",
4+
"alias_explicit": false,
5+
"column": "uncommitted",
6+
"created_at_millis": 1778118725524,
7+
"creator": "Alexander Terp",
8+
"description": "From #123 thread: auto-detect on Windows ships now (PowerShell first, cmd.exe fallback) but creates portability variance across machines. Add a script-header macro/pragma like '@shell bash' or 'requires_shell = bash' that lets a script declare its required shell and fail-fast if unavailable. Complements (does not replace) auto-detect: macro is for 'this script needs bash', auto-detect is for 'I just want shell commands to work'. Prior art: shebangs, Go's //go:build, Python's # -*- coding -*-. Could live alongside the existing args/header block.",
9+
"id": "a_1BidxHsvp",
10+
"position": "wiU",
11+
"title": "Windows: @shell macro for pinning shell target",
12+
"type": "feature",
13+
"updated_at_millis": 1778118725524
14+
}
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
{
2+
"_v": 2,
3+
"alias": "windows-docs-for",
4+
"alias_explicit": false,
5+
"column": "high",
6+
"created_at_millis": 1778118731762,
7+
"creator": "Alexander Terp",
8+
"description": "From #123 thread: user couldn't 'go build' or 'go install' on Windows because tree-sitter requires CGO and gcc. They fixed it by installing MinGW (WinLibs) and putting gcc on PATH. Add a 'Building from Source on Windows' section to the docs noting: CGO_ENABLED=1 needed, gcc must be on PATH, recommend WinLibs/MinGW or MSYS2. Note that pre-built binaries (rad_windows_amd64.zip in releases) avoid this entirely - most users should use those.",
9+
"id": "a_1Bie7LvAa",
10+
"position": "t!U",
11+
"title": "Windows: docs for building from source (CGO/MinGW)",
12+
"type": "chore",
13+
"updated_at_millis": 1778119792090
14+
}
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
{
2+
"_v": 2,
3+
"alias": "windows-uplift-cross",
4+
"alias_explicit": false,
5+
"column": "next",
6+
"created_at_millis": 1778118738164,
7+
"creator": "Alexander Terp",
8+
"description": "The 'windows' branch has commits adding cross-platform testing for macOS/Windows (e085011a) and platform-normalization improvements (64227082, 5fdf8fd8). Review and uplift the relevant pieces to main so we get Windows coverage in CI. Without this, Windows fixes (like the auto-shell-detect for #123) only get verified by users hitting issues, not by automation.",
9+
"id": "a_1BieHfp6E",
10+
"position": "t!UU",
11+
"title": "Windows: uplift cross-platform CI from windows branch",
12+
"type": "excellence",
13+
"updated_at_millis": 1778119788265
14+
}
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
{
2+
"_v": 2,
3+
"alias": "fix-vet-warnings-in",
4+
"alias_explicit": false,
5+
"column": "next",
6+
"created_at_millis": 1778119513772,
7+
"creator": "Alexander Terp",
8+
"description": "Found while working on #123 fix: 'make test' currently only runs ./core/testing/... not ./core/..., so unit tests in core/ (fuzzy_test.go, shell_test.go) don't run in ./dev --validate. Switching to ./core/... exposes pre-existing vet warnings - 'non-constant format string in call to NewErrorStrf' in core/funcs.go and core/func_parse_date.go (16 instances). Fix these (likely just s/NewErrorStrf/NewErrorStr/ where the arg is already-formatted) then expand the Makefile test target to ./core/... so unit tests are actually exercised by validate.",
9+
"id": "a_1BiySevmD",
10+
"position": "t!U",
11+
"title": "Fix vet warnings in core, expand make test to ./core/...",
12+
"type": "excellence",
13+
"updated_at_millis": 1778119773514
14+
}
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
{
2+
"_v": 2,
3+
"alias": "surface-shell",
4+
"alias_explicit": false,
5+
"column": "low",
6+
"created_at_millis": 1778121587222,
7+
"creator": "Alexander Terp",
8+
"description": "Found in code review of #123 fix: when shell resolution panics (e.g. 'Cannot run shell cmd as no shell found' at core/shell.go:208, or 'Failed to run command' at :186) it propagates as a string panic, not *RadPanic. handlePanicRecovery treats string panics as internal bugs and tells the user 'This is a bug in Rad. Please report it at: https://github.com/amterp/rad/issues. Panic: \u003cmessage\u003e'. The user-actionable text gets buried inside a 'this is our fault' template. This predates the Windows fix but became more visible now that Windows users can hit shell errors. Convert these to proper RadErrors / *RadPanics so they surface as clean user-facing errors instead.",
9+
"id": "a_1BjqOvRU8",
10+
"position": "xRUU",
11+
"title": "Surface shell-resolution panics as user errors, not 'bug in Rad'",
12+
"type": "bug",
13+
"updated_at_millis": 1778121587222
14+
}

.kan/boards/main/config.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
default_column = "uncommitted"
22
id = "3d0a0ABE"
3-
kan_schema = "board/10"
3+
kan_schema = "board/11"
44
name = "main"
55

66
[card_display]

core/shell.go

Lines changed: 68 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"os"
88
"os/exec"
9+
"strings"
910

1011
"github.com/amterp/rad/rts/rl"
1112
)
@@ -197,33 +198,84 @@ func realShellExecutor(invocation ShellInvocation) (string, string, int) {
197198
return stdout, stderr, exitCode
198199
}
199200

201+
// resolveCmdSimple resolves the shell to use for the given command string and
202+
// returns a prepared *exec.Cmd. Panics with a user-facing message if no shell
203+
// can be found.
200204
func resolveCmdSimple(cmdStr string) *exec.Cmd {
201-
if shell := os.Getenv("SHELL"); shell != "" {
202-
return buildCmd(shell, cmdStr)
205+
path, flag, err := resolveShell(os.Getenv, exec.LookPath, IsWindows())
206+
if err != nil {
207+
panic(err.Error())
203208
}
209+
return buildCmd(path, flag, cmdStr)
210+
}
204211

205-
if _, err := exec.LookPath("/bin/sh"); err == nil {
206-
return buildCmd("/bin/sh", cmdStr)
212+
// resolveShell picks a shell to use for executing a command string. It is a
213+
// pure function: all platform/env dependencies are passed in so it is testable
214+
// without mutating global state.
215+
//
216+
// Resolution order:
217+
// 1. SHELL env var if set, but on Windows only if it actually resolves to an
218+
// executable. Git Bash / MSYS2 / Cygwin set SHELL to a Unix-style path
219+
// (e.g. /usr/bin/bash) that native Win32 exec can't find, so on Windows
220+
// we fall through to the candidate chain in that case rather than crash.
221+
// 2. Windows: pwsh.exe -> powershell.exe -> cmd.exe
222+
// 3. Other: /bin/sh
223+
//
224+
// Returns the resolved shell path and the flag to use for command-string
225+
// invocation (e.g. "-c" for POSIX shells and PowerShell, "/c" for cmd.exe).
226+
func resolveShell(
227+
getEnv func(string) string,
228+
lookPath func(string) (string, error),
229+
isWindows bool,
230+
) (path, flag string, err error) {
231+
if shell := strings.TrimSpace(getEnv("SHELL")); shell != "" {
232+
if !isWindows {
233+
return shell, shellExecFlag(shell), nil
234+
}
235+
// On Windows, only honor SHELL if it actually resolves - otherwise
236+
// fall through. This rescues the common Git Bash case where SHELL is
237+
// set to /usr/bin/bash but the native Win32 binary can't see it.
238+
if resolved, lookErr := lookPath(shell); lookErr == nil {
239+
return resolved, shellExecFlag(resolved), nil
240+
}
207241
}
208242

209-
panic("Cannot run shell cmd as no shell found. Please set the SHELL environment variable.")
210-
}
211-
212-
func resolveCmd(i *Interpreter, shellNode rl.Node, cmdStr string) *exec.Cmd {
213-
if shell := os.Getenv("SHELL"); shell != "" {
214-
return buildCmd(shell, cmdStr)
243+
var candidates []string
244+
if isWindows {
245+
candidates = []string{"pwsh.exe", "powershell.exe", "cmd.exe"}
246+
} else {
247+
candidates = []string{"/bin/sh"}
215248
}
216249

217-
if _, err := exec.LookPath("/bin/sh"); err == nil {
218-
return buildCmd("/bin/sh", cmdStr)
250+
for _, c := range candidates {
251+
if resolved, lookErr := lookPath(c); lookErr == nil {
252+
return resolved, shellExecFlag(resolved), nil
253+
}
219254
}
220255

221-
i.emitError(rl.ErrGenericRuntime, shellNode, "Cannot run shell cmd as no shell found. Please set the SHELL environment variable")
222-
panic(UNREACHABLE)
256+
return "", "", errors.New("Cannot run shell cmd as no shell found. Please set the SHELL environment variable")
257+
}
258+
259+
// shellExecFlag returns the flag a given shell expects for invoking a command
260+
// string. Defaults to "-c" (POSIX shells, bash/zsh, and PowerShell which
261+
// accepts "-c" as a short form of "-Command"). Only cmd.exe needs "/c".
262+
//
263+
// We don't use filepath.Base because its separator handling is GOOS-specific
264+
// (only "/" on Unix), which would mis-handle Windows-style paths that may
265+
// arrive via env vars or mixed environments.
266+
func shellExecFlag(shellPath string) string {
267+
if i := strings.LastIndexAny(shellPath, `/\`); i >= 0 {
268+
shellPath = shellPath[i+1:]
269+
}
270+
base := strings.TrimSuffix(strings.ToLower(shellPath), ".exe")
271+
if base == "cmd" {
272+
return "/c"
273+
}
274+
return "-c"
223275
}
224276

225-
func buildCmd(shellStr string, cmdStr string) *exec.Cmd {
226-
cmd := exec.Command(shellStr, "-c", cmdStr)
277+
func buildCmd(shellStr string, flag string, cmdStr string) *exec.Cmd {
278+
cmd := exec.Command(shellStr, flag, cmdStr)
227279
cmd.Stdin = RIo.StdIn.Unwrap()
228280
return cmd
229281
}

0 commit comments

Comments
 (0)