From fc67b21ccdf54b981767f87f6c5efdcc2f347dce Mon Sep 17 00:00:00 2001 From: Allison Chiang Date: Tue, 9 Jun 2026 14:32:28 -0400 Subject: [PATCH 1/3] init --- go.mod | 6 +++- services/shell/builtin/builtin.go | 59 +++++++++++++++++++------------ 2 files changed, 42 insertions(+), 23 deletions(-) diff --git a/go.mod b/go.mod index 45b9c5d02f9..599f9e1ecfa 100644 --- a/go.mod +++ b/go.mod @@ -20,8 +20,8 @@ require ( github.com/bufbuild/buf v1.30.0 github.com/charmbracelet/huh v0.8.0 github.com/charmbracelet/huh/spinner v0.0.0-20240917123815-c9b2c9cdb7b6 + github.com/charmbracelet/x/xpty v0.1.2 github.com/chenzhekl/goply v0.0.0-20190930133256-258c2381defd - github.com/creack/pty v1.1.24 github.com/disintegration/imaging v1.6.2 github.com/docker/go-units v0.5.0 github.com/edaniels/gobag v1.0.7-0.20220607183102-4242cd9e2848 @@ -181,8 +181,11 @@ require ( github.com/charmbracelet/lipgloss v1.1.0 // indirect github.com/charmbracelet/x/ansi v0.9.3 // indirect github.com/charmbracelet/x/cellbuf v0.0.13 // indirect + github.com/charmbracelet/x/conpty v0.1.0 // indirect + github.com/charmbracelet/x/errors v0.0.0-20240508181413-e8d8b6e2de86 // indirect github.com/charmbracelet/x/exp/strings v0.0.0-20240722160745-212f7b056ed0 // indirect github.com/charmbracelet/x/term v0.2.1 // indirect + github.com/charmbracelet/x/termios v0.1.1 // indirect github.com/chewxy/hm v1.0.0 // indirect github.com/chewxy/math32 v1.0.8 // indirect github.com/clipperhouse/stringish v0.1.1 // indirect @@ -191,6 +194,7 @@ require ( github.com/containerd/console v1.0.5 // indirect github.com/containerd/stargz-snapshotter/estargz v0.15.1 // indirect github.com/cpuguy83/go-md2man/v2 v2.0.7 // indirect + github.com/creack/pty v1.1.24 // indirect github.com/cyphar/filepath-securejoin v0.4.1 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/decred/dcrd/dcrec/secp256k1/v4 v4.2.0 // indirect diff --git a/services/shell/builtin/builtin.go b/services/shell/builtin/builtin.go index 78cb45783f0..e0e1636ff5d 100644 --- a/services/shell/builtin/builtin.go +++ b/services/shell/builtin/builtin.go @@ -11,7 +11,7 @@ import ( "sync" "time" - "github.com/creack/pty" + "github.com/charmbracelet/x/xpty" "go.viam.com/utils" "go.viam.com/rdk/logging" @@ -48,22 +48,38 @@ type builtIn struct { func (svc *builtIn) Shell(ctx context.Context, extra map[string]interface{}) ( chan<- string, chan<- map[string]interface{}, <-chan shell.Output, error, ) { + ctxCancel, cancel := context.WithCancel(ctx) + + // Pick the platform default interactive shell. + shellPath := "/bin/sh" + shellArgs := []string{"-i"} + shellEnv := []string{"TERM=xterm-256color"} if runtime.GOOS == "windows" { - return nil, nil, nil, errors.New("shell not supported on windows yet; sorry") + // powershell is preferred when present; fall back to cmd.exe. Inherit the parent + // environment (nil) so the shell has PATH, SystemRoot, etc. + shellPath = "cmd.exe" + if ps, lookErr := exec.LookPath("powershell.exe"); lookErr == nil { + shellPath = ps + } + shellArgs = nil + shellEnv = nil + } else if sh, ok := os.LookupEnv("SHELL"); ok { + shellPath = sh } - defaultShellPath, ok := os.LookupEnv("SHELL") - if !ok { - defaultShellPath = "/bin/sh" + // xpty allocates a unix pty or a Windows ConPTY behind one interface. Start with a + // default size; the real window size arrives via the first window-change OOB message. + pty, err := xpty.NewPty(80, 24) + if err != nil { + cancel() + return nil, nil, nil, err } - - ctxCancel, cancel := context.WithCancel(ctx) //nolint:gosec - cmd := exec.CommandContext(ctxCancel, defaultShellPath, "-i") - cmd.Env = []string{"TERM=xterm-256color"} - f, err := pty.Start(cmd) - if err != nil { + cmd := exec.Command(shellPath, shellArgs...) + cmd.Env = shellEnv + if err := pty.Start(cmd); err != nil { cancel() + utils.UncheckedError(pty.Close()) return nil, nil, nil, err } @@ -92,10 +108,7 @@ func (svc *builtIn) Shell(ctx context.Context, extra map[string]interface{}) ( return } lastSet = time.Now() - if err := pty.Setsize(f, &pty.Winsize{ - Rows: uint16(rows), - Cols: uint16(cols), - }); err != nil { + if err := pty.Resize(int(cols), int(rows)); err != nil { svc.logger.CErrorw(ctx, "error setting pty window size", "error", err) } default: @@ -121,10 +134,12 @@ func (svc *builtIn) Shell(ctx context.Context, extra map[string]interface{}) ( utils.PanicCapturingGo(func() { defer svc.activeBackgroundWorkers.Done() defer cancel() - if err := cmd.Wait(); err != nil { - svc.logger.CDebugw(ctx, "error waiting for cmd", "error", err) + // WaitProcess blocks until the shell exits, and kills it if ctxCancel is cancelled + // first (it also works around the Go runtime not reaping ConPTY processes on Windows). + if err := xpty.WaitProcess(ctxCancel, cmd); err != nil { + svc.logger.CDebugw(ctx, "error waiting for shell process", "error", err) } - if err := f.Close(); err != nil { + if err := pty.Close(); err != nil { svc.logger.CDebugw(ctx, "error closing pty", "error", err) } }) @@ -142,7 +157,7 @@ func (svc *builtIn) Shell(ctx context.Context, extra map[string]interface{}) ( return default: } - n, err := f.Read(data[:]) + n, err := pty.Read(data[:]) if err != nil { if !errors.Is(err, io.EOF) && !errors.Is(err, os.ErrClosed) { svc.logger.CErrorw(ctx, "error reading output", "error", err) @@ -170,19 +185,19 @@ func (svc *builtIn) Shell(ctx context.Context, extra map[string]interface{}) ( select { case inputData, ok := <-input: if ok { - if _, err := f.WriteString(inputData); err != nil { + if _, err := pty.Write([]byte(inputData)); err != nil { svc.logger.CErrorw(ctx, "error writing data", "error", err) return } } else { - if _, err := f.Write([]byte{4}); err != nil { + if _, err := pty.Write([]byte{4}); err != nil { svc.logger.CErrorw(ctx, "error writing EOT", "error", err) return } return } case <-ctx.Done(): - if _, err := f.Write([]byte{4}); err != nil { + if _, err := pty.Write([]byte{4}); err != nil { svc.logger.CErrorw(ctx, "error writing EOT", "error", err) return } From 51d3d3ed25e42bdd284bc3d5fa2ad0d98af5f730 Mon Sep 17 00:00:00 2001 From: Allison Chiang Date: Tue, 9 Jun 2026 16:05:27 -0400 Subject: [PATCH 2/3] Update builtin.go --- services/shell/builtin/builtin.go | 45 +++++++++++++++---------------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/services/shell/builtin/builtin.go b/services/shell/builtin/builtin.go index e0e1636ff5d..9261aa41ab3 100644 --- a/services/shell/builtin/builtin.go +++ b/services/shell/builtin/builtin.go @@ -48,38 +48,35 @@ type builtIn struct { func (svc *builtIn) Shell(ctx context.Context, extra map[string]interface{}) ( chan<- string, chan<- map[string]interface{}, <-chan shell.Output, error, ) { - ctxCancel, cancel := context.WithCancel(ctx) - - // Pick the platform default interactive shell. - shellPath := "/bin/sh" + defaultShellPath, ok := os.LookupEnv("SHELL") + if !ok { + defaultShellPath = "/bin/sh" + } shellArgs := []string{"-i"} shellEnv := []string{"TERM=xterm-256color"} if runtime.GOOS == "windows" { - // powershell is preferred when present; fall back to cmd.exe. Inherit the parent - // environment (nil) so the shell has PATH, SystemRoot, etc. - shellPath = "cmd.exe" + // powershell when available, else cmd.exe; inherit the parent environment (nil). + defaultShellPath = "cmd.exe" if ps, lookErr := exec.LookPath("powershell.exe"); lookErr == nil { - shellPath = ps + defaultShellPath = ps } shellArgs = nil shellEnv = nil - } else if sh, ok := os.LookupEnv("SHELL"); ok { - shellPath = sh } - // xpty allocates a unix pty or a Windows ConPTY behind one interface. Start with a - // default size; the real window size arrives via the first window-change OOB message. - pty, err := xpty.NewPty(80, 24) + ctxCancel, cancel := context.WithCancel(ctx) + //nolint:gosec + cmd := exec.Command(defaultShellPath, shellArgs...) + cmd.Env = shellEnv + // xpty gives a unix pty or a Windows ConPTY behind one interface. + f, err := xpty.NewPty(80, 24) if err != nil { cancel() return nil, nil, nil, err } - //nolint:gosec - cmd := exec.Command(shellPath, shellArgs...) - cmd.Env = shellEnv - if err := pty.Start(cmd); err != nil { + if err := f.Start(cmd); err != nil { cancel() - utils.UncheckedError(pty.Close()) + utils.UncheckedError(f.Close()) return nil, nil, nil, err } @@ -108,7 +105,7 @@ func (svc *builtIn) Shell(ctx context.Context, extra map[string]interface{}) ( return } lastSet = time.Now() - if err := pty.Resize(int(cols), int(rows)); err != nil { + if err := f.Resize(int(cols), int(rows)); err != nil { svc.logger.CErrorw(ctx, "error setting pty window size", "error", err) } default: @@ -139,7 +136,7 @@ func (svc *builtIn) Shell(ctx context.Context, extra map[string]interface{}) ( if err := xpty.WaitProcess(ctxCancel, cmd); err != nil { svc.logger.CDebugw(ctx, "error waiting for shell process", "error", err) } - if err := pty.Close(); err != nil { + if err := f.Close(); err != nil { svc.logger.CDebugw(ctx, "error closing pty", "error", err) } }) @@ -157,7 +154,7 @@ func (svc *builtIn) Shell(ctx context.Context, extra map[string]interface{}) ( return default: } - n, err := pty.Read(data[:]) + n, err := f.Read(data[:]) if err != nil { if !errors.Is(err, io.EOF) && !errors.Is(err, os.ErrClosed) { svc.logger.CErrorw(ctx, "error reading output", "error", err) @@ -185,19 +182,19 @@ func (svc *builtIn) Shell(ctx context.Context, extra map[string]interface{}) ( select { case inputData, ok := <-input: if ok { - if _, err := pty.Write([]byte(inputData)); err != nil { + if _, err := f.Write([]byte(inputData)); err != nil { svc.logger.CErrorw(ctx, "error writing data", "error", err) return } } else { - if _, err := pty.Write([]byte{4}); err != nil { + if _, err := f.Write([]byte{4}); err != nil { svc.logger.CErrorw(ctx, "error writing EOT", "error", err) return } return } case <-ctx.Done(): - if _, err := pty.Write([]byte{4}); err != nil { + if _, err := f.Write([]byte{4}); err != nil { svc.logger.CErrorw(ctx, "error writing EOT", "error", err) return } From 1e7adec71145a857fba87023254bc906107b157d Mon Sep 17 00:00:00 2001 From: Allison Chiang Date: Thu, 11 Jun 2026 15:13:31 -0400 Subject: [PATCH 3/3] Update builtin.go --- services/shell/builtin/builtin.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/services/shell/builtin/builtin.go b/services/shell/builtin/builtin.go index 9261aa41ab3..402e5f8d644 100644 --- a/services/shell/builtin/builtin.go +++ b/services/shell/builtin/builtin.go @@ -131,8 +131,6 @@ func (svc *builtIn) Shell(ctx context.Context, extra map[string]interface{}) ( utils.PanicCapturingGo(func() { defer svc.activeBackgroundWorkers.Done() defer cancel() - // WaitProcess blocks until the shell exits, and kills it if ctxCancel is cancelled - // first (it also works around the Go runtime not reaping ConPTY processes on Windows). if err := xpty.WaitProcess(ctxCancel, cmd); err != nil { svc.logger.CDebugw(ctx, "error waiting for shell process", "error", err) }