From c5c591b60beb717f33922af749b3d1bc46ab5de9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Sun, 9 Aug 2026 02:03:13 +0200 Subject: [PATCH] refactor(pty): collapse duplicated PTY and cmdline helpers into common/ptyexec (#2457) Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 5 (1M context) --- .github/workflows/backend-tests.yml | 2 +- apps/backend/AGENTS.md | 7 +- .../agent/loginpty/cmdline_windows.go | 90 ------ .../internal/agent/loginpty/manager.go | 5 +- .../internal/agent/loginpty/pty_handle.go | 10 - .../internal/agent/loginpty/pty_windows.go | 110 -------- .../agent/loginpty/pty_windows_test.go | 69 ----- .../agentctl/server/process/cmdline.go | 89 ------ .../agentctl/server/process/cmdline_test.go | 52 ---- .../agentctl/server/process/interactive_io.go | 5 +- .../server/process/interactive_lifecycle.go | 34 ++- .../process/interactive_lifecycle_test.go | 35 +++ .../server/process/process_signal_unix.go | 2 +- .../server/process/process_signal_windows.go | 9 +- .../agentctl/server/process/pty_handle.go | 12 - .../agentctl/server/process/pty_unix.go | 36 --- .../agentctl/server/process/pty_windows.go | 124 --------- .../server/process/pty_windows_test.go | 82 ------ .../internal/common/ptyexec/cmdline.go | 166 +++++++++++ .../internal/common/ptyexec/cmdline_test.go | 262 ++++++++++++++++++ .../loginpty => common/ptyexec}/pty_unix.go | 6 +- .../internal/common/ptyexec/pty_windows.go | 83 ++++++ .../common/ptyexec/pty_windows_test.go | 34 +++ .../internal/common/ptyexec/ptyexec.go | 32 +++ 24 files changed, 657 insertions(+), 699 deletions(-) delete mode 100644 apps/backend/internal/agent/loginpty/cmdline_windows.go delete mode 100644 apps/backend/internal/agent/loginpty/pty_handle.go delete mode 100644 apps/backend/internal/agent/loginpty/pty_windows.go delete mode 100644 apps/backend/internal/agent/loginpty/pty_windows_test.go delete mode 100644 apps/backend/internal/agentctl/server/process/cmdline.go delete mode 100644 apps/backend/internal/agentctl/server/process/cmdline_test.go create mode 100644 apps/backend/internal/agentctl/server/process/interactive_lifecycle_test.go delete mode 100644 apps/backend/internal/agentctl/server/process/pty_handle.go delete mode 100644 apps/backend/internal/agentctl/server/process/pty_unix.go delete mode 100644 apps/backend/internal/agentctl/server/process/pty_windows.go delete mode 100644 apps/backend/internal/agentctl/server/process/pty_windows_test.go create mode 100644 apps/backend/internal/common/ptyexec/cmdline.go create mode 100644 apps/backend/internal/common/ptyexec/cmdline_test.go rename apps/backend/internal/{agent/loginpty => common/ptyexec}/pty_unix.go (80%) create mode 100644 apps/backend/internal/common/ptyexec/pty_windows.go create mode 100644 apps/backend/internal/common/ptyexec/pty_windows_test.go create mode 100644 apps/backend/internal/common/ptyexec/ptyexec.go diff --git a/.github/workflows/backend-tests.yml b/.github/workflows/backend-tests.yml index ec06868cec..01bed54b2e 100644 --- a/.github/workflows/backend-tests.yml +++ b/.github/workflows/backend-tests.yml @@ -378,7 +378,7 @@ jobs: # a self-hosting Go helper (see testmain_test.go) so they run on # every platform. - name: Test windows-sensitive packages - run: go test -race -v ./internal/agentctl/server/process/... ./internal/agent/runtime/agentctl/launcher/... + run: go test -race -v ./internal/agentctl/server/process/... ./internal/agent/runtime/agentctl/launcher/... ./internal/common/ptyexec/... # Keep port-collision coverage targeted: the instance package contains # broader stdio MCP fixture tests that depend on Unix command behavior. diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 35fddf95b9..7561de25d5 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -211,10 +211,9 @@ Client (WS) ← Orchestrator ← Lifecycle Manager ←──── stream update - Provider pattern for DI; stderr for logs, stdout for ACP only. - Pass context through chains; event bus for cross-component comm. -- Production Git commands must use `subproc.NewGitCommand` with a classified - `subproc.RunGit*` helper, or hold a classified admission slot across streaming - `Start`/`Wait`. Do not construct raw Git commands outside - `internal/common/subproc`; choose `interactive`, `lifecycle`, or `background`. +- Production Git commands must use `subproc.NewGitCommand` with a classified `subproc.RunGit*` helper, or hold a classified admission slot across streaming `Start`/`Wait`. Do not construct raw Git commands outside `internal/common/subproc`; choose `interactive`, `lifecycle`, or `background`. +- **Cross-tier shared code belongs in `internal/common/`.** agentctl ships as a standalone binary uploaded into containers, so neither it nor the backend may own code the other imports — put shared logic in a neutral `internal/common/*` package instead of importing across the boundary or forking a copy (`ptyexec` is the worked example; `subproc`, `gitref`, `securityutil` are older ones). Duplicating to "avoid the dependency" is how the two PTY copies drifted. +- **Pure computation that only matters on Windows should not carry `//go:build windows`.** One CI job runs on Windows and it tests a package allowlist, so tagged code is easily unverified — loginpty's copy of the Win32 quoting was compiled by no job at all. Keep platform API calls behind the tag, leave string/path helpers untagged, and add packages needing native coverage to the `test-windows` job in `.github/workflows/backend-tests.yml`. - **Event-bus wildcard parity:** New NATS wildcard subscriptions must verify equivalent `MemoryEventBus` semantics in `go test ./internal/events/bus`. - **Repository provider identity:** Provider-backed repositories are keyed by workspace, provider, normalized `provider_host` origin, full owner/namespace, and name. Persist `provider_host` when importing or resolving a remote; do not infer self-managed GitLab rows from owner/name alone. Legacy rows with an empty host have unknown identity and must fail closed for provider write/link operations. - **PR status sync coverage:** A `github_pr_watches` row is unique per (session, repository, branch) and gets re-pointed when the session's branch moves on, so watches are not a complete index of a task's PRs; a watch-only sweep leaves every PR from an earlier branch frozen at its last-observed state, which the UI renders as an open, mergeable PR long after it merged. `service_pr_unwatched.go` reconciles the `github_task_prs` rows no watch points at, and deliberately writes **lifecycle fields only** (`state` / `merged_at` / `closed_at`) — check and review aggregates belong to the watch-driven sync that actually fetches reviews and check runs, and the per-PR REST status marks its counts populated even with nothing to count, so widening this path to aggregates persists 0/0 over good data (it broke six PR-popover e2e specs exactly that way). Terminal and detached rows are excluded so the set stays bounded. **The backend is the only writer of `github_task_prs`:** the PR-feedback read makes the same upstream calls the status sync does (`GetPR` + `ListPRReviews` + `ListCheckRuns`, plus comments), so it persists through `SyncTaskPR` (`service_pr_feedback_sync.go`) and lets `github.task_pr.updated` converge every surface. The frontend must never patch PR lifecycle fields into its own store copy — that write cannot reach the DB, so it desyncs the panel from the kanban card and evaporates on reload (it also masked the sync hole above for months). Both derivations share `newPRStatus` so they cannot drift; the persist rides *inside* the TTL cache's fetch closure, so a cache hit costs no write; failures are logged and swallowed because this is a read endpoint; and the row lookup is workspace-scoped (`ListTaskPRsByPRNumber`) to match the credential scope the read was authorized under. Finally, `merged` is the one state `SyncTaskPR` refuses to leave (`resolveTerminalMergeState`) since GitHub cannot un-merge a PR — do **not** generalize that into a `merged > closed > open` rank, because `closed -> open` is a real transition (a PR can be reopened) and ranking it as a regression pins reopened PRs closed forever. diff --git a/apps/backend/internal/agent/loginpty/cmdline_windows.go b/apps/backend/internal/agent/loginpty/cmdline_windows.go deleted file mode 100644 index 74dedafeec..0000000000 --- a/apps/backend/internal/agent/loginpty/cmdline_windows.go +++ /dev/null @@ -1,90 +0,0 @@ -//go:build windows - -package loginpty - -import "strings" - -// Cmdline-quoting helpers for Windows. ConPTY takes a single command-line -// string (not argv), so we reassemble cmd.Args following the MSDN -// CommandLineToArgvW rules — same algorithm as syscall.EscapeArg on Windows. -// -// Duplicated from internal/agentctl/server/process/cmdline.go to avoid having -// the kandev backend reach into the agentctl sidecar package. Both copies -// implement the same well-specified Win32 contract, so drift risk is low. - -// scanArgChars inspects each byte of s and returns whether any character -// requires backslash-escaping (double-quote or backslash) and whether -// the argument contains whitespace (space or tab). -func scanArgChars(s string) (needsBackslash, hasSpace bool) { - for i := 0; i < len(s); i++ { - switch s[i] { - case '"', '\\': - needsBackslash = true - case ' ', '\t': - hasSpace = true - } - } - return needsBackslash, hasSpace -} - -// appendEscapedBytes appends the bytes of s to b applying MSDN -// CommandLineToArgvW backslash-doubling rules for double-quote characters. -// Returns the updated byte slice and the trailing slash count. -func appendEscapedBytes(b []byte, s string) ([]byte, int) { - slashes := 0 - for i := 0; i < len(s); i++ { - c := s[i] - switch c { - default: - slashes = 0 - case '\\': - slashes++ - case '"': - for ; slashes > 0; slashes-- { - b = append(b, '\\') - } - b = append(b, '\\') - } - b = append(b, c) - } - return b, slashes -} - -// escapeArg rewrites a command-line argument following MSDN -// CommandLineToArgvW rules: backslashes are doubled only before quotes, -// quotes are backslash-escaped, and the argument is wrapped in quotes only -// when it contains whitespace. -func escapeArg(s string) string { - if len(s) == 0 { - return `""` - } - needsBackslash, hasSpace := scanArgChars(s) - if !needsBackslash && !hasSpace { - return s - } - if !needsBackslash { - return `"` + s + `"` - } - var b []byte - if hasSpace { - b = append(b, '"') - } - b, slashes := appendEscapedBytes(b, s) - if hasSpace { - for ; slashes > 0; slashes-- { - b = append(b, '\\') - } - b = append(b, '"') - } - return string(b) -} - -// buildCmdLine joins arguments into a single command-line string with proper -// quoting for Windows CreateProcess / ConPTY. -func buildCmdLine(args []string) string { - escaped := make([]string, len(args)) - for i, arg := range args { - escaped[i] = escapeArg(arg) - } - return strings.Join(escaped, " ") -} diff --git a/apps/backend/internal/agent/loginpty/manager.go b/apps/backend/internal/agent/loginpty/manager.go index 39d9a991aa..25580c1b8e 100644 --- a/apps/backend/internal/agent/loginpty/manager.go +++ b/apps/backend/internal/agent/loginpty/manager.go @@ -21,6 +21,7 @@ import ( "github.com/google/uuid" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/common/ptyexec" "go.uber.org/zap" ) @@ -366,7 +367,7 @@ type Session struct { lifetime sessionLifetime mu sync.Mutex cmd *exec.Cmd - pty ptyHandle + pty ptyexec.PtyHandle running bool startedAt time.Time finishedAt *time.Time @@ -408,7 +409,7 @@ func (s *Session) start(cmd []string, cols, rows uint16) error { c := exec.Command(cmd[0], cmd[1:]...) // #nosec G204 — command is hard-coded per agent c.Env = buildLoginEnv() - handle, err := startPTYWithSize(c, cols, rows) + handle, err := ptyexec.Start(c, cols, rows) if err != nil { return fmt.Errorf("start pty: %w", err) } diff --git a/apps/backend/internal/agent/loginpty/pty_handle.go b/apps/backend/internal/agent/loginpty/pty_handle.go deleted file mode 100644 index d6521fc21b..0000000000 --- a/apps/backend/internal/agent/loginpty/pty_handle.go +++ /dev/null @@ -1,10 +0,0 @@ -package loginpty - -import "io" - -// ptyHandle abstracts PTY operations across Unix and Windows. -// Unix wraps creack/pty (*os.File); Windows wraps ConPTY. -type ptyHandle interface { - io.ReadWriteCloser - Resize(cols, rows uint16) error -} diff --git a/apps/backend/internal/agent/loginpty/pty_windows.go b/apps/backend/internal/agent/loginpty/pty_windows.go deleted file mode 100644 index 393d0f89c4..0000000000 --- a/apps/backend/internal/agent/loginpty/pty_windows.go +++ /dev/null @@ -1,110 +0,0 @@ -//go:build windows - -package loginpty - -import ( - "fmt" - "os" - "os/exec" - "path/filepath" - "strings" - "sync" - - "github.com/UserExistsError/conpty" -) - -// windowsPTY wraps a Windows ConPTY pseudo-console. -// -// Close is guarded by sync.Once because the upstream conpty library has no -// internal synchronization: calling its Close twice double-frees the underlying -// Windows handles and triggers STATUS_HEAP_CORRUPTION (0xC0000374). Session.stop -// and the readLoop both end up closing the PTY (stop on shutdown / timeout, the -// child exit naturally drops the handle), so a guard here is mandatory. -type windowsPTY struct { - cpty *conpty.ConPty - closeOnce sync.Once - closeErr error -} - -func (p *windowsPTY) Read(b []byte) (int, error) { return p.cpty.Read(b) } -func (p *windowsPTY) Write(b []byte) (int, error) { return p.cpty.Write(b) } - -func (p *windowsPTY) Close() error { - p.closeOnce.Do(func() { - p.closeErr = p.cpty.Close() - }) - return p.closeErr -} - -func (p *windowsPTY) Resize(cols, rows uint16) error { - return p.cpty.Resize(int(cols), int(rows)) -} - -// startPTYWithSize starts cmd in a Windows ConPTY with the given dimensions. -// ConPTY manages process creation internally, so we build a command line from -// cmd.Args, hand it to conpty.Start, then resolve the PID back into cmd.Process -// so the supervise goroutine can Wait() / Kill() through the usual exec.Cmd API. -func startPTYWithSize(cmd *exec.Cmd, cols, rows uint16) (ptyHandle, error) { - cmdLine := resolveConPtyCmdLine(cmd) - - opts := []conpty.ConPtyOption{ - conpty.ConPtyDimensions(int(cols), int(rows)), - } - if cmd.Dir != "" { - opts = append(opts, conpty.ConPtyWorkDir(cmd.Dir)) - } - if cmd.Env != nil { - opts = append(opts, conpty.ConPtyEnv(cmd.Env)) - } - - cpty, err := conpty.Start(cmdLine, opts...) - if err != nil { - return nil, err - } - - pid := cpty.Pid() - proc, err := os.FindProcess(int(pid)) - if err != nil { - _ = cpty.Close() - return nil, fmt.Errorf("find conpty process %d: %w", pid, err) - } - cmd.Process = proc - - return &windowsPTY{cpty: cpty}, nil -} - -// resolveConPtyCmdLine produces the command line conpty.Start should run. -// -// Win32 CreateProcessW (which ConPTY uses internally) doesn't apply PATHEXT -// and can't execute .cmd / .bat scripts directly. exec.Command already -// PATHEXT-resolved cmd.Path for us, so we substitute it for the bare name in -// cmd.Args[0], and we wrap batch scripts with "cmd.exe /c" so the interpreter -// handles them. Without this, npm-installed login CLIs (claude.cmd, -// gemini.cmd, etc.) fail to launch under ConPTY with "Failed to create -// console process: The system cannot find the file specified". See also the -// agentctl process package's identical fix for passthrough sessions. -func resolveConPtyCmdLine(cmd *exec.Cmd) string { - args := cmd.Args - switch { - case len(args) == 0 && cmd.Path == "": - return "" - case len(args) == 0: - args = []string{cmd.Path} - case cmd.Path != "": - args = append([]string{cmd.Path}, args[1:]...) - } - if isBatchScript(args[0]) { - args = append([]string{"cmd.exe", "/c"}, args...) - } - return buildCmdLine(args) -} - -// isBatchScript reports whether path ends in .cmd or .bat — the two -// CreateProcessW-incompatible Windows script extensions npm shims may use. -func isBatchScript(path string) bool { - switch strings.ToLower(filepath.Ext(path)) { - case ".cmd", ".bat": - return true - } - return false -} diff --git a/apps/backend/internal/agent/loginpty/pty_windows_test.go b/apps/backend/internal/agent/loginpty/pty_windows_test.go deleted file mode 100644 index 3ac5cd05dc..0000000000 --- a/apps/backend/internal/agent/loginpty/pty_windows_test.go +++ /dev/null @@ -1,69 +0,0 @@ -//go:build windows - -package loginpty - -import ( - "os/exec" - "strings" - "testing" -) - -// TestWindowsPTY_DoubleCloseSafe pins down the sync.Once guard on -// windowsPTY.Close. The upstream UserExistsError/conpty library has no -// internal synchronization — a second Close would double-free the -// underlying Windows handles and trigger STATUS_HEAP_CORRUPTION -// (0xC0000374), the same crash class as the original #894 fix in the -// agentctl process package. We deliberately keep this implementation -// duplicated from internal/agentctl/server/process/pty_windows.go (the -// kandev backend shouldn't reach into the sidecar's internals), so the -// safety guard is independently verified here. -func TestWindowsPTY_DoubleCloseSafe(t *testing.T) { - cmd := exec.Command("cmd.exe", "/c", "exit", "0") - pty, err := startPTYWithSize(cmd, 80, 24) - if err != nil { - t.Fatalf("startPTYWithSize: %v", err) - } - - if err := pty.Close(); err != nil { - t.Fatalf("first Close: %v", err) - } - // Second Close must not panic, must not double-free, and must return - // the same memoized error value as the first call. - if err := pty.Close(); err != nil { - t.Fatalf("second Close returned error: %v (expected nil from sync.Once)", err) - } -} - -// TestResolveConPtyCmdLine_CmdShimIsWrapped mirrors the agentctl/process -// package's equivalent test. The CreateProcessW/PATHEXT/.cmd-shim issue -// is identical for login PTY sessions: a bare "claude" or "gemini" Args[0] -// wouldn't reach the resolver, and the .cmd resolved via cmd.Path can't -// run under CreateProcess without the cmd.exe /c wrapper. -func TestResolveConPtyCmdLine_CmdShimIsWrapped(t *testing.T) { - cmd := &exec.Cmd{ - Path: `C:\Users\test\AppData\Roaming\npm\claude.cmd`, - Args: []string{"claude"}, - } - got := resolveConPtyCmdLine(cmd) - want := `cmd.exe /c C:\Users\test\AppData\Roaming\npm\claude.cmd` - if got != want { - t.Errorf("resolveConPtyCmdLine(.cmd shim):\n got: %s\n want: %s", got, want) - } -} - -// TestResolveConPtyCmdLine_ExePassesThrough — .exe binaries should NOT -// gain an intermediate cmd.exe wrapper; that would needlessly fork -// another process and obscure the real PID for signal handling. -func TestResolveConPtyCmdLine_ExePassesThrough(t *testing.T) { - cmd := &exec.Cmd{ - Path: `C:\Program Files\Git\bin\bash.exe`, - Args: []string{"bash", "-l"}, - } - got := resolveConPtyCmdLine(cmd) - if strings.HasPrefix(got, "cmd.exe /c ") { - t.Errorf("resolveConPtyCmdLine wrapped a non-batch executable: %s", got) - } - if !strings.Contains(got, cmd.Path) { - t.Errorf("resolveConPtyCmdLine should substitute resolved cmd.Path %q for Args[0], got: %s", cmd.Path, got) - } -} diff --git a/apps/backend/internal/agentctl/server/process/cmdline.go b/apps/backend/internal/agentctl/server/process/cmdline.go deleted file mode 100644 index 1d61390cf9..0000000000 --- a/apps/backend/internal/agentctl/server/process/cmdline.go +++ /dev/null @@ -1,89 +0,0 @@ -package process - -import "strings" - -// scanArgChars inspects each byte of s and returns whether any character -// requires backslash-escaping (double-quote or backslash) and whether -// the argument contains whitespace (space or tab). -func scanArgChars(s string) (needsBackslash, hasSpace bool) { - for i := 0; i < len(s); i++ { - switch s[i] { - case '"', '\\': - needsBackslash = true - case ' ', '\t': - hasSpace = true - } - } - return needsBackslash, hasSpace -} - -// appendEscapedBytes appends the bytes of s to b applying MSDN -// CommandLineToArgvW backslash-doubling rules for double-quote characters. -// Returns the updated byte slice and the trailing slash count. -func appendEscapedBytes(b []byte, s string) ([]byte, int) { - slashes := 0 - for i := 0; i < len(s); i++ { - c := s[i] - switch c { - default: - slashes = 0 - case '\\': - slashes++ - case '"': - for ; slashes > 0; slashes-- { - b = append(b, '\\') - } - b = append(b, '\\') - } - b = append(b, c) - } - return b, slashes -} - -// escapeArg rewrites a command-line argument following the MSDN -// CommandLineToArgvW parsing rules (same algorithm as Go's -// syscall.EscapeArg on Windows): -// -// - every backslash (\) is doubled, but only if immediately -// followed by a double quote ("); -// - every double quote (") is escaped with a backslash (\); -// - finally, the argument is wrapped in double quotes only if -// it contains spaces or tabs. -// - an empty string becomes "" (two double-quote characters). -func escapeArg(s string) string { - if len(s) == 0 { - return `""` - } - - needsBackslash, hasSpace := scanArgChars(s) - - if !needsBackslash && !hasSpace { - return s - } - if !needsBackslash { - return `"` + s + `"` - } - - var b []byte - if hasSpace { - b = append(b, '"') - } - b, slashes := appendEscapedBytes(b, s) - if hasSpace { - for ; slashes > 0; slashes-- { - b = append(b, '\\') - } - b = append(b, '"') - } - return string(b) -} - -// buildCmdLine joins arguments into a single command-line string -// with proper quoting for Windows CreateProcess. -func buildCmdLine(args []string) string { - escaped := make([]string, len(args)) - for i, arg := range args { - escaped[i] = escapeArg(arg) - } - return strings.Join(escaped, " ") -} diff --git a/apps/backend/internal/agentctl/server/process/cmdline_test.go b/apps/backend/internal/agentctl/server/process/cmdline_test.go deleted file mode 100644 index ff45bd3a9e..0000000000 --- a/apps/backend/internal/agentctl/server/process/cmdline_test.go +++ /dev/null @@ -1,52 +0,0 @@ -package process - -import "testing" - -func TestEscapeArg(t *testing.T) { - tests := []struct { - name string - in string - want string - }{ - {"empty", "", `""`}, - {"simple", "hello", "hello"}, - {"with space", "hello world", `"hello world"`}, - {"with tab", "hello\tworld", `"hello` + "\t" + `world"`}, - {"with quote", `say "hi"`, `"say \"hi\""`}, - {"backslash no quote", `a\b`, `a\b`}, - {"backslash before quote", `a\"`, `a\\\"`}, - {"trailing backslash with space", `a b\`, `"a b\\"`}, - {"multiple trailing backslashes with space", `a b\\`, `"a b\\\\"`}, - {"only backslashes", `\\`, `\\`}, - {"backslash then quote", `\\"`, `\\\\\"`}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := escapeArg(tt.in) - if got != tt.want { - t.Errorf("escapeArg(%q) = %q, want %q", tt.in, got, tt.want) - } - }) - } -} - -func TestBuildCmdLine(t *testing.T) { - tests := []struct { - name string - args []string - want string - }{ - {"single", []string{"cmd.exe"}, "cmd.exe"}, - {"simple args", []string{"git", "status"}, "git status"}, - {"arg with space", []string{"C:\\Program Files\\app.exe", "-f"}, `"C:\Program Files\app.exe" -f`}, - {"arg with quote", []string{"echo", `"hello"`}, `echo \"hello\"`}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := buildCmdLine(tt.args) - if got != tt.want { - t.Errorf("buildCmdLine(%v) = %q, want %q", tt.args, got, tt.want) - } - }) - } -} diff --git a/apps/backend/internal/agentctl/server/process/interactive_io.go b/apps/backend/internal/agentctl/server/process/interactive_io.go index 164d1c8927..6b3f2afc61 100644 --- a/apps/backend/internal/agentctl/server/process/interactive_io.go +++ b/apps/backend/internal/agentctl/server/process/interactive_io.go @@ -3,6 +3,7 @@ package process import ( "time" + "github.com/kandev/kandev/internal/common/ptyexec" "go.uber.org/zap" ) @@ -53,7 +54,7 @@ func (r *InteractiveRunner) readOutput(proc *interactiveProcess) { // respondToTerminalQueries sends synthetic terminal responses (DSR/DA1) to the PTY // when no direct output writer (real terminal) is connected yet. -func (r *InteractiveRunner) respondToTerminalQueries(proc *interactiveProcess, ptyInstance PtyHandle, data []byte) { +func (r *InteractiveRunner) respondToTerminalQueries(proc *interactiveProcess, ptyInstance ptyexec.PtyHandle, data []byte) { if containsDSRQuery(data) { response := "\x1b[1;1R" if _, err := ptyInstance.Write([]byte(response)); err != nil { @@ -79,7 +80,7 @@ func (r *InteractiveRunner) respondToTerminalQueries(proc *interactiveProcess, p // It responds to terminal queries, feeds the status tracker, buffers the output, // routes it to the direct writer or event bus, and manages the prompt-pattern window. // Returns the updated recentOutput string. -func (r *InteractiveRunner) processOutputData(proc *interactiveProcess, ptyInstance PtyHandle, data []byte, recentOutput string) string { +func (r *InteractiveRunner) processOutputData(proc *interactiveProcess, ptyInstance ptyexec.PtyHandle, data []byte, recentOutput string) string { dataStr := string(data) // Respond to cursor position queries (DSR) if no terminal is connected yet. diff --git a/apps/backend/internal/agentctl/server/process/interactive_lifecycle.go b/apps/backend/internal/agentctl/server/process/interactive_lifecycle.go index e45b5d57b4..1d9468d834 100644 --- a/apps/backend/internal/agentctl/server/process/interactive_lifecycle.go +++ b/apps/backend/internal/agentctl/server/process/interactive_lifecycle.go @@ -3,6 +3,7 @@ package process import ( "context" "fmt" + "math" "os" "os/exec" "regexp" @@ -11,14 +12,33 @@ import ( "github.com/google/uuid" "github.com/kandev/kandev/internal/agentctl/types" + "github.com/kandev/kandev/internal/common/ptyexec" "go.uber.org/zap" ) +// Fallback terminal dimensions used when a caller supplies none, or supplies a +// value a PTY cannot represent. +const ( + defaultPTYCols = 120 + defaultPTYRows = 40 +) + +// ptyDimension narrows a terminal dimension to the uint16 that pty.Winsize and +// ptyexec.PtyHandle.Resize take. Out-of-range values fall back to the default +// rather than silently wrapping: InteractiveStartRequest.DefaultCols/DefaultRows +// are plain ints off the wire, so nothing upstream bounds them. +func ptyDimension(v, fallback int) uint16 { + if v <= 0 || v > math.MaxUint16 { + return uint16(fallback) + } + return uint16(v) +} + // interactiveProcess represents a running interactive PTY process. type interactiveProcess struct { info InteractiveProcessInfo cmd *exec.Cmd - ptmx PtyHandle // PTY handle (Unix: creack/pty, Windows: ConPTY) + ptmx ptyexec.PtyHandle // PTY handle (Unix: creack/pty, Windows: ConPTY) buffer *ringBuffer // Turn detection @@ -186,10 +206,10 @@ func (r *InteractiveRunner) immediateStartProcess(req InteractiveStartRequest, p } if cols <= 0 { - cols = 120 + cols = defaultPTYCols } if rows <= 0 { - rows = 40 + rows = defaultPTYRows } var startErr error proc.startOnce.Do(func() { @@ -231,7 +251,7 @@ func (r *InteractiveRunner) startProcess(proc *interactiveProcess, cols, rows in // Start process in PTY with exact dimensions from frontend // Unix: creack/pty, Windows: ConPTY - ptmx, err := startPTYWithSize(cmd, cols, rows) + ptmx, err := ptyexec.Start(cmd, ptyDimension(cols, defaultPTYCols), ptyDimension(rows, defaultPTYRows)) if err != nil { return fmt.Errorf("failed to start pty: %w", err) } @@ -555,11 +575,7 @@ func (r *InteractiveRunner) wait(proc *interactiveProcess) { // subsequent WriteStdin returns "process not found" which is logged. defer proc.firstIdleOnce.Do(func() { close(proc.firstIdleCh) }) - proc.mu.Lock() - ptyHandle := proc.ptmx - proc.mu.Unlock() - - exitCode, signalName, err := waitPtyProcess(proc.cmd, ptyHandle) + exitCode, signalName, err := waitPtyProcess(proc.cmd) status := types.ProcessStatusExited if err != nil { status = types.ProcessStatusFailed diff --git a/apps/backend/internal/agentctl/server/process/interactive_lifecycle_test.go b/apps/backend/internal/agentctl/server/process/interactive_lifecycle_test.go new file mode 100644 index 0000000000..340cac09a4 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/interactive_lifecycle_test.go @@ -0,0 +1,35 @@ +package process + +import ( + "math" + "testing" +) + +// TestPTYDimension covers the narrowing guard in front of ptyexec.Start. +// InteractiveStartRequest.DefaultCols/DefaultRows arrive as plain ints off the +// wire with nothing upstream bounding them, so an out-of-range value must land +// on the default instead of wrapping into a bogus small terminal. +func TestPTYDimension(t *testing.T) { + tests := []struct { + name string + in int + fallback int + want uint16 + }{ + {"typical", 120, defaultPTYCols, 120}, + {"one", 1, defaultPTYCols, 1}, + {"max representable", math.MaxUint16, defaultPTYCols, math.MaxUint16}, + {"zero falls back", 0, defaultPTYCols, defaultPTYCols}, + {"negative falls back", -1, defaultPTYRows, defaultPTYRows}, + // Without the upper guard these would wrap: 65536 -> 0 and 100000 -> 34464. + {"just over max falls back", math.MaxUint16 + 1, defaultPTYCols, defaultPTYCols}, + {"far over max falls back", 100000, defaultPTYRows, defaultPTYRows}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ptyDimension(tt.in, tt.fallback); got != tt.want { + t.Errorf("ptyDimension(%d, %d) = %d, want %d", tt.in, tt.fallback, got, tt.want) + } + }) + } +} diff --git a/apps/backend/internal/agentctl/server/process/process_signal_unix.go b/apps/backend/internal/agentctl/server/process/process_signal_unix.go index 3594ea14dc..6d70c6945b 100644 --- a/apps/backend/internal/agentctl/server/process/process_signal_unix.go +++ b/apps/backend/internal/agentctl/server/process/process_signal_unix.go @@ -15,7 +15,7 @@ func terminateProcess(p *os.Process) error { // waitPtyProcess waits for the PTY process to exit and returns exit info. // On Unix, uses cmd.Wait() which inspects WaitStatus for signal information. -func waitPtyProcess(cmd *exec.Cmd, _ PtyHandle) (exitCode int, signalName string, err error) { +func waitPtyProcess(cmd *exec.Cmd) (exitCode int, signalName string, err error) { err = cmd.Wait() if err == nil { return 0, "", nil diff --git a/apps/backend/internal/agentctl/server/process/process_signal_windows.go b/apps/backend/internal/agentctl/server/process/process_signal_windows.go index 2c768beb3f..168e703aa6 100644 --- a/apps/backend/internal/agentctl/server/process/process_signal_windows.go +++ b/apps/backend/internal/agentctl/server/process/process_signal_windows.go @@ -14,9 +14,12 @@ func terminateProcess(p *os.Process) error { } // waitPtyProcess waits for the PTY process to exit and returns exit info. -// On Windows, uses cmd.Process.Wait() since the process may have been started -// via ConPTY rather than cmd.Start(). -func waitPtyProcess(cmd *exec.Cmd, _ PtyHandle) (exitCode int, signalName string, err error) { +// On Windows, uses cmd.Process.Wait() rather than cmd.Wait(): ConPTY started the +// process, so cmd.Wait() would fail with "exec: not started". cmd.Process is an +// independent handle to the same PID (resolved via os.FindProcess in +// ptyexec.Start), so waiting on it does not depend on the ConPTY object's +// lifetime — which is why neither platform needs the PtyHandle here. +func waitPtyProcess(cmd *exec.Cmd) (exitCode int, signalName string, err error) { state, err := cmd.Process.Wait() if err != nil { return 1, "", err diff --git a/apps/backend/internal/agentctl/server/process/pty_handle.go b/apps/backend/internal/agentctl/server/process/pty_handle.go deleted file mode 100644 index c3759854d9..0000000000 --- a/apps/backend/internal/agentctl/server/process/pty_handle.go +++ /dev/null @@ -1,12 +0,0 @@ -package process - -import "io" - -// PtyHandle abstracts PTY operations across Unix and Windows. -// On Unix, this wraps creack/pty (*os.File). -// On Windows, this wraps Windows ConPTY. -type PtyHandle interface { - io.ReadWriteCloser - // Resize changes the PTY window size. - Resize(cols, rows uint16) error -} diff --git a/apps/backend/internal/agentctl/server/process/pty_unix.go b/apps/backend/internal/agentctl/server/process/pty_unix.go deleted file mode 100644 index 97e80b68ae..0000000000 --- a/apps/backend/internal/agentctl/server/process/pty_unix.go +++ /dev/null @@ -1,36 +0,0 @@ -//go:build !windows - -package process - -import ( - "os" - "os/exec" - - "github.com/creack/pty" -) - -// unixPTY wraps a Unix PTY master file descriptor. -type unixPTY struct { - f *os.File -} - -func (p *unixPTY) Read(b []byte) (int, error) { return p.f.Read(b) } -func (p *unixPTY) Write(b []byte) (int, error) { return p.f.Write(b) } -func (p *unixPTY) Close() error { return p.f.Close() } - -func (p *unixPTY) Resize(cols, rows uint16) error { - return pty.Setsize(p.f, &pty.Winsize{Cols: cols, Rows: rows}) -} - -// startPTYWithSize starts the command in a Unix PTY with the given dimensions. -// The command is started via pty.StartWithSize which calls cmd.Start() internally. -func startPTYWithSize(cmd *exec.Cmd, cols, rows int) (PtyHandle, error) { - f, err := pty.StartWithSize(cmd, &pty.Winsize{ - Cols: uint16(cols), - Rows: uint16(rows), - }) - if err != nil { - return nil, err - } - return &unixPTY{f: f}, nil -} diff --git a/apps/backend/internal/agentctl/server/process/pty_windows.go b/apps/backend/internal/agentctl/server/process/pty_windows.go deleted file mode 100644 index f11234464b..0000000000 --- a/apps/backend/internal/agentctl/server/process/pty_windows.go +++ /dev/null @@ -1,124 +0,0 @@ -//go:build windows - -package process - -import ( - "fmt" - "os" - "os/exec" - "path/filepath" - "strings" - "sync" - - "github.com/UserExistsError/conpty" -) - -// windowsPTY wraps a Windows ConPTY pseudo-console. -// -// Close is guarded by sync.Once because the upstream conpty library has no -// internal synchronization: calling its Close twice double-frees the underlying -// Windows handles and triggers STATUS_HEAP_CORRUPTION (0xC0000374), crashing -// the whole backend. Stop() in interactive_lifecycle.go closes the PTY when -// the user closes a terminal tab, and wait() also closes it once cmd.Wait -// returns — without the gate, both paths hit the same handles. -type windowsPTY struct { - cpty *conpty.ConPty - closeOnce sync.Once - closeErr error -} - -func (p *windowsPTY) Read(b []byte) (int, error) { return p.cpty.Read(b) } -func (p *windowsPTY) Write(b []byte) (int, error) { return p.cpty.Write(b) } - -func (p *windowsPTY) Close() error { - p.closeOnce.Do(func() { - p.closeErr = p.cpty.Close() - }) - return p.closeErr -} - -func (p *windowsPTY) Resize(cols, rows uint16) error { - return p.cpty.Resize(int(cols), int(rows)) -} - -// startPTYWithSize starts the command in a Windows ConPTY with the given dimensions. -// ConPTY manages process creation internally, so this builds a command line from -// the exec.Cmd and starts the process via ConPTY. After this call, cmd.Process -// is set so callers can manage the process lifecycle. -func startPTYWithSize(cmd *exec.Cmd, cols, rows int) (PtyHandle, error) { - cmdLine := resolveConPtyCmdLine(cmd) - - opts := []conpty.ConPtyOption{ - conpty.ConPtyDimensions(cols, rows), - } - if cmd.Dir != "" { - opts = append(opts, conpty.ConPtyWorkDir(cmd.Dir)) - } - - // Pass environment variables directly to the child process via ConPTY. - if cmd.Env != nil { - opts = append(opts, conpty.ConPtyEnv(cmd.Env)) - } - - cpty, err := conpty.Start(cmdLine, opts...) - if err != nil { - return nil, err - } - - // Set cmd.Process so callers can use PID, Kill, Wait, etc. - pid := cpty.Pid() - proc, err := os.FindProcess(int(pid)) - if err != nil { - _ = cpty.Close() - return nil, fmt.Errorf("failed to find ConPTY process %d: %w", pid, err) - } - cmd.Process = proc - - return &windowsPTY{cpty: cpty}, nil -} - -// resolveConPtyCmdLine produces the command line conpty.Start should run. -// -// Win32 CreateProcessW (which ConPTY uses internally) has two limitations that -// matter here: -// -// 1. It does NOT apply PATHEXT — a bare "opencode" won't resolve to -// "opencode.cmd" the way Go's exec.LookPath does. exec.Command on Windows -// already PATHEXT-resolved cmd.Path for us, but cmd.Args[0] is still the -// unresolved name. We have to substitute cmd.Path so CreateProcess sees a -// concrete file. -// -// 2. It cannot execute .cmd / .bat scripts directly — those are interpreted -// by cmd.exe, not by the Win32 loader. We wrap such commands with -// "cmd.exe /c