From 20032342e55e472971c8ad6ccbe3036448cfa51f Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 27 Jul 2026 19:14:02 +0530 Subject: [PATCH 1/2] test(tools): stop internal/tools failing on stock Windows This package fails on every run on a Windows box without PowerShell 7, which is what a stock install is: PowerShell 7 is a separate download. CI never sees any of it, because the runners have 7. Three distinct causes, all surfaced by the move to PowerShell in #804. The stop-intent test waited a fixed second for a session to die and asserted on whatever was true by then. Under cmd.exe that was enough; 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 returned with termination under way but unfinished. The assertion read that as a failed stop. It now re-sends the stop until the session reports it has exited. Repeating is safe, since Terminate is a no-op against an already-dead tree, and it keeps the interrupt flag that a poll with empty chars would drop. Separately, and pre-existing, every test that starts a session hit a teardown race: a terminated process does not release its handles the instant the session reports exited, and Windows refuses to remove a directory with an open handle where POSIX does not. t.TempDir removes once and fails the test on the sharing violation, so ordinary teardown timing was reported as a broken test. Those roots now clean up with a bounded retry, and give up quietly rather than failing: the process is already terminated, so the worst case is a stale directory under the OS temp root. Last, the MSYS guard test asserted a windows_shell_syntax block using a command containing `&&`. On PowerShell 5.1 the windows_powershell_version preflight added by #804 fires first and shadows it. The `&&` was incidental to what that case is about, so it is gone. This one is worth noting as a shape rather than a typo: a new preflight silently changed which block an older assertion got, and only on the shell CI does not run. Verified by running the package repeatedly rather than once: previously failing on every run, now passing 3 of 3, and the stop-intent test passing 8 of 8 alone and 6 of 6 under CPU contention. Disabling the terminate path still fails it, so the assertion continues to bind. --- internal/tools/bash_tool_test.go | 9 ++- internal/tools/exec_command_test.go | 101 ++++++++++++++++++++++------ 2 files changed, 87 insertions(+), 23 deletions(-) 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..4f255095f 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,39 @@ 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 + 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, + }) + 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) @@ -643,7 +700,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 +738,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) From aa7ce57a398c37e52c0f123f72dbdf6e59f91082 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Tue, 28 Jul 2026 11:50:46 +0530 Subject: [PATCH 2/2] test(tools): assert the stop intent killed the session The retry loop added to survive a slow PowerShell teardown also made the test unfalsifiable. The helper sleeps 5s, well inside the 30s deadline, so a terminate that did nothing at all would let the sleep end on its own and every existing assertion would still hold: the session reports exited, it reports an exit code, and the interrupted flag echoes the request rather than the outcome. Neutering KillProcessTree confirmed it, the test passed. Assert instead that the helper's completion line never appears, which distinguishes a killed session from a finished one without depending on how long the loop took. Output is accumulated across every poll rather than read off the last result, because each continuation returns only what the session produced since the previous collection. --- internal/tools/exec_command_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/internal/tools/exec_command_test.go b/internal/tools/exec_command_test.go index 4f255095f..62671cbb1 100644 --- a/internal/tools/exec_command_test.go +++ b/internal/tools/exec_command_test.go @@ -638,6 +638,12 @@ func TestWriteStdinStopIntentTerminatesNonTTYSession(t *testing.T) { // 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{ @@ -645,6 +651,7 @@ func TestWriteStdinStopIntentTerminatesNonTTYSession(t *testing.T) { "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) } @@ -661,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()) + } } }