diff --git a/internal/tools/bash_tool_test.go b/internal/tools/bash_tool_test.go index f00f6e346..297f9816a 100644 --- a/internal/tools/bash_tool_test.go +++ b/internal/tools/bash_tool_test.go @@ -625,7 +625,14 @@ func TestBashToolRequireEscalatedMsysGuard(t *testing.T) { t.Run("approved require_escalated still blocks an unrelated syntax issue", func(t *testing.T) { result := registry.RunWithOptions(context.Background(), "bash", map[string]any{ - "command": `cd /d/tmp/zero-pr-158 && dir`, + // No `&&` here on purpose. The POSIX-style path is what this case is + // about, and on Windows PowerShell 5.1 a chained command trips the + // separate windows_powershell_version preflight first, which shadows + // the block being asserted. That only shows up on 5.1: the runners + // have PowerShell 7, where && is valid and the version check does not + // fire, so the substitution kept this green in CI while failing on the + // shell a stock Windows box actually gets. + "command": `cd /d/tmp/zero-pr-158`, "sandbox_permissions": string(SandboxPermissionsRequireEscalated), }, RunOptions{ PermissionGranted: true, diff --git a/internal/tools/exec_command_test.go b/internal/tools/exec_command_test.go index 343423009..62671cbb1 100644 --- a/internal/tools/exec_command_test.go +++ b/internal/tools/exec_command_test.go @@ -18,7 +18,7 @@ import ( ) func TestIndependentExecCommandConstructorsShareDefaultManager(t *testing.T) { - root := t.TempDir() + root := execTestRoot(t) execTool := NewScopedExecCommandTool(root, nil, nil) writeTool := NewWriteStdinTool(nil) @@ -90,7 +90,7 @@ func TestExecCommandToolDescribesHostShellSyntax(t *testing.T) { } func TestExecCommandReturnsSessionAndWriteStdinPollsCompletion(t *testing.T) { - root := t.TempDir() + root := execTestRoot(t) manager := newExecSessionManager() execTool := NewScopedExecCommandTool(root, nil, manager) writeTool := NewWriteStdinTool(manager) @@ -138,7 +138,7 @@ func TestExecCommandReturnsSessionAndWriteStdinPollsCompletion(t *testing.T) { } func TestExecCommandRequireEscalatedBypassesNativeSandboxAfterApproval(t *testing.T) { - root := t.TempDir() + root := execTestRoot(t) manager := newExecSessionManager() registry := NewRegistry() registry.Register(NewScopedExecCommandTool(root, nil, manager)) @@ -187,7 +187,7 @@ func TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval(t *testing.T) if runtime.GOOS != "windows" { t.Skip("windows-only MSYS sandbox guard") } - root := t.TempDir() + root := execTestRoot(t) manager := newExecSessionManager() registry := NewRegistry() registry.Register(NewScopedExecCommandTool(root, nil, manager)) @@ -224,7 +224,7 @@ func TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval(t *testing.T) } func TestExecCommandReturnsExitCodeWhenCommandCompletesDuringInitialYield(t *testing.T) { - root := t.TempDir() + root := execTestRoot(t) manager := newExecSessionManager() execTool := NewScopedExecCommandTool(root, nil, manager) @@ -263,7 +263,7 @@ func TestExecCommandReportsWorkspaceChanges(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("uses POSIX shell syntax") } - root := t.TempDir() + root := execTestRoot(t) result := NewScopedExecCommandTool(root, nil, newExecSessionManager()).Run(context.Background(), map[string]any{ "cmd": "mkdir -p src node_modules/pkg && printf 'export {}' > src/main.ts && printf generated > node_modules/pkg/index.js", "yield_time_ms": 30000, @@ -357,7 +357,7 @@ func executionRequestHasCapability(request execution.Request, kind execution.Cap } func TestExecCommandForegroundServerReturnsSessionAndServesHTTP(t *testing.T) { - root := t.TempDir() + root := execTestRoot(t) manager := newExecSessionManager() execTool := NewScopedExecCommandTool(root, nil, manager) writeTool := NewWriteStdinTool(manager) @@ -411,7 +411,7 @@ func parseListeningAddress(output string) string { } func TestExecCommandReapsFinishedUnpolledSession(t *testing.T) { - root := t.TempDir() + root := execTestRoot(t) manager := execution.NewProcessManager(execution.ProcessManagerOptions{CompletedRetention: 10 * time.Millisecond}) execTool := NewScopedExecCommandTool(root, nil, manager) @@ -537,7 +537,7 @@ func TestWriteStdinInterruptTerminatesSession(t *testing.T) { } func TestWriteStdinRejectsInputForNonTTYSession(t *testing.T) { - root := t.TempDir() + root := execTestRoot(t) manager := newExecSessionManager() execTool := NewScopedExecCommandTool(root, nil, manager) writeTool := NewWriteStdinTool(manager) @@ -568,9 +568,43 @@ func TestWriteStdinRejectsInputForNonTTYSession(t *testing.T) { manager.Stop(sessionID) } +// execTestRoot is t.TempDir with a cleanup that tolerates Windows still holding +// the directory. +// +// A terminated process does not release its handles the instant the session +// reports it exited, and Windows refuses to remove a directory with an open +// handle where POSIX does not. t.TempDir's own cleanup removes once and fails +// the test on a sharing violation, which turns ordinary teardown timing into a +// test failure with nothing actually wrong underneath it. +func execTestRoot(t *testing.T) string { + t.Helper() + + root, err := os.MkdirTemp("", "zero-exec-test-") + if err != nil { + t.Fatalf("create test root: %v", err) + } + t.Cleanup(func() { + deadline := time.Now().Add(10 * time.Second) + for { + if err := os.RemoveAll(root); err == nil { + return + } + if time.Now().After(deadline) { + // Deliberately not a failure. The directory is under the OS temp + // root and the process holding it has already been terminated, so + // the worst case is a stale directory, not a broken test. + t.Logf("test root %s still held after the cleanup deadline; leaving it", root) + return + } + time.Sleep(50 * time.Millisecond) + } + }) + return root +} + func TestWriteStdinStopIntentTerminatesNonTTYSession(t *testing.T) { for _, chars := range []string{`\u0003`, "exit\n"} { - root := t.TempDir() + root := execTestRoot(t) manager := newExecSessionManager() execTool := NewScopedExecCommandTool(root, nil, manager) writeTool := NewWriteStdinTool(manager) @@ -587,16 +621,46 @@ func TestWriteStdinStopIntentTerminatesNonTTYSession(t *testing.T) { t.Fatalf("session_id is not numeric: %v", err) } - result := writeTool.Run(context.Background(), map[string]any{ - "session_id": sessionID, - "chars": chars, - "yield_time_ms": 1000, - }) - if result.Status != StatusOK { - t.Fatalf("stop input %q status = %s: %s", chars, result.Status, result.Output) - } - if result.Meta["session_id"] != "" { - t.Fatalf("stop input %q should not leave session running, meta=%#v output=%q", chars, result.Meta, result.Output) + // Re-send the stop until the session reports it has exited, rather than + // waiting a fixed slice and asserting on whatever is true by then. + // + // A single 1s yield used to be enough because the session ran under + // cmd.exe. Under PowerShell the tree is slower to stand up and tear down, + // and the FIRST stop in a fresh process pays the interpreter's cold start, + // so it routinely returns with termination already under way but not + // finished. The old assertion read that as "the stop did not work" and + // failed, intermittently, on whichever of the two inputs happened to run + // first. CI never saw it because the runners have PowerShell 7 while a + // stock Windows box only has 5.1. + // + // Repeating the same input rather than polling with empty chars is + // deliberate: Terminate is a no-op against an already-dead tree, so it is + // safe to repeat, and it keeps the interrupt flag that an empty-chars poll + // would drop, which is what the interrupted assertion below needs. + var result Result + // Every poll's output is kept, not just the last one. Each Continue + // returns only what the session produced since the previous collection, + // so a marker printed just before the poll that observes the exit lands + // in the earlier result and would be invisible to a check against the + // final one alone. + var collected strings.Builder + deadline := time.Now().Add(30 * time.Second) + for { + result = writeTool.Run(context.Background(), map[string]any{ + "session_id": sessionID, + "chars": chars, + "yield_time_ms": 500, + }) + collected.WriteString(result.Output) + if result.Status != StatusOK { + t.Fatalf("stop input %q status = %s: %s", chars, result.Status, result.Output) + } + if result.Meta["session_id"] == "" { + break + } + if time.Now().After(deadline) { + t.Fatalf("stop input %q left the session running past the deadline, meta=%#v output=%q", chars, result.Meta, result.Output) + } } if result.Meta["exit_code"] == "" { t.Fatalf("stop input %q should report exit_code, meta=%#v output=%q", chars, result.Meta, result.Output) @@ -604,6 +668,17 @@ func TestWriteStdinStopIntentTerminatesNonTTYSession(t *testing.T) { if result.Meta["interrupted"] != "true" { t.Fatalf("stop input %q should report interrupted metadata, meta=%#v output=%q", chars, result.Meta, result.Output) } + // The session has to have been killed rather than allowed to finish. + // Without this the retry loop makes the test unfalsifiable: the helper + // sleeps 5s, well inside the deadline, so a terminate that did nothing at + // all would let the sleep end on its own and every assertion above would + // still hold. The interrupted flag does not close the hole either, since + // it echoes the request rather than the outcome. The helper prints this + // line only on the way out of a completed sleep, so its absence is what + // separates the two, and it says nothing about how long the loop took. + if strings.Contains(collected.String(), "long sleep finished") { + t.Fatalf("stop input %q let the session run to completion instead of terminating it, output=%q", chars, collected.String()) + } } } @@ -643,7 +718,7 @@ func TestExecCommandTTYSessionAcceptsInputOnLinux(t *testing.T) { if runtime.GOOS != "linux" { t.Skip("pty transport is currently implemented for linux") } - root := t.TempDir() + root := execTestRoot(t) manager := newExecSessionManager() execTool := NewScopedExecCommandTool(root, nil, manager) writeTool := NewWriteStdinTool(manager) @@ -681,7 +756,7 @@ func TestExecCommandTTYSessionAcceptsInputOnLinux(t *testing.T) { } func TestExecSessionSnapshotsAndStopAll(t *testing.T) { - root := t.TempDir() + root := execTestRoot(t) manager := newExecSessionManager() execTool := NewScopedExecCommandTool(root, nil, manager).(execCommandTool)