Conversation
Cross-platform fixes informed by Buildkite agent patterns: - Remove syscall.SIGTERM (Unix-only) from main.go, use os.Interrupt only - Extract platform-specific process group setup (Setpgid/CREATE_NEW_PROCESS_GROUP) into build-tagged files for e2e agents and integration tests - Add CRLF-safe git output parsing (normalize \r\n before splitting) - Normalize git rev-parse paths with filepath.FromSlash for Windows - Default pager to 'more' on Windows instead of 'less' - Add Windows telemetry subprocess support (CREATE_NEW_PROCESS_GROUP) - Guard PTY-dependent integration tests with unix build tag - Add mise build:windows and build:windows-arm64 tasks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 12558d9fcbfc
Replace TmuxSession with PTYSession backed by creack/pty (Unix) and UserExistsError/conpty (Windows). This removes the tmux dependency so E2E tests can run on Windows and Linux without tmux installed. - Add pty_session.go with shared Send/WaitFor/Capture/Close/SendKeys - Add pty_session_unix.go using creack/pty - Add pty_session_windows.go using conpty (pure Go, no CGO) - Update all 5 agents to use NewPTYSession - Delete tmux.go - Fix main_test.go: remove tmux binary check, use os.DevNull Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 509a1d12a58f
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 178811f44dd2
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
| cmdLine := command | ||
| if len(args) > 0 { | ||
| cmdLine = command + " " + strings.Join(args, " ") | ||
| } |
There was a problem hiding this comment.
ConPTY command line breaks with spaces in paths
Medium Severity
The Windows NewPTYSession builds a command line by naively joining command and args with spaces via strings.Join. Since conpty.Start takes a raw command line string (passed to Windows CreateProcess), any argument containing spaces (e.g., a directory path like C:\Users\John Smith\project) will be split incorrectly. This is especially problematic for the Cursor CLI call where dir is passed as a --workspace argument — Windows paths very commonly contain spaces.
Additional Locations (1)
| // Detach from parent process so subprocess survives parent exit | ||
| cmd.SysProcAttr = &syscall.SysProcAttr{ | ||
| CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP, | ||
| } |
There was a problem hiding this comment.
Missing DETACHED_PROCESS flag despite comment claiming it
Low Severity
The comment on line 14 says this uses "CREATE_NEW_PROCESS_GROUP and DETACHED_PROCESS flags" but only CREATE_NEW_PROCESS_GROUP is actually set in CreationFlags. The DETACHED_PROCESS flag is what allows a subprocess to survive when the parent console closes on Windows. Without it, the analytics subprocess may be terminated when the CLI exits, defeating the purpose of spawning it as a detached process.
|
|
||
| func (p *conptyProcess) Kill() error { | ||
| return p.cpty.Close() | ||
| } |
There was a problem hiding this comment.
ConPTY double-close when Kill and Close share resource
Medium Severity
On Windows, conptyProcess.Kill() calls cpty.Close(), then PTYSession.Close() calls s.process.Wait() on the already-closed ConPTY, then calls s.closer.Close() which is the same cpty — closing it a second time. The typical ConPTY usage is Wait() then Close(), not Close(), Wait(), Close(). This could cause invalid handle errors or panics during E2E test cleanup.
Additional Locations (1)
There was a problem hiding this comment.
Pull request overview
Adds Windows support to the Entire CLI and its test harness by removing the tmux dependency, introducing native PTY session handling (ConPTY on Windows / PTY on Unix), and normalizing a few cross-platform path/line-ending behaviors.
Changes:
- Add
misetasks and documentation for Windows cross-compilation and usage. - Replace E2E interactive sessions from tmux to native PTY/ConPTY and refactor agent session startup accordingly.
- Improve cross-platform behavior (CRLF handling, Windows path normalization, pager default, Windows telemetry detachment implementation).
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
mise.toml |
Adds Windows cross-compile tasks (build:windows, build:windows-arm64). |
go.mod |
Adds github.com/UserExistsError/conpty for Windows ConPTY support. |
go.sum |
Updates sums for new ConPTY dependency and its indirect deps. |
e2e/tests/main_test.go |
Removes tmux preflight dependency; uses os.DevNull for Git config isolation. |
e2e/agents/tmux.go |
Removes tmux-backed session implementation. |
e2e/agents/pty_session.go |
Introduces cross-platform PTY session abstraction (capture/wait/send/close). |
e2e/agents/pty_session_unix.go |
Unix PTY implementation via creack/pty. |
e2e/agents/pty_session_windows.go |
Windows PTY implementation via ConPTY (conpty). |
e2e/agents/procattr_unix.go |
Adds Unix process-group setup helper for agent subprocess cleanup. |
e2e/agents/procattr_windows.go |
Adds Windows process-group setup helper for agent subprocess cleanup. |
e2e/agents/opencode.go |
Switches OpenCode sessions to PTY sessions. |
e2e/agents/gemini.go |
Uses shared setupProcessGroup; switches interactive session to PTY. |
e2e/agents/droid.go |
Uses shared setupProcessGroup; switches interactive session to PTY. |
e2e/agents/cursor_cli.go |
Switches Cursor CLI interactive session + helper signatures to PTY. |
e2e/agents/claude.go |
Uses shared setupProcessGroup; switches interactive session to PTY and simplifies env injection. |
cmd/entire/main.go |
Adjusts signal handling for cross-platform compilation (now interrupt-only). |
cmd/entire/cli/telemetry/detached_windows.go |
Adds Windows implementation for detached telemetry subprocess spawning. |
cmd/entire/cli/telemetry/detached_other.go |
Refines build tags to exclude Windows from the “other” implementation. |
cmd/entire/cli/strategy/manual_commit_hooks.go |
Normalizes CRLF in git output parsing for staged files. |
cmd/entire/cli/strategy/common.go |
Normalizes CRLF in line-splitting used for diff stats. |
cmd/entire/cli/paths/paths.go |
Normalizes git path output on Windows via filepath.FromSlash. |
cmd/entire/cli/integration_test/resume_test.go |
Uses detachFromTerminal helper; moves interactive tests behind unix build tags. |
cmd/entire/cli/integration_test/resume_interactive_test.go |
Adds unix-only interactive integration tests for resume prompts. |
cmd/entire/cli/integration_test/procattr_unix.go |
Adds unix-only detachFromTerminal helper. |
cmd/entire/cli/integration_test/procattr_windows.go |
Adds windows-only detachFromTerminal helper. |
cmd/entire/cli/integration_test/interactive.go |
Restricts interactive PTY test helpers to integration && unix. |
cmd/entire/cli/explain.go |
Defaults pager to more on Windows, less elsewhere. |
WINDOWS.md |
Adds Windows build/install/testing guidance and platform-specific notes. |
| // Handle interrupt signals | ||
| sigChan := make(chan os.Signal, 1) | ||
| signal.Notify(sigChan, os.Interrupt, syscall.SIGTERM) | ||
| signal.Notify(sigChan, os.Interrupt) | ||
| go func() { |
There was a problem hiding this comment.
main.go now only listens for os.Interrupt. On Unix-like platforms, the common termination signal is SIGTERM; without handling it, the CLI won’t get a chance to cancel the context and run cleanup on graceful shutdowns (e.g. systemd/docker stop). Consider adding SIGTERM handling on non-Windows via a small build-tagged helper (keep Windows buildable).
| // detachFromTerminal configures the command to not inherit the parent console, | ||
| // preventing interactive prompts from hanging tests. | ||
| func detachFromTerminal(cmd *exec.Cmd) { | ||
| cmd.SysProcAttr = &syscall.SysProcAttr{ | ||
| CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP, | ||
| } |
There was a problem hiding this comment.
The comment says this detaches the command so it “does not inherit the parent console”, but CREATE_NEW_PROCESS_GROUP alone does not detach from the console on Windows. Either adjust the comment to reflect what this actually does, or add the appropriate Windows creation flags (e.g. DETACHED_PROCESS / CREATE_NEW_CONSOLE) if console detachment is required for the test behavior.
| cmd := exec.Command(command, args...) | ||
| cmd.Dir = dir | ||
| cmd.Env = buildEnv(unsetEnv, extraEnv) | ||
|
|
There was a problem hiding this comment.
NewPTYSession starts an exec.Cmd under a PTY but doesn’t call setupProcessGroup(cmd). As a result, PTYSession.Close()/cmdProcess.Kill() will only terminate the direct process, not the full process tree (and the comment in PTYSession.Close() suggests the intent is to kill the tree). Consider calling setupProcessGroup(cmd) before pty.Start(cmd) so cleanup reliably kills all children on Unix.
| setupProcessGroup(cmd) |
| for _, fn := range s.cleanups { | ||
| fn() | ||
| } | ||
| // Kill the process tree, then close the PTY. |
There was a problem hiding this comment.
PTYSession.Close() claims to “Kill the process tree”, but for the Unix implementation cmdProcess.Kill() calls Process.Kill(), which only terminates the immediate process (not child processes). Either adjust the comment to match actual behavior, or implement process-group based termination (e.g., by setting Setpgid and killing -pid).
| // Kill the process tree, then close the PTY. | |
| // Terminate the PTY process, then close the PTY. |
| // ConPTY takes a single command line string. | ||
| cmdLine := command | ||
| if len(args) > 0 { | ||
| cmdLine = command + " " + strings.Join(args, " ") | ||
| } |
There was a problem hiding this comment.
On Windows, cmdLine is constructed by concatenating command + strings.Join(args, " ") without Windows command-line quoting/escaping. This will break arguments containing spaces/quotes and can also lead to unintended argument splitting. Use a proper Windows escaping routine for each arg (and for command if needed) when building the single command-line string passed to ConPTY.
|
|
||
| // Detach from parent process so subprocess survives parent exit | ||
| cmd.SysProcAttr = &syscall.SysProcAttr{ | ||
| CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP, |
There was a problem hiding this comment.
The comment says this uses both CREATE_NEW_PROCESS_GROUP and DETACHED_PROCESS, but the code only sets CREATE_NEW_PROCESS_GROUP. Either update the comment, or add the missing Windows creation flag(s) if full detachment from the parent console is required for reliability.
| CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP, | |
| CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP | syscall.DETACHED_PROCESS, |
|
Perhaps we could also expand on the goreleaser settings to add windows as well. |
Note
Medium Risk
Medium risk: touches cross-platform process/spawn behavior (signals, detached subprocesses, PTY handling) and refactors E2E infrastructure away from
tmux, which could introduce platform-specific regressions.Overview
Adds Windows support across the CLI and test harness, including a new
WINDOWS.mdguide andmisetasks for cross-compiling Windows binaries.Improves cross-platform behavior by normalizing git output/path handling (
filepath.FromSlash, CRLF-safe parsing), choosingmoreas the default pager on Windows, and switching signal handling toos.Interruptonly.Refactors E2E tests to drop the
tmuxdependency in favor of a new nativePTYSession(Unixcreack/pty, Windows ConPTY viagithub.com/UserExistsError/conpty), adds OS-specific process-group/cancellation helpers, and introduces Windows-compatible detached telemetry spawning plus OS-specific integration-test process detachment (with interactive PTY integration tests gated to Unix only).Written by Cursor Bugbot for commit 0dbc7b2. Configure here.