fix(windows): make conpty Close idempotent and kill orphan agentctl (#900)
This commit is contained in:
Carlos Florêncio
@@ -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/...
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -0,0 +1 @@
|
||||
24
|
||||
@@ -0,0 +1 @@
|
||||
24
|
||||
+66
-23
@@ -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"
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 <program> [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)
|
||||
}
|
||||
}
|
||||
@@ -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 <program> [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)
|
||||
}
|
||||
+1
-1
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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, " ")
|
||||
}
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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"
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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_")
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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() {}
|
||||
@@ -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
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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,
|
||||
|
||||
@@ -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 <script> <args...>" so the batch interpreter handles them.
|
||||
//
|
||||
// Without this, npm-installed CLIs (which install as .cmd shims on Windows)
|
||||
// fail to launch under ConPTY with "Failed to create console process: The
|
||||
// system cannot find the file specified" — even though `where.exe` and
|
||||
// `exec.LookPath` both find them. This is the same family of issue handled in
|
||||
// apps/cli/src/web.ts for Node's spawn().
|
||||
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
|
||||
}
|
||||
|
||||
@@ -0,0 +1,82 @@
|
||||
//go:build windows
|
||||
|
||||
package process
|
||||
|
||||
import (
|
||||
"os/exec"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TestWindowsPTY_DoubleCloseSafe guards against the double-free that crashed
|
||||
// the backend on Windows when a terminal tab was closed (issue #894). Both
|
||||
// InteractiveRunner.Stop and InteractiveRunner.wait close the PTY handle, so
|
||||
// the wrapper must collapse the second call into a no-op — otherwise the
|
||||
// underlying conpty library double-frees its Windows handles and triggers
|
||||
// STATUS_HEAP_CORRUPTION.
|
||||
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 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 guards against the regression that
|
||||
// broke passthrough/login sessions for every npm-installed CLI on Windows:
|
||||
// CreateProcessW (used by ConPTY) doesn't apply PATHEXT and can't execute
|
||||
// .cmd / .bat scripts directly. exec.Command resolves the binary into
|
||||
// cmd.Path; we must (a) use that resolved path instead of the bare Args[0]
|
||||
// and (b) wrap batch scripts with cmd.exe /c so the interpreter runs them.
|
||||
func TestResolveConPtyCmdLine_CmdShimIsWrapped(t *testing.T) {
|
||||
cmd := &exec.Cmd{
|
||||
Path: `C:\Users\test\AppData\Roaming\npm\opencode.cmd`,
|
||||
Args: []string{"opencode", "--model", "big-pickle"},
|
||||
}
|
||||
got := resolveConPtyCmdLine(cmd)
|
||||
want := `cmd.exe /c C:\Users\test\AppData\Roaming\npm\opencode.cmd --model big-pickle`
|
||||
if got != want {
|
||||
t.Errorf("resolveConPtyCmdLine(.cmd shim):\n got: %s\n want: %s", got, want)
|
||||
}
|
||||
}
|
||||
|
||||
// TestResolveConPtyCmdLine_ExePassesThrough confirms .exe binaries (and any
|
||||
// non-batch path) skip the cmd.exe wrapper — wrapping would lose access to
|
||||
// signals and add a needless intermediate process for the common case.
|
||||
func TestResolveConPtyCmdLine_ExePassesThrough(t *testing.T) {
|
||||
cmd := &exec.Cmd{
|
||||
Path: `C:\Windows\System32\cmd.exe`,
|
||||
Args: []string{"cmd.exe", "/c", "exit", "0"},
|
||||
}
|
||||
got := resolveConPtyCmdLine(cmd)
|
||||
if strings.HasPrefix(got, "cmd.exe /c cmd.exe") {
|
||||
t.Errorf("resolveConPtyCmdLine wrapped a non-batch executable: %s", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestResolveConPtyCmdLine_PathSubstitutedForArgs0 confirms cmd.Path is what
|
||||
// CreateProcessW receives, not the unresolved cmd.Args[0]. Without this,
|
||||
// "opencode" reaches CreateProcessW and fails because the loader can't
|
||||
// resolve a bare name without PATHEXT.
|
||||
func TestResolveConPtyCmdLine_PathSubstitutedForArgs0(t *testing.T) {
|
||||
cmd := &exec.Cmd{
|
||||
Path: `C:\Tools\foo.exe`,
|
||||
Args: []string{"foo", "bar"},
|
||||
}
|
||||
got := resolveConPtyCmdLine(cmd)
|
||||
if !strings.HasPrefix(got, `C:\Tools\foo.exe `) {
|
||||
t.Errorf("expected cmd.Path as first token; got: %s", got)
|
||||
}
|
||||
if strings.HasPrefix(got, "foo ") {
|
||||
t.Errorf("first token is unresolved Args[0]; got: %s", got)
|
||||
}
|
||||
}
|
||||
@@ -12,9 +12,11 @@ func TestInteractiveRunner_Start(t *testing.T) {
|
||||
log := newTestLogger(t)
|
||||
runner := NewInteractiveRunner(nil, log, 2*1024*1024)
|
||||
|
||||
cmd, env := fixtureExec("echo hello")
|
||||
req := InteractiveStartRequest{
|
||||
SessionID: "test-session",
|
||||
Command: []string{"echo", "hello"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
ImmediateStart: true,
|
||||
DefaultCols: 80,
|
||||
DefaultRows: 24,
|
||||
@@ -35,24 +37,25 @@ func TestInteractiveRunner_Start(t *testing.T) {
|
||||
t.Errorf("Start() Status = %v, want %v", info.Status, types.ProcessStatusRunning)
|
||||
}
|
||||
|
||||
// Wait for process to exit
|
||||
time.Sleep(500 * time.Millisecond)
|
||||
|
||||
// Process should have completed
|
||||
procInfo, ok := runner.Get(info.ID, false)
|
||||
if !ok {
|
||||
// Process may have been removed after exit, which is expected
|
||||
return
|
||||
}
|
||||
if procInfo.Status == types.ProcessStatusRunning {
|
||||
t.Error("Process should have exited")
|
||||
// Poll for exit. A fixed sleep was flaky on slow CI runners — the test
|
||||
// binary spawned via PTY can take longer than half a second to print
|
||||
// `echo hello` and exit on Windows GitHub Actions hosts.
|
||||
deadline := time.Now().Add(5 * time.Second)
|
||||
for time.Now().Before(deadline) {
|
||||
procInfo, ok := runner.Get(info.ID, false)
|
||||
if !ok || procInfo.Status != types.ProcessStatusRunning {
|
||||
return // exited and (optionally) cleaned up
|
||||
}
|
||||
time.Sleep(50 * time.Millisecond)
|
||||
}
|
||||
t.Error("Process should have exited within 5s")
|
||||
}
|
||||
|
||||
func TestInteractiveRunner_Start_ValidationErrors(t *testing.T) {
|
||||
log := newTestLogger(t)
|
||||
runner := NewInteractiveRunner(nil, log, 2*1024*1024)
|
||||
|
||||
validCmd, validEnv := fixtureExec("echo")
|
||||
tests := []struct {
|
||||
name string
|
||||
req InteractiveStartRequest
|
||||
@@ -61,7 +64,8 @@ func TestInteractiveRunner_Start_ValidationErrors(t *testing.T) {
|
||||
{
|
||||
name: "missing session_id",
|
||||
req: InteractiveStartRequest{
|
||||
Command: []string{"echo"},
|
||||
Command: validCmd,
|
||||
Env: validEnv,
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
@@ -76,7 +80,8 @@ func TestInteractiveRunner_Start_ValidationErrors(t *testing.T) {
|
||||
name: "valid request",
|
||||
req: InteractiveStartRequest{
|
||||
SessionID: "test",
|
||||
Command: []string{"echo"},
|
||||
Command: validCmd,
|
||||
Env: validEnv,
|
||||
ImmediateStart: true,
|
||||
},
|
||||
wantErr: false,
|
||||
@@ -97,11 +102,13 @@ func TestInteractiveRunner_DeferredStart(t *testing.T) {
|
||||
log := newTestLogger(t)
|
||||
runner := NewInteractiveRunner(nil, log, 2*1024*1024)
|
||||
|
||||
// Start without ImmediateStart - process should be deferred
|
||||
// Use 'cat' which blocks waiting for input, giving us time to check status
|
||||
// Start without ImmediateStart - process should be deferred.
|
||||
// `cat` blocks waiting for input, giving us time to check status.
|
||||
cmd, env := fixtureExec("cat")
|
||||
req := InteractiveStartRequest{
|
||||
SessionID: "deferred-session",
|
||||
Command: []string{"cat"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
}
|
||||
|
||||
info, err := runner.Start(context.Background(), req)
|
||||
@@ -144,9 +151,11 @@ func TestInteractiveRunner_WriteStdin(t *testing.T) {
|
||||
runner := NewInteractiveRunner(nil, log, 2*1024*1024)
|
||||
|
||||
// Start cat process that echoes input
|
||||
cmd, env := fixtureExec("cat")
|
||||
req := InteractiveStartRequest{
|
||||
SessionID: "stdin-test",
|
||||
Command: []string{"cat"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
ImmediateStart: true,
|
||||
DefaultCols: 80,
|
||||
DefaultRows: 24,
|
||||
@@ -177,9 +186,11 @@ func TestInteractiveRunner_Stop(t *testing.T) {
|
||||
runner := NewInteractiveRunner(nil, log, 2*1024*1024)
|
||||
|
||||
// Start a long-running process
|
||||
cmd, env := fixtureExec("sleep 60")
|
||||
req := InteractiveStartRequest{
|
||||
SessionID: "stop-test",
|
||||
Command: []string{"sleep", "60"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
ImmediateStart: true,
|
||||
DefaultCols: 80,
|
||||
DefaultRows: 24,
|
||||
|
||||
@@ -12,9 +12,11 @@ func TestInteractiveRunner_IsUserShellIsolation(t *testing.T) {
|
||||
runner := NewInteractiveRunner(nil, log, 2*1024*1024)
|
||||
|
||||
// Start a regular (non-user-shell) process
|
||||
cmd, env := fixtureExec("cat")
|
||||
agentReq := InteractiveStartRequest{
|
||||
SessionID: "isolation-test",
|
||||
Command: []string{"cat"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
ImmediateStart: true,
|
||||
DefaultCols: 80,
|
||||
DefaultRows: 24,
|
||||
@@ -27,7 +29,8 @@ func TestInteractiveRunner_IsUserShellIsolation(t *testing.T) {
|
||||
// Start a user shell process for the same session
|
||||
shellReq := InteractiveStartRequest{
|
||||
SessionID: "isolation-test",
|
||||
Command: []string{"cat"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
IsUserShell: true,
|
||||
ImmediateStart: true,
|
||||
DefaultCols: 80,
|
||||
|
||||
@@ -2,6 +2,7 @@ package process
|
||||
|
||||
import (
|
||||
"context"
|
||||
"runtime"
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
@@ -14,9 +15,11 @@ func TestInteractiveRunner_GetBuffer(t *testing.T) {
|
||||
log := newTestLogger(t)
|
||||
runner := NewInteractiveRunner(nil, log, 2*1024*1024)
|
||||
|
||||
cmd, env := fixtureExec("echo buffered")
|
||||
req := InteractiveStartRequest{
|
||||
SessionID: "buffer-test",
|
||||
Command: []string{"echo", "buffered output"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
ImmediateStart: true,
|
||||
DefaultCols: 80,
|
||||
DefaultRows: 24,
|
||||
@@ -65,9 +68,11 @@ func TestInteractiveRunner_Callbacks(t *testing.T) {
|
||||
mu.Unlock()
|
||||
})
|
||||
|
||||
cmd, env := fixtureExec("echo callback")
|
||||
req := InteractiveStartRequest{
|
||||
SessionID: "callback-test",
|
||||
Command: []string{"echo", "callback test"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
ImmediateStart: true,
|
||||
DefaultCols: 80,
|
||||
DefaultRows: 24,
|
||||
@@ -90,6 +95,15 @@ func TestInteractiveRunner_Callbacks(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestInteractiveRunner_TurnCompleteCallback(t *testing.T) {
|
||||
// This test hardcodes `bash -c "echo '$ '"` to produce a literal "$ "
|
||||
// prompt and exercise the PromptPattern turn-complete detector. Migrating
|
||||
// it to fixtureExec would lose the bash-only quoting/echo semantics the
|
||||
// detector depends on. Windows CI passes only because windows-latest ships
|
||||
// Git Bash in PATH; a bare Windows host without Git Bash would fail at
|
||||
// runner.Start. Skip rather than pretend it's portable.
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("requires bash on PATH (Git Bash on Windows); turn-complete pattern is bash-specific")
|
||||
}
|
||||
log := newTestLogger(t)
|
||||
runner := NewInteractiveRunner(nil, log, 2*1024*1024)
|
||||
|
||||
@@ -133,9 +147,11 @@ func TestInteractiveRunner_DirectOutput(t *testing.T) {
|
||||
log := newTestLogger(t)
|
||||
runner := NewInteractiveRunner(nil, log, 2*1024*1024)
|
||||
|
||||
cmd, env := fixtureExec("cat")
|
||||
req := InteractiveStartRequest{
|
||||
SessionID: "direct-output-test",
|
||||
Command: []string{"cat"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
ImmediateStart: true,
|
||||
DefaultCols: 80,
|
||||
DefaultRows: 24,
|
||||
@@ -201,9 +217,11 @@ func TestInteractiveRunner_GetPtyWriter(t *testing.T) {
|
||||
log := newTestLogger(t)
|
||||
runner := NewInteractiveRunner(nil, log, 2*1024*1024)
|
||||
|
||||
cmd, env := fixtureExec("cat")
|
||||
req := InteractiveStartRequest{
|
||||
SessionID: "pty-writer-test",
|
||||
Command: []string{"cat"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
ImmediateStart: true,
|
||||
DefaultCols: 80,
|
||||
DefaultRows: 24,
|
||||
@@ -307,9 +325,11 @@ func TestInteractiveRunner_IsProcessRunning(t *testing.T) {
|
||||
}
|
||||
|
||||
// Start a process
|
||||
cmd, env := fixtureExec("sleep 10")
|
||||
req := InteractiveStartRequest{
|
||||
SessionID: "running-test",
|
||||
Command: []string{"sleep", "10"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
ImmediateStart: true,
|
||||
DefaultCols: 80,
|
||||
DefaultRows: 24,
|
||||
@@ -352,9 +372,11 @@ func TestInteractiveRunner_IsProcessReadyOrPending(t *testing.T) {
|
||||
}
|
||||
|
||||
// Start a deferred process (not started yet)
|
||||
cmd, env := fixtureExec("cat")
|
||||
req := InteractiveStartRequest{
|
||||
SessionID: "pending-test",
|
||||
Command: []string{"cat"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
// ImmediateStart: false (deferred)
|
||||
}
|
||||
|
||||
|
||||
@@ -60,9 +60,11 @@ func TestInteractiveRunner_ResizeByProcessID(t *testing.T) {
|
||||
runner := NewInteractiveRunner(nil, log, 2*1024*1024)
|
||||
|
||||
// Start a deferred process
|
||||
cmd, env := fixtureExec("cat")
|
||||
req := InteractiveStartRequest{
|
||||
SessionID: "resize-pid-test",
|
||||
Command: []string{"cat"},
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
}
|
||||
info, err := runner.Start(context.Background(), req)
|
||||
if err != nil {
|
||||
|
||||
@@ -48,10 +48,12 @@ func TestProcessRunnerCapturesOutput(t *testing.T) {
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
|
||||
defer cancel()
|
||||
|
||||
cmd, env := fixtureShellExec("echo-then-sleep hello 2")
|
||||
info, err := runner.Start(ctx, StartProcessRequest{
|
||||
SessionID: "session-1",
|
||||
Kind: "dev",
|
||||
Command: "printf 'hello'; sleep 2",
|
||||
Command: cmd,
|
||||
Env: env,
|
||||
WorkingDir: "",
|
||||
})
|
||||
if err != nil {
|
||||
|
||||
@@ -0,0 +1,137 @@
|
||||
package process
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"io"
|
||||
"os"
|
||||
"os/exec"
|
||||
"runtime"
|
||||
"strconv"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// kandevTestFixtureEnv activates fixture-binary mode in this test binary.
|
||||
// Tests use fixtureExec / fixtureShellExec (below) to spawn known commands
|
||||
// (sleep / echo / cat) without depending on Unix-only shell tooling — the
|
||||
// test binary re-invokes itself with this env var set, and TestMain dispatches
|
||||
// to runFixture before the normal test runner starts. This replaces the
|
||||
// previous fixtures that hardcoded `sleep`, `cat`, `echo`, `printf` which
|
||||
// don't exist as standalone executables on Windows.
|
||||
const kandevTestFixtureEnv = "KANDEV_TEST_FIXTURE"
|
||||
|
||||
// TestMain branches into fixture-binary mode when the activation env var
|
||||
// is set; otherwise it runs the test suite normally.
|
||||
func TestMain(m *testing.M) {
|
||||
if spec := os.Getenv(kandevTestFixtureEnv); spec != "" {
|
||||
runFixture(spec)
|
||||
return
|
||||
}
|
||||
os.Exit(m.Run())
|
||||
}
|
||||
|
||||
// runFixture executes a whitespace-separated command spec and exits.
|
||||
// Supported commands:
|
||||
//
|
||||
// sleep <secs> — sleep for <secs> seconds
|
||||
// echo <args...> — print args joined by spaces, plus newline
|
||||
// cat — copy stdin to stdout until EOF
|
||||
// echo-then-sleep <msg> <secs> — print msg, then sleep <secs>
|
||||
//
|
||||
// New commands can be added here as tests need them; the goal is to keep the
|
||||
// surface tiny so the helper stays inspectable.
|
||||
func runFixture(spec string) {
|
||||
parts := strings.Fields(spec)
|
||||
if len(parts) == 0 {
|
||||
fmt.Fprintln(os.Stderr, "fixture: empty spec")
|
||||
os.Exit(2)
|
||||
}
|
||||
switch parts[0] {
|
||||
case "sleep":
|
||||
if len(parts) != 2 {
|
||||
fmt.Fprintln(os.Stderr, "fixture: sleep takes 1 arg")
|
||||
os.Exit(2)
|
||||
}
|
||||
secs, err := strconv.Atoi(parts[1])
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "fixture: sleep: bad seconds %q\n", parts[1])
|
||||
os.Exit(2)
|
||||
}
|
||||
time.Sleep(time.Duration(secs) * time.Second)
|
||||
case "echo":
|
||||
fmt.Println(strings.Join(parts[1:], " "))
|
||||
case "cat":
|
||||
_, _ = io.Copy(os.Stdout, os.Stdin)
|
||||
case "echo-then-sleep":
|
||||
if len(parts) != 3 {
|
||||
fmt.Fprintln(os.Stderr, "fixture: echo-then-sleep takes 2 args")
|
||||
os.Exit(2)
|
||||
}
|
||||
fmt.Println(parts[1])
|
||||
// Force the kernel to flush this write to the parent's pipe before
|
||||
// we sleep. On Windows in particular, an anonymous pipe's first
|
||||
// small write can sit in the per-process buffer until the writer
|
||||
// makes a second write or exits — that would defeat the point of a
|
||||
// "print, then wait" fixture used by output-capture tests.
|
||||
_ = os.Stdout.Sync()
|
||||
secs, err := strconv.Atoi(parts[2])
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "fixture: echo-then-sleep: bad seconds %q\n", parts[2])
|
||||
os.Exit(2)
|
||||
}
|
||||
time.Sleep(time.Duration(secs) * time.Second)
|
||||
default:
|
||||
fmt.Fprintf(os.Stderr, "fixture: unknown command %q\n", parts[0])
|
||||
os.Exit(2)
|
||||
}
|
||||
os.Exit(0)
|
||||
}
|
||||
|
||||
// fixtureExec returns the (Command argv, Env) pair tests pass to runners that
|
||||
// take a Command []string and an Env map (e.g. InteractiveStartRequest). The
|
||||
// runner spawns this test binary with the env var set, TestMain dispatches
|
||||
// to runFixture, and the spec is executed in-process.
|
||||
func fixtureExec(spec string) (command []string, env map[string]string) {
|
||||
return []string{os.Args[0]}, map[string]string{kandevTestFixtureEnv: spec}
|
||||
}
|
||||
|
||||
// fixtureShellExec returns the (Command string, Env) pair tests pass to
|
||||
// runners that take a single shell-string Command (e.g. StartProcessRequest,
|
||||
// which runs via `sh -lc` on Unix and `cmd /c` on Windows). The spec travels
|
||||
// in the env var so we don't have to worry about per-shell argument quoting.
|
||||
//
|
||||
// Quoting is platform-specific because cmd.exe's `/c` parser strips at most
|
||||
// one outer pair of quotes — pre-quoting the path on Windows turns into
|
||||
// `cmd /c "\"C:\path\""` after Go's exec escaping, and cmd then fails to
|
||||
// resolve the executable. We leave Windows paths bare (Go's syscall.EscapeArg
|
||||
// adds quotes only if needed) and double-quote on Unix where sh splits on
|
||||
// whitespace.
|
||||
func fixtureShellExec(spec string) (command string, env map[string]string) {
|
||||
if runtime.GOOS == "windows" {
|
||||
return os.Args[0], map[string]string{kandevTestFixtureEnv: spec}
|
||||
}
|
||||
return `"` + os.Args[0] + `"`, map[string]string{kandevTestFixtureEnv: spec}
|
||||
}
|
||||
|
||||
// fixtureCmd builds an *exec.Cmd ready to run a fixture spec via this test
|
||||
// binary. Used by tests that bypass the runner abstractions and call
|
||||
// exec.Command directly.
|
||||
func fixtureCmd(spec string) *exec.Cmd {
|
||||
cmd := exec.Command(os.Args[0])
|
||||
cmd.Env = fixtureEnvSlice(spec)
|
||||
return cmd
|
||||
}
|
||||
|
||||
// fixtureEnvSlice returns os.Environ() plus the fixture activation variable,
|
||||
// formatted as a []string suitable for exec.Cmd.Env / InstanceConfig.AgentEnv.
|
||||
func fixtureEnvSlice(spec string) []string {
|
||||
return append(os.Environ(), kandevTestFixtureEnv+"="+spec)
|
||||
}
|
||||
|
||||
// fixtureArgs returns the argv to spawn this test binary in fixture mode.
|
||||
// Used together with fixtureEnvSlice when populating fields like
|
||||
// InstanceConfig.AgentArgs.
|
||||
func fixtureArgs() []string {
|
||||
return []string{os.Args[0]}
|
||||
}
|
||||
@@ -81,6 +81,9 @@ func TestFindVscodeIPCSocket_NoSockets(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestFindVscodeIPCSocket_ReturnsMostRecent(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("vscode IPC uses Unix sockets on Linux/macOS and named pipes on Windows; no shared production code path here")
|
||||
}
|
||||
// Use /tmp directly to avoid macOS Unix socket path length limits.
|
||||
tmpDir, err := os.MkdirTemp("/tmp", "vscode-ipc-test-")
|
||||
require.NoError(t, err)
|
||||
@@ -118,6 +121,9 @@ func TestFindVscodeIPCSocket_IgnoresNonSockFiles(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestWaitForVscodeIPCSocket_SocketAppearsAfterDelay(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("vscode IPC uses Unix sockets on Linux/macOS and named pipes on Windows; no shared production code path here")
|
||||
}
|
||||
tmpDir, err := os.MkdirTemp("/tmp", "vscode-ipc-test-")
|
||||
require.NoError(t, err)
|
||||
defer os.RemoveAll(tmpDir) //nolint:errcheck
|
||||
|
||||
@@ -3,6 +3,7 @@ package process
|
||||
import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"runtime"
|
||||
"testing"
|
||||
|
||||
"github.com/kandev/kandev/internal/agentctl/types"
|
||||
@@ -93,6 +94,9 @@ func TestResolveNonExistentPath(t *testing.T) {
|
||||
})
|
||||
|
||||
t.Run("permission error is propagated", func(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("chmod 0o000 does not block traversal on Windows the way POSIX does")
|
||||
}
|
||||
if os.Getuid() == 0 {
|
||||
t.Skip("skipping permission test: root bypasses filesystem permission checks")
|
||||
}
|
||||
|
||||
@@ -4,11 +4,15 @@ import (
|
||||
"context"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"runtime"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
func TestWorkspaceTracker_StopsWhenWorkDirDeleted(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("Windows refuses to unlink a directory while a process holds a handle inside it; the scenario this test exercises cannot occur on Windows")
|
||||
}
|
||||
isolateTestGitEnv(t)
|
||||
|
||||
repoDir, cleanup := setupTestRepo(t)
|
||||
|
||||
@@ -3,6 +3,7 @@ package process
|
||||
import (
|
||||
"context"
|
||||
"os"
|
||||
"runtime"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
@@ -318,6 +319,9 @@ func TestOverlapGuard_ResetsAfterNormalCycle(t *testing.T) {
|
||||
// TestOverlapGuard_WorkDirDeletedResetsFlag verifies that the overlap guard
|
||||
// is reset when the work directory is deleted, so the goroutine exits cleanly.
|
||||
func TestOverlapGuard_WorkDirDeletedResetsFlag(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("Windows refuses to unlink a directory while a process holds a handle inside it; the scenario this test exercises cannot occur on Windows")
|
||||
}
|
||||
isolateTestGitEnv(t)
|
||||
|
||||
repoDir, cleanup := setupTestRepo(t)
|
||||
|
||||
@@ -201,7 +201,11 @@ func TestMonitorLoop_FastToPausedStopsPolling(t *testing.T) {
|
||||
if err := os.WriteFile(filepath.Join(repoDir, "before_pause.txt"), []byte("x"), 0o644); err != nil {
|
||||
t.Fatalf("failed to write: %v", err)
|
||||
}
|
||||
if !waitForFileChangeNotification(sub, 1*time.Second) {
|
||||
// 3s rather than 1s to absorb GitHub Actions windows-latest slowness
|
||||
// under `-race`. The neighbouring test at line ~142 uses 2s for the same
|
||||
// kind of check; this one needs a touch more because it runs after a
|
||||
// PollMode change that has its own settle time.
|
||||
if !waitForFileChangeNotification(sub, 3*time.Second) {
|
||||
t.Fatal("setup: expected fast mode to emit notification before pausing")
|
||||
}
|
||||
|
||||
|
||||
@@ -6,6 +6,7 @@ import (
|
||||
"os"
|
||||
"os/exec"
|
||||
"path/filepath"
|
||||
"runtime"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
@@ -64,6 +65,12 @@ func setupTestRepo(t *testing.T) (string, func()) {
|
||||
runGit(t, localDir, "config", "user.email", "test@test.com")
|
||||
runGit(t, localDir, "config", "user.name", "Test User")
|
||||
runGit(t, localDir, "config", "core.hooksPath", "/dev/null") // Disable hooks in test repo
|
||||
// Pin line-ending handling so on-disk content survives commit/checkout
|
||||
// roundtrips byte-for-byte. Without this, Windows defaults to
|
||||
// core.autocrlf=true and converts LF to CRLF on checkout, which breaks
|
||||
// tests that compare os.ReadFile output against an LF-only literal.
|
||||
runGit(t, localDir, "config", "core.autocrlf", "false")
|
||||
runGit(t, localDir, "config", "core.eol", "lf")
|
||||
|
||||
// Create initial commit
|
||||
writeFile(t, localDir, "README.md", "# Test Repo")
|
||||
@@ -759,6 +766,9 @@ func TestApplyFileDiff_RegularFile(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestApplyFileDiff_Symlink(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("creating POSIX symlinks requires Windows Developer Mode or admin")
|
||||
}
|
||||
repoDir, cleanup := setupTestRepo(t)
|
||||
defer cleanup()
|
||||
|
||||
@@ -1102,6 +1112,15 @@ func TestGetFileContentAtRef(t *testing.T) {
|
||||
})
|
||||
|
||||
t.Run("absolute path rejected", func(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
// filepath.IsAbs is platform-specific: on Windows it only treats
|
||||
// drive-letter paths (C:\...) as absolute, so `/etc/passwd` is
|
||||
// considered relative and the product's path-traversal check
|
||||
// (correctly) doesn't fire. A meaningful Windows version of this
|
||||
// case would use a path like `C:\Windows\System32\config\sam`
|
||||
// and is left for a separate cleanup.
|
||||
t.Skip("absolute-path semantics differ on Windows")
|
||||
}
|
||||
_, _, _, err := wt.GetFileContentAtRef(ctx, "/etc/passwd", "HEAD")
|
||||
if err == nil {
|
||||
t.Fatal("expected error for absolute path, got nil")
|
||||
|
||||
@@ -0,0 +1,80 @@
|
||||
// Package shellexec runs scripts through a host-appropriate shell.
|
||||
//
|
||||
// On Unix it uses the requested shell directly (/bin/sh or /bin/bash). On
|
||||
// Windows it prefers bash.exe from Git for Windows when available — that's
|
||||
// what the user already has installed if they cloned the repo via git — and
|
||||
// falls back to cmd.exe /c when no bash is found. The Git-Bash preference
|
||||
// keeps Bourne-style scripts portable (pipelines, redirects, expansions);
|
||||
// the cmd.exe fallback handles the simple-command cases that don't need a
|
||||
// real POSIX shell (the agent installer's `npm install -g ...` for example).
|
||||
package shellexec
|
||||
|
||||
import (
|
||||
"context"
|
||||
"os"
|
||||
"os/exec"
|
||||
"runtime"
|
||||
)
|
||||
|
||||
// Shell tells CommandContext what dialect the script is written in. The hint
|
||||
// is only consulted on Unix; on Windows the same resolution (Git Bash, then
|
||||
// cmd.exe) is used regardless of hint, because the practical alternatives
|
||||
// don't differ in a way that affects script compatibility.
|
||||
type Shell string
|
||||
|
||||
const (
|
||||
// PosixSh — Bourne-compatible script. Picks /bin/sh on Unix.
|
||||
PosixSh Shell = "sh"
|
||||
// Bash — explicitly bash. Picks /bin/bash on Unix.
|
||||
Bash Shell = "bash"
|
||||
)
|
||||
|
||||
// CommandContext returns an exec.Cmd that runs `script` under the host shell.
|
||||
// The returned Cmd is not started — the caller wires stdout/stderr/env then
|
||||
// calls Start/Run/Output as usual.
|
||||
func CommandContext(ctx context.Context, shell Shell, script string) *exec.Cmd {
|
||||
prog, args := resolve(shell, script)
|
||||
return exec.CommandContext(ctx, prog, args...)
|
||||
}
|
||||
|
||||
// resolve picks the shell program and argv-prefix appropriate for the host.
|
||||
// Exposed for tests.
|
||||
func resolve(shell Shell, script string) (string, []string) {
|
||||
if runtime.GOOS == "windows" {
|
||||
if bash := findWindowsBash(); bash != "" {
|
||||
return bash, []string{"-c", script}
|
||||
}
|
||||
return "cmd", []string{"/c", script}
|
||||
}
|
||||
if shell == Bash {
|
||||
return "bash", []string{"-c", script}
|
||||
}
|
||||
return "sh", []string{"-c", script}
|
||||
}
|
||||
|
||||
// findWindowsBash looks for bash.exe, preferring Git-for-Windows install
|
||||
// locations over whatever PATH happens to resolve. Returns "" if none found.
|
||||
// Exposed for tests via package-level overrides; the production code reads
|
||||
// from disk.
|
||||
//
|
||||
// Order matters: on a Windows 10/11 box with WSL installed,
|
||||
// C:\Windows\System32\bash.exe (the WSL launcher) is in PATH ahead of any
|
||||
// Git addition. Calling that would run our scripts inside a Linux
|
||||
// subsystem where Windows-native paths, npm.cmd, node.exe, etc. are
|
||||
// invisible — agent install scripts would silently fail. Probe the known
|
||||
// Git paths first, then fall back to PATH for non-standard Git installs.
|
||||
var findWindowsBash = func() string {
|
||||
for _, candidate := range []string{
|
||||
`C:\Program Files\Git\bin\bash.exe`,
|
||||
`C:\Program Files\Git\usr\bin\bash.exe`,
|
||||
`C:\Program Files (x86)\Git\bin\bash.exe`,
|
||||
} {
|
||||
if _, err := os.Stat(candidate); err == nil {
|
||||
return candidate
|
||||
}
|
||||
}
|
||||
if p, err := exec.LookPath("bash"); err == nil {
|
||||
return p
|
||||
}
|
||||
return ""
|
||||
}
|
||||
@@ -0,0 +1,80 @@
|
||||
package shellexec
|
||||
|
||||
import (
|
||||
"context"
|
||||
"runtime"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestResolve_Unix(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("unix-only resolution")
|
||||
}
|
||||
t.Run("sh hint", func(t *testing.T) {
|
||||
prog, args := resolve(PosixSh, "echo hi")
|
||||
if prog != "sh" {
|
||||
t.Errorf("prog = %q, want sh", prog)
|
||||
}
|
||||
if len(args) != 2 || args[0] != "-c" || args[1] != "echo hi" {
|
||||
t.Errorf("args = %v, want [-c echo hi]", args)
|
||||
}
|
||||
})
|
||||
t.Run("bash hint", func(t *testing.T) {
|
||||
prog, args := resolve(Bash, "echo hi")
|
||||
if prog != "bash" {
|
||||
t.Errorf("prog = %q, want bash", prog)
|
||||
}
|
||||
if len(args) != 2 || args[0] != "-c" || args[1] != "echo hi" {
|
||||
t.Errorf("args = %v, want [-c echo hi]", args)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func TestResolve_Windows_PrefersBash(t *testing.T) {
|
||||
if runtime.GOOS != "windows" {
|
||||
t.Skip("windows-only resolution")
|
||||
}
|
||||
original := findWindowsBash
|
||||
t.Cleanup(func() { findWindowsBash = original })
|
||||
findWindowsBash = func() string { return `C:\fake\bash.exe` }
|
||||
|
||||
prog, args := resolve(PosixSh, "echo hi")
|
||||
if prog != `C:\fake\bash.exe` {
|
||||
t.Errorf("prog = %q, want fake bash path", prog)
|
||||
}
|
||||
if len(args) != 2 || args[0] != "-c" || args[1] != "echo hi" {
|
||||
t.Errorf("args = %v", args)
|
||||
}
|
||||
}
|
||||
|
||||
func TestResolve_Windows_FallsBackToCmd(t *testing.T) {
|
||||
if runtime.GOOS != "windows" {
|
||||
t.Skip("windows-only resolution")
|
||||
}
|
||||
original := findWindowsBash
|
||||
t.Cleanup(func() { findWindowsBash = original })
|
||||
findWindowsBash = func() string { return "" }
|
||||
|
||||
prog, args := resolve(PosixSh, "echo hi")
|
||||
if prog != "cmd" {
|
||||
t.Errorf("prog = %q, want cmd", prog)
|
||||
}
|
||||
if len(args) != 2 || args[0] != "/c" || args[1] != "echo hi" {
|
||||
t.Errorf("args = %v", args)
|
||||
}
|
||||
}
|
||||
|
||||
// TestCommandContext_RunsScript end-to-end — picks whatever the host has and
|
||||
// confirms the script actually runs. Uses a portable `echo` that works in
|
||||
// every supported shell (sh, bash, cmd).
|
||||
func TestCommandContext_RunsScript(t *testing.T) {
|
||||
cmd := CommandContext(context.Background(), PosixSh, "echo hello-shellexec")
|
||||
out, err := cmd.CombinedOutput()
|
||||
if err != nil {
|
||||
t.Fatalf("run: %v (output: %s)", err, out)
|
||||
}
|
||||
if !strings.Contains(string(out), "hello-shellexec") {
|
||||
t.Errorf("output = %q, want substring hello-shellexec", out)
|
||||
}
|
||||
}
|
||||
@@ -510,6 +510,46 @@ func TestHandleCreateTask_AutoResolvesWorkspaceAndWorkflow(t *testing.T) {
|
||||
assert.Equal(t, ws.MessageTypeResponse, resp.Type, "should succeed with auto-resolved workspace and workflow")
|
||||
}
|
||||
|
||||
// TestCreateTask_GitHubURLOnly_LeavesDefaultBranchEmpty pins the upstream
|
||||
// contract that produced the production "base branch does not exist" failure
|
||||
// for task 01b82e73. When an MCP caller passes only a github_url (no
|
||||
// default_branch, no base_branch), the resulting Repository row has an empty
|
||||
// default_branch and the TaskRepository row has an empty base_branch — the
|
||||
// service layer never probes the upstream remote.
|
||||
//
|
||||
// This test documents that contract so any future change there (e.g. the
|
||||
// service learns to probe the remote up front) is an intentional decision,
|
||||
// and so the executor-side backfill that compensates for this isn't
|
||||
// accidentally treated as redundant.
|
||||
func TestCreateTask_GitHubURLOnly_LeavesDefaultBranchEmpty(t *testing.T) {
|
||||
svc, repo := newTestTaskService(t)
|
||||
ctx := context.Background()
|
||||
|
||||
require.NoError(t, repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "Test"}))
|
||||
require.NoError(t, repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "Board"}))
|
||||
|
||||
task, err := svc.CreateTask(ctx, &service.CreateTaskRequest{
|
||||
WorkspaceID: "ws-1",
|
||||
WorkflowID: "wf-1",
|
||||
Title: "subtask via bare github url",
|
||||
Repositories: []service.TaskRepositoryInput{
|
||||
{GitHubURL: "https://github.com/acme/never-seen"},
|
||||
},
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, task.Repositories, 1)
|
||||
assert.Empty(t, task.Repositories[0].BaseBranch,
|
||||
"task_repositories.base_branch should be empty when caller passes neither base_branch nor default_branch — executor backfill compensates downstream")
|
||||
|
||||
createdRepo, err := svc.GetRepository(ctx, task.Repositories[0].RepositoryID)
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, createdRepo)
|
||||
assert.Empty(t, createdRepo.DefaultBranch,
|
||||
"repositories.default_branch should be empty: FindOrCreateRepository does not probe the remote — the executor backfills it after clone")
|
||||
assert.Equal(t, "acme", createdRepo.ProviderOwner)
|
||||
assert.Equal(t, "never-seen", createdRepo.ProviderName)
|
||||
}
|
||||
|
||||
func TestHandleCreateTask_AutoResolveFailsWithMultipleWorkflows(t *testing.T) {
|
||||
svc, repo := newTestTaskService(t)
|
||||
ctx := context.Background()
|
||||
|
||||
@@ -481,6 +481,11 @@ type RepoCloner interface {
|
||||
// RepoUpdater updates repository records in the database.
|
||||
type RepoUpdater interface {
|
||||
UpdateRepositoryLocalPath(ctx context.Context, repositoryID, localPath string) error
|
||||
// UpdateRepositoryDefaultBranch persists the integration branch detected
|
||||
// from the local clone. Called after a fresh provider-backed clone when
|
||||
// the repository row was created without an upstream-derived value
|
||||
// (e.g. via the MCP create_task path that takes a bare github URL).
|
||||
UpdateRepositoryDefaultBranch(ctx context.Context, repositoryID, defaultBranch string) error
|
||||
}
|
||||
|
||||
// ExecutorConfig holds configuration for the Executor
|
||||
|
||||
@@ -7,6 +7,7 @@ import (
|
||||
"time"
|
||||
|
||||
"github.com/kandev/kandev/internal/agent/lifecycle"
|
||||
"github.com/kandev/kandev/internal/common/gitref"
|
||||
"github.com/kandev/kandev/internal/task/models"
|
||||
v1 "github.com/kandev/kandev/pkg/api/v1"
|
||||
"go.uber.org/zap"
|
||||
@@ -112,6 +113,15 @@ func (e *Executor) resolveTaskRepoInfo(ctx context.Context, tr *models.TaskRepos
|
||||
}
|
||||
}
|
||||
|
||||
// Backfill default_branch from the local clone when missing. This fires for
|
||||
// two cases: (1) a freshly cloned provider-backed repo whose row was created
|
||||
// without an upstream-derived value (e.g. the MCP create_task path that
|
||||
// takes a bare github URL), and (2) an already-cloned row that escaped a
|
||||
// prior backfill (e.g. a launch that ran before this code existed). Without
|
||||
// this, the BaseBranch fallback below stays empty and surfaces to the user
|
||||
// as "base branch does not exist" from worktree.Manager.Create.
|
||||
e.backfillRepoDefaultBranch(ctx, repo, repo.LocalPath)
|
||||
|
||||
info.Repository = repo
|
||||
info.RepositoryPath = repo.LocalPath
|
||||
info.WorktreeBranchPrefix = repo.WorktreeBranchPrefix
|
||||
@@ -167,9 +177,43 @@ func (e *Executor) ensureRepoCloned(ctx context.Context, repo *models.Repository
|
||||
}
|
||||
}
|
||||
|
||||
// Note: default_branch backfill is intentionally driven from
|
||||
// resolveTaskRepoInfo (the caller), not here. That way it also runs for
|
||||
// rows whose local_path was already populated by a prior launch but whose
|
||||
// default_branch was never persisted (e.g. rows created before the
|
||||
// backfill existed).
|
||||
|
||||
return localPath, nil
|
||||
}
|
||||
|
||||
// backfillRepoDefaultBranch populates repo.DefaultBranch from the local clone
|
||||
// (in memory + DB) when it's empty. Best-effort: on any failure we log and
|
||||
// continue, since the launch still has the legacy worktree-manager fallback
|
||||
// to fall back on if it can find a branch by another route.
|
||||
func (e *Executor) backfillRepoDefaultBranch(ctx context.Context, repo *models.Repository, localPath string) {
|
||||
if repo.DefaultBranch != "" || localPath == "" {
|
||||
return
|
||||
}
|
||||
detected, err := gitref.DefaultBranch(localPath)
|
||||
if err != nil || detected == "" {
|
||||
e.logger.Debug("could not detect default branch from clone; leaving empty",
|
||||
zap.String("repository_id", repo.ID),
|
||||
zap.String("local_path", localPath),
|
||||
zap.Error(err))
|
||||
return
|
||||
}
|
||||
repo.DefaultBranch = detected
|
||||
if e.repoUpdater == nil {
|
||||
return
|
||||
}
|
||||
if updateErr := e.repoUpdater.UpdateRepositoryDefaultBranch(ctx, repo.ID, detected); updateErr != nil {
|
||||
e.logger.Warn("failed to persist detected default branch after clone",
|
||||
zap.String("repository_id", repo.ID),
|
||||
zap.String("default_branch", detected),
|
||||
zap.Error(updateErr))
|
||||
}
|
||||
}
|
||||
|
||||
// persistLaunchState updates the session record after a successful agent launch.
|
||||
// The executors_running row is now written by the lifecycle manager itself in
|
||||
// lockstep with executionStore.Add (see lifecycle.persistExecutorRunning) — this
|
||||
|
||||
+283
@@ -0,0 +1,283 @@
|
||||
package executor
|
||||
|
||||
import (
|
||||
"context"
|
||||
"os"
|
||||
"os/exec"
|
||||
"path/filepath"
|
||||
"sync"
|
||||
"testing"
|
||||
|
||||
"github.com/kandev/kandev/internal/task/models"
|
||||
)
|
||||
|
||||
// TestResolveTaskRepoInfo_BackfillsDefaultBranchAfterClone is the regression
|
||||
// guard for the production bug where MCP create_task with a bare github URL
|
||||
// produced a Repository row with empty default_branch and a TaskRepository
|
||||
// row with empty base_branch. Result: worktree.Manager.Create returned
|
||||
// "base branch does not exist" because BaseBranch was empty by the time it
|
||||
// was called.
|
||||
//
|
||||
// After the fix, ensureRepoCloned reads origin/HEAD from the freshly cloned
|
||||
// repo via gitref.DefaultBranch and persists it onto the Repository row,
|
||||
// which lets resolveTaskRepoInfo's existing fallback lift the value into
|
||||
// repoInfo.BaseBranch.
|
||||
func TestResolveTaskRepoInfo_BackfillsDefaultBranchAfterClone(t *testing.T) {
|
||||
originPath := initBareOriginWithMain(t)
|
||||
clonePath := filepath.Join(t.TempDir(), "clone")
|
||||
runGitInTest(t, "", "clone", originPath, clonePath)
|
||||
|
||||
repo := newMockRepository()
|
||||
repo.repositories["repo-1"] = &models.Repository{
|
||||
ID: "repo-1",
|
||||
Provider: "github",
|
||||
ProviderOwner: "acme",
|
||||
ProviderName: "thing",
|
||||
LocalPath: "", // forces the on-demand clone path
|
||||
DefaultBranch: "", // mirrors the buggy MCP-created row
|
||||
}
|
||||
taskRepo := &models.TaskRepository{
|
||||
ID: "tr-1",
|
||||
TaskID: "task-1",
|
||||
RepositoryID: "repo-1",
|
||||
BaseBranch: "", // mirrors task_repositories row left empty by MCP path
|
||||
}
|
||||
|
||||
updater := &recordingRepoUpdater{}
|
||||
exc := newTestExecutor(t, &mockAgentManager{}, repo)
|
||||
exc.SetRepoCloner(&fakeRepoCloner{returnPath: clonePath}, updater)
|
||||
|
||||
info, err := exc.resolveTaskRepoInfo(context.Background(), taskRepo)
|
||||
if err != nil {
|
||||
t.Fatalf("resolveTaskRepoInfo: %v", err)
|
||||
}
|
||||
if info.BaseBranch != "main" {
|
||||
t.Errorf("BaseBranch: got %q, want %q", info.BaseBranch, "main")
|
||||
}
|
||||
if info.Repository == nil || info.Repository.DefaultBranch != "main" {
|
||||
t.Errorf("Repository.DefaultBranch: got %q, want %q", info.Repository.DefaultBranch, "main")
|
||||
}
|
||||
if got := updater.getDefaultBranch("repo-1"); got != "main" {
|
||||
t.Errorf("UpdateRepositoryDefaultBranch: got %q, want %q", got, "main")
|
||||
}
|
||||
}
|
||||
|
||||
// TestResolveTaskRepoInfo_BackfillsDefaultBranchForAlreadyClonedRepo guards
|
||||
// against the second-launch regression: a previous failed attempt populated
|
||||
// repositories.local_path but left default_branch empty (because the backfill
|
||||
// code didn't exist yet), so on the next launch resolveTaskRepoInfo skipped
|
||||
// the clone path entirely and never looked at origin/HEAD. Same surface
|
||||
// error: "base branch does not exist".
|
||||
func TestResolveTaskRepoInfo_BackfillsDefaultBranchForAlreadyClonedRepo(t *testing.T) {
|
||||
originPath := initBareOriginWithMain(t)
|
||||
clonePath := filepath.Join(t.TempDir(), "clone")
|
||||
runGitInTest(t, "", "clone", originPath, clonePath)
|
||||
|
||||
repo := newMockRepository()
|
||||
repo.repositories["repo-1"] = &models.Repository{
|
||||
ID: "repo-1",
|
||||
Provider: "github",
|
||||
ProviderOwner: "acme",
|
||||
ProviderName: "thing",
|
||||
LocalPath: clonePath, // already cloned by a prior (failed) launch
|
||||
DefaultBranch: "", // never persisted
|
||||
}
|
||||
taskRepo := &models.TaskRepository{ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1"}
|
||||
|
||||
updater := &recordingRepoUpdater{}
|
||||
exc := newTestExecutor(t, &mockAgentManager{}, repo)
|
||||
// No cloner needed: LocalPath is already set so ensureRepoCloned never runs.
|
||||
exc.SetRepoCloner(nil, updater)
|
||||
|
||||
info, err := exc.resolveTaskRepoInfo(context.Background(), taskRepo)
|
||||
if err != nil {
|
||||
t.Fatalf("resolveTaskRepoInfo: %v", err)
|
||||
}
|
||||
if info.BaseBranch != "main" {
|
||||
t.Errorf("BaseBranch: got %q, want %q", info.BaseBranch, "main")
|
||||
}
|
||||
if got := updater.getDefaultBranch("repo-1"); got != "main" {
|
||||
t.Errorf("UpdateRepositoryDefaultBranch: got %q, want %q", got, "main")
|
||||
}
|
||||
}
|
||||
|
||||
// TestResolveTaskRepoInfo_LeavesNonEmptyDefaultBranchAlone guards against the
|
||||
// backfill stomping on an already-populated default_branch.
|
||||
func TestResolveTaskRepoInfo_LeavesNonEmptyDefaultBranchAlone(t *testing.T) {
|
||||
originPath := initBareOriginWithMain(t)
|
||||
clonePath := filepath.Join(t.TempDir(), "clone")
|
||||
runGitInTest(t, "", "clone", originPath, clonePath)
|
||||
|
||||
repo := newMockRepository()
|
||||
repo.repositories["repo-1"] = &models.Repository{
|
||||
ID: "repo-1",
|
||||
Provider: "github",
|
||||
ProviderOwner: "acme",
|
||||
ProviderName: "thing",
|
||||
LocalPath: "",
|
||||
DefaultBranch: "develop", // already set; backfill must not overwrite
|
||||
}
|
||||
taskRepo := &models.TaskRepository{ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1"}
|
||||
|
||||
updater := &recordingRepoUpdater{}
|
||||
exc := newTestExecutor(t, &mockAgentManager{}, repo)
|
||||
exc.SetRepoCloner(&fakeRepoCloner{returnPath: clonePath}, updater)
|
||||
|
||||
info, err := exc.resolveTaskRepoInfo(context.Background(), taskRepo)
|
||||
if err != nil {
|
||||
t.Fatalf("resolveTaskRepoInfo: %v", err)
|
||||
}
|
||||
if info.BaseBranch != "develop" {
|
||||
t.Errorf("BaseBranch should follow the existing default_branch: got %q, want %q", info.BaseBranch, "develop")
|
||||
}
|
||||
if got := updater.getDefaultBranch("repo-1"); got != "" {
|
||||
t.Errorf("UpdateRepositoryDefaultBranch should not be called when DefaultBranch is set, got %q", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestResolveTaskRepoInfo_ExplicitTaskBaseBranchWinsOverBackfill pins the
|
||||
// precedence: a TaskRepository.BaseBranch set by the caller (e.g. agent
|
||||
// targeting a non-default branch on purpose) must NOT be overwritten by the
|
||||
// backfilled default_branch. The backfill should still persist the detected
|
||||
// branch so future tasks on the same repo get the fallback for free.
|
||||
func TestResolveTaskRepoInfo_ExplicitTaskBaseBranchWinsOverBackfill(t *testing.T) {
|
||||
originPath := initBareOriginWithMain(t)
|
||||
clonePath := filepath.Join(t.TempDir(), "clone")
|
||||
runGitInTest(t, "", "clone", originPath, clonePath)
|
||||
|
||||
repo := newMockRepository()
|
||||
repo.repositories["repo-1"] = &models.Repository{
|
||||
ID: "repo-1",
|
||||
Provider: "github",
|
||||
ProviderOwner: "acme",
|
||||
ProviderName: "thing",
|
||||
LocalPath: clonePath,
|
||||
DefaultBranch: "",
|
||||
}
|
||||
taskRepo := &models.TaskRepository{
|
||||
ID: "tr-1",
|
||||
TaskID: "task-1",
|
||||
RepositoryID: "repo-1",
|
||||
BaseBranch: "feature-x", // explicit, must survive
|
||||
}
|
||||
|
||||
updater := &recordingRepoUpdater{}
|
||||
exc := newTestExecutor(t, &mockAgentManager{}, repo)
|
||||
exc.SetRepoCloner(nil, updater)
|
||||
|
||||
info, err := exc.resolveTaskRepoInfo(context.Background(), taskRepo)
|
||||
if err != nil {
|
||||
t.Fatalf("resolveTaskRepoInfo: %v", err)
|
||||
}
|
||||
if info.BaseBranch != "feature-x" {
|
||||
t.Errorf("BaseBranch should follow explicit task value, got %q want %q", info.BaseBranch, "feature-x")
|
||||
}
|
||||
if got := updater.getDefaultBranch("repo-1"); got != "main" {
|
||||
t.Errorf("backfill should still persist detected default for future tasks: got %q want %q", got, "main")
|
||||
}
|
||||
}
|
||||
|
||||
// TestResolveTaskRepoInfo_BackfillIsBestEffortWhenLocalPathBroken guards the
|
||||
// "log and continue" contract for gitref.DefaultBranch failures. If the local
|
||||
// path doesn't point at a git repo (e.g. a stub directory created for a
|
||||
// quick-chat task), resolveTaskRepoInfo must NOT return an error: the backfill
|
||||
// silently no-ops and BaseBranch stays empty (the worktree manager owns the
|
||||
// downstream "no branch" handling).
|
||||
func TestResolveTaskRepoInfo_BackfillIsBestEffortWhenLocalPathBroken(t *testing.T) {
|
||||
notAGitRepo := t.TempDir() // empty dir, no .git inside
|
||||
|
||||
repo := newMockRepository()
|
||||
repo.repositories["repo-1"] = &models.Repository{
|
||||
ID: "repo-1",
|
||||
Provider: "github",
|
||||
ProviderOwner: "acme",
|
||||
ProviderName: "thing",
|
||||
LocalPath: notAGitRepo,
|
||||
DefaultBranch: "",
|
||||
}
|
||||
taskRepo := &models.TaskRepository{ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1"}
|
||||
|
||||
updater := &recordingRepoUpdater{}
|
||||
exc := newTestExecutor(t, &mockAgentManager{}, repo)
|
||||
exc.SetRepoCloner(nil, updater)
|
||||
|
||||
info, err := exc.resolveTaskRepoInfo(context.Background(), taskRepo)
|
||||
if err != nil {
|
||||
t.Fatalf("resolveTaskRepoInfo should not fail when backfill cannot detect: %v", err)
|
||||
}
|
||||
if info.BaseBranch != "" {
|
||||
t.Errorf("BaseBranch should stay empty when detection fails, got %q", info.BaseBranch)
|
||||
}
|
||||
if got := updater.getDefaultBranch("repo-1"); got != "" {
|
||||
t.Errorf("UpdateRepositoryDefaultBranch should not be called on detection failure, got %q", got)
|
||||
}
|
||||
}
|
||||
|
||||
// fakeRepoCloner returns a fixed local path for any clone request.
|
||||
type fakeRepoCloner struct{ returnPath string }
|
||||
|
||||
func (f *fakeRepoCloner) EnsureCloned(_ context.Context, _, _, _ string) (string, error) {
|
||||
return f.returnPath, nil
|
||||
}
|
||||
|
||||
func (f *fakeRepoCloner) BuildCloneURL(_, owner, name string) (string, error) {
|
||||
return "https://github.com/" + owner + "/" + name + ".git", nil
|
||||
}
|
||||
|
||||
// recordingRepoUpdater captures calls so tests can assert on what was persisted.
|
||||
type recordingRepoUpdater struct {
|
||||
mu sync.Mutex
|
||||
defaultBranch map[string]string
|
||||
}
|
||||
|
||||
func (r *recordingRepoUpdater) UpdateRepositoryLocalPath(_ context.Context, _, _ string) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
func (r *recordingRepoUpdater) UpdateRepositoryDefaultBranch(_ context.Context, repositoryID, defaultBranch string) error {
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
if r.defaultBranch == nil {
|
||||
r.defaultBranch = make(map[string]string)
|
||||
}
|
||||
r.defaultBranch[repositoryID] = defaultBranch
|
||||
return nil
|
||||
}
|
||||
|
||||
func (r *recordingRepoUpdater) getDefaultBranch(repositoryID string) string {
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
return r.defaultBranch[repositoryID]
|
||||
}
|
||||
|
||||
// initBareOriginWithMain creates a bare git repo with one commit on `main`.
|
||||
// Cloning from this path produces a working tree whose origin/HEAD points at
|
||||
// origin/main, which is what gitref.DefaultBranch reads.
|
||||
func initBareOriginWithMain(t *testing.T) string {
|
||||
t.Helper()
|
||||
root := t.TempDir()
|
||||
originPath := filepath.Join(root, "origin.git")
|
||||
workPath := filepath.Join(root, "work")
|
||||
runGitInTest(t, "", "init", "--bare", "-b", "main", originPath)
|
||||
runGitInTest(t, "", "clone", originPath, workPath)
|
||||
runGitInTest(t, workPath, "config", "user.email", "test@example.com")
|
||||
runGitInTest(t, workPath, "config", "user.name", "Test User")
|
||||
if err := os.WriteFile(filepath.Join(workPath, "README.md"), []byte("hi\n"), 0o644); err != nil {
|
||||
t.Fatalf("write README: %v", err)
|
||||
}
|
||||
runGitInTest(t, workPath, "add", "README.md")
|
||||
runGitInTest(t, workPath, "commit", "-m", "initial")
|
||||
runGitInTest(t, workPath, "push", "origin", "main")
|
||||
return originPath
|
||||
}
|
||||
|
||||
func runGitInTest(t *testing.T, cwd string, args ...string) {
|
||||
t.Helper()
|
||||
cmd := exec.Command("git", args...)
|
||||
if cwd != "" {
|
||||
cmd.Dir = cwd
|
||||
}
|
||||
if out, err := cmd.CombinedOutput(); err != nil {
|
||||
t.Fatalf("git %v failed: %v\n%s", args, err, out)
|
||||
}
|
||||
}
|
||||
@@ -27,16 +27,17 @@ type DirectoryListing struct {
|
||||
// ListDirectory returns the immediate subdirectories of path. When path is
|
||||
// empty it falls back to $HOME. The picker is deliberately *not* anchored
|
||||
// to discoveryRoots — users browsing for a starting folder for a repo-less
|
||||
// task may legitimately want /tmp, /var/log/foo, or any other directory
|
||||
// they have read access to. The repo-discover endpoint stays locked down
|
||||
// to discoveryRoots; this one trusts the local-process boundary that runs
|
||||
// kandev on the user's own machine.
|
||||
// task may legitimately want /tmp, /var/log/foo, C:\Users, or any other
|
||||
// directory they have read access to. The repo-discover endpoint stays
|
||||
// locked down to discoveryRoots; this one trusts the local-process
|
||||
// boundary that runs kandev on the user's own machine.
|
||||
//
|
||||
// Filesystem operations go through os.Root opened at "/", which the Go
|
||||
// stdlib enforces at the syscall level (no symlink escape, traversal in
|
||||
// the relative path is rooted to "/"). That's a no-op for security because
|
||||
// "/" is the filesystem root — but it's what CodeQL recognises as a
|
||||
// path-injection sanitizer, so the lint stays green.
|
||||
// Filesystem operations go through os.Root opened at the volume root
|
||||
// ("/" on Unix, "C:\" / UNC share on Windows). The Go stdlib enforces
|
||||
// containment at the syscall level (no symlink escape, no traversal out
|
||||
// of the root). That's a no-op for security because the volume root
|
||||
// covers every path the user could pick on that volume — but it's what
|
||||
// CodeQL recognises as a path-injection sanitizer, so the lint stays green.
|
||||
//
|
||||
// Hidden (".") directories are excluded.
|
||||
func (s *Service) ListDirectory(ctx context.Context, path string) (DirectoryListing, error) {
|
||||
@@ -75,25 +76,21 @@ func resolveListingPath(path string) (string, error) {
|
||||
return filepath.Clean(abs), nil
|
||||
}
|
||||
|
||||
// readSubdirsBoundedAtFilesystemRoot opens "/" via os.Root and reads the
|
||||
// directory at the relative path inside it. Using os.Root here is for
|
||||
// CodeQL's benefit: the Go stdlib refuses symlink escape and rejects
|
||||
// traversal segments in the relative path, and CodeQL models os.Root.Open
|
||||
// as a sanitizer for the path-injection query. Because the root is "/",
|
||||
// containment is trivially satisfied — every absolute path on the host is
|
||||
// inside it.
|
||||
// readSubdirsBoundedAtFilesystemRoot opens the volume containing abs via
|
||||
// os.Root and reads the directory at the volume-relative path inside it.
|
||||
// Using os.Root here is for CodeQL's benefit: the Go stdlib refuses symlink
|
||||
// escape and rejects traversal segments in the relative path, and CodeQL
|
||||
// models os.Root.Open as a sanitizer for the path-injection query. The
|
||||
// containment is trivially satisfied because the root is the volume root —
|
||||
// every accessible path on that volume is inside it.
|
||||
func readSubdirsBoundedAtFilesystemRoot(abs string) ([]os.DirEntry, error) {
|
||||
root, err := os.OpenRoot("/")
|
||||
rootPath, rel := splitAbsForRoot(abs)
|
||||
root, err := os.OpenRoot(rootPath)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("open root: %w", err)
|
||||
return nil, fmt.Errorf("open root %q: %w", rootPath, err)
|
||||
}
|
||||
defer func() { _ = root.Close() }()
|
||||
|
||||
rel := strings.TrimPrefix(abs, "/")
|
||||
if rel == "" {
|
||||
rel = "."
|
||||
}
|
||||
|
||||
info, err := root.Stat(rel)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
@@ -110,6 +107,38 @@ func readSubdirsBoundedAtFilesystemRoot(abs string) ([]os.DirEntry, error) {
|
||||
return dir.ReadDir(-1)
|
||||
}
|
||||
|
||||
// splitAbsForRoot splits an absolute path into (rootPath, rel) suitable for
|
||||
// os.OpenRoot + os.Root.Stat/Open. os.Root rejects absolute paths and drive
|
||||
// letters inside its relative arg, so we have to pre-split on the volume.
|
||||
//
|
||||
// On Unix this is the historical "/" + path-minus-leading-slash pair. On
|
||||
// Windows the root is the volume — "C:\" for drive-letter paths,
|
||||
// "\\server\share\" for UNC — and the relative is what remains after the
|
||||
// volume. Without this split, Windows paths like "C:\Users\carlo" passed as
|
||||
// the relative arg trip os.Root's "path escapes from parent" check, even
|
||||
// though they're inside the conceptual filesystem root.
|
||||
func splitAbsForRoot(abs string) (rootPath, rel string) {
|
||||
vol := filepath.VolumeName(abs)
|
||||
if vol == "" {
|
||||
// Unix absolute path (or a non-volume Windows path, which shouldn't
|
||||
// happen after filepath.Abs).
|
||||
rel = strings.TrimPrefix(abs, "/")
|
||||
if rel == "" {
|
||||
rel = "."
|
||||
}
|
||||
return "/", rel
|
||||
}
|
||||
// Windows drive-rooted or UNC absolute path. filepath.VolumeName returns
|
||||
// "C:" for "C:\foo" and "\\server\share" for UNC paths — append the
|
||||
// separator to get the actual openable root directory.
|
||||
rootPath = vol + string(filepath.Separator)
|
||||
rel = strings.TrimPrefix(abs, rootPath)
|
||||
if rel == "" {
|
||||
rel = "."
|
||||
}
|
||||
return rootPath, rel
|
||||
}
|
||||
|
||||
// parentPath returns the parent of abs, or "" when abs is the filesystem
|
||||
// root. The picker's "up" button stops at "/" so the user can't navigate
|
||||
// past the top of the host filesystem.
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"context"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"runtime"
|
||||
"testing"
|
||||
)
|
||||
|
||||
@@ -76,25 +77,75 @@ func TestListDirectory_RejectsNonDirectory(t *testing.T) {
|
||||
func TestListDirectory_BrowsesOutsideHome(t *testing.T) {
|
||||
// The picker deliberately lets users browse any directory the kandev
|
||||
// process has read access to, not just $HOME or the discoveryRoots.
|
||||
// /tmp is the canonical "outside $HOME but accessible" path on most
|
||||
// Unix CI runners; assert we can list it without error.
|
||||
svc := &Service{}
|
||||
got, err := svc.ListDirectory(context.Background(), "/tmp")
|
||||
if err != nil {
|
||||
t.Fatalf("ListDirectory(/tmp): %v", err)
|
||||
// Pick a canonical "outside $HOME but accessible" path per platform.
|
||||
target := "/tmp"
|
||||
if runtime.GOOS == "windows" {
|
||||
target = os.Getenv("SystemDrive") + `\` // e.g. "C:\"
|
||||
if target == `\` {
|
||||
target = `C:\`
|
||||
}
|
||||
}
|
||||
if got.Path != "/tmp" {
|
||||
t.Errorf("got Path = %q; want /tmp", got.Path)
|
||||
svc := &Service{}
|
||||
got, err := svc.ListDirectory(context.Background(), target)
|
||||
if err != nil {
|
||||
t.Fatalf("ListDirectory(%q): %v", target, err)
|
||||
}
|
||||
if got.Path != filepath.Clean(target) {
|
||||
t.Errorf("got Path = %q; want %q", got.Path, filepath.Clean(target))
|
||||
}
|
||||
}
|
||||
|
||||
func TestSplitAbsForRoot(t *testing.T) {
|
||||
// Each case spells out the exact OS it applies to so the test runs the
|
||||
// right branches no matter where the suite executes. filepath.VolumeName
|
||||
// only returns non-empty on Windows, so the Windows-shape cases are
|
||||
// behaviour-noisy on Unix and vice versa — we filter accordingly.
|
||||
cases := []struct {
|
||||
name string
|
||||
os string // "windows", "unix", or "" for both
|
||||
in string
|
||||
wantRoot string
|
||||
wantRel string
|
||||
}{
|
||||
{name: "unix-typical", os: "unix", in: "/home/user", wantRoot: "/", wantRel: "home/user"},
|
||||
{name: "unix-root", os: "unix", in: "/", wantRoot: "/", wantRel: "."},
|
||||
{name: "windows-drive-typical", os: "windows", in: `C:\Users\carlo`, wantRoot: `C:\`, wantRel: `Users\carlo`},
|
||||
{name: "windows-drive-root", os: "windows", in: `C:\`, wantRoot: `C:\`, wantRel: "."},
|
||||
{name: "windows-unc", os: "windows", in: `\\server\share\sub`, wantRoot: `\\server\share\`, wantRel: "sub"},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
if tc.os == "windows" && runtime.GOOS != "windows" {
|
||||
t.Skip("windows-only path shape")
|
||||
}
|
||||
if tc.os == "unix" && runtime.GOOS == "windows" {
|
||||
t.Skip("unix-only path shape")
|
||||
}
|
||||
gotRoot, gotRel := splitAbsForRoot(tc.in)
|
||||
if gotRoot != tc.wantRoot {
|
||||
t.Errorf("rootPath = %q; want %q", gotRoot, tc.wantRoot)
|
||||
}
|
||||
if gotRel != tc.wantRel {
|
||||
t.Errorf("rel = %q; want %q", gotRel, tc.wantRel)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestListDirectory_ParentEmptyAtFilesystemRoot(t *testing.T) {
|
||||
target := "/"
|
||||
if runtime.GOOS == "windows" {
|
||||
target = os.Getenv("SystemDrive") + `\`
|
||||
if target == `\` {
|
||||
target = `C:\`
|
||||
}
|
||||
}
|
||||
svc := &Service{}
|
||||
got, err := svc.ListDirectory(context.Background(), "/")
|
||||
got, err := svc.ListDirectory(context.Background(), target)
|
||||
if err != nil {
|
||||
t.Fatalf("ListDirectory(/): %v", err)
|
||||
t.Fatalf("ListDirectory(%q): %v", target, err)
|
||||
}
|
||||
if got.Parent != "" {
|
||||
t.Errorf("expected empty parent at /, got %q", got.Parent)
|
||||
t.Errorf("expected empty parent at %q, got %q", target, got.Parent)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -12,6 +12,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/task/models"
|
||||
)
|
||||
|
||||
@@ -213,7 +214,7 @@ func (h *DefaultScriptMessageHandler) createScriptMessage(ctx context.Context, r
|
||||
// runScriptWithOutput runs the script and captures output, streaming it to the message.
|
||||
// The passed context should already have an appropriate timeout set.
|
||||
func (h *DefaultScriptMessageHandler) runScriptWithOutput(ctx context.Context, req ScriptExecutionRequest, msg *models.Message) (int, error) {
|
||||
cmd := exec.CommandContext(ctx, "sh", "-c", req.Script)
|
||||
cmd := shellexec.CommandContext(ctx, shellexec.PosixSh, req.Script)
|
||||
cmd.Dir = req.WorkingDir
|
||||
|
||||
stdoutPipe, err := cmd.StdoutPipe()
|
||||
@@ -286,8 +287,8 @@ func (h *DefaultScriptMessageHandler) runScriptWithoutMessage(ctx context.Contex
|
||||
zap.String("script_type", req.ScriptType),
|
||||
zap.String("command", req.Script))
|
||||
|
||||
// Run script with sh -c
|
||||
cmd := exec.CommandContext(ctx, "sh", "-c", req.Script)
|
||||
// Run script under the host shell (sh -c on Unix, bash/cmd on Windows).
|
||||
cmd := shellexec.CommandContext(ctx, shellexec.PosixSh, req.Script)
|
||||
cmd.Dir = req.WorkingDir
|
||||
|
||||
// Capture output for logging
|
||||
|
||||
@@ -40,7 +40,7 @@
|
||||
"vitest": "^1.6.0"
|
||||
},
|
||||
"scripts": {
|
||||
"dev": "unset npm_config_prefix && tsx src/cli.ts",
|
||||
"dev": "tsx src/cli.ts",
|
||||
"build": "tsc -p tsconfig.json",
|
||||
"bundle": "esbuild dist/cli.js --bundle --platform=node --format=cjs --outfile=dist/cli.bundle.js",
|
||||
"start": "node dist/cli.js",
|
||||
|
||||
+39
-1
@@ -1,4 +1,5 @@
|
||||
import { spawn } from "node:child_process";
|
||||
import fs from "node:fs";
|
||||
import path from "node:path";
|
||||
|
||||
import { devKandevHome, HEALTH_TIMEOUT_MS_DEV } from "./constants";
|
||||
@@ -39,7 +40,12 @@ export async function runDev({ repoRoot, backendPort, webPort }: DevOptions): Pr
|
||||
const supervisor = createProcessSupervisor();
|
||||
supervisor.attachSignalHandlers();
|
||||
|
||||
const backendProc = spawn("make", ["-C", path.join("apps", "backend"), "dev"], {
|
||||
const { cmd: backendCmd, args: backendArgs } = withWinjobWrap(repoRoot, "make", [
|
||||
"-C",
|
||||
path.join("apps", "backend"),
|
||||
"dev",
|
||||
]);
|
||||
const backendProc = spawn(backendCmd, backendArgs, {
|
||||
cwd: repoRoot,
|
||||
env: backendEnv,
|
||||
stdio: "inherit",
|
||||
@@ -119,3 +125,35 @@ export function resolveDevBackendEnv(repoRoot: string): DevBackendEnv {
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
// withWinjobWrap on Windows prepends apps/backend/bin/winjob.exe to a spawn
|
||||
// command so the child runs inside a Job Object configured with
|
||||
// JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE. That makes "kill the whole backend
|
||||
// subtree" a kernel-level guarantee tied to winjob's process exit, instead of
|
||||
// relying on the bash → make → pnpm → node → make → sh → kandev signal chain
|
||||
// (which drops Ctrl-C at multiple links because MSYS bash, native Win32
|
||||
// processes, and Node disagree on signal propagation semantics).
|
||||
//
|
||||
// On Unix this is a passthrough — POSIX process groups already give us
|
||||
// reliable cascading termination.
|
||||
//
|
||||
// If the winjob binary isn't built yet (the user ran `make dev` before
|
||||
// `make -C apps/backend build-winjob`), we fall back to a direct spawn and
|
||||
// emit a one-line note. The supervisor's tree-kill still handles the happy
|
||||
// path; users only notice the gap if Ctrl-C drops mid-chain.
|
||||
function withWinjobWrap(
|
||||
repoRoot: string,
|
||||
cmd: string,
|
||||
args: string[],
|
||||
): { cmd: string; args: string[] } {
|
||||
if (process.platform !== "win32") return { cmd, args };
|
||||
const winjob = path.join(repoRoot, "apps", "backend", "bin", "winjob.exe");
|
||||
if (!fs.existsSync(winjob)) {
|
||||
console.warn(
|
||||
`[kandev] winjob.exe not built — Ctrl-C may leak processes on Windows. ` +
|
||||
`Run \`make -C apps/backend build-winjob\` once to enable.`,
|
||||
);
|
||||
return { cmd, args };
|
||||
}
|
||||
return { cmd: winjob, args: [cmd, ...args] };
|
||||
}
|
||||
|
||||
+37
-3
@@ -34,18 +34,52 @@ function isPortInUse(port: number, host: string): Promise<boolean> {
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Tries to bind a port on the given host. Returns true if the bind succeeds
|
||||
* (port is free). Closes immediately on success.
|
||||
*
|
||||
* This catches Windows "phantom" port reservations: Hyper-V/WSL silently
|
||||
* reserve random port ranges at boot that don't appear in netstat or via a
|
||||
* connect probe (nothing is listening, so connect-check thinks the port is
|
||||
* free) — but bind fails with "Only one usage of each socket address". A
|
||||
* connect-only check causes kandev's backend to choose a reserved port and
|
||||
* then die when it tries to actually listen on it.
|
||||
*/
|
||||
function canBindPort(port: number, host: string): Promise<boolean> {
|
||||
return new Promise((resolve) => {
|
||||
const server = net.createServer();
|
||||
server.once("error", () => resolve(false));
|
||||
server.listen(port, host, () => {
|
||||
server.close(() => resolve(true));
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Checks if a port is available by probing both IPv4 and IPv6 loopback.
|
||||
*
|
||||
* Uses a connect-based check: if we can connect to the port on either
|
||||
* 127.0.0.1 or ::1, something is already listening and the port is taken.
|
||||
* Uses BOTH a connect check and a bind check:
|
||||
* - connect: detects ports where a listener is bound with SO_REUSEADDR
|
||||
* (Node's default on macOS — bind-only check would falsely succeed)
|
||||
* - bind: detects Windows phantom reservations (Hyper-V/WSL) and
|
||||
* ports in TIME_WAIT that connect-only check misses
|
||||
*
|
||||
* The port is available IFF nobody answers a connect AND a fresh bind
|
||||
* succeeds — covers macOS, Linux, and Windows.
|
||||
*/
|
||||
async function isPortAvailable(port: number): Promise<boolean> {
|
||||
// Run connect probes first, then the bind probe — they cannot share the
|
||||
// port concurrently. On loopback, server.listen() completes in the kernel
|
||||
// before a connect SYN to the same address is processed, so a concurrent
|
||||
// canBindPort+isPortInUse pair can answer each other and report a free
|
||||
// port as occupied. Sequencing keeps the bind probe's temporary listener
|
||||
// out of the connect probes' view.
|
||||
const [v4InUse, v6InUse] = await Promise.all([
|
||||
isPortInUse(port, "127.0.0.1"),
|
||||
isPortInUse(port, "::1"),
|
||||
]);
|
||||
return !v4InUse && !v6InUse;
|
||||
if (v4InUse || v6InUse) return false;
|
||||
return canBindPort(port, "127.0.0.1");
|
||||
}
|
||||
|
||||
async function reserveSpecificPort(port: number, host = "127.0.0.1"): Promise<net.Server | null> {
|
||||
|
||||
@@ -4,6 +4,7 @@ import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
import { getBinaryName } from "./platform";
|
||||
import { attachRingBuffer, cleanOldReleases, findCachedRelease } from "./run";
|
||||
|
||||
// Mock CACHE_DIR to use a temp directory.
|
||||
@@ -27,7 +28,10 @@ const PLATFORM = "macos-arm64";
|
||||
function createFakeCachedRelease(cacheDir: string, tag: string, platform: string) {
|
||||
const binDir = path.join(cacheDir, tag, platform, "kandev", "bin");
|
||||
fs.mkdirSync(binDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(binDir, "kandev"), "fake-binary");
|
||||
// findCachedRelease looks up the binary by host-platform name (kandev on
|
||||
// Unix, kandev.exe on Windows) via getBinaryName, so the fixture must match
|
||||
// or the lookup never resolves on Windows.
|
||||
fs.writeFileSync(path.join(binDir, getBinaryName("kandev")), "fake-binary");
|
||||
}
|
||||
|
||||
describe("findCachedRelease", () => {
|
||||
|
||||
+20
-1
@@ -3,6 +3,24 @@ import { readFileSync } from "node:fs";
|
||||
|
||||
import { createProcessSupervisor } from "./process";
|
||||
|
||||
// Node CLIs that install as .cmd shims on Windows. Anything in this set goes
|
||||
// through cmd.exe /c on win32 so we can spawn the shim safely:
|
||||
// - Node's spawn doesn't apply PATHEXT, so spawn("pnpm", …) hits ENOENT.
|
||||
// - spawn("pnpm.cmd", …) directly returns EINVAL on Node 21+ (the
|
||||
// CVE-2024-27980 fix forbids direct .bat/.cmd spawning).
|
||||
// - spawn(…, {shell:true}) works but trips DEP0190 (the args-aren't-escaped
|
||||
// security warning) and forks an extra cmd.exe per spawn.
|
||||
// Going through cmd.exe explicitly is the cleanest option: no warning, no
|
||||
// EINVAL, and the args (-C, --filter, identifier-like strings) contain no
|
||||
// cmd metacharacters that need extra escaping.
|
||||
const WIN_SHIM_COMMANDS = new Set(["pnpm", "npm", "npx", "yarn"]);
|
||||
|
||||
function resolveWindowsShim(command: string, args: string[]): { command: string; args: string[] } {
|
||||
if (process.platform !== "win32") return { command, args };
|
||||
if (!WIN_SHIM_COMMANDS.has(command)) return { command, args };
|
||||
return { command: "cmd.exe", args: ["/c", command, ...args] };
|
||||
}
|
||||
|
||||
let _isWSL: boolean | undefined;
|
||||
function isWSL(): boolean {
|
||||
if (_isWSL === undefined) {
|
||||
@@ -52,7 +70,8 @@ export function launchWebApp({
|
||||
quiet = false,
|
||||
}: WebLaunchOptions): ChildProcess {
|
||||
const stdio: StdioOptions = quiet ? ["ignore", "pipe", "pipe"] : "inherit";
|
||||
const proc = spawn(command, args, { cwd, env, stdio });
|
||||
const resolved = resolveWindowsShim(command, args);
|
||||
const proc = spawn(resolved.command, resolved.args, { cwd, env, stdio });
|
||||
supervisor.children.push(proc);
|
||||
|
||||
// In quiet mode, only forward stderr
|
||||
|
||||
@@ -2,6 +2,10 @@
|
||||
"name": "kandev-monorepo",
|
||||
"version": "0.1.0",
|
||||
"private": true,
|
||||
"packageManager": "pnpm@9.15.9",
|
||||
"engines": {
|
||||
"node": "^24"
|
||||
},
|
||||
"scripts": {
|
||||
"dev": "pnpm -r run dev",
|
||||
"dev:web": "pnpm --filter @kandev/web dev",
|
||||
|
||||
@@ -8,7 +8,9 @@ import {
|
||||
IconClipboard,
|
||||
IconDownload,
|
||||
IconExternalLink,
|
||||
IconLoader2,
|
||||
IconPlus,
|
||||
IconRefresh,
|
||||
IconTerminal2,
|
||||
} from "@tabler/icons-react";
|
||||
import { Badge } from "@kandev/ui/badge";
|
||||
@@ -249,7 +251,12 @@ function InstalledAgentsSection({
|
||||
disabled={rescanning}
|
||||
className="cursor-pointer"
|
||||
>
|
||||
{rescanning ? "Rescanning..." : "Rescan"}
|
||||
{rescanning ? (
|
||||
<IconLoader2 className="h-4 w-4 mr-2 animate-spin" />
|
||||
) : (
|
||||
<IconRefresh className="h-4 w-4 mr-2" />
|
||||
)}
|
||||
Rescan
|
||||
</Button>
|
||||
</div>
|
||||
</div>
|
||||
@@ -468,7 +475,9 @@ function useAgentPageState() {
|
||||
availableAgents.find((item: AvailableAgent) => item.name === name)?.model_config?.status;
|
||||
|
||||
const handleRescan = async () => {
|
||||
if (rescanning) return;
|
||||
if (rescanning) {
|
||||
return;
|
||||
}
|
||||
setRescanning(true);
|
||||
try {
|
||||
const [discoveryResp, availableResp] = await Promise.all([
|
||||
|
||||
Reference in New Issue
Block a user