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