From a5108de926a857b000d0c65a4ccf30464673699a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 15 May 2026 16:04:35 +0100 Subject: [PATCH] fix(windows): make conpty Close idempotent and kill orphan agentctl (#900) --- .github/workflows/backend-tests.yml | 41 +++ Makefile | 43 ++- apps/.node-version | 1 + apps/.nvmrc | 1 + apps/backend/Makefile | 89 ++++-- apps/backend/cmd/kandev/orchestrator.go | 10 + apps/backend/cmd/winjob/main_other.go | 32 ++ apps/backend/cmd/winjob/main_windows.go | 126 ++++++++ apps/backend/go.mod | 2 +- .../agent/lifecycle/env_preparer_local.go | 3 +- .../agent/loginpty/cmdline_windows.go | 90 ++++++ .../internal/agent/loginpty/manager.go | 22 +- .../internal/agent/loginpty/pty_handle.go | 10 + .../internal/agent/loginpty/pty_unix.go | 33 ++ .../internal/agent/loginpty/pty_windows.go | 110 +++++++ .../agent/loginpty/pty_windows_test.go | 69 +++++ .../settings/controller/agent_discovery.go | 3 +- .../settings/controller/agent_install_test.go | 78 +++++ .../agent/settings/controller/install_job.go | 53 +++- .../agentctl/client/launcher/launcher.go | 24 ++ .../client/launcher/lifecycle_unix.go | 15 + .../client/launcher/lifecycle_windows.go | 112 +++++++ .../client/launcher/lifecycle_windows_test.go | 100 +++++++ .../agentctl/server/process/manager_test.go | 6 +- .../agentctl/server/process/pty_windows.go | 73 ++++- .../server/process/pty_windows_test.go | 82 +++++ .../server/process/runner_lifecycle_test.go | 49 +-- .../server/process/runner_shells_test.go | 7 +- .../server/process/runner_state_test.go | 34 ++- .../server/process/runner_terminal_test.go | 4 +- .../agentctl/server/process/runner_test.go | 4 +- .../agentctl/server/process/testmain_test.go | 137 +++++++++ .../agentctl/server/process/vscode_test.go | 6 + .../server/process/workspace_files_test.go | 4 + .../server/process/workspace_monitor_test.go | 4 + .../server/process/workspace_overlap_test.go | 4 + .../process/workspace_poll_mode_loop_test.go | 6 +- .../server/process/workspace_tracker_test.go | 19 ++ .../internal/common/shellexec/shellexec.go | 80 +++++ .../common/shellexec/shellexec_test.go | 80 +++++ .../internal/mcp/handlers/handlers_test.go | 40 +++ .../orchestrator/executor/executor.go | 5 + .../orchestrator/executor/executor_resume.go | 44 +++ ...ecutor_resume_clone_default_branch_test.go | 283 ++++++++++++++++++ .../task/service/directory_listing.go | 75 +++-- .../task/service/directory_listing_test.go | 73 ++++- .../worktree/script_message_handler.go | 7 +- apps/cli/package.json | 2 +- apps/cli/src/dev.ts | 40 ++- apps/cli/src/ports.ts | 40 ++- apps/cli/src/run.test.ts | 6 +- apps/cli/src/web.ts | 21 +- apps/package.json | 4 + apps/web/app/settings/agents/page.tsx | 13 +- 54 files changed, 2197 insertions(+), 122 deletions(-) create mode 100644 apps/.node-version create mode 100644 apps/.nvmrc create mode 100644 apps/backend/cmd/winjob/main_other.go create mode 100644 apps/backend/cmd/winjob/main_windows.go create mode 100644 apps/backend/internal/agent/loginpty/cmdline_windows.go create mode 100644 apps/backend/internal/agent/loginpty/pty_handle.go create mode 100644 apps/backend/internal/agent/loginpty/pty_unix.go create mode 100644 apps/backend/internal/agent/loginpty/pty_windows.go create mode 100644 apps/backend/internal/agent/loginpty/pty_windows_test.go create mode 100644 apps/backend/internal/agentctl/client/launcher/lifecycle_unix.go create mode 100644 apps/backend/internal/agentctl/client/launcher/lifecycle_windows.go create mode 100644 apps/backend/internal/agentctl/client/launcher/lifecycle_windows_test.go create mode 100644 apps/backend/internal/agentctl/server/process/pty_windows_test.go create mode 100644 apps/backend/internal/agentctl/server/process/testmain_test.go create mode 100644 apps/backend/internal/common/shellexec/shellexec.go create mode 100644 apps/backend/internal/common/shellexec/shellexec_test.go create mode 100644 apps/backend/internal/orchestrator/executor/executor_resume_clone_default_branch_test.go diff --git a/.github/workflows/backend-tests.yml b/.github/workflows/backend-tests.yml index 86822403b9..3bba8d7002 100644 --- a/.github/workflows/backend-tests.yml +++ b/.github/workflows/backend-tests.yml @@ -118,3 +118,44 @@ jobs: apps/backend/coverage.out apps/backend/test-results.json retention-days: 30 + + test-windows: + name: Backend (windows) + runs-on: windows-latest + timeout-minutes: 15 + # Least-privilege: this job only reads source and runs tests, no PR/issue + # writes or release operations. Flagged by CodeQL workflow audit + matching + # CodeRabbit nitpick. + permissions: + contents: read + defaults: + run: + working-directory: apps/backend + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v6 + with: + go-version-file: apps/backend/go.mod + cache-dependency-path: apps/backend/go.sum + + # Compile-checks every `*_windows.go` file (pty/conpty wrapper, job-object + # launcher, shell, signals, sysprocattr, vscode cleanup, …). The rest of + # the suite is build-tag-gated out automatically. + - name: Build + run: go build ./... + + - name: Vet + run: go vet ./... + + # Scope to packages with non-trivial Windows-specific code paths. + # Tests that genuinely cannot run on Windows (Unix sockets, POSIX + # symlinks under default Windows ACLs, delete-while-open) call t.Skip + # in source with a documented reason. The shell-fixture tests now use + # 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/agentctl/client/launcher/... diff --git a/Makefile b/Makefile index 909168c889..4256e2fe58 100644 --- a/Makefile +++ b/Makefile @@ -76,6 +76,7 @@ help: @echo "" @echo "Testing:" @echo " test Run all tests (backend + web + cli)" + @echo " test-windows Run Windows-clean subset (curated backend + web + cli)" @echo " test-backend Run backend tests" @echo " test-web Run web app tests" @echo " test-cli Run CLI tests" @@ -104,11 +105,30 @@ help: # Development # +# Build the linux/amd64 agentctl on every host except Linux/x86_64 (where the +# host build IS already linux/amd64). Computed up here, OS-conditionally, so +# the recipe doesn't shell out to `uname` — that fails on Windows under cmd +# and produces spurious "CreateProcess(NULL, uname -s, ...) failed" warnings +# on every Make invocation. +ifeq ($(OS),Windows_NT) + HOST_IS_LINUX_AMD64 := 0 +else + ifeq ($(shell uname -s)/$(shell uname -m),Linux/x86_64) + HOST_IS_LINUX_AMD64 := 1 + else + HOST_IS_LINUX_AMD64 := 0 + endif +endif + .PHONY: dev dev: -ifneq ($(shell uname -s)/$(shell uname -m),Linux/x86_64) +ifneq ($(HOST_IS_LINUX_AMD64),1) @echo "Building Linux agentctl for remote executors..." @$(MAKE) -C $(BACKEND_DIR) build-agentctl-linux +endif +ifeq ($(OS),Windows_NT) + @echo "Building winjob (Ctrl-C-safe wrapper for Windows)..." + @$(MAKE) -C $(BACKEND_DIR) build-winjob endif @echo "Launching via CLI (auto ports)..." @cd $(APPS_DIR) && $(PNPM) -C cli dev -- dev @@ -222,6 +242,27 @@ install-web: test: test-backend test-web test-cli @printf "\n$(GREEN)$(BOLD)✓ All tests complete!$(RESET)\n" +# Curated Windows-clean test run. Mirrors the test-windows job in +# .github/workflows/backend-tests.yml: the backend portion skips ~24 tests +# with Unix-only fixtures (sleep/cat/echo in test inputs, POSIX symlinks, +# delete-while-open). Web and CLI use vitest, which is cross-platform. +# Shrink the backend skip list as fixtures get cleaned up. +# +# Deliberately uses plain `echo` and inlines pnpm invocations (rather than +# depending on test-backend/test-web/test-cli) so it does not pull in the +# `@printf` and `$(shell uname ...)` calls used by other targets — those +# fail on cmd.exe (no printf.exe, no uname.exe) and would break the run on +# Windows even though they are cosmetic on Unix. +.PHONY: test-windows +test-windows: + @echo "[backend] Running Windows-clean subset..." + @$(MAKE) -C $(BACKEND_DIR) test-windows + @echo "[web] Running tests..." + @cd $(APPS_DIR) && $(PNPM) --filter @kandev/web test + @echo "[cli] Running tests..." + @cd $(APPS_DIR) && $(PNPM) --filter kandev test + @echo "Windows-clean test subset complete." + .PHONY: test-sprites-e2e test-sprites-e2e: @$(MAKE) -C $(BACKEND_DIR) test-sprites-e2e diff --git a/apps/.node-version b/apps/.node-version new file mode 100644 index 0000000000..a45fd52cc5 --- /dev/null +++ b/apps/.node-version @@ -0,0 +1 @@ +24 diff --git a/apps/.nvmrc b/apps/.nvmrc new file mode 100644 index 0000000000..a45fd52cc5 --- /dev/null +++ b/apps/.nvmrc @@ -0,0 +1 @@ +24 diff --git a/apps/backend/Makefile b/apps/backend/Makefile index c0c8cd2644..7531ec538c 100644 --- a/apps/backend/Makefile +++ b/apps/backend/Makefile @@ -8,14 +8,37 @@ GOFLAGS := -v LDFLAGS := -s -w # Cross-platform commands +# +# On Windows, GNU Make invokes commands through cmd.exe, which does not +# support Unix `NAME=VAL cmd` env-var-prefix syntax. Instead, `set NAME=VAL&` +# (single ampersand) chains a subsequent command in the same shell, scoping +# the env var to one recipe line — the closest cmd analog to the Unix form. +# `EXE` adds the .exe suffix to build outputs; Go does NOT auto-append it +# when `-o` is explicit. +# +# Install GNU Make on Windows with `winget install ezwinports.make` (or via +# scoop/chocolatey). Targets that rely on Unix builtins (run, dev, +# start-debug, build-cross, build-agentctl-linux) remain Unix-only. ifeq ($(OS),Windows_NT) RM = cmd /c del /s /q RMDIR = cmd /c rmdir /s /q + # cmd's `mkdir` errors when the dir already exists, unlike Unix `mkdir -p`. + # Call sites use `-@$(MKDIR) $(BUILD_DIR)` (note the `-` prefix) so the + # "already exists" failure is tolerated; on Unix the prefix is a no-op. MKDIR = cmd /c mkdir + CGO_PREFIX = set CGO_ENABLED=1& + # Cross-compile prefix for the linux/amd64 agentctl bundled with the host + # build (used by remote-Sprites executors). cmd has no Unix env-var-prefix + # syntax; `set X=Y&` chains `set` and the next command in the same shell. + GO_LINUX_AMD64 = set CGO_ENABLED=0& set GOOS=linux& set GOARCH=amd64& + EXE = .exe else RM = rm -f RMDIR = rm -rf MKDIR = mkdir -p + CGO_PREFIX = CGO_ENABLED=1 + GO_LINUX_AMD64 = CGO_ENABLED=0 GOOS=linux GOARCH=amd64 + EXE = endif # Build info @@ -25,7 +48,7 @@ BUILD_TIME := $(shell date -u '+%Y-%m-%dT%H:%M:%SZ' 2>/dev/null || powershell -c LDFLAGS += -X main.Version=$(VERSION) -X main.Commit=$(COMMIT) -X main.BuildTime=$(BUILD_TIME) -.PHONY: all build build-all build-agentctl build-agentctl-linux build-acpdbg acpdbg build-mock-agent build-mock-agent-linux build-preview clean run dev start-debug test test-e2e test-sprites-e2e lint fmt vet help +.PHONY: all build build-all build-agentctl build-agentctl-linux build-acpdbg acpdbg build-mock-agent build-mock-agent-linux build-preview build-winjob clean run dev start-debug test test-e2e test-sprites-e2e lint fmt vet help ## Default target all: build @@ -33,50 +56,59 @@ all: build ## Build the preview CLI for deploying PR preview environments to Sprites build-preview: @echo "Building preview..." - @$(MKDIR) $(BUILD_DIR) - $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/preview ./cmd/preview + -@$(MKDIR) $(BUILD_DIR) + $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/preview$(EXE) ./cmd/preview ## Build the mock-agent binary for testing build-mock-agent: @echo "Building mock-agent..." - @$(MKDIR) $(BUILD_DIR) - $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/mock-agent ./cmd/mock-agent + -@$(MKDIR) $(BUILD_DIR) + $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/mock-agent$(EXE) ./cmd/mock-agent + +## Build the winjob helper — wraps a subprocess in a Windows Job Object so +## Ctrl-C at the top of the chain reliably terminates the entire subtree +## (kandev, agentctl, etc.) instead of leaking them. No-op passthrough on Unix. +## See apps/cli/src/dev.ts for how the CLI launcher uses it. +build-winjob: + @echo "Building winjob..." + -@$(MKDIR) $(BUILD_DIR) + $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/winjob$(EXE) ./cmd/winjob ## Build and run acpdbg — usage: make acpdbg ARGS="probe auggie" acpdbg: build-acpdbg - @$(BUILD_DIR)/acpdbg $(ARGS) + @$(BUILD_DIR)/acpdbg$(EXE) $(ARGS) ## Build the acpdbg binary — raw ACP JSON-RPC debugger (see .agents/skills/acp-debug) build-acpdbg: @echo "Building acpdbg..." - @$(MKDIR) $(BUILD_DIR) - $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/acpdbg ./cmd/acpdbg + -@$(MKDIR) $(BUILD_DIR) + $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/acpdbg$(EXE) ./cmd/acpdbg ## Build the unified kandev binary. Always includes the linux/amd64 agentctl ## variant because remote executors (local Docker, remote Docker, Sprites) ## bind-mount it into containers — without it, the first Docker session on a ## non-Linux host fails with "agentctl linux binary not found". -build: build-agentctl build-agentctl-linux build-mock-agent build-acpdbg +build: build-agentctl build-agentctl-linux build-mock-agent build-acpdbg build-winjob @echo "Building $(BINARY_NAME)..." - @$(MKDIR) $(BUILD_DIR) - CGO_ENABLED=1 $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/$(BINARY_NAME) ./cmd/kandev + -@$(MKDIR) $(BUILD_DIR) + $(CGO_PREFIX) $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/$(BINARY_NAME)$(EXE) ./cmd/kandev ## Build all binaries (unified + individual services + agentctl) build-all: build build-agentctl @echo "Building individual services..." - $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/agent-manager ./cmd/agent-manager + $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/agent-manager$(EXE) ./cmd/agent-manager ## Build the agentctl sidecar binary (includes embedded MCP server) build-agentctl: @echo "Building agentctl..." - @$(MKDIR) $(BUILD_DIR) - $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/agentctl ./cmd/agentctl + -@$(MKDIR) $(BUILD_DIR) + $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/agentctl$(EXE) ./cmd/agentctl ## Build agentctl for linux/amd64 (used by Sprites remote executor) build-agentctl-linux: @echo "Building agentctl for linux/amd64..." - @$(MKDIR) $(BUILD_DIR) - GOOS=linux GOARCH=amd64 $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/agentctl-linux-amd64 ./cmd/agentctl + -@$(MKDIR) $(BUILD_DIR) + $(GO_LINUX_AMD64) $(GO) build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/agentctl-linux-amd64 ./cmd/agentctl ## Build mock-agent for linux/amd64 (used by Docker E2E tests) build-mock-agent-linux: @@ -94,22 +126,32 @@ clean: ## Run the application run: build @echo "Running $(BINARY_NAME)..." - ./$(BUILD_DIR)/$(BINARY_NAME) + ./$(BUILD_DIR)/$(BINARY_NAME)$(EXE) ## Run in development mode (with pprof enabled) dev: build @echo "Running $(BINARY_NAME) in development mode..." - @KANDEV_DEBUG_PPROF_ENABLED=true exec ./$(BUILD_DIR)/$(BINARY_NAME) + @KANDEV_DEBUG_PPROF_ENABLED=true exec ./$(BUILD_DIR)/$(BINARY_NAME)$(EXE) ## Run in debug mode (pprof + debug logging) start-debug: build @echo "Running $(BINARY_NAME) in debug mode (pprof + debug logging)..." - @KANDEV_DEBUG_PPROF_ENABLED=true KANDEV_LOG_LEVEL=debug exec ./$(BUILD_DIR)/$(BINARY_NAME) + @KANDEV_DEBUG_PPROF_ENABLED=true KANDEV_LOG_LEVEL=debug exec ./$(BUILD_DIR)/$(BINARY_NAME)$(EXE) ## Run tests test: @echo "Running tests..." - CGO_ENABLED=1 $(GO) test ./... + $(CGO_PREFIX) $(GO) test ./... + +## Run only the Windows-sensitive packages — mirrors the test-windows job in +## .github/workflows/backend-tests.yml. The broader suite uses Unix-only +## shell commands in test fixtures and would need GOOS guards to be portable; +## this narrow scope is the regression coverage for the conpty/job-object +## bug class. Tests that genuinely cannot run on Windows skip themselves via +## runtime.GOOS guards in source. +test-windows: + @echo "Running Windows-sensitive packages..." + $(CGO_PREFIX) $(GO) test -v ./internal/agentctl/server/process/... ./internal/agentctl/client/launcher/... ## Run E2E tests (real agents, costs money — manual only) ## Set agent commands via env vars: E2E_CLAUDE_CODE_COMMAND, E2E_AMP_COMMAND @@ -128,7 +170,7 @@ test-sprites-e2e: build-agentctl-linux ## Run tests with coverage test-coverage: @echo "Running tests with coverage..." - CGO_ENABLED=1 $(GO) test -v -coverprofile=coverage.out ./... + $(CGO_PREFIX) $(GO) test -v -coverprofile=coverage.out ./... $(GO) tool cover -html=coverage.out -o coverage.html ## Run linter (requires golangci-lint) @@ -159,7 +201,7 @@ deps: ## Build for multiple platforms build-cross: @echo "Building for multiple platforms..." - @$(MKDIR) $(BUILD_DIR) + -@$(MKDIR) $(BUILD_DIR) CGO_ENABLED=1 GOOS=linux GOARCH=amd64 $(GO) build -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/$(BINARY_NAME)-linux-amd64 ./cmd/kandev CGO_ENABLED=1 GOOS=darwin GOARCH=amd64 $(GO) build -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/$(BINARY_NAME)-darwin-amd64 ./cmd/kandev CGO_ENABLED=1 GOOS=darwin GOARCH=arm64 $(GO) build -ldflags "$(LDFLAGS)" -o $(BUILD_DIR)/$(BINARY_NAME)-darwin-arm64 ./cmd/kandev @@ -182,7 +224,8 @@ help: @echo " run Build and run the application" @echo " dev Run in development mode (pprof enabled)" @echo " start-debug Run in debug mode (pprof + debug logging)" - @echo " test Run tests" + @echo " test Run tests (full suite — fails on Windows pending fixture cleanup)" + @echo " test-windows Run only the Windows-clean subset (matches CI windows-latest job)" @echo " test-e2e Run E2E adapter tests (real agents, costs money)" @echo " test-coverage Run tests with coverage report" @echo " lint Run golangci-lint" diff --git a/apps/backend/cmd/kandev/orchestrator.go b/apps/backend/cmd/kandev/orchestrator.go index a83e71e307..099fe90a40 100644 --- a/apps/backend/cmd/kandev/orchestrator.go +++ b/apps/backend/cmd/kandev/orchestrator.go @@ -334,6 +334,16 @@ func (u *repoLocalPathUpdater) UpdateRepositoryLocalPath(ctx context.Context, re return err } +func (u *repoLocalPathUpdater) UpdateRepositoryDefaultBranch(ctx context.Context, repositoryID, defaultBranch string) error { + if repositoryID == "" || defaultBranch == "" { + return fmt.Errorf("UpdateRepositoryDefaultBranch: repositoryID and defaultBranch must be non-empty") + } + _, err := u.svc.UpdateRepository(ctx, repositoryID, &taskservice.UpdateRepositoryRequest{ + DefaultBranch: &defaultBranch, + }) + return err +} + // repositoryResolverAdapter resolves GitHub repos by cloning + finding/creating DB records. type repositoryResolverAdapter struct { cloner *repoclone.Cloner diff --git a/apps/backend/cmd/winjob/main_other.go b/apps/backend/cmd/winjob/main_other.go new file mode 100644 index 0000000000..01d8c2b0f4 --- /dev/null +++ b/apps/backend/cmd/winjob/main_other.go @@ -0,0 +1,32 @@ +//go:build !windows + +// winjob is a Win32 Job Object wrapper for the CLI launcher's backend spawn. +// On Unix it's a transparent passthrough: process groups already give us +// reliable cascading termination when the parent dies (Pdeathsig on Linux, +// process-group SIGINT on signal delivery to the foreground job). +package main + +import ( + "fmt" + "os" + "os/exec" +) + +func main() { + if len(os.Args) < 2 { + fmt.Fprintln(os.Stderr, "usage: winjob [args...]") + os.Exit(2) + } + cmd := exec.Command(os.Args[1], os.Args[2:]...) + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Env = os.Environ() + if err := cmd.Run(); err != nil { + if exitErr, ok := err.(*exec.ExitError); ok { + os.Exit(exitErr.ExitCode()) + } + fmt.Fprintf(os.Stderr, "winjob: %v\n", err) + os.Exit(1) + } +} diff --git a/apps/backend/cmd/winjob/main_windows.go b/apps/backend/cmd/winjob/main_windows.go new file mode 100644 index 0000000000..47a1a4ce27 --- /dev/null +++ b/apps/backend/cmd/winjob/main_windows.go @@ -0,0 +1,126 @@ +//go:build windows + +// winjob spawns a child program inside a Windows Job Object configured with +// JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, so the OS terminates the child (and its +// own descendants, which inherit the job) the moment winjob itself exits — by +// Ctrl-C, by parent kill, by crash, by anything. +// +// It exists because the chain that runs `make dev` on Windows from Git Bash +// (bash → make → pnpm → node → make → sh → kandev) drops signals between +// MSYS-aware and native-Win32 processes. Even if Node's tree-kill supervisor +// runs, the chain has multiple links that can leak processes when Ctrl-C only +// reaches the top-level shell. Wrapping the backend spawn in winjob makes +// cleanup a kernel-level guarantee instead of a "please forward" chain. +// +// On Unix this binary is a transparent passthrough (see main_other.go) — POSIX +// process groups already give us reliable cascading termination. +package main + +import ( + "fmt" + "os" + "os/exec" + "os/signal" + "unsafe" + + "golang.org/x/sys/windows" +) + +func main() { + if len(os.Args) < 2 { + fmt.Fprintln(os.Stderr, "usage: winjob [args...]") + os.Exit(2) + } + + // Ignore Ctrl-C in winjob itself. The child receives CTRL_C_EVENT via the + // shared console (Go's default handler exits on it). When the child exits, + // winjob proceeds past cmd.Wait and closes the job handle. If winjob took + // the Ctrl-C first and exited before the child, KILL_ON_JOB_CLOSE would + // still fire — but giving the child a chance to clean up is friendlier + // for processes that handle SIGINT themselves (e.g. flush state, save a + // session). + signalCh := make(chan os.Signal, 1) + signal.Notify(signalCh, os.Interrupt) + go func() { + for range signalCh { + // Drop the signal; nothing to do — the child got it too. + } + }() + + job, err := createKillOnCloseJob() + if err != nil { + die("create job: %v", err) + } + // Don't `defer CloseHandle(job)` — we want the OS to close it as part of + // process teardown so KILL_ON_JOB_CLOSE fires correctly on any exit path + // including os.Exit and panics. Manual close on success is also fine + // because the child has already exited by then. + + cmd := exec.Command(os.Args[1], os.Args[2:]...) + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Env = os.Environ() + if err := cmd.Start(); err != nil { + _ = windows.CloseHandle(job) + die("start child: %v", err) + } + + if err := assignProcessToJob(job, cmd.Process.Pid); err != nil { + _ = cmd.Process.Kill() + _ = windows.CloseHandle(job) + die("assign job: %v", err) + } + + waitErr := cmd.Wait() + _ = windows.CloseHandle(job) + if waitErr != nil { + if exitErr, ok := waitErr.(*exec.ExitError); ok { + os.Exit(exitErr.ExitCode()) + } + die("wait: %v", waitErr) + } +} + +func createKillOnCloseJob() (windows.Handle, error) { + job, err := windows.CreateJobObject(nil, nil) + if err != nil { + return 0, fmt.Errorf("CreateJobObject: %w", err) + } + info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ + BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ + LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }, + } + if _, err := windows.SetInformationJobObject( + job, + windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&info)), + uint32(unsafe.Sizeof(info)), + ); err != nil { + _ = windows.CloseHandle(job) + return 0, fmt.Errorf("SetInformationJobObject: %w", err) + } + return job, nil +} + +func assignProcessToJob(job windows.Handle, pid int) error { + procHandle, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, + false, + uint32(pid), + ) + if err != nil { + return fmt.Errorf("OpenProcess(%d): %w", pid, err) + } + defer windows.CloseHandle(procHandle) + if err := windows.AssignProcessToJobObject(job, procHandle); err != nil { + return fmt.Errorf("AssignProcessToJobObject: %w", err) + } + return nil +} + +func die(format string, args ...any) { + fmt.Fprintf(os.Stderr, "winjob: "+format+"\n", args...) + os.Exit(1) +} diff --git a/apps/backend/go.mod b/apps/backend/go.mod index 2db8acdccc..211f49098a 100644 --- a/apps/backend/go.mod +++ b/apps/backend/go.mod @@ -26,6 +26,7 @@ require ( go.opentelemetry.io/otel/trace v1.43.0 go.uber.org/zap v1.26.0 golang.org/x/sync v0.20.0 + golang.org/x/sys v0.42.0 gopkg.in/yaml.v3 v3.0.1 ) @@ -101,7 +102,6 @@ require ( golang.org/x/arch v0.3.0 // indirect golang.org/x/crypto v0.49.0 // indirect golang.org/x/net v0.52.0 // indirect - golang.org/x/sys v0.42.0 // indirect golang.org/x/term v0.41.0 // indirect golang.org/x/text v0.35.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260401024825-9d38bb4040a9 // indirect diff --git a/apps/backend/internal/agent/lifecycle/env_preparer_local.go b/apps/backend/internal/agent/lifecycle/env_preparer_local.go index 9ba62d54d1..ca7cb64e48 100644 --- a/apps/backend/internal/agent/lifecycle/env_preparer_local.go +++ b/apps/backend/internal/agent/lifecycle/env_preparer_local.go @@ -13,6 +13,7 @@ import ( "go.uber.org/zap" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/common/shellexec" "github.com/kandev/kandev/internal/worktree" ) @@ -180,7 +181,7 @@ const setupScriptStreamInterval = 100 * time.Millisecond // streaming combined stdout/stderr to onOutput (if non-nil) as it runs. // Returns the full accumulated output (trimmed) and any execution error. func runSetupScript(ctx context.Context, script, workDir string, env map[string]string, onOutput func(current string)) (string, error) { - cmd := exec.CommandContext(ctx, "bash", "-c", script) + cmd := shellexec.CommandContext(ctx, shellexec.Bash, script) if workDir != "" { cmd.Dir = workDir } diff --git a/apps/backend/internal/agent/loginpty/cmdline_windows.go b/apps/backend/internal/agent/loginpty/cmdline_windows.go new file mode 100644 index 0000000000..74dedafeec --- /dev/null +++ b/apps/backend/internal/agent/loginpty/cmdline_windows.go @@ -0,0 +1,90 @@ +//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 a7b3e9ad50..4dcdd3f48e 100644 --- a/apps/backend/internal/agent/loginpty/manager.go +++ b/apps/backend/internal/agent/loginpty/manager.go @@ -19,7 +19,6 @@ import ( "sync" "time" - "github.com/creack/pty" "github.com/google/uuid" "github.com/kandev/kandev/internal/common/logger" "go.uber.org/zap" @@ -160,6 +159,13 @@ loop: for { select { case info = <-exited: + // Natural child exit. On Windows ConPTY, the underlying handle does + // NOT return EOF when the child process terminates, so the readLoop + // would block on Read until something closes the PTY. Closing here + // unblocks it so the readDone wait below resolves promptly. On Unix + // the master already saw EOF from the slave close — this is just an + // extra (idempotent) Close that the windowsPTY sync.Once also guards. + sess.stop() break loop case <-idle.C: sess.log.Info("login session idle timeout — terminating") @@ -250,7 +256,7 @@ type Session struct { mu sync.Mutex cmd *exec.Cmd - pty *os.File + pty ptyHandle running bool startedAt time.Time finishedAt *time.Time @@ -275,13 +281,13 @@ 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() - f, err := pty.StartWithSize(c, &pty.Winsize{Cols: cols, Rows: rows}) + handle, err := startPTYWithSize(c, cols, rows) if err != nil { return fmt.Errorf("start pty: %w", err) } s.cmd = c - s.pty = f + s.pty = handle s.running = true s.log.Info("login session started", zap.String("session_id", s.ID), @@ -413,7 +419,7 @@ func (s *Session) Resize(cols, rows uint16) error { if !running || pf == nil { return ErrSessionNotRunning } - return pty.Setsize(pf, &pty.Winsize{Cols: cols, Rows: rows}) + return pf.Resize(cols, rows) } // Status snapshot for HTTP responses. @@ -452,9 +458,9 @@ func (s *Session) Status() Status { // stop tears the PTY down idempotently. The `!s.running` guard is flipped // *under the same lock* that we use to read pty/cmd, so concurrent callers // (timeout case in supervise + external Manager.Stop, etc.) don't double- -// close the file descriptor on the way out. A double Close on *os.File is -// safe in isolation, but if the OS recycles the fd between the two Close() -// calls, the second close hits a different open file. +// close the underlying handle on the way out. The Windows ConPTY backing +// also has its own sync.Once on Close because its upstream library has no +// internal synchronization and a double-close triggers STATUS_HEAP_CORRUPTION. func (s *Session) stop() { s.mu.Lock() if !s.running { diff --git a/apps/backend/internal/agent/loginpty/pty_handle.go b/apps/backend/internal/agent/loginpty/pty_handle.go new file mode 100644 index 0000000000..d6521fc21b --- /dev/null +++ b/apps/backend/internal/agent/loginpty/pty_handle.go @@ -0,0 +1,10 @@ +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_unix.go b/apps/backend/internal/agent/loginpty/pty_unix.go new file mode 100644 index 0000000000..2e16df8eb1 --- /dev/null +++ b/apps/backend/internal/agent/loginpty/pty_unix.go @@ -0,0 +1,33 @@ +//go:build !windows + +package loginpty + +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 cmd under a Unix PTY with the given dimensions. +// pty.StartWithSize calls cmd.Start() internally, so cmd.Process is set on return. +func startPTYWithSize(cmd *exec.Cmd, cols, rows uint16) (ptyHandle, error) { + f, err := pty.StartWithSize(cmd, &pty.Winsize{Cols: cols, Rows: rows}) + if err != nil { + return nil, err + } + return &unixPTY{f: f}, nil +} diff --git a/apps/backend/internal/agent/loginpty/pty_windows.go b/apps/backend/internal/agent/loginpty/pty_windows.go new file mode 100644 index 0000000000..393d0f89c4 --- /dev/null +++ b/apps/backend/internal/agent/loginpty/pty_windows.go @@ -0,0 +1,110 @@ +//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 new file mode 100644 index 0000000000..3ac5cd05dc --- /dev/null +++ b/apps/backend/internal/agent/loginpty/pty_windows_test.go @@ -0,0 +1,69 @@ +//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/agent/settings/controller/agent_discovery.go b/apps/backend/internal/agent/settings/controller/agent_discovery.go index d8f5d2461c..62352b2bc9 100644 --- a/apps/backend/internal/agent/settings/controller/agent_discovery.go +++ b/apps/backend/internal/agent/settings/controller/agent_discovery.go @@ -5,10 +5,9 @@ import ( "database/sql" "errors" "fmt" - "time" - "os/exec" "runtime" + "time" "github.com/kandev/kandev/internal/agent/agents" "github.com/kandev/kandev/internal/agent/discovery" diff --git a/apps/backend/internal/agent/settings/controller/agent_install_test.go b/apps/backend/internal/agent/settings/controller/agent_install_test.go index f4248d7492..a3c50b73d4 100644 --- a/apps/backend/internal/agent/settings/controller/agent_install_test.go +++ b/apps/backend/internal/agent/settings/controller/agent_install_test.go @@ -242,3 +242,81 @@ func TestRingBuffer_DropsOldestOnLineBoundary(t *testing.T) { t.Errorf("ring buffer missing newest write, got %q", got) } } + +// TestIsInstallNpmEnvVar mirrors TestIsNpmEnvVar in agentctl/server/process, +// guarding against the two filters drifting apart and verifying that +// legitimate npm config (registry, proxy, auth, custom .npmrc) survives so +// install scripts behind corporate registries still work. +func TestIsInstallNpmEnvVar(t *testing.T) { + tests := []struct { + key string + expected bool + }{ + // Poison: pnpm-injected workspace dir. + {"npm_config_prefix", true}, + {"npm_config_dir", true}, + {"npm_config_user_agent", true}, + {"npm_execpath", true}, + {"npm_node_execpath", true}, + // Per-script context, never user config. + {"npm_package_name", true}, + {"npm_package_version", true}, + {"npm_lifecycle_event", true}, + + // Legitimate npm config that must survive (would break installs + // behind corporate registries / proxies / private auth otherwise). + {"npm_config_registry", false}, + {"npm_config_proxy", false}, + {"npm_config_https-proxy", false}, + {"npm_config_userconfig", false}, + {"npm_config_globalconfig", false}, + {"npm_config_//registry.npmjs.org/:_authToken", false}, + {"npm_config_strict-ssl", false}, + {"npm_config_cafile", false}, + + // Unrelated env. + {"PATH", false}, + {"HOME", false}, + {"NPM_TOKEN", false}, + {"NPMRC", false}, + {"npm_not_a_config", false}, + } + for _, tt := range tests { + t.Run(tt.key, func(t *testing.T) { + if got := isInstallNpmEnvVar(tt.key); got != tt.expected { + t.Errorf("isInstallNpmEnvVar(%q) = %v, want %v", tt.key, got, tt.expected) + } + }) + } +} + +func TestFilteredInstallEnv(t *testing.T) { + // Poison vars pnpm injects. + t.Setenv("npm_config_prefix", "/workspace/apps/cli") + t.Setenv("npm_config_user_agent", "pnpm/9.15.9") + t.Setenv("npm_package_name", "kandev") + t.Setenv("npm_lifecycle_event", "dev") + // Legitimate config a user might have in their shell. + t.Setenv("npm_config_registry", "https://registry.corp.example.com/") + t.Setenv("npm_config_https-proxy", "http://proxy.corp.example.com:8080") + // Unrelated env. + t.Setenv("KANDEV_TEST_KEEP", "yes") + + got := make(map[string]string) + for _, entry := range filteredInstallEnv() { + if eq := strings.IndexByte(entry, '='); eq > 0 { + got[entry[:eq]] = entry[eq+1:] + } + } + + for _, k := range []string{"npm_config_prefix", "npm_config_user_agent", "npm_package_name", "npm_lifecycle_event"} { + if _, ok := got[k]; ok { + t.Errorf("%s should have been filtered", k) + } + } + for _, k := range []string{"npm_config_registry", "npm_config_https-proxy", "KANDEV_TEST_KEEP"} { + if _, ok := got[k]; !ok { + t.Errorf("%s should have been kept", k) + } + } +} diff --git a/apps/backend/internal/agent/settings/controller/install_job.go b/apps/backend/internal/agent/settings/controller/install_job.go index f171b82278..418e8d8534 100644 --- a/apps/backend/internal/agent/settings/controller/install_job.go +++ b/apps/backend/internal/agent/settings/controller/install_job.go @@ -7,12 +7,15 @@ import ( "errors" "fmt" "io" + "os" "os/exec" + "strings" "sync" "time" "github.com/google/uuid" "github.com/kandev/kandev/internal/agent/settings/dto" + "github.com/kandev/kandev/internal/common/shellexec" ws "github.com/kandev/kandev/pkg/websocket" "go.uber.org/zap" ) @@ -57,7 +60,14 @@ func defaultStreamingInstallRunner( script string, onChunk func(chunk string), ) error { - cmd := exec.CommandContext(ctx, "/bin/sh", "-c", script) + cmd := shellexec.CommandContext(ctx, shellexec.PosixSh, script) + // Strip npm/pnpm-injected env vars before invoking the install script. + // When kandev is launched via `pnpm dev`, pnpm sets npm_config_prefix + // (and friends) to the workspace package directory; if we let those + // flow through to `npm install -g ...` the package lands in the + // workspace's bin/ instead of the user's real npm prefix and the + // freshly installed CLI is invisible to the discovery LookPath check. + cmd.Env = filteredInstallEnv() stdout, err := cmd.StdoutPipe() if err != nil { return err @@ -385,3 +395,44 @@ func (r *ringBuffer) String() string { defer r.mu.Unlock() return string(r.data) } + +// filteredInstallEnv returns os.Environ() with npm/pnpm-injected configuration +// variables removed. pnpm exports npm_config_prefix (and friends) to scripts +// it spawns; if those leak into a child `npm install -g ...` the package gets +// installed into the workspace package dir instead of the user's global npm +// prefix. Diverges from isNpmEnvVar in agentctl/server/{config,process}: those +// blanket-strip every npm_config_* to silence npx warnings, but install scripts +// genuinely need the user's legitimate npm config (registry, proxy, auth +// tokens, custom .npmrc) to reach private registries or work behind a corporate +// proxy. We only strip the specific keys pnpm injects with workspace-local +// values, plus the per-script context vars that npm install never reads. +func filteredInstallEnv() []string { + parent := os.Environ() + out := make([]string, 0, len(parent)) + for _, entry := range parent { + eq := strings.IndexByte(entry, '=') + if eq <= 0 { + out = append(out, entry) + continue + } + if isInstallNpmEnvVar(entry[:eq]) { + continue + } + out = append(out, entry) + } + return out +} + +func isInstallNpmEnvVar(key string) bool { + switch key { + case "npm_config_prefix", // pnpm sets to workspace dir; redirects -g installs + "npm_config_dir", // same: pnpm-injected workspace dir + "npm_config_user_agent", // pnpm/X.Y.Z; misleading and harmless to drop + "npm_execpath", // path to pnpm binary, not npm + "npm_node_execpath": // node binary path from pnpm context + return true + } + // Per-script context, never user config. + return strings.HasPrefix(key, "npm_package_") || + strings.HasPrefix(key, "npm_lifecycle_") +} diff --git a/apps/backend/internal/agentctl/client/launcher/launcher.go b/apps/backend/internal/agentctl/client/launcher/launcher.go index df7f784600..1d3f52cf21 100644 --- a/apps/backend/internal/agentctl/client/launcher/launcher.go +++ b/apps/backend/internal/agentctl/client/launcher/launcher.go @@ -42,6 +42,16 @@ type Launcher struct { // breaking the pipe. agentctl detects the break and self-terminates. parentPipe *os.File + // jobHandle holds a platform-specific kernel object used to enforce + // "kill the child if the parent dies" without relying on the agentctl + // process noticing on its own. On Windows it stores a Job Object handle + // (see lifecycle_windows.go) and is accessed atomically. Unused on Unix + // — there, the inherited pipe in launcher_pipe_unix.go covers the same + // role; we keep the field on the shared struct so platform-specific + // install/release methods (lifecycle_{unix,windows}.go) compile against + // the same Launcher type. + jobHandle uintptr //nolint:unused // referenced from lifecycle_windows.go + // authToken is retrieved via handshake after agentctl starts. // agentctl generates its own token; the launcher retrieves it using the bootstrap nonce. authToken string @@ -173,6 +183,7 @@ func (l *Launcher) Start(ctx context.Context) error { l.logger.Warn("failed to kill agentctl process after failed health check", zap.Error(killErr)) } l.closeParentPipe() + l.releaseChildLifecycle() return fmt.Errorf("agentctl failed to become healthy: %w", err) } @@ -226,6 +237,15 @@ func (l *Launcher) buildAndStartProcess(nonce string) error { closeChildPipeEnd(l.cmd) l.parentPipe = pipeWrite + // Bind the child to a kill-on-parent-exit kernel primitive (Job Object on + // Windows, no-op on Unix where the inherited liveness pipe already covers + // this). Failure is non-fatal: agentctl still works, but a parent crash + // may leak an agentctl.exe that holds the control port (issue #892). + if err := l.installChildLifecycle(l.cmd); err != nil { + l.logger.Warn("failed to install child lifecycle protection; agentctl may outlive a parent crash", + zap.Error(err)) + } + l.logger.Info("agentctl process started", zap.Int("pid", l.cmd.Process.Pid)) go l.pipeOutput("stdout", bufio.NewScanner(stdout)) @@ -245,6 +265,7 @@ func (l *Launcher) performHandshake(ctx context.Context, nonce string) (string, l.logger.Warn("failed to kill agentctl after handshake failure", zap.Error(killErr)) } l.closeParentPipe() + l.releaseChildLifecycle() return "", fmt.Errorf("agentctl handshake failed: %w", err) } return token, nil @@ -287,6 +308,7 @@ func (l *Launcher) Stop(ctx context.Context) error { select { case <-l.exited: l.logger.Info("agentctl stopped gracefully") + l.releaseChildLifecycle() return nil case <-ctx.Done(): l.logger.Warn("graceful shutdown timed out, force killing") @@ -294,8 +316,10 @@ func (l *Launcher) Stop(ctx context.Context) error { // Wait a bit for the kill to take effect select { case <-l.exited: + l.releaseChildLifecycle() return nil case <-time.After(1 * time.Second): + l.releaseChildLifecycle() return fmt.Errorf("agentctl did not exit after force kill") } } diff --git a/apps/backend/internal/agentctl/client/launcher/lifecycle_unix.go b/apps/backend/internal/agentctl/client/launcher/lifecycle_unix.go new file mode 100644 index 0000000000..c8e7261408 --- /dev/null +++ b/apps/backend/internal/agentctl/client/launcher/lifecycle_unix.go @@ -0,0 +1,15 @@ +//go:build !windows + +package launcher + +import "os/exec" + +// installChildLifecycle is a no-op on Unix. The Unix parent-liveness path uses +// the inherited pipe in launcher_pipe_unix.go (the kernel closes the write-end +// when the parent dies, signaling agentctl to shut down). +func (l *Launcher) installChildLifecycle(_ *exec.Cmd) error { + return nil +} + +// releaseChildLifecycle is a no-op on Unix. +func (l *Launcher) releaseChildLifecycle() {} diff --git a/apps/backend/internal/agentctl/client/launcher/lifecycle_windows.go b/apps/backend/internal/agentctl/client/launcher/lifecycle_windows.go new file mode 100644 index 0000000000..fea44dc5b8 --- /dev/null +++ b/apps/backend/internal/agentctl/client/launcher/lifecycle_windows.go @@ -0,0 +1,112 @@ +//go:build windows + +package launcher + +import ( + "fmt" + "os/exec" + "sync/atomic" + "unsafe" + + "go.uber.org/zap" + "golang.org/x/sys/windows" +) + +// installChildLifecycle binds the agentctl child to a Windows Job Object with +// JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE so the kernel terminates the child when +// the launcher (backend) process exits — including unexpected crashes. Without +// this, a crashed parent leaves agentctl.exe alive and holding its control +// port (41001 by default), forcing the user to kill the process by hand or +// reboot before kandev can start again (issue #892). +// +// The handle is intentionally kept open for the lifetime of the parent +// process: the OS closes it as part of normal process teardown on every exit +// path (clean shutdown, panic, signal, hard kill), and that close is exactly +// what releases the kill-on-close interlock. releaseChildLifecycle is provided +// for the rare case where the launcher is restarted within the same process. +func (l *Launcher) installChildLifecycle(cmd *exec.Cmd) error { + if cmd == nil || cmd.Process == nil { + return fmt.Errorf("installChildLifecycle: process not started") + } + + job, err := createKillOnCloseJob() + if err != nil { + return err + } + + procHandle, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, + false, + uint32(cmd.Process.Pid), + ) + if err != nil { + _ = windows.CloseHandle(job) + return fmt.Errorf("OpenProcess(pid=%d): %w", cmd.Process.Pid, err) + } + defer windows.CloseHandle(procHandle) + + if err := windows.AssignProcessToJobObject(job, procHandle); err != nil { + _ = windows.CloseHandle(job) + return fmt.Errorf("AssignProcessToJobObject: %w", err) + } + + // Defensive: if a previous lifecycle was somehow installed without a + // matching release (unexpected restart, future code change), close the + // stale handle instead of silently leaking it. Doing the swap *after* the + // new job is assigned avoids killing the new agentctl before we've stored + // its handle — the new job has KILL_ON_JOB_CLOSE so we must hold a + // reference at all times. + previous := atomic.SwapUintptr(&l.jobHandle, uintptr(job)) + if previous != 0 { + if err := windows.CloseHandle(windows.Handle(previous)); err != nil { + l.logger.Warn("failed to close stale job object handle", zap.Error(err)) + } else { + l.logger.Warn("closed stale job object handle from previous install") + } + } + l.logger.Info("agentctl bound to kill-on-close job object", + zap.Int("pid", cmd.Process.Pid)) + return nil +} + +// releaseChildLifecycle closes the job-object handle if one was installed. +// Safe to call at any point after the child was started: on normal shutdown +// paths the child has already exited and this is a plain handle close; in +// the force-kill timeout branch of Stop the child may still be alive, and the +// handle close acts as a backstop — KILL_ON_JOB_CLOSE terminates every +// remaining process in the job. Also safe to call when no handle was set or +// after a previous release. +func (l *Launcher) releaseChildLifecycle() { + handle := atomic.SwapUintptr(&l.jobHandle, 0) + if handle == 0 { + return + } + if err := windows.CloseHandle(windows.Handle(handle)); err != nil { + l.logger.Warn("failed to close job object handle", zap.Error(err)) + } +} + +// createKillOnCloseJob creates an unnamed Job Object whose handle, when the +// last reference is closed by the OS, terminates every process assigned to it. +func createKillOnCloseJob() (windows.Handle, error) { + job, err := windows.CreateJobObject(nil, nil) + if err != nil { + return 0, fmt.Errorf("CreateJobObject: %w", err) + } + + info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ + BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ + LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }, + } + if _, err := windows.SetInformationJobObject( + job, + windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&info)), + uint32(unsafe.Sizeof(info)), + ); err != nil { + _ = windows.CloseHandle(job) + return 0, fmt.Errorf("SetInformationJobObject: %w", err) + } + return job, nil +} diff --git a/apps/backend/internal/agentctl/client/launcher/lifecycle_windows_test.go b/apps/backend/internal/agentctl/client/launcher/lifecycle_windows_test.go new file mode 100644 index 0000000000..eed36d74f0 --- /dev/null +++ b/apps/backend/internal/agentctl/client/launcher/lifecycle_windows_test.go @@ -0,0 +1,100 @@ +//go:build windows + +package launcher + +import ( + "os/exec" + "sync/atomic" + "testing" + "time" + + "github.com/kandev/kandev/internal/common/logger" + "golang.org/x/sys/windows" +) + +func newTestLauncher(t *testing.T) *Launcher { + t.Helper() + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console", OutputPath: "stderr"}) + if err != nil { + t.Fatalf("logger: %v", err) + } + return &Launcher{logger: log} +} + +func TestCreateKillOnCloseJob(t *testing.T) { + job, err := createKillOnCloseJob() + if err != nil { + t.Fatalf("createKillOnCloseJob: %v", err) + } + if job == 0 { + t.Fatal("got null job handle") + } + if err := windows.CloseHandle(job); err != nil { + t.Errorf("CloseHandle: %v", err) + } +} + +func TestReleaseChildLifecycle_NoHandleIsNoop(t *testing.T) { + l := newTestLauncher(t) + // Must not panic when no handle has been installed. + l.releaseChildLifecycle() + if atomic.LoadUintptr(&l.jobHandle) != 0 { + t.Error("jobHandle should remain zero") + } +} + +func TestReleaseChildLifecycle_Idempotent(t *testing.T) { + job, err := createKillOnCloseJob() + if err != nil { + t.Fatalf("createKillOnCloseJob: %v", err) + } + + l := newTestLauncher(t) + atomic.StoreUintptr(&l.jobHandle, uintptr(job)) + + l.releaseChildLifecycle() + // Second call must not double-close the handle. + l.releaseChildLifecycle() + if atomic.LoadUintptr(&l.jobHandle) != 0 { + t.Error("jobHandle should be cleared after release") + } +} + +// TestInstallChildLifecycle_KillOnHandleClose verifies the core invariant: a +// process bound to the job is terminated by the OS the moment the last handle +// to the job is closed. This is what protects against orphaned agentctl.exe +// when the backend crashes (issue #892) — Windows closes our handles for us +// as part of process teardown. +func TestInstallChildLifecycle_KillOnHandleClose(t *testing.T) { + cmd := exec.Command("cmd.exe", "/c", "ping", "-n", "30", "127.0.0.1") + if err := cmd.Start(); err != nil { + t.Fatalf("start child: %v", err) + } + defer func() { + if cmd.ProcessState == nil || !cmd.ProcessState.Exited() { + _ = cmd.Process.Kill() + } + }() + + l := newTestLauncher(t) + if err := l.installChildLifecycle(cmd); err != nil { + t.Fatalf("installChildLifecycle: %v", err) + } + if atomic.LoadUintptr(&l.jobHandle) == 0 { + t.Fatal("jobHandle was not stored") + } + + exited := make(chan error, 1) + go func() { exited <- cmd.Wait() }() + + // Closing the only handle to the job must kill the assigned process. + l.releaseChildLifecycle() + + select { + case <-exited: + // Either err or nil exit, both fine — what matters is that the + // process did not survive the handle close. + case <-time.After(5 * time.Second): + t.Fatal("child still alive 5s after job-handle close") + } +} diff --git a/apps/backend/internal/agentctl/server/process/manager_test.go b/apps/backend/internal/agentctl/server/process/manager_test.go index 3ce1e83c71..da6dd80be0 100644 --- a/apps/backend/internal/agentctl/server/process/manager_test.go +++ b/apps/backend/internal/agentctl/server/process/manager_test.go @@ -67,7 +67,7 @@ func TestStartProcessPipes_FailsAfterProcessStarted(t *testing.T) { // Regression test: documents the bug where cmd.Start() was called inside // buildFinalCommand() before pipes were created. // exec.Cmd pipes cannot be created after Start() is called. - cmd := exec.Command("sleep", "10") + cmd := fixtureCmd("sleep 10") require.NoError(t, cmd.Start()) t.Cleanup(func() { _ = cmd.Process.Kill() }) @@ -101,9 +101,9 @@ func TestManager_Start_PipesCreatedBeforeProcessStart(t *testing.T) { m := &Manager{ cfg: &config.InstanceConfig{ - AgentArgs: []string{"cat"}, + AgentArgs: fixtureArgs(), WorkDir: workDir, - AgentEnv: []string{"PATH=/usr/bin:/bin"}, + AgentEnv: fixtureEnvSlice("cat"), }, logger: log, adapter: stub, diff --git a/apps/backend/internal/agentctl/server/process/pty_windows.go b/apps/backend/internal/agentctl/server/process/pty_windows.go index 115f81c2d2..f11234464b 100644 --- a/apps/backend/internal/agentctl/server/process/pty_windows.go +++ b/apps/backend/internal/agentctl/server/process/pty_windows.go @@ -6,18 +6,36 @@ 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 + 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 { return p.cpty.Close() } + +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)) @@ -28,10 +46,7 @@ func (p *windowsPTY) Resize(cols, rows uint16) error { // 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 := buildCmdLine(cmd.Args) - if len(cmd.Args) == 0 { - cmdLine = escapeArg(cmd.Path) - } + cmdLine := resolveConPtyCmdLine(cmd) opts := []conpty.ConPtyOption{ conpty.ConPtyDimensions(cols, rows), @@ -61,3 +76,49 @@ func startPTYWithSize(cmd *exec.Cmd, cols, rows int) (PtyHandle, error) { 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