diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 26351b9c24..e112d19edf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -342,6 +342,27 @@ jobs: # blocked forever. -v makes RUN/PASS evidence explicit in CI logs. run: go test ./pkg/agent -v -run '^TestCodexWindowsInheritedStdoutDescendantCleanupIsBounded$' -count=1 -timeout=5m + - name: Test Windows OpenClaw npm shim interpreter resolution + working-directory: server + # #6061: every OpenClaw task failed execenv prep on a Windows host with + # a bare `exit status 1` and no stderr. A batch shim resolves and runs + # fine while the `node` it re-execs is unreachable, and npm's real + # template prefers a co-located node.exe over PATH — none of which can + # be proven without a real cmd.exe host. These tests pin: the positive + # control (node on PATH → success), that a missing node surfaces + # cmd.exe's own stderr (the first run of this job disproved #6061's + # premise that it does not), that a genuinely silent shim DOES reach the + # new diagnostic, that a co-located interpreter is credited, that a + # context timeout is not misdiagnosed as a missing interpreter, and that + # TEMP/TMP are NOT load-bearing (the originally reported root cause, + # since retracted upstream). + # Scoped to the windows-tagged shim tests — the package's legacy + # OpenClaw HOME tests are not Windows-safe; the backend job still runs + # the full package plus the cross-platform half on Linux. + # -v so a skip (no node on the runner) is visible instead of passing + # silently as "ok". + run: go test ./internal/daemon/execenv -v -run '^TestWindowsOpenclawShim' -count=1 -timeout=5m + - name: Build Windows CLI helper entrypoint working-directory: server run: go build ./cmd/multica diff --git a/server/internal/daemon/execenv/openclaw_config.go b/server/internal/daemon/execenv/openclaw_config.go index 2c9ce64f78..def71d6590 100644 --- a/server/internal/daemon/execenv/openclaw_config.go +++ b/server/internal/daemon/execenv/openclaw_config.go @@ -28,19 +28,40 @@ const openclawConfigFile = "openclaw-config.json" // at 0o600 next to the wrapper. const openclawUserSnapshotFile = "openclaw-user-snapshot.json" -// openclawCLITimeout caps each `openclaw config ...` invocation during task -// setup. The CLI is fast (<200ms normal); 5s leaves headroom for a cold -// node start without letting a hung CLI stall task dispatch indefinitely. +// openclawCLITimeout is the context deadline set on each `openclaw config ...` +// invocation during task setup. The CLI is fast (<200ms normal); 5s leaves +// headroom for a cold node start. +// +// It is a deadline, not a guaranteed cap — see the gap below. +// +// Known gap (deliberately not fixed here): this deadline does not actually +// bound the call when the CLI leaves a descendant holding stdout. +// CommandContext kills only the direct child, and cmd.Output() blocks in +// Wait() until the stdout pipe closes, so the call runs for the descendant's +// lifetime. Measured on linux/dash: a shim whose backgrounded child slept 6s +// took 6.01s against a 150ms deadline. An npm shim is that shape on Windows +// (cmd.exe → node). +// +// A cmd.WaitDelay backstop bounds the call but leaves the descendant running +// (measured: returns in 2.17s with the grandchild still in state S), trading a +// hang for a process leak — and on Unix nothing reaps it, because +// preparationProcessController.finish() is a no-op there. Closing this properly +// needs process-tree ownership (Unix process group, Windows Job Object) so the +// deadline can terminate the whole tree, which is its own change with its own +// risk surface. Tracked in MUL-5467; this file intentionally keeps the existing +// behaviour rather than shipping half of it. const openclawCLITimeout = 5 * time.Second // OpenclawConfigPrep is the input to prepareOpenclawConfig. Only OpenclawBin // is meaningful in production — Timeout is here for tests that need a tight -// cap to assert error paths. +// deadline to assert error paths. type OpenclawConfigPrep struct { // OpenclawBin is the openclaw CLI binary to invoke for config introspection. // Empty means resolve "openclaw" from PATH at exec time. OpenclawBin string - // Timeout caps each CLI invocation. Zero falls back to openclawCLITimeout. + // Timeout sets the context deadline for each CLI invocation — not a + // guaranteed cap on how long the call takes; see openclawCLITimeout. Zero + // falls back to openclawCLITimeout. Timeout time.Duration // McpConfig is the agent's saved `mcp_config` JSON (Claude-style // `{"mcpServers": {"": {...}}}`). When non-null the wrapper pins @@ -760,6 +781,23 @@ var openclawExec = execOpenclawCLI // stderr is captured separately and appended to error messages — failures // here surface up to the daemon log, and a `openclaw doctor` hint there is // more useful than just an exit code. +// +// When the CLI is a batch shim that exits non-zero and says nothing at all, +// openclawShimDiagnostic adds the interpreter-resolution detail that a bare +// `exit status 1` hides (MUL-5422 / #6061). Real stderr always wins — the +// diagnostic is a fallback for the silent case, not a replacement. +// +// Attribution order matters. openclawCLITimeout kills the child via +// CommandContext, and a killed process surfaces as *exec.ExitError +// ("signal: killed") — indistinguishable by type from a genuine exit 1. So the +// context is checked FIRST; otherwise a timeout gets reported as "node is not +// on PATH, install Node.js", sending the user to fix something that was never +// broken. +// +// In that branch the CONTEXT error is what gets %w-wrapped, not the process +// error, so errors.Is(err, context.DeadlineExceeded) holds for callers that +// check cancellation the standard way. The process error is still printed for +// diagnosis, just not as the wrapped cause. func execOpenclawCLI(ctx context.Context, bin string, args ...string) (string, error) { cmd := exec.CommandContext(ctx, bin, args...) cmd.Env = os.Environ() @@ -768,9 +806,18 @@ func execOpenclawCLI(ctx context.Context, bin string, args ...string) (string, e raw, err := cmd.Output() if err != nil { stderrMsg := strings.TrimSpace(stderr.String()) + if ctxErr := ctx.Err(); ctxErr != nil { + if stderrMsg != "" { + return "", fmt.Errorf("openclaw %s: %w (process: %v; stderr: %s)", strings.Join(args, " "), ctxErr, err, stderrMsg) + } + return "", fmt.Errorf("openclaw %s: %w (process: %v)", strings.Join(args, " "), ctxErr, err) + } if stderrMsg != "" { return "", fmt.Errorf("openclaw %s: %w (stderr: %s)", strings.Join(args, " "), err, stderrMsg) } + if diag := openclawShimDiagnostic(bin, err); diag != "" { + return "", fmt.Errorf("openclaw %s: %w (%s)", strings.Join(args, " "), err, diag) + } return "", fmt.Errorf("openclaw %s: %w", strings.Join(args, " "), err) } return string(raw), nil diff --git a/server/internal/daemon/execenv/openclaw_shim.go b/server/internal/daemon/execenv/openclaw_shim.go new file mode 100644 index 0000000000..fd16fae44a --- /dev/null +++ b/server/internal/daemon/execenv/openclaw_shim.go @@ -0,0 +1,153 @@ +package execenv + +import ( + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" +) + +// openclawShimExtensions are the batch-wrapper extensions npm uses when it +// installs the `openclaw` command on Windows. The shim is not the real +// program: it re-execs OpenClaw's JavaScript entrypoint through an +// interpreter, so a shim path that resolves and is executable says nothing +// about whether the interpreter it depends on is reachable. +var openclawShimExtensions = map[string]struct{}{ + ".cmd": {}, + ".bat": {}, +} + +// openclawShimInterpreter is the interpreter an npm-installed OpenClaw shim +// re-execs. OpenClaw's package `bin` entry points at `openclaw.mjs`, whose +// shebang is `#!/usr/bin/env node`, so an unreachable `node` breaks the shim +// from the inside while the shim itself still looks valid to the daemon. +const openclawShimInterpreter = "node" + +// isOpenclawShimPath reports whether bin is a batch shim rather than a directly +// executable binary. +// +// Keyed on the file extension alone, deliberately not on runtime.GOOS: a +// `.cmd`/`.bat` shim only ever appears on Windows in production, and testing +// the extension instead of the host OS lets the whole diagnostic be exercised +// from the normal Linux/macOS test job rather than only on a Windows runner. +// +// A batch extension is NOT proof the file is an npm shim — an operator can +// point MULTICA_OPENCLAW_PATH at any batch file. The diagnostic below is +// therefore phrased conditionally and never asserts npm shim semantics as fact. +func isOpenclawShimPath(bin string) bool { + ext := strings.ToLower(filepath.Ext(strings.TrimSpace(bin))) + _, ok := openclawShimExtensions[ext] + return ok +} + +// openclawInterpreterOrigin describes where a shim's interpreter was found, +// without disclosing an absolute path. See openclawShimDiagnostic for why the +// path itself is withheld. +type openclawInterpreterOrigin struct { + found bool + // where is a human phrase ("alongside the shim", "on the daemon PATH"), + // empty when the interpreter was not found at all. + where string +} + +// findOpenclawShimInterpreter resolves the interpreter the way npm's generated +// shim does, in npm's own order. +// +// npm's cmd-shim template emits: +// +// IF EXIST "%dp0%\node.exe" ( SET "_prog=%dp0%\node.exe" ) ELSE ( SET "_prog=node" ) +// +// so a Node binary sitting next to the shim wins over PATH entirely. Checking +// only PATH (as the first version of this diagnostic did) would report "node is +// not resolvable" for an install that actually runs fine off its co-located +// interpreter — a confidently wrong root cause, which is worse than no hint. +func findOpenclawShimInterpreter(shimPath string) openclawInterpreterOrigin { + dir := filepath.Dir(shimPath) + // `.exe` first, matching npm's IF EXIST check; the bare name keeps the + // helper meaningful on the non-Windows hosts the tests run on. + for _, name := range []string{openclawShimInterpreter + ".exe", openclawShimInterpreter} { + if info, err := os.Stat(filepath.Join(dir, name)); err == nil && !info.IsDir() { + return openclawInterpreterOrigin{found: true, where: "alongside the shim"} + } + } + if _, err := exec.LookPath(openclawShimInterpreter); err == nil { + return openclawInterpreterOrigin{found: true, where: "on the daemon PATH"} + } + return openclawInterpreterOrigin{} +} + +// openclawShimDiagnostic explains a batch-shim invocation that failed without +// writing anything to stderr, and returns "" when it has nothing to add. +// +// Why this exists (MUL-5422 / #6061): a Windows user reported every OpenClaw +// task failing in execenv prep with a bare `exit status 1` and no stderr. The +// daemon pins `openclaw` to an absolute path, so the failing command looked +// correct; what the error could not show is that a shim's interpreter lookup is +// a second, invisible resolution step that can fail on its own. +// +// Scope note: CI on windows-latest showed that when `node` is genuinely missing, +// cmd.exe's "'node' is not recognized" DOES reach Go's stderr pipe, so that case +// takes the caller's stderr branch and never arrives here. This diagnostic is +// the fallback for a shim that fails while saying nothing at all — which is what +// #6061's daemon log actually showed, and remains unexplained. +// +// This only enriches error text — it never changes control flow, and never +// suppresses a real stderr message (callers try stderr first). +// +// # Redaction +// +// The returned string is NOT local-log-only: on prep failure it travels through +// reportTerminalTask → Client.FailTask to the server and is persisted as the +// task's error. A Windows shim path embeds the account name and install layout +// (`C:\Users\\AppData\Roaming\npm\...`), so this reports only the shim's +// base name, whether the interpreter resolved, and a PATH entry count — never an +// absolute path and never the PATH contents. +func openclawShimDiagnostic(bin string, runErr error) string { + // Only an actual non-zero exit is in scope. A missing binary or permission + // error already describes itself. + // + // Note this gate is necessary but NOT sufficient: a context timeout kills + // the child and also surfaces as *exec.ExitError ("signal: killed"), which + // would be misdiagnosed here as an interpreter problem. Callers must + // attribute context cancellation before consulting this function. + var exitErr *exec.ExitError + if !errors.As(runErr, &exitErr) { + return "" + } + if !isOpenclawShimPath(bin) { + return "" + } + + name := filepath.Base(strings.TrimSpace(bin)) + pathSummary := openclawPathEntrySummary() + origin := findOpenclawShimInterpreter(bin) + if !origin.found { + return fmt.Sprintf( + "no stderr output; if %s is an npm-generated shim it re-execs %q, which resolves neither "+ + "alongside the shim nor on the daemon PATH (%s) — install Node.js, or restart the daemon "+ + "from an environment where %q is on PATH", + name, openclawShimInterpreter, pathSummary, openclawShimInterpreter, + ) + } + // The interpreter being reachable is the more valuable report: it clears + // PATH of blame and redirects to the remaining hypotheses (PATH drift + // between the runtime `--version` gate and task prep, or a broken install). + return fmt.Sprintf( + "no stderr output; %q resolves %s, so the interpreter is reachable — if %s is an "+ + "npm-generated shim, check the OpenClaw install itself rather than the daemon PATH (%s)", + openclawShimInterpreter, origin.where, name, pathSummary, + ) +} + +// openclawPathEntrySummary describes the daemon PATH by size alone. A count is +// enough to tell "the daemon inherited a stripped environment" apart from "PATH +// looks normal but the interpreter still is not on it", without copying the +// user's PATH into a task error that is persisted server-side. +func openclawPathEntrySummary() string { + if n := len(filepath.SplitList(os.Getenv("PATH"))); n != 1 { + return fmt.Sprintf("%d entries", n) + } + return "1 entry" +} diff --git a/server/internal/daemon/execenv/openclaw_shim_test.go b/server/internal/daemon/execenv/openclaw_shim_test.go new file mode 100644 index 0000000000..ca2038ed44 --- /dev/null +++ b/server/internal/daemon/execenv/openclaw_shim_test.go @@ -0,0 +1,466 @@ +package execenv + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + "time" +) + +// TestIsOpenclawShimPath locks the shim-detection surface. Case-insensitivity +// matters because Windows PATH resolution is case-insensitive and npm/PATHEXT +// can hand back `OPENCLAW.CMD`; paths containing spaces and non-ASCII segments +// are included because those are the Windows install locations most likely to +// be mis-parsed, and #6061's open questions called them out explicitly. +func TestIsOpenclawShimPath(t *testing.T) { + t.Parallel() + cases := []struct { + name string + bin string + want bool + }{ + {"npm cmd shim", `C:\Users\dev\AppData\Roaming\npm\openclaw.cmd`, true}, + {"uppercase extension", `C:\npm\OPENCLAW.CMD`, true}, + {"mixed case extension", `C:\npm\openclaw.Cmd`, true}, + {"legacy bat shim", `C:\npm\openclaw.bat`, true}, + {"path with spaces", `C:\Program Files\node modules\openclaw.cmd`, true}, + {"path with unicode segment", `C:\用户\开发\npm\openclaw.cmd`, true}, + {"surrounding whitespace", " C:\\npm\\openclaw.cmd ", true}, + {"real executable", `C:\npm\openclaw.exe`, false}, + {"powershell shim is not a batch shim", `C:\npm\openclaw.ps1`, false}, + {"unix binary without extension", "/usr/local/bin/openclaw", false}, + {"unix path with dotted directory", "/opt/openclaw.cmd.d/openclaw", false}, + {"empty", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := isOpenclawShimPath(tc.bin); got != tc.want { + t.Fatalf("isOpenclawShimPath(%q) = %v, want %v", tc.bin, got, tc.want) + } + }) + } +} + +// exitError produces a real *exec.ExitError so the diagnostic's errors.As gate +// is exercised against the same type production sees, not a stand-in. +// +// The interpreter is invoked by absolute path on purpose: callers stub PATH to +// control the interpreter lookup, and a PATH-dependent helper would break +// depending on the order those two happen in. +func exitError(t *testing.T) error { + t.Helper() + var cmd *exec.Cmd + if runtime.GOOS == "windows" { + shell := os.Getenv("ComSpec") + if shell == "" { + shell = filepath.Join(os.Getenv("SystemRoot"), "System32", "cmd.exe") + } + cmd = exec.Command(shell, "/c", "exit 1") + } else { + cmd = exec.Command("/bin/sh", "-c", "exit 1") + } + err := cmd.Run() + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) { + t.Fatalf("expected *exec.ExitError, got %T (%v)", err, err) + } + return err +} + +// pathWithout points PATH at an empty directory so the interpreter cannot +// resolve. Setting PATH rather than clearing it keeps LookPath on its normal +// code path instead of its empty-PATH special case. +func pathWithout(t *testing.T) { + t.Helper() + t.Setenv("PATH", t.TempDir()) +} + +// writeFakeInterpreter drops an executable named like the interpreter into dir. +// It only has to be resolvable — the diagnostic reports lookup results and +// never runs it. +func writeFakeInterpreter(t *testing.T, dir, name string) string { + t.Helper() + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatalf("write fake interpreter: %v", err) + } + return p +} + +// pathWithFakeNode puts a resolvable interpreter on PATH and nowhere else. +func pathWithFakeNode(t *testing.T) { + t.Helper() + dir := t.TempDir() + name := openclawShimInterpreter + if runtime.GOOS == "windows" { + name += ".exe" + } + writeFakeInterpreter(t, dir, name) + t.Setenv("PATH", dir) +} + +// TestOpenclawShimDiagnosticNamesUnreachableInterpreter is the core #6061 +// regression: a silent shim exit must be reported as an unreachable +// interpreter, with an actionable next step, instead of a bare exit code. +func TestOpenclawShimDiagnosticNamesUnreachableInterpreter(t *testing.T) { + pathWithout(t) + shim := filepath.Join(t.TempDir(), "openclaw.cmd") + got := openclawShimDiagnostic(shim, exitError(t)) + if got == "" { + t.Fatal("expected a diagnostic for a silent .cmd shim failure, got none") + } + for _, want := range []string{ + "resolves neither alongside the shim nor on the daemon PATH", + openclawShimInterpreter, + "openclaw.cmd", + "install Node.js", + } { + if !strings.Contains(got, want) { + t.Errorf("diagnostic missing %q\ngot: %s", want, got) + } + } +} + +// TestOpenclawShimDiagnosticFindsColocatedInterpreter is Sol-Boy's must-fix 2. +// npm's cmd-shim template checks `%dp0%\node.exe` BEFORE falling back to PATH: +// +// IF EXIST "%dp0%\node.exe" ( SET "_prog=%dp0%\node.exe" ) ELSE ( SET "_prog=node" ) +// +// So an install whose Node sits next to the shim runs fine with nothing on +// PATH. Reporting "node is not resolvable" there would be confidently wrong, +// which is worse than staying quiet. +func TestOpenclawShimDiagnosticFindsColocatedInterpreter(t *testing.T) { + pathWithout(t) // nothing on PATH — only the co-located copy can be found + dir := t.TempDir() + shim := filepath.Join(dir, "openclaw.cmd") + name := openclawShimInterpreter + if runtime.GOOS == "windows" { + name += ".exe" + } + writeFakeInterpreter(t, dir, name) + + got := openclawShimDiagnostic(shim, exitError(t)) + if got == "" { + t.Fatal("expected a diagnostic, got none") + } + if !strings.Contains(got, "alongside the shim") { + t.Errorf("diagnostic should credit the co-located interpreter\ngot: %s", got) + } + if strings.Contains(got, "resolves neither") { + t.Errorf("diagnostic must not claim the interpreter is unreachable\ngot: %s", got) + } +} + +// TestOpenclawShimDiagnosticReportsInterpreterOnPath guards the other +// direction, which is the evidence that actually discriminates between the +// competing #6061 hypotheses. If the interpreter resolves, the diagnostic must +// say so rather than blaming PATH, otherwise the next bug report gets steered +// toward the wrong root cause. +func TestOpenclawShimDiagnosticReportsInterpreterOnPath(t *testing.T) { + pathWithFakeNode(t) + shim := filepath.Join(t.TempDir(), "openclaw.cmd") + got := openclawShimDiagnostic(shim, exitError(t)) + if got == "" { + t.Fatal("expected a diagnostic, got none") + } + if !strings.Contains(got, "on the daemon PATH") || !strings.Contains(got, "the interpreter is reachable") { + t.Errorf("diagnostic should clear PATH of blame\ngot: %s", got) + } + if strings.Contains(got, "resolves neither") { + t.Errorf("diagnostic must not claim the interpreter is unreachable\ngot: %s", got) + } +} + +// TestOpenclawShimDiagnosticIsPhrasedConditionally is the rest of must-fix 2. A +// batch extension does not prove npm authorship — an operator can point +// MULTICA_OPENCLAW_PATH at any batch file — so the text must not assert npm +// shim semantics as established fact for whatever failed. +func TestOpenclawShimDiagnosticIsPhrasedConditionally(t *testing.T) { + pathWithout(t) + shim := filepath.Join(t.TempDir(), "custom-wrapper.cmd") + got := openclawShimDiagnostic(shim, exitError(t)) + if got == "" { + t.Fatal("expected a diagnostic, got none") + } + if !strings.Contains(got, "if custom-wrapper.cmd is an npm-generated shim") { + t.Errorf("diagnostic should be conditional about npm authorship\ngot: %s", got) + } +} + +// TestOpenclawShimDiagnosticRedactsLocalPaths is Sol-Boy's must-fix 3, and the +// reason it matters is the blast radius: on prep failure this text is not +// log-local. It travels reportTerminalTask → Client.FailTask and is persisted +// server-side as the task error, so an absolute Windows shim path would upload +// the account name and install layout. +func TestOpenclawShimDiagnosticRedactsLocalPaths(t *testing.T) { + secretDir := filepath.Join(t.TempDir(), "Users", "a-real-person", "AppData", "Roaming", "npm") + if err := os.MkdirAll(secretDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + shim := filepath.Join(secretDir, "openclaw.cmd") + pathDir := filepath.Join(t.TempDir(), "another-private-location") + if err := os.MkdirAll(pathDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + name := openclawShimInterpreter + if runtime.GOOS == "windows" { + name += ".exe" + } + interpreter := writeFakeInterpreter(t, pathDir, name) + t.Setenv("PATH", pathDir) + + got := openclawShimDiagnostic(shim, exitError(t)) + if got == "" { + t.Fatal("expected a diagnostic, got none") + } + for _, leak := range []string{secretDir, shim, pathDir, interpreter, "a-real-person", "another-private-location"} { + if strings.Contains(got, leak) { + t.Errorf("diagnostic leaked local path detail %q\ngot: %s", leak, got) + } + } + if !strings.Contains(got, "openclaw.cmd") { + t.Errorf("diagnostic should still name the shim's base name\ngot: %s", got) + } + if !strings.Contains(got, "1 entry") { + t.Errorf("diagnostic should summarise PATH as a count\ngot: %s", got) + } +} + +// TestOpenclawShimDiagnosticStaysSilentOutOfScope pins the no-op cases. A +// diagnostic attached to a missing binary or a normal native executable would +// be actively misleading. +func TestOpenclawShimDiagnosticStaysSilentOutOfScope(t *testing.T) { + pathWithout(t) + realExit := exitError(t) + cases := []struct { + name string + bin string + err error + }{ + {"native executable", `C:\npm\openclaw.exe`, realExit}, + {"unix binary", "/usr/local/bin/openclaw", realExit}, + {"binary not found", `C:\npm\openclaw.cmd`, exec.ErrNotFound}, + {"nil error", `C:\npm\openclaw.cmd`, nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := openclawShimDiagnostic(tc.bin, tc.err); got != "" { + t.Fatalf("expected no diagnostic, got: %s", got) + } + }) + } +} + +// TestOpenclawShimDiagnosticSurvivesWrappedError confirms the errors.As gate +// still fires when the exit error arrives wrapped, which is how it reaches this +// code once callers have annotated it. +func TestOpenclawShimDiagnosticSurvivesWrappedError(t *testing.T) { + pathWithout(t) + wrapped := errors.Join(errors.New("openclaw config file"), exitError(t)) + shim := filepath.Join(t.TempDir(), "openclaw.cmd") + if got := openclawShimDiagnostic(shim, wrapped); got == "" { + t.Fatal("expected diagnostic through a wrapped exit error, got none") + } +} + +// writeShim creates an executable named with a `.cmd` extension running body. +// +// On Unix the shebang makes a `.cmd`-named file genuinely executable, so the +// full execOpenclawCLI integration path is provable on the normal test job. The +// real npm-shim reproduction lives in the windows-tagged test file. +func writeShim(t *testing.T, dir, unixBody, windowsBody string) string { + t.Helper() + shim := filepath.Join(dir, "openclaw.cmd") + body := unixBody + if runtime.GOOS == "windows" { + body = windowsBody + } + if err := os.WriteFile(shim, []byte(body), 0o755); err != nil { + t.Fatalf("write shim: %v", err) + } + return shim +} + +// TestExecOpenclawCLIAnnotatesSilentShimFailure is the end-to-end proof that +// the diagnostic reaches the error the daemon logs and reports. Before this +// change the message stopped at `exit status 1`, which is what left #6061's +// reporter running their own subprocess experiments to find the cause. +func TestExecOpenclawCLIAnnotatesSilentShimFailure(t *testing.T) { + shim := writeShim(t, t.TempDir(), "#!/bin/sh\nexit 1\n", "@echo off\r\nexit /b 1\r\n") + // Set PATH after creating the shim: the shim is invoked by absolute path, + // while the interpreter lookup must miss. + pathWithout(t) + + _, err := execOpenclawCLI(context.Background(), shim, "config", "file") + if err == nil { + t.Fatal("expected the shim failure to surface as an error") + } + msg := err.Error() + if !strings.Contains(msg, "openclaw config file") { + t.Errorf("error should name the failing subcommand\ngot: %s", msg) + } + if !strings.Contains(msg, "resolves neither alongside the shim nor on the daemon PATH") { + t.Errorf("error should carry the shim diagnostic\ngot: %s", msg) + } +} + +// TestExecOpenclawCLITimeoutIsNotMisdiagnosedAsMissingInterpreter is Sol-Boy's +// must-fix 1, exercised through the real code path rather than by handing the +// diagnostic a synthetic context error. +// +// openclawCLITimeout kills the child through CommandContext, and a killed +// process surfaces as *exec.ExitError ("signal: killed") — the same type a +// genuine exit 1 produces. Without checking the context first, a slow or hung +// CLI was reported as "node is not resolvable, install Node.js", pointing the +// user at something that was never broken. +// +// The shim sleeps only briefly on purpose. execOpenclawCLI sets no WaitDelay +// (see openclawCLITimeout's note on why that is left alone), so cmd.Output() +// stays parked until the output pipes os/exec manages for it — stdout AND +// stderr, since both are set to in-memory writers — reach EOF. A long sleep +// would just make this test hostage to that; it would not leak. The `sleep` +// here inherits those write ends and holds them until it exits, so by the time +// this call returns the helper has finished. (That is a property of this +// helper, not a general rule: a process may close its pipes and keep running.) +func TestExecOpenclawCLITimeoutIsNotMisdiagnosedAsMissingInterpreter(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("covered by TestWindowsOpenclawShimTimeoutIsNotMisdiagnosed with a real cmd.exe host") + } + // Resolve the blocking helper BEFORE PATH is stripped and embed it by + // absolute path. The shim has to keep running with an empty PATH, so it + // cannot rely on a PATH lookup of its own: `sh` on macOS quietly falls back + // to a default PATH, but dash on Linux does not, which made a PATH-relative + // `sleep` pass locally and fail in CI with "sleep: not found". + sleepBin, err := exec.LookPath("sleep") + if err != nil { + t.Skipf("no sleep binary available to build a slow shim: %v", err) + } + shim := writeShim(t, t.TempDir(), "#!/bin/sh\n"+sleepBin+" 1\n", "") + pathWithout(t) // an interpreter lookup, if reached, would report "missing" + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + _, err = execOpenclawCLI(ctx, shim, "config", "file") + if err == nil { + t.Fatal("expected the timed-out invocation to fail") + } + msg := err.Error() + t.Logf("timeout error: %s", msg) + + // The nit from round 2: the context error must be the wrapped cause, so + // standard cancellation checks work instead of only string matching. + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("errors.Is(err, context.DeadlineExceeded) must hold\ngot: %s", msg) + } + for _, forbidden := range []string{"install Node.js", "resolves neither", "the interpreter is reachable"} { + if strings.Contains(msg, forbidden) { + t.Errorf("timeout must not be diagnosed as an interpreter problem (found %q)\ngot: %s", forbidden, msg) + } + } +} + +// TestExecOpenclawCLICancellationIsWrapped pins the same cancellation contract +// for an explicitly cancelled context, not just a deadline, so a caller can +// distinguish "we gave up" from "the CLI failed" without parsing strings. +func TestExecOpenclawCLICancellationIsWrapped(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("shell shim shape is covered by the windows-tagged tests") + } + sleepBin, err := exec.LookPath("sleep") + if err != nil { + t.Skipf("no sleep binary available to build a slow shim: %v", err) + } + shim := writeShim(t, t.TempDir(), "#!/bin/sh\n"+sleepBin+" 1\n", "") + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(100 * time.Millisecond) + cancel() + }() + defer cancel() + + _, err = execOpenclawCLI(ctx, shim, "config", "file") + if err == nil { + t.Fatal("expected the cancelled invocation to fail") + } + if !errors.Is(err, context.Canceled) { + t.Errorf("errors.Is(err, context.Canceled) must hold\ngot: %s", err) + } +} + +// TestExecOpenclawCLIPrefersRealStderr guarantees the diagnostic never masks a +// genuine message from the CLI. This is not hypothetical: windows-latest CI +// showed that a missing `node` DOES reach Go's stderr pipe as "'node' is not +// recognized", so on real Windows the missing-interpreter case takes this +// branch and the diagnostic is only a fallback for a truly silent failure. +func TestExecOpenclawCLIPrefersRealStderr(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("covered by the windows-tagged shim tests with a real cmd.exe host") + } + shim := writeShim(t, t.TempDir(), "#!/bin/sh\necho 'openclaw doctor says hello' >&2\nexit 1\n", "") + pathWithout(t) + + _, err := execOpenclawCLI(context.Background(), shim, "config", "file") + if err == nil { + t.Fatal("expected the shim failure to surface as an error") + } + msg := err.Error() + if !strings.Contains(msg, "openclaw doctor says hello") { + t.Errorf("real stderr must be preserved\ngot: %s", msg) + } + if strings.Contains(msg, "no stderr output") { + t.Errorf("diagnostic must not fire when stderr is present\ngot: %s", msg) + } +} + +// TestExecOpenclawCLIMissingTempDoesNotChangeOutcome pins the root cause #6061 +// originally reported and then retracted. The reporter's own follow-up +// experiment showed `{PATH, SystemRoot}` alone succeeds, so TEMP/TMP must not +// be load-bearing for the OpenClaw CLI invocation. Locking that keeps a future +// change from quietly reintroducing a temp-dir dependency and resurrecting a +// root cause we already ruled out. +func TestExecOpenclawCLIMissingTempDoesNotChangeOutcome(t *testing.T) { + shim := writeShim(t, t.TempDir(), + "#!/bin/sh\necho '/tmp/openclaw/config.json'\n", + "@echo off\r\necho C:\\openclaw\\config.json\r\n", + ) + t.Setenv("TEMP", "") + t.Setenv("TMP", "") + + out, err := execOpenclawCLI(context.Background(), shim, "config", "file") + if err != nil { + t.Fatalf("invocation must not depend on TEMP/TMP: %v", err) + } + if strings.TrimSpace(out) == "" { + t.Fatal("expected the shim's stdout to be returned") + } +} + +// TestExecOpenclawCLIHandlesShimInPathWithSpacesAndUnicode covers the install +// locations #6061's open questions flagged as unverified. A directory +// containing a space or non-ASCII characters must not break invocation or +// mangle the captured output. +func TestExecOpenclawCLIHandlesShimInPathWithSpacesAndUnicode(t *testing.T) { + for _, segment := range []string{"Program Files", "用户 開發", "café dir"} { + t.Run(segment, func(t *testing.T) { + dir := filepath.Join(t.TempDir(), segment) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("mkdir %q: %v", dir, err) + } + shim := writeShim(t, dir, "#!/bin/sh\necho 'ok-marker'\n", "@echo off\r\necho ok-marker\r\n") + out, err := execOpenclawCLI(context.Background(), shim, "config", "file") + if err != nil { + t.Fatalf("shim in %q should be invocable: %v", dir, err) + } + if !strings.Contains(out, "ok-marker") { + t.Fatalf("expected shim stdout to survive intact, got %q", out) + } + }) + } +} diff --git a/server/internal/daemon/execenv/openclaw_shim_windows_test.go b/server/internal/daemon/execenv/openclaw_shim_windows_test.go new file mode 100644 index 0000000000..def58e457c --- /dev/null +++ b/server/internal/daemon/execenv/openclaw_shim_windows_test.go @@ -0,0 +1,290 @@ +//go:build windows + +package execenv + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" +) + +// This file is the Windows half of the MUL-5422 / #6061 regression. The +// cross-platform file proves the diagnostic's logic; only a real cmd.exe host +// can prove how a batch shim actually behaves, which is where #6061's central +// claim lived ("the error goes to the .cmd layer and Go's stderr pipe misses +// it"). The first CI run of this job disproved that claim, and the assertions +// below now encode the observed behaviour instead of the reported guess. +// +// Run via the ci.yml `windows-execenv` job, which scopes -run patterns to the +// Windows-safe tests in this package. + +// npmCmdShimBody reproduces npm's actual generated cmd-shim, not a +// hand-simplified `node ...` one-liner. Faithfulness matters here: the real +// template resolves a co-located `node.exe` BEFORE falling back to PATH, and a +// simplified shim would hide exactly the case Sol-Boy's must-fix 2 called out. +// +// Source: https://github.com/npm/cmd-shim/blob/main/lib/index.js (writeShim_), +// with longProg="%dp0%\node.exe", prog="node", target="%dp0%\openclaw.mjs". +func npmCmdShimBody() string { + return "@ECHO off\r\n" + + "GOTO start\r\n" + + ":find_dp0\r\n" + + "SET dp0=%~dp0\r\n" + + "EXIT /b\r\n" + + ":start\r\n" + + "SETLOCAL\r\n" + + "CALL :find_dp0\r\n" + + "\r\n" + + "IF EXIST \"%dp0%\\node.exe\" (\r\n" + + " SET \"_prog=%dp0%\\node.exe\"\r\n" + + ") ELSE (\r\n" + + " SET \"_prog=node\"\r\n" + + " SET PATHEXT=%PATHEXT:;.JS;=;%\r\n" + + ")\r\n" + + "\r\n" + + "endLocal & goto #_undefined_# 2>NUL || title %COMSPEC% & " + + "set PATHEXT=%PATHEXT:;.JS;=;% & \"%_prog%\" \"%dp0%\\openclaw.mjs\" %*\r\n" +} + +// writeNpmStyleShim writes npm's real shim plus the JS entrypoint it re-execs. +func writeNpmStyleShim(t *testing.T, dir string) string { + t.Helper() + entry := filepath.Join(dir, "openclaw.mjs") + if err := os.WriteFile(entry, []byte("console.log(String.raw`C:\\openclaw\\config.json`);\n"), 0o644); err != nil { + t.Fatalf("write entrypoint: %v", err) + } + shim := filepath.Join(dir, "openclaw.cmd") + if err := os.WriteFile(shim, []byte(npmCmdShimBody()), 0o644); err != nil { + t.Fatalf("write shim: %v", err) + } + return shim +} + +// nodeDir returns the directory holding a real node.exe, skipping the test when +// the runner has no Node installed. +func nodeDir(t *testing.T) string { + t.Helper() + resolved, err := exec.LookPath("node") + if err != nil { + t.Skipf("no node on PATH, cannot exercise a real npm shim: %v", err) + } + abs, err := filepath.Abs(resolved) + if err != nil { + t.Fatalf("resolve node path: %v", err) + } + return filepath.Dir(abs) +} + +// systemPath is the minimum PATH a batch shim needs to run at all (cmd.exe and +// friends live there), without any Node directory on it. +func systemPath(t *testing.T) string { + t.Helper() + root := os.Getenv("SystemRoot") + if root == "" { + root = `C:\Windows` + } + return strings.Join([]string{filepath.Join(root, "System32"), root}, string(os.PathListSeparator)) +} + +// TestWindowsOpenclawShimMissingNodeSurfacesCmdStderr records what actually +// happens on Windows when a real npm shim cannot find its interpreter. +// +// The first CI run of this job showed cmd.exe's "'node' is not recognized" +// reaching Go's stderr pipe, which refutes #6061's premise and means this case +// takes execOpenclawCLI's stderr branch — the shim diagnostic is NOT involved. +// Asserting that directly (rather than "either branch is fine") is deliberate: +// the earlier disjunction let the diagnostic go completely unexercised on +// Windows while still reporting a pass. If this ever flips, this test fails +// loudly and TestWindowsOpenclawShimSilentFailureIsDiagnosed still proves the +// fallback works. +func TestWindowsOpenclawShimMissingNodeSurfacesCmdStderr(t *testing.T) { + nodeDir(t) // skip early if the runner has no Node to remove from PATH + shim := writeNpmStyleShim(t, t.TempDir()) + t.Setenv("PATH", systemPath(t)) + + out, err := execOpenclawCLI(context.Background(), shim, "config", "file") + if err == nil { + t.Fatalf("shim must fail without node on PATH, got output %q", out) + } + msg := err.Error() + t.Logf("observed error: %s", msg) + + if !strings.Contains(strings.ToLower(msg), "not recognized") { + t.Errorf("expected cmd.exe's own stderr to reach Go's pipe; if this now fails, "+ + "the platform behaviour changed and the shim diagnostic is carrying this case instead\ngot: %s", msg) + } + if !strings.Contains(msg, "openclaw config file") { + t.Errorf("error should name the failing subcommand\ngot: %s", msg) + } +} + +// TestWindowsOpenclawShimSilentFailureIsDiagnosed is Sol-Boy's must-fix 4: prove +// the new fallback actually executes on Windows. The missing-node case above +// never reaches it because cmd.exe supplies stderr, so a shim that exits +// non-zero while writing nothing at all is the only way to cover this branch — +// and that silent shape is precisely what #6061's daemon log reported. +func TestWindowsOpenclawShimSilentFailureIsDiagnosed(t *testing.T) { + dir := t.TempDir() + shim := filepath.Join(dir, "openclaw.cmd") + // No output on either stream, non-zero exit. + if err := os.WriteFile(shim, []byte("@ECHO off\r\nexit /b 1\r\n"), 0o644); err != nil { + t.Fatalf("write shim: %v", err) + } + t.Setenv("PATH", systemPath(t)) // no Node anywhere, and none co-located + + _, err := execOpenclawCLI(context.Background(), shim, "config", "file") + if err == nil { + t.Fatal("expected the silent shim to fail") + } + msg := err.Error() + t.Logf("observed error: %s", msg) + if !strings.Contains(msg, "resolves neither alongside the shim nor on the daemon PATH") { + t.Fatalf("the shim diagnostic must carry a silent failure\ngot: %s", msg) + } + // Redaction holds on the platform where the path is actually sensitive. + if strings.Contains(msg, dir) { + t.Errorf("diagnostic leaked the absolute shim path\ngot: %s", msg) + } + if !strings.Contains(msg, "openclaw.cmd") { + t.Errorf("diagnostic should name the shim's base name\ngot: %s", msg) + } +} + +// TestWindowsOpenclawShimColocatedNodeIsCredited covers must-fix 2 on the real +// platform. npm's template runs `%dp0%\node.exe` when it exists, so an install +// with a co-located interpreter is healthy even with nothing on PATH. The +// diagnostic must credit that instead of claiming Node is unreachable. +// +// A placeholder file is enough: this asserts our resolution order, and the file +// is only ever stat'd, never executed. +func TestWindowsOpenclawShimColocatedNodeIsCredited(t *testing.T) { + dir := t.TempDir() + shim := filepath.Join(dir, "openclaw.cmd") + if err := os.WriteFile(shim, []byte("@ECHO off\r\nexit /b 1\r\n"), 0o644); err != nil { + t.Fatalf("write shim: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "node.exe"), []byte("placeholder"), 0o644); err != nil { + t.Fatalf("write co-located node.exe: %v", err) + } + t.Setenv("PATH", systemPath(t)) // nothing Node-ish on PATH + + _, err := execOpenclawCLI(context.Background(), shim, "config", "file") + if err == nil { + t.Fatal("expected the silent shim to fail") + } + msg := err.Error() + if !strings.Contains(msg, "alongside the shim") { + t.Errorf("diagnostic should credit the co-located interpreter\ngot: %s", msg) + } + if strings.Contains(msg, "resolves neither") { + t.Errorf("diagnostic must not claim Node is unreachable\ngot: %s", msg) + } +} + +// TestWindowsOpenclawShimTimeoutIsNotMisdiagnosed is must-fix 1 on Windows: a +// slow shim killed by the context deadline must not be reported as a missing +// interpreter. Go surfaces the kill as *exec.ExitError, so only the explicit +// context check keeps this correct. +// +// The shim waits only briefly: execOpenclawCLI sets no WaitDelay, so +// cmd.Output() stays parked until the output pipes os/exec manages for it +// (stdout and stderr both) reach EOF. `ping` inherits those write ends and +// holds them until it exits, so the ~2s observed in CI is it finishing rather +// than a process left behind; a long wait would only make the test slower. +func TestWindowsOpenclawShimTimeoutIsNotMisdiagnosed(t *testing.T) { + dir := t.TempDir() + shim := filepath.Join(dir, "openclaw.cmd") + // ~2s: ping sends 3 packets one second apart. + body := "@ECHO off\r\nping -n 3 127.0.0.1 >NUL\r\n" + if err := os.WriteFile(shim, []byte(body), 0o644); err != nil { + t.Fatalf("write shim: %v", err) + } + t.Setenv("PATH", systemPath(t)) + + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + _, err := execOpenclawCLI(ctx, shim, "config", "file") + if err == nil { + t.Fatal("expected the timed-out invocation to fail") + } + msg := err.Error() + t.Logf("observed error: %s", msg) + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("errors.Is(err, context.DeadlineExceeded) must hold\ngot: %s", msg) + } + for _, forbidden := range []string{"install Node.js", "resolves neither", "the interpreter is reachable"} { + if strings.Contains(msg, forbidden) { + t.Errorf("timeout must not be diagnosed as an interpreter problem (found %q)\ngot: %s", forbidden, msg) + } + } +} + +// TestWindowsOpenclawShimSucceedsWithNodeOnPath is the positive control. The +// same real npm shim, same absolute path, differing only by whether Node is +// reachable — this is what makes interpreter resolution the proven +// discriminator rather than an assumed one. +func TestWindowsOpenclawShimSucceedsWithNodeOnPath(t *testing.T) { + dir := nodeDir(t) + shim := writeNpmStyleShim(t, t.TempDir()) + t.Setenv("PATH", strings.Join([]string{dir, systemPath(t)}, string(os.PathListSeparator))) + + out, err := execOpenclawCLI(context.Background(), shim, "config", "file") + if err != nil { + t.Fatalf("shim should succeed with node on PATH: %v", err) + } + if !strings.Contains(out, `C:\openclaw\config.json`) { + t.Fatalf("expected the entrypoint's stdout, got %q", out) + } +} + +// TestWindowsOpenclawShimSucceedsWithoutTempVars closes out the root cause +// #6061 first reported and then retracted: with Node reachable but TEMP and TMP +// both unset, the shim must still succeed. Node falls back to a system temp +// directory, so these variables are not load-bearing — and pinning that stops a +// future change from reintroducing the dependency. +func TestWindowsOpenclawShimSucceedsWithoutTempVars(t *testing.T) { + dir := nodeDir(t) + shim := writeNpmStyleShim(t, t.TempDir()) + t.Setenv("PATH", strings.Join([]string{dir, systemPath(t)}, string(os.PathListSeparator))) + t.Setenv("TEMP", "") + t.Setenv("TMP", "") + + out, err := execOpenclawCLI(context.Background(), shim, "config", "file") + if err != nil { + t.Fatalf("shim must not depend on TEMP/TMP: %v", err) + } + if !strings.Contains(out, `C:\openclaw\config.json`) { + t.Fatalf("expected the entrypoint's stdout, got %q", out) + } +} + +// TestWindowsOpenclawShimInPathWithSpacesAndUnicode covers the install +// locations #6061's open questions flagged. `%~dp0` expansion and Go's batch +// argument handling both have to survive a directory with a space or non-ASCII +// characters — `C:\Program Files\...` is a completely ordinary install target. +func TestWindowsOpenclawShimInPathWithSpacesAndUnicode(t *testing.T) { + nodeRoot := nodeDir(t) + for _, segment := range []string{"Program Files copy", "用户 開發"} { + t.Run(segment, func(t *testing.T) { + dir := filepath.Join(t.TempDir(), segment) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("mkdir %q: %v", dir, err) + } + shim := writeNpmStyleShim(t, dir) + t.Setenv("PATH", strings.Join([]string{nodeRoot, systemPath(t)}, string(os.PathListSeparator))) + + out, err := execOpenclawCLI(context.Background(), shim, "config", "file") + if err != nil { + t.Fatalf("shim in %q should be invocable: %v", dir, err) + } + if !strings.Contains(out, `C:\openclaw\config.json`) { + t.Fatalf("expected the entrypoint's stdout, got %q", out) + } + }) + } +}