From 30963d08e52b38a4ab9e96909fe184c056a4cd31 Mon Sep 17 00:00:00 2001 From: cjimti Date: Mon, 3 Aug 2026 10:19:56 -0700 Subject: [PATCH 1/2] Add the PTY service behind the embedded terminal internal/pty owns session lifecycle, scrollback and platform termination; internal/app composes it and drains it on shutdown. Uses aymanbagabas/go-pty on both platforms rather than splitting creack/pty and go-pty: it wraps creack on Unix and owns process creation, so gosec G204 never fires and no suppression is needed. Raises three structural ratchets with justification in the diff. Closes #2 --- .github/workflows/ci.yml | 9 + go.mod | 7 +- go.sum | 18 + godobject_budget_test.go | 8 +- internal/app/app.go | 22 +- internal/app/app_test.go | 38 ++ internal/pty/hangup_unix.go | 42 +++ internal/pty/hangup_windows.go | 28 ++ internal/pty/manager.go | 129 +++++++ internal/pty/pty.go | 173 +++++++++ internal/pty/pty_test.go | 644 +++++++++++++++++++++++++++++++++ internal/pty/ring.go | 57 +++ internal/pty/ring_test.go | 115 ++++++ internal/pty/session.go | 225 ++++++++++++ package_budget_test.go | 21 +- package_graph_test.go | 15 +- 16 files changed, 1539 insertions(+), 12 deletions(-) create mode 100644 internal/pty/hangup_unix.go create mode 100644 internal/pty/hangup_windows.go create mode 100644 internal/pty/manager.go create mode 100644 internal/pty/pty.go create mode 100644 internal/pty/pty_test.go create mode 100644 internal/pty/ring.go create mode 100644 internal/pty/ring_test.go create mode 100644 internal/pty/session.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 20a8742..8eee1ef 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -176,6 +176,15 @@ jobs: - name: Verify dependencies run: go mod verify + # The PTY service is the one package whose behaviour is genuinely + # per-platform: creack/pty on Unix, ConPTY on Windows, and a termination + # path that has no SIGHUP to send on Windows at all. The Test job above + # only ever exercises the Linux half, so a ConPTY regression would reach + # a release having passed every gate. This runs the same tests `make + # test` runs, on each OS in the matrix. + - name: Smoke-test the PTY service on this platform + run: go test -race -shuffle=on -count=1 ./internal/pty/... + - name: Install the Wails CLI run: go install github.com/wailsapp/wails/v2/cmd/wails@v2.13.0 diff --git a/go.mod b/go.mod index 5ba73e2..467f001 100644 --- a/go.mod +++ b/go.mod @@ -6,11 +6,15 @@ go 1.26.5 // otherwise pull into this module's build, test, coverage and lint denominator. ignore frontend/node_modules -require github.com/wailsapp/wails/v2 v2.13.0 +require ( + github.com/aymanbagabas/go-pty v0.2.3 + github.com/wailsapp/wails/v2 v2.13.0 +) require ( git.sr.ht/~jackmordaunt/go-toast/v2 v2.0.3 // indirect github.com/bep/debounce v1.2.1 // indirect + github.com/creack/pty v1.1.24 // indirect github.com/go-ole/go-ole v1.3.0 // indirect github.com/godbus/dbus/v5 v5.1.0 // indirect github.com/google/uuid v1.6.0 // indirect @@ -29,6 +33,7 @@ require ( github.com/rivo/uniseg v0.4.7 // indirect github.com/samber/lo v1.49.1 // indirect github.com/tkrajina/go-reflector v0.5.8 // indirect + github.com/u-root/u-root v0.16.0 // indirect github.com/valyala/bytebufferpool v1.0.0 // indirect github.com/valyala/fasttemplate v1.2.2 // indirect github.com/wailsapp/go-webview2 v1.0.22 // indirect diff --git a/go.sum b/go.sum index b664a33..589108a 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,11 @@ git.sr.ht/~jackmordaunt/go-toast/v2 v2.0.3 h1:N3IGoHHp9pb6mj1cbXbuaSXV/UMKwmbKLf53nQmtqMA= git.sr.ht/~jackmordaunt/go-toast/v2 v2.0.3/go.mod h1:QtOLZGz8olr4qH2vWK0QH0w0O4T9fEIjMuWpKUsH7nc= +github.com/aymanbagabas/go-pty v0.2.3 h1:hsqcTIUV8I4iTSh3HQl61CR2wh0YPS6gHOYLhAfWu/E= +github.com/aymanbagabas/go-pty v0.2.3/go.mod h1:GLkgQovzqN5A1xMB79yHWiG1rhcquZCjkwKQGKFPdPg= github.com/bep/debounce v1.2.1 h1:v67fRdBA9UQu2NhLFXrSg0Brw7CexQekrBwDMM8bzeY= github.com/bep/debounce v1.2.1/go.mod h1:H8yggRPQKLUhUoqrJC1bO2xNya7vanpDl7xR3ISbCJ0= +github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= +github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/go-ole/go-ole v1.3.0 h1:Dt6ye7+vXGIKZ7Xtk4s6/xVdGDQynvom7xCFEdWr6uE= @@ -12,6 +16,8 @@ github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/gorilla/websocket v1.5.3 h1:saDtZ6Pbx/0u+bgYQ3q96pZgCzfhKXGPqt7kZ72aNNg= github.com/gorilla/websocket v1.5.3/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE= +github.com/hugelgupf/vmtest v0.0.0-20240307030256-5d9f3d34a58d h1:nP8SfQJqruIVSWYJTuYc37jLHEY1Z0fF+zKSrs3K/C8= +github.com/hugelgupf/vmtest v0.0.0-20240307030256-5d9f3d34a58d/go.mod h1:B63hDJMhTupLWCHwopAyEo7wRFowx9kOc8m8j1sfOqE= github.com/jchv/go-winloader v0.0.0-20210711035445-715c2860da7e h1:Q3+PugElBCf4PFpxhErSzU3/PY5sFL5Z6rfv4AbGAck= github.com/jchv/go-winloader v0.0.0-20210711035445-715c2860da7e/go.mod h1:alcuEEnZsY1WQsagKhZDsoPCRoOijYqhZvPwLG0kzVs= github.com/labstack/echo/v4 v4.13.3 h1:pwhpCPrTl5qry5HRdM5FwdXnhXSLSY+WE+YQSeCaafY= @@ -51,6 +57,10 @@ github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/tkrajina/go-reflector v0.5.8 h1:yPADHrwmUbMq4RGEyaOUpz2H90sRsETNVpjzo3DLVQQ= github.com/tkrajina/go-reflector v0.5.8/go.mod h1:ECbqLgccecY5kPmPmXg1MrHW585yMcDkVl6IvJe64T4= +github.com/u-root/gobusybox/src v0.0.0-20250101170133-2e884e4509c7 h1:dtiVT4SeBUc/vHtwI2HjDZN+FCKTstQBxugIxJEGo9g= +github.com/u-root/gobusybox/src v0.0.0-20250101170133-2e884e4509c7/go.mod h1:PW3wGFCHjdHxAhra5FKvcARbCGqGfentYuPKmuhv8DY= +github.com/u-root/u-root v0.16.0 h1:wY40O83MBVks97+Is0WlFlOPSwKQMIrWP9R1IsrExg8= +github.com/u-root/u-root v0.16.0/go.mod h1:yL/XdSSW27PdGLgUh4MNRBy54mKM+TBLzpwiB4nwj90= github.com/valyala/bytebufferpool v1.0.0 h1:GqA5TC/0021Y/b9FG4Oi9Mr3q7XYx6KllzawFIhcdPw= github.com/valyala/bytebufferpool v1.0.0/go.mod h1:6bBcMArwyJ5K/AmCkWv1jt77kVWyCJ6HpOuEn7z0Csc= github.com/valyala/fasttemplate v1.2.2 h1:lxLXG0uE3Qnshl9QyaK6XJxMXlQZELvChBOCmQD0Loo= @@ -63,9 +73,13 @@ github.com/wailsapp/wails/v2 v2.13.0 h1:S7OgXWpj72V91unF8iDWJKbcS9ZpwCT3R0QVru4v github.com/wailsapp/wails/v2 v2.13.0/go.mod h1:nVr/wSIEZ7xxKPkzK65mjpKpaOPQI2k4pvLwGR/i4kc= golang.org/x/crypto v0.51.0 h1:IBPXwPfKxY7cWQZ38ZCIRPI50YLeevDLlLnyC5wRGTI= golang.org/x/crypto v0.51.0/go.mod h1:8AdwkbraGNABw2kOX6YFPs3WM22XqI4EXEd8g+x7Oc8= +golang.org/x/mod v0.35.0 h1:Ww1D637e6Pg+Zb2KrWfHQUnH2dQRLBQyAtpr/haaJeM= +golang.org/x/mod v0.35.0/go.mod h1:+GwiRhIInF8wPm+4AoT6L0FA1QWAad3OMdTRx4tFYlU= golang.org/x/net v0.0.0-20210505024714-0287a6fb4125/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= golang.org/x/net v0.54.0 h1:2zJIZAxAHV/OHCDTCOHAYehQzLfSXuf/5SoL/Dv6w/w= golang.org/x/net v0.54.0/go.mod h1:Sj4oj8jK6XmHpBZU/zWHw3BV3abl4Kvi+Ut7cQcY+cQ= +golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= +golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.0.0-20200810151505-1b9f1253b3ed/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= @@ -75,9 +89,13 @@ golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.44.0 h1:ildZl3J4uzeKP07r2F++Op7E9B29JRUy+a27EibtBTQ= golang.org/x/sys v0.44.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= +golang.org/x/term v0.43.0 h1:S4RLU2sB31O/NCl+zFN9Aru9A/Cq2aqKpTZJ6B+DwT4= +golang.org/x/term v0.43.0/go.mod h1:lrhlHNdQJHO+1qVYiHfFKVuVioJIheAc3fBSMFYEIsk= golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.37.0 h1:Cqjiwd9eSg8e0QAkyCaQTNHFIIzWtidPahFWR83rTrc= golang.org/x/text v0.37.0/go.mod h1:a5sjxXGs9hsn/AJVwuElvCAo9v8QYLzvavO5z2PiM38= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= +golang.org/x/tools v0.44.0 h1:UP4ajHPIcuMjT1GqzDWRlalUEoY+uzoZKnhOjbIPD2c= +golang.org/x/tools v0.44.0/go.mod h1:KA0AfVErSdxRZIsOVipbv3rQhVXTnlU6UhKxHd1seDI= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/godobject_budget_test.go b/godobject_budget_test.go index b8ddfd1..e0ba952 100644 --- a/godobject_budget_test.go +++ b/godobject_budget_test.go @@ -27,7 +27,13 @@ const ( // loose fields where one owner struct belonged. If raising it by more than // one per service, the question to answer in review is why the service is // not one handle. - maxAppFields = 1 + // + // 1 -> 2 in #2: the PTY service arrives as a single *pty.Manager handle. + // Every terminal behavior — create, write, resize, kill, scrollback — + // lives in internal/pty and is reached through that one field, so this is + // the one-handle-per-service case the paragraph above describes, not + // state accumulating on the coordinator. + maxAppFields = 2 // maxAppMethods caps methods with an App receiver, counting value and // pointer receivers alike. Pinned at today's actual with zero slack. diff --git a/internal/app/app.go b/internal/app/app.go index 3e3d79f..91bd5b4 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -5,12 +5,14 @@ package app import ( + "context" "embed" "github.com/wailsapp/wails/v2/pkg/options" "github.com/wailsapp/wails/v2/pkg/options/assetserver" "github.com/txn2/m6t/internal/buildinfo" + "github.com/txn2/m6t/internal/pty" ) // Window geometry. m6t is a single-window app: the three-pane project @@ -34,13 +36,19 @@ var windowBackground = &options.RGBA{R: 22, G: 24, B: 29, A: 1} // asset server) out of it. type App struct { info buildinfo.Info + + // terminals owns the PTY sessions behind the embedded terminal. It is one + // handle rather than loose state because the app composes services, it + // does not implement them: everything the terminal does lives in + // internal/pty and is reached through here. + terminals *pty.Manager } // newApp builds the binding. It is unexported because Options is the only // supported way to construct the application: an App that is not bound into // the window options is unreachable from the frontend. func newApp() *App { - return &App{info: buildinfo.Get()} + return &App{info: buildinfo.Get(), terminals: pty.New()} } // Version reports the build identity to the frontend, which shows it in the @@ -54,6 +62,8 @@ func (a *App) Version() buildinfo.Info { // bound methods, embedded assets — is covered by tests rather than asserted in // a comment. func Options(assets embed.FS) *options.App { + application := newApp() + return &options.App{ Title: windowTitle, Width: windowWidth, @@ -62,6 +72,14 @@ func Options(assets embed.FS) *options.App { MinHeight: windowMinHeight, AssetServer: &assetserver.Options{Assets: assets}, BackgroundColour: windowBackground, - Bind: []any{newApp()}, + Bind: []any{application}, + + // PTYs are backend-owned and outlive every window in the app, so the + // only thing that ends them is the app ending. Without this hook, + // quitting m6t would leave the user's shells — and whatever they were + // running — orphaned behind it. + OnShutdown: func(context.Context) { + application.terminals.Shutdown() + }, } } diff --git a/internal/app/app_test.go b/internal/app/app_test.go index d5f9748..2dc2c6d 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -1,10 +1,14 @@ package app import ( + "context" "embed" + "errors" + "runtime" "testing" "github.com/txn2/m6t/internal/buildinfo" + "github.com/txn2/m6t/internal/pty" ) //go:embed testdata @@ -75,6 +79,40 @@ func TestOptionsBindOnlyTheApp(t *testing.T) { } } +// PTYs are backend-owned and outlive every window, so the app quitting is the +// only thing that ends them. Without the shutdown hook, closing m6t would +// orphan the user's shells and whatever they were running. +func TestShutdownTerminatesEveryTerminalSession(t *testing.T) { + opts := Options(testAssets) + + application, ok := opts.Bind[0].(*App) + if !ok { + t.Fatalf("Bind[0] is %T, want *App", opts.Bind[0]) + } + if opts.OnShutdown == nil { + t.Fatal("OnShutdown is nil; quitting the app would leave its PTY sessions running") + } + + id, err := application.terminals.Create(pty.Options{Command: longRunningCommand()}) + if err != nil { + t.Fatalf("creating a terminal session: %v", err) + } + + opts.OnShutdown(context.Background()) + + if _, err := application.terminals.Attach(id); !errors.Is(err, pty.ErrNoSuchSession) { + t.Errorf("session %s survived shutdown: Attach error = %v, want ErrNoSuchSession", id, err) + } +} + +// longRunningCommand returns an argv that stays alive until it is killed. +func longRunningCommand() []string { + if runtime.GOOS == "windows" { + return []string{"cmd.exe", "/c", "ping -n 61 127.0.0.1 >NUL"} + } + return []string{"/bin/sh", "-c", "sleep 60"} +} + func TestEachCallBuildsItsOwnBinding(t *testing.T) { first := Options(testAssets).Bind[0] second := Options(testAssets).Bind[0] diff --git a/internal/pty/hangup_unix.go b/internal/pty/hangup_unix.go new file mode 100644 index 0000000..c48d983 --- /dev/null +++ b/internal/pty/hangup_unix.go @@ -0,0 +1,42 @@ +//go:build !windows + +package pty + +import ( + "fmt" + "os" + "syscall" +) + +// hangup asks the child to end the way closing a terminal window does, giving +// a shell the chance to run its exit traps. +func hangup(proc *os.Process) error { + return signalGroup(proc, syscall.SIGHUP) +} + +// forceKill ends the child unconditionally. It is the follow-up for a child +// that ignored the hangup. +func forceKill(proc *os.Process) error { + return signalGroup(proc, syscall.SIGKILL) +} + +// signalGroup signals the child's whole process group, falling back to the +// child alone if the group cannot be determined. +// +// The group is the point. go-pty starts the child in its own session, so a +// shell that has spawned `claude` or `vim` is the leader of a group containing +// them. Signaling only the shell would reap the shell and orphan everything +// it started — which is exactly the "no zombie processes" failure this is +// written to avoid. +func signalGroup(proc *os.Process, sig syscall.Signal) error { + if pgid, err := syscall.Getpgid(proc.Pid); err == nil { + if err := syscall.Kill(-pgid, sig); err != nil { + return fmt.Errorf("signaling process group %d: %w", pgid, err) + } + return nil + } + if err := proc.Signal(sig); err != nil { + return fmt.Errorf("signaling process %d: %w", proc.Pid, err) + } + return nil +} diff --git a/internal/pty/hangup_windows.go b/internal/pty/hangup_windows.go new file mode 100644 index 0000000..74a3df1 --- /dev/null +++ b/internal/pty/hangup_windows.go @@ -0,0 +1,28 @@ +//go:build windows + +package pty + +import ( + "fmt" + "os" +) + +// hangup ends the child. +// +// Windows has no SIGHUP. A console application is ended by closing its +// pseudoconsole or by terminating the process outright, and go-pty owns the +// pseudoconsole handle, so termination is the honest option here. The +// consequence is that the killGrace window in session.kill buys a Windows +// child nothing — it is already gone when the grace period would have started. +func hangup(proc *os.Process) error { + if err := proc.Kill(); err != nil { + return fmt.Errorf("terminating process %d: %w", proc.Pid, err) + } + return nil +} + +// forceKill ends the child unconditionally. On Windows there is nothing +// gentler than hangup to escalate from, so the two are the same call. +func forceKill(proc *os.Process) error { + return hangup(proc) +} diff --git a/internal/pty/manager.go b/internal/pty/manager.go new file mode 100644 index 0000000..98ee771 --- /dev/null +++ b/internal/pty/manager.go @@ -0,0 +1,129 @@ +package pty + +import ( + "fmt" + "strconv" + "sync" +) + +// Manager owns the live PTY sessions. m6t holds one for the whole +// application: PTYs are backend-owned and survive project-tab switches +// (DESIGN.md §3.2), so their lifetime is the app's, not any window's. +// +// A Manager is safe for concurrent use. +type Manager struct { + mu sync.Mutex + sessions map[SessionID]*session + lastID uint64 +} + +// New returns a Manager holding no sessions. +func New() *Manager { + return &Manager{sessions: make(map[SessionID]*session)} +} + +// Create starts a session and returns its identifier. The session runs until +// its child exits, and stays registered after that until Kill or Shutdown +// drops it. +func (m *Manager) Create(opts Options) (SessionID, error) { + s, err := start(opts) + if err != nil { + return "", err + } + + m.mu.Lock() + m.lastID++ + id := SessionID("pty-" + strconv.FormatUint(m.lastID, 10)) + m.sessions[id] = s + m.mu.Unlock() + + go s.run() + + return id, nil +} + +// Attach returns a consumer's view of a session: its scrollback, its +// subsequent output and its exit status. +func (m *Manager) Attach(id SessionID) (Attachment, error) { + s, err := m.lookup(id) + if err != nil { + return Attachment{}, err + } + return s.attach(), nil +} + +// Write sends input to a session's child. +func (m *Manager) Write(id SessionID, p []byte) error { + s, err := m.lookup(id) + if err != nil { + return err + } + return s.write(p) +} + +// Resize changes the window size a session's child sees. A zero dimension is +// replaced by the default rather than passed through, because a terminal of +// zero columns is not a size any child can render at. +func (m *Manager) Resize(id SessionID, cols, rows uint16) error { + s, err := m.lookup(id) + if err != nil { + return err + } + width, height := size(cols, rows) + return s.resize(width, height) +} + +// Kill terminates a session's child and forgets the session. It returns once +// the child has been reaped. Killing an already-exited session is valid and +// simply drops it. +func (m *Manager) Kill(id SessionID) error { + m.mu.Lock() + s, ok := m.sessions[id] + delete(m.sessions, id) + m.mu.Unlock() + + if !ok { + return fmt.Errorf("session %s: %w", id, ErrNoSuchSession) + } + s.kill() + return nil +} + +// Shutdown terminates every session and returns once they have all been +// reaped. It is what the application calls on the way out, and the reason +// closing m6t does not leave orphaned shells behind. +// +// Sessions are killed concurrently: each one may wait out killGrace, and +// serializing that would multiply the app's shutdown time by the number of +// open terminals. +func (m *Manager) Shutdown() { + m.mu.Lock() + live := make([]*session, 0, len(m.sessions)) + for id, s := range m.sessions { + live = append(live, s) + delete(m.sessions, id) + } + m.mu.Unlock() + + var wg sync.WaitGroup + wg.Add(len(live)) + for _, s := range live { + go func() { + defer wg.Done() + s.kill() + }() + } + wg.Wait() +} + +// lookup resolves an identifier to its session. +func (m *Manager) lookup(id SessionID) (*session, error) { + m.mu.Lock() + defer m.mu.Unlock() + + s, ok := m.sessions[id] + if !ok { + return nil, fmt.Errorf("session %s: %w", id, ErrNoSuchSession) + } + return s, nil +} diff --git a/internal/pty/pty.go b/internal/pty/pty.go new file mode 100644 index 0000000..de1ecc1 --- /dev/null +++ b/internal/pty/pty.go @@ -0,0 +1,173 @@ +// Package pty owns the pseudo-terminal sessions behind m6t's embedded +// terminal (DESIGN.md §3.2, §8). It spawns the user's real login shell +// attached to a real PTY, keeps a bounded scrollback per session, and hands +// output to whichever consumers are attached. +// +// The package knows nothing about transports, terminal tabs or the Wails +// bridge. It takes an argv slice and a window size and returns an identifier; +// the loopback stream server and the UI are built on top of that seam rather +// than inside it, which is what lets either change without touching process +// lifecycle. +// +// Ownership contract: a session outlives its child process. When the child +// exits, the session records the status, notifies attached consumers and stops +// producing output, but stays registered so a consumer can still attach and +// replay the final scrollback — the "your shell exited" state a terminal tab +// shows. The owner drops the session with Kill, and Shutdown drops all of +// them. Nothing else removes a session from a Manager. +package pty + +import ( + "errors" + "os" + "time" +) + +const ( + // defaultCols and defaultRows size a session whose caller did not say. + // They are the classic terminal geometry, chosen because a shell that + // starts at 0x0 renders its prompt at the wrong width until the first + // resize arrives. + defaultCols = 80 + defaultRows = 24 + + // scrollbackBytes bounds the per-session replay buffer. It is what a + // consumer attaching late receives, and it is the reason a session with + // nobody reading it cannot grow without limit. + scrollbackBytes = 256 << 10 + + // readChunkBytes sizes one read from the PTY master. Large enough that a + // screenful of output arrives in one chunk, small enough that the first + // byte of a prompt is not held back waiting for a full buffer. + readChunkBytes = 32 << 10 + + // consumerQueue is how many chunks a single consumer may fall behind + // before its chunks start being dropped. Dropping is deliberate: the + // alternative is back-pressure onto the shell, which would let a stalled + // WebSocket freeze the user's terminal. + consumerQueue = 64 + + // killGrace is how long a child gets between the hangup and the SIGKILL. + // It is the window in which a shell runs its exit traps and flushes; a + // shell that ignores the hangup does not get to hold up app shutdown. + killGrace = 2 * time.Second + + // drainGrace bounds the wait for the output pump to see EOF after the + // child exits. Normally EOF arrives immediately; a grandchild still + // holding the slave end open is what this timeout exists for. + drainGrace = 250 * time.Millisecond + + // terminalType is the TERM the child sees. xterm.js implements the + // xterm-256color capabilities (DESIGN.md §8), so claiming anything else + // would misreport what the frontend can render. + terminalType = "xterm-256color" + + // windowsGOOS is the runtime.GOOS value for Windows. + windowsGOOS = "windows" +) + +// ErrNoSuchSession reports an operation naming a session the Manager does not +// hold — one never created, or one already killed. +var ErrNoSuchSession = errors.New("no such pty session") + +// SessionID identifies one session within a Manager. It is opaque: callers +// pass it back, they do not parse it. +type SessionID string + +// Options describes a session to create. The zero value is valid and starts +// the user's login shell at the default size in the process's own working +// directory. +type Options struct { + // Cwd is the child's working directory. Empty means inherit the + // application's. + Cwd string + + // Env holds additional environment entries in "KEY=value" form. They are + // appended to the application's own environment, so a caller can override + // an inherited variable by naming it again. + Env []string + + // Cols and Rows are the initial window size. Zero means the default. + Cols uint16 + Rows uint16 + + // Command is the argv to run. Empty means the user's login shell, which + // is what a terminal tab wants; the project's "run Claude Code" action + // (DESIGN.md §6) is what passes an explicit argv. + Command []string +} + +// Exit reports how a session's child process ended. +type Exit struct { + // Code is the child's exit status, or -1 when it was terminated by a + // signal rather than exiting on its own. -1 is what a killed session + // reports on every platform, which is why the signal number is not + // carried here: it has no meaning on Windows. + Code int +} + +// Attachment is one consumer's view of a session: what the session has already +// produced, what it produces next, and how it ended. +type Attachment struct { + // Replay is the scrollback at the moment of attaching — up to + // scrollbackBytes of the most recent output. It is a fresh copy the + // consumer owns. + Replay []byte + + // Chunks carries output produced after the attach. It is closed when the + // child exits. + // + // A consumer that stops reading does not stall the child: once it is + // consumerQueue chunks behind, further chunks are dropped rather than + // queued. Dropping loses bytes in the middle of the stream, so a consumer + // that cares about rendering fidelity — a terminal does — should treat a + // resumed read as a reason to redraw from a fresh Attach rather than + // assume continuity. + // + // Chunks are read-only and shared: every consumer of the same session + // receives the same backing array for a given chunk. Modifying one would + // corrupt what the others see. + Chunks <-chan []byte + + // Exited receives the exit status exactly once and is then closed. + // Attaching to an already-exited session yields a channel that already + // holds the status, so a late consumer never blocks here. + Exited <-chan Exit +} + +// shellFor returns the argv of the user's login shell for the named platform. +// +// The environment lookup and GOOS are parameters rather than reads of the +// ambient process so this decision is testable for every platform from any +// platform — the alternative is a rule that only the machine running the test +// ever exercises. +func shellFor(goos string, lookupEnv func(string) string) []string { + if goos == windowsGOOS { + return []string{"powershell.exe"} + } + if shell := lookupEnv("SHELL"); shell != "" { + return []string{shell} + } + return []string{"/bin/sh"} +} + +// environ builds the child's environment: everything the application has, then +// TERM, then the caller's additions. Later entries win, so a caller can +// override TERM and TERM overrides an inherited one. +func environ(extra []string) []string { + env := os.Environ() + env = append(env, "TERM="+terminalType) + return append(env, extra...) +} + +// size resolves the requested window size, substituting the default for a +// dimension the caller left at zero. +func size(cols, rows uint16) (width, height uint16) { + if cols == 0 { + cols = defaultCols + } + if rows == 0 { + rows = defaultRows + } + return cols, rows +} diff --git a/internal/pty/pty_test.go b/internal/pty/pty_test.go new file mode 100644 index 0000000..c3eae3d --- /dev/null +++ b/internal/pty/pty_test.go @@ -0,0 +1,644 @@ +package pty + +import ( + "errors" + "path/filepath" + "runtime" + "slices" + "strings" + "testing" + "time" + + xpty "github.com/aymanbagabas/go-pty" +) + +// readTimeout bounds every wait on a real child process. Generous, because +// these tests spawn shells on shared CI runners; a wait that hits it is a +// genuine failure, not a slow machine. +const readTimeout = 20 * time.Second + +// shell returns an argv running the given script through the platform's shell. +// The two scripts must do the same thing; they are separate because cmd.exe is +// not sh. +func shell(unixScript, windowsScript string) []string { + if runtime.GOOS == windowsGOOS { + return []string{"cmd.exe", "/c", windowsScript} + } + return []string{"/bin/sh", "-c", unixScript} +} + +// interactiveShell returns an argv for a bare shell reading from the terminal. +func interactiveShell() []string { + if runtime.GOOS == windowsGOOS { + return []string{"cmd.exe"} + } + return []string{"/bin/sh"} +} + +// requireSession returns the live session behind an ID. Tests reach past the +// Manager deliberately: waiting on the child directly is what lets them assert +// on lifecycle without an attached consumer changing the thing being measured. +func requireSession(t *testing.T, m *Manager, id SessionID) *session { + t.Helper() + s, err := m.lookup(id) + if err != nil { + t.Fatalf("looking up %s: %v", id, err) + } + return s +} + +// waitForExit blocks until the session's child has been reaped. +func waitForExit(t *testing.T, s *session) { + t.Helper() + select { + case <-s.done: + case <-time.After(readTimeout): + t.Fatal("timed out waiting for the child to exit") + } +} + +// readUntil collects output from an attachment until it contains want. +func readUntil(t *testing.T, a Attachment, want string) { + t.Helper() + var seen strings.Builder + seen.Write(a.Replay) + if strings.Contains(seen.String(), want) { + return + } + + deadline := time.After(readTimeout) + for { + select { + case chunk, open := <-a.Chunks: + if !open { + t.Fatalf("the session ended before producing %q; output was %q", want, seen.String()) + } + seen.Write(chunk) + if strings.Contains(seen.String(), want) { + return + } + case <-deadline: + t.Fatalf("timed out waiting for %q; output was %q", want, seen.String()) + } + } +} + +// awaitExit returns the exit status an attachment reports. +func awaitExit(t *testing.T, a Attachment) Exit { + t.Helper() + select { + case e := <-a.Exited: + return e + case <-time.After(readTimeout): + t.Fatal("timed out waiting for the exit event") + return Exit{} + } +} + +// The acceptance path from the issue: run a command, read its output, resize, +// kill, observe the exit event, and leave no unreaped process behind. +func TestSessionStreamsOutputResizesAndDiesWhenKilled(t *testing.T) { + m := New() + id, err := m.Create(Options{ + Command: shell("echo hello; sleep 60", "echo hello & ping -n 61 127.0.0.1 >NUL"), + Cols: 100, + Rows: 40, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + + s := requireSession(t, m, id) + attached, err := m.Attach(id) + if err != nil { + t.Fatalf("Attach: %v", err) + } + + readUntil(t, attached, "hello") + + if err := m.Resize(id, 120, 50); err != nil { + t.Errorf("Resize: %v", err) + } + + if err := m.Kill(id); err != nil { + t.Fatalf("Kill: %v", err) + } + + // Kill returns only once the child is reaped: a populated ProcessState is + // the proof there is no zombie left behind it. + if s.cmd.ProcessState == nil { + t.Error("Kill returned before the child was reaped; the process was left unwaited") + } + if got := awaitExit(t, attached); got.Code == 0 { + t.Errorf("exit code = 0 after a kill, want a non-zero or signaled status") + } + if _, err := m.Attach(id); !errors.Is(err, ErrNoSuchSession) { + t.Errorf("Attach after Kill: error = %v, want ErrNoSuchSession", err) + } +} + +func TestExitStatusCarriesTheChildsExitCode(t *testing.T) { + m := New() + id, err := m.Create(Options{Command: shell("exit 7", "exit 7")}) + if err != nil { + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { _ = m.Kill(id) }) + + attached, err := m.Attach(id) + if err != nil { + t.Fatalf("Attach: %v", err) + } + + if got := awaitExit(t, attached); got.Code != 7 { + t.Errorf("exit = %+v, want Code 7", got) + } +} + +// The child must never be throttled by the absence of a reader. This runs a +// command that produces more output than the scrollback holds with nothing +// attached at all, and checks it ran to completion anyway. +func TestChildRunsToCompletionWithNoConsumerAttached(t *testing.T) { + if runtime.GOOS == windowsGOOS { + t.Skip("the bulk-output script is POSIX sh; the Windows path is covered by the spawn/echo/kill tests") + } + + // 400 lines of ~1KB each: comfortably more than scrollbackBytes, and + // built without seq/awk so it runs on any POSIX shell. + const marker = "BULK-OUTPUT-COMPLETE" + script := "s=" + strings.Repeat("x", 64) + "; s=$s$s$s$s; s=$s$s$s$s; " + + "i=0; while [ $i -lt 400 ]; do echo \"$s\"; i=$((i+1)); done; echo " + marker + + m := New() + id, err := m.Create(Options{Command: shell(script, "")}) + if err != nil { + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { _ = m.Kill(id) }) + + // Nothing is attached while the child runs; wait on the child itself. + waitForExit(t, requireSession(t, m, id)) + + attached, err := m.Attach(id) + if err != nil { + t.Fatalf("Attach after exit: %v", err) + } + replay := string(attached.Replay) + + if !strings.Contains(replay, marker) { + t.Error("the child did not reach the end of its output; it was blocked by having no reader") + } + if len(attached.Replay) != scrollbackBytes { + t.Errorf("scrollback holds %d bytes, want it filled to %d", len(attached.Replay), scrollbackBytes) + } + if got := awaitExit(t, attached); got.Code != 0 { + t.Errorf("exit = %+v, want Code 0", got) + } +} + +// A consumer that stops reading must be dropped from, not block the child. +func TestSlowConsumerDoesNotStallTheChild(t *testing.T) { + if runtime.GOOS == windowsGOOS { + t.Skip("the bulk-output script is POSIX sh; the Windows path is covered by the spawn/echo/kill tests") + } + + script := "s=" + strings.Repeat("x", 64) + "; s=$s$s$s$s; s=$s$s$s$s; " + + "i=0; while [ $i -lt 400 ]; do echo \"$s\"; i=$((i+1)); done" + + m := New() + id, err := m.Create(Options{Command: shell(script, "")}) + if err != nil { + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { _ = m.Kill(id) }) + + // Attached and deliberately never read: the queue fills, chunks are + // dropped, and the child still has to finish. + if _, err := m.Attach(id); err != nil { + t.Fatalf("Attach: %v", err) + } + + waitForExit(t, requireSession(t, m, id)) +} + +func TestWriteReachesTheChild(t *testing.T) { + m := New() + // The shell echoes what is typed, so the command text must not contain + // the answer — otherwise the echo alone would satisfy the assertion. + id, err := m.Create(Options{Command: interactiveShell()}) + if err != nil { + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { _ = m.Kill(id) }) + + attached, err := m.Attach(id) + if err != nil { + t.Fatalf("Attach: %v", err) + } + + input := "expr 6 \\* 7\n" + if runtime.GOOS == windowsGOOS { + input = "set /a 6*7\r\n" + } + if err := m.Write(id, []byte(input)); err != nil { + t.Fatalf("Write: %v", err) + } + + readUntil(t, attached, "42") +} + +func TestAttachAfterExitReplaysScrollbackAndReportsTheStatus(t *testing.T) { + m := New() + id, err := m.Create(Options{Command: shell("echo persisted; exit 3", "echo persisted & exit 3")}) + if err != nil { + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { _ = m.Kill(id) }) + + waitForExit(t, requireSession(t, m, id)) + + attached, err := m.Attach(id) + if err != nil { + t.Fatalf("Attach after exit: %v", err) + } + if !strings.Contains(string(attached.Replay), "persisted") { + t.Errorf("replay = %q, want it to contain the output produced before the exit", attached.Replay) + } + // A late consumer must not hang waiting for output that will never come. + if _, open := <-attached.Chunks; open { + t.Error("Chunks delivered a value on an exited session; it must be closed") + } + if got := awaitExit(t, attached); got.Code != 3 { + t.Errorf("exit = %+v, want Code 3", got) + } +} + +func TestUnknownSessionIsRejectedByEveryMethod(t *testing.T) { + m := New() + const missing SessionID = "pty-does-not-exist" + + if _, err := m.Attach(missing); !errors.Is(err, ErrNoSuchSession) { + t.Errorf("Attach: error = %v, want ErrNoSuchSession", err) + } + if err := m.Write(missing, []byte("x")); !errors.Is(err, ErrNoSuchSession) { + t.Errorf("Write: error = %v, want ErrNoSuchSession", err) + } + if err := m.Resize(missing, 80, 24); !errors.Is(err, ErrNoSuchSession) { + t.Errorf("Resize: error = %v, want ErrNoSuchSession", err) + } + if err := m.Kill(missing); !errors.Is(err, ErrNoSuchSession) { + t.Errorf("Kill: error = %v, want ErrNoSuchSession", err) + } +} + +func TestCreateFailsWhenTheCommandCannotStart(t *testing.T) { + m := New() + + id, err := m.Create(Options{Command: []string{"m6t-no-such-binary-exists-here"}}) + if err == nil { + t.Fatalf("Create succeeded with id %q for a binary that does not exist", id) + } + if id != "" { + t.Errorf("Create returned id %q alongside an error, want the empty id", id) + } + // A failed start must leave nothing registered. + if len(m.sessions) != 0 { + t.Errorf("Manager holds %d sessions after a failed Create, want 0", len(m.sessions)) + } +} + +func TestCreateRunsTheChildInTheRequestedDirectory(t *testing.T) { + dir := t.TempDir() + // macOS reports /var as a symlink to /private/var, so compare against the + // resolved path rather than the one t.TempDir handed back. + resolved, err := filepath.EvalSymlinks(dir) + if err != nil { + t.Fatalf("resolving %s: %v", dir, err) + } + + m := New() + id, err := m.Create(Options{Cwd: dir, Command: shell("pwd", "cd")}) + if err != nil { + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { _ = m.Kill(id) }) + + attached, err := m.Attach(id) + if err != nil { + t.Fatalf("Attach: %v", err) + } + + readUntil(t, attached, filepath.Base(resolved)) +} + +func TestCreateExportsTheCallersEnvironment(t *testing.T) { + m := New() + id, err := m.Create(Options{ + Env: []string{"M6T_TEST_VAR=session-scoped-value"}, + Command: shell("echo $M6T_TEST_VAR", "echo %M6T_TEST_VAR%"), + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { _ = m.Kill(id) }) + + attached, err := m.Attach(id) + if err != nil { + t.Fatalf("Attach: %v", err) + } + + readUntil(t, attached, "session-scoped-value") +} + +func TestShutdownKillsEverySession(t *testing.T) { + m := New() + stayAlive := shell("sleep 60", "ping -n 61 127.0.0.1 >NUL") + + sessions := make([]*session, 0, 3) + ids := make([]SessionID, 0, 3) + for range 3 { + id, err := m.Create(Options{Command: stayAlive}) + if err != nil { + t.Fatalf("Create: %v", err) + } + ids = append(ids, id) + sessions = append(sessions, requireSession(t, m, id)) + } + + m.Shutdown() + + if len(m.sessions) != 0 { + t.Errorf("Manager holds %d sessions after Shutdown, want 0", len(m.sessions)) + } + for i, s := range sessions { + if s.cmd.ProcessState == nil { + t.Errorf("session %s was not reaped by Shutdown", ids[i]) + } + } +} + +// The hangup is a request; SIGKILL is what makes it a guarantee. A shell that +// traps SIGHUP — which a user's rc file is free to do — must not be able to +// hold the app open past killGrace. +func TestKillEscalatesToSigkillWhenTheChildIgnoresTheHangup(t *testing.T) { + if runtime.GOOS == windowsGOOS { + t.Skip("Windows has no SIGHUP to ignore; hangup terminates the child outright") + } + + const trapReady = "TRAP-INSTALLED" + + m := New() + // The trailing echo stops sh exec-ing into sleep, so the process holding + // the ignored disposition is the shell itself. + id, err := m.Create(Options{ + Command: shell(`trap "" HUP; echo `+trapReady+`; sleep 60; echo done`, ""), + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + s := requireSession(t, m, id) + + // Kill only once the trap is actually installed. Signaling earlier races + // the shell's own startup, and SIGHUP at default disposition kills it + // instantly — which would pass a test of the escalation path without ever + // exercising it. + attached, err := m.Attach(id) + if err != nil { + t.Fatalf("Attach: %v", err) + } + readUntil(t, attached, trapReady) + + start := time.Now() + if err := m.Kill(id); err != nil { + t.Fatalf("Kill: %v", err) + } + elapsed := time.Since(start) + + if s.cmd.ProcessState == nil { + t.Fatal("Kill returned before the child was reaped") + } + if elapsed < killGrace { + t.Errorf("Kill returned after %v, before the %v grace elapsed; the child was never given "+ + "its chance to exit cleanly", elapsed, killGrace) + } + if elapsed > killGrace+readTimeout { + t.Errorf("Kill took %v; a child ignoring the hangup must still die at the grace deadline", elapsed) + } +} + +// Once a session's PTY is closed, input and resize must report that rather +// than silently succeeding against a dead terminal. +func TestWriteAndResizeFailOnceTheSessionHasBeenKilled(t *testing.T) { + m := New() + id, err := m.Create(Options{Command: shell("sleep 60", "ping -n 61 127.0.0.1 >NUL")}) + if err != nil { + t.Fatalf("Create: %v", err) + } + + s := requireSession(t, m, id) + if err := m.Kill(id); err != nil { + t.Fatalf("Kill: %v", err) + } + + if err := s.write([]byte("input\n")); err == nil { + t.Error("write to a killed session succeeded; it must report the closed terminal") + } + if err := s.resize(100, 40); err == nil { + t.Error("resize of a killed session succeeded; it must report the closed terminal") + } +} + +func TestShutdownOnAnEmptyManagerIsSafe(t *testing.T) { + m := New() + m.Shutdown() + + // Shutdown must leave the Manager usable rather than in a state where the + // session map has been nilled out from under the next Create. + if _, err := m.Create(Options{Command: shell("exit 0", "exit 0")}); err != nil { + t.Errorf("Create after Shutdown on an empty Manager: %v", err) + } +} + +func TestSessionIDsAreUnique(t *testing.T) { + m := New() + t.Cleanup(m.Shutdown) + + seen := map[SessionID]bool{} + for range 3 { + id, err := m.Create(Options{Command: shell("exit 0", "exit 0")}) + if err != nil { + t.Fatalf("Create: %v", err) + } + if seen[id] { + t.Fatalf("Create returned the already-issued id %q", id) + } + seen[id] = true + } +} + +func TestShellForPicksTheUsersLoginShell(t *testing.T) { + tests := []struct { + name string + goos string + env map[string]string + want []string + }{ + { + name: "unix uses SHELL when it is set", + goos: "darwin", + env: map[string]string{"SHELL": "/usr/bin/fish"}, + want: []string{"/usr/bin/fish"}, + }, + { + name: "unix falls back to /bin/sh when SHELL is unset", + goos: "linux", + env: map[string]string{}, + want: []string{"/bin/sh"}, + }, + { + name: "unix falls back when SHELL is set but empty", + goos: "linux", + env: map[string]string{"SHELL": ""}, + want: []string{"/bin/sh"}, + }, + { + name: "windows uses PowerShell and ignores SHELL", + goos: windowsGOOS, + env: map[string]string{"SHELL": "/usr/bin/fish"}, + want: []string{"powershell.exe"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := shellFor(tt.goos, func(k string) string { return tt.env[k] }) + if strings.Join(got, " ") != strings.Join(tt.want, " ") { + t.Errorf("shellFor(%q) = %v, want %v", tt.goos, got, tt.want) + } + }) + } +} + +func TestEnvironInheritsTheProcessEnvironmentAndSetsTerm(t *testing.T) { + t.Setenv("M6T_INHERITED_MARKER", "inherited") + + env := environ([]string{"EXTRA=added"}) + + want := map[string]string{ + "M6T_INHERITED_MARKER": "inherited", + "TERM": terminalType, + "EXTRA": "added", + } + for key, value := range want { + if !slices.Contains(env, key+"="+value) { + t.Errorf("environ() is missing %s=%s; got %v", key, value, env) + } + } +} + +// A caller's own entry must win over the inherited one, which is what makes +// Options.Env an override rather than a suggestion. +func TestEnvironLetsTheCallerOverrideAnInheritedVariable(t *testing.T) { + t.Setenv("M6T_OVERRIDDEN", "from-process") + + env := environ([]string{"M6T_OVERRIDDEN=from-caller"}) + + last := "" + for _, entry := range env { + if strings.HasPrefix(entry, "M6T_OVERRIDDEN=") { + last = entry + } + } + if last != "M6T_OVERRIDDEN=from-caller" { + t.Errorf("the last M6T_OVERRIDDEN entry is %q, want the caller's value to win", last) + } +} + +func TestEnvironOverridesTermForTheCaller(t *testing.T) { + t.Setenv("TERM", "dumb") + + env := environ(nil) + + last := "" + for _, entry := range env { + if strings.HasPrefix(entry, "TERM=") { + last = entry + } + } + if last != "TERM="+terminalType { + t.Errorf("the last TERM entry is %q, want TERM=%s", last, terminalType) + } +} + +func TestSizeSubstitutesDefaultsForZeroDimensions(t *testing.T) { + tests := []struct { + name string + cols, rows uint16 + wantCols, wantRows uint16 + }{ + {"both given", 120, 50, 120, 50}, + {"both zero", 0, 0, defaultCols, defaultRows}, + {"zero columns only", 0, 50, defaultCols, 50}, + {"zero rows only", 120, 0, 120, defaultRows}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotCols, gotRows := size(tt.cols, tt.rows) + if gotCols != tt.wantCols || gotRows != tt.wantRows { + t.Errorf("size(%d, %d) = (%d, %d), want (%d, %d)", + tt.cols, tt.rows, gotCols, gotRows, tt.wantCols, tt.wantRows) + } + }) + } +} + +func TestResizeRejectsNothingAndAppliesDefaults(t *testing.T) { + m := New() + id, err := m.Create(Options{Command: shell("sleep 60", "ping -n 61 127.0.0.1 >NUL")}) + if err != nil { + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { _ = m.Kill(id) }) + + // A zero dimension must not reach the PTY, which cannot render at 0. + if err := m.Resize(id, 0, 0); err != nil { + t.Errorf("Resize with zero dimensions: %v", err) + } +} + +// A child that never ran leaves no ProcessState to read a code from. -1 is +// what Exit documents for "did not exit on its own", and the alternative — a +// zero-valued Exit — would report a clean exit for a session that failed. +func TestExitStatusReportsMinusOneWithoutAProcessState(t *testing.T) { + if got := exitStatus(&xpty.Cmd{}, errors.New("wait failed")); got.Code != -1 { + t.Errorf("exit = %+v, want Code -1 when the child produced no state", got) + } + if got := exitStatus(&xpty.Cmd{}, nil); got.Code != 0 { + t.Errorf("exit = %+v, want Code 0 when the wait succeeded with no state", got) + } +} + +func TestDefaultShellIsUsedWhenNoCommandIsGiven(t *testing.T) { + if runtime.GOOS == windowsGOOS { + t.Skip("PowerShell startup is too slow to gate this test on; the Unix path pins the behavior") + } + t.Setenv("SHELL", "/bin/sh") + + m := New() + id, err := m.Create(Options{}) + if err != nil { + t.Fatalf("Create with no command: %v", err) + } + t.Cleanup(func() { _ = m.Kill(id) }) + + attached, err := m.Attach(id) + if err != nil { + t.Fatalf("Attach: %v", err) + } + if err := m.Write(id, []byte("expr 6 \\* 7\n")); err != nil { + t.Fatalf("Write: %v", err) + } + + readUntil(t, attached, "42") +} diff --git a/internal/pty/ring.go b/internal/pty/ring.go new file mode 100644 index 0000000..c871e21 --- /dev/null +++ b/internal/pty/ring.go @@ -0,0 +1,57 @@ +package pty + +// ring is a fixed-capacity byte buffer holding the most recent bytes written +// to it. +// +// It is the mechanism behind the "a session survives having nobody reading it" +// rule. Writing never blocks and never fails: the alternative — an unbounded +// buffer — turns a terminal nobody is watching into a memory leak, and +// back-pressure onto the PTY turns it into a frozen shell. Dropping the oldest +// output is the only option that costs the user nothing they were still +// looking at. +// +// ring is not safe for concurrent use; the session owning it holds the lock. +type ring struct { + buf []byte + start int // index of the oldest byte held + size int // bytes currently held +} + +// newRing returns a ring holding at most capacity bytes. capacity must be +// positive. +func newRing(capacity int) *ring { + return &ring{buf: make([]byte, capacity)} +} + +// write appends p, evicting the oldest bytes when there is not enough room. +func (r *ring) write(p []byte) { + capacity := len(r.buf) + + // A single write larger than the whole buffer makes every byte already + // held unreachable, so only p's own tail survives. + if len(p) >= capacity { + copy(r.buf, p[len(p)-capacity:]) + r.start, r.size = 0, capacity + return + } + + end := (r.start + r.size) % capacity + n := copy(r.buf[end:], p) + copy(r.buf, p[n:]) + + if overflow := r.size + len(p) - capacity; overflow > 0 { + r.start = (r.start + overflow) % capacity + r.size = capacity + return + } + r.size += len(p) +} + +// snapshot returns a copy of the held bytes, oldest first. The caller owns the +// result; the ring keeps writing into its own storage. +func (r *ring) snapshot() []byte { + out := make([]byte, r.size) + n := copy(out, r.buf[r.start:min(r.start+r.size, len(r.buf))]) + copy(out[n:], r.buf[:r.size-n]) + return out +} diff --git a/internal/pty/ring_test.go b/internal/pty/ring_test.go new file mode 100644 index 0000000..dd5bf94 --- /dev/null +++ b/internal/pty/ring_test.go @@ -0,0 +1,115 @@ +package pty + +import ( + "bytes" + "strings" + "testing" +) + +func TestRingHoldsWhatFitsInOrder(t *testing.T) { + r := newRing(8) + r.write([]byte("abc")) + r.write([]byte("de")) + + if got := string(r.snapshot()); got != "abcde" { + t.Errorf("snapshot = %q, want %q", got, "abcde") + } +} + +func TestRingEvictsOldestOnceFull(t *testing.T) { + r := newRing(4) + r.write([]byte("abcd")) + r.write([]byte("ef")) + + // "ab" is the oldest and must be the part that went. + if got := string(r.snapshot()); got != "cdef" { + t.Errorf("snapshot = %q, want %q", got, "cdef") + } +} + +func TestRingWrapsAroundTheEndOfItsStorage(t *testing.T) { + r := newRing(5) + r.write([]byte("abcd")) + r.write([]byte("efg")) // wraps past the end of the backing array + + if got := string(r.snapshot()); got != "cdefg" { + t.Errorf("snapshot = %q, want %q", got, "cdefg") + } +} + +func TestRingKeepsOnlyTheTailOfAnOversizedWrite(t *testing.T) { + r := newRing(4) + r.write([]byte("xy")) + r.write([]byte("abcdefgh")) + + if got := string(r.snapshot()); got != "efgh" { + t.Errorf("snapshot = %q, want %q", got, "efgh") + } +} + +func TestRingSnapshotDoesNotAliasTheBuffer(t *testing.T) { + r := newRing(4) + r.write([]byte("abcd")) + + snap := r.snapshot() + r.write([]byte("wxyz")) + + if got := string(snap); got != "abcd" { + t.Errorf("snapshot changed to %q after a later write; it must be a copy", got) + } +} + +func TestRingNeverExceedsItsCapacity(t *testing.T) { + const capacity = 16 + r := newRing(capacity) + for range 100 { + r.write([]byte("0123456789")) + } + + snap := r.snapshot() + if len(snap) != capacity { + t.Errorf("snapshot is %d bytes, want the capacity %d", len(snap), capacity) + } + // 1000 bytes of a repeating 10-byte cycle: the last 16 end "...6789". + if !bytes.HasSuffix(snap, []byte("6789")) { + t.Errorf("snapshot %q does not end with the most recent bytes", snap) + } +} + +func TestRingEmptySnapshotIsEmpty(t *testing.T) { + if got := newRing(8).snapshot(); len(got) != 0 { + t.Errorf("a ring nothing was written to snapshots as %q, want empty", got) + } +} + +// The scrollback is what a late consumer replays, so its capacity is a product +// decision (DESIGN.md §8), not an implementation detail free to drift. +func TestScrollbackIsTheDocumentedSize(t *testing.T) { + if want := 256 * 1024; scrollbackBytes != want { + t.Errorf("scrollbackBytes = %d, want %d", scrollbackBytes, want) + } +} + +func TestRingHandlesAWriteExactlyItsCapacity(t *testing.T) { + r := newRing(4) + r.write([]byte("ab")) + r.write([]byte("wxyz")) + + if got := string(r.snapshot()); got != "wxyz" { + t.Errorf("snapshot = %q, want %q", got, "wxyz") + } +} + +func TestRingAccumulatesAcrossManySmallWrites(t *testing.T) { + r := newRing(1024) + var want strings.Builder + for i := range 50 { + chunk := strings.Repeat(string(rune('a'+i%26)), 3) + r.write([]byte(chunk)) + want.WriteString(chunk) + } + + if got := string(r.snapshot()); got != want.String() { + t.Errorf("snapshot = %q, want %q", got, want.String()) + } +} diff --git a/internal/pty/session.go b/internal/pty/session.go new file mode 100644 index 0000000..07c226f --- /dev/null +++ b/internal/pty/session.go @@ -0,0 +1,225 @@ +package pty + +import ( + "bytes" + "fmt" + "os" + "runtime" + "sync" + "time" + + xpty "github.com/aymanbagabas/go-pty" +) + +// consumer is one attached reader's delivery queue. +type consumer struct { + chunks chan []byte + exited chan Exit +} + +// session is one running PTY and the child process attached to it. +type session struct { + pty xpty.Pty + cmd *xpty.Cmd + + // done closes when the child has been reaped and the exit status + // published. Waiting on it is how kill knows there is no process left. + done chan struct{} + + mu sync.Mutex + scrollback *ring + consumers []*consumer + exited bool + exit Exit +} + +// start opens a PTY and launches opts.Command in it, or the user's login shell +// when no command is given. The returned session is live but not yet pumping +// output; the caller starts run. +func start(opts Options) (*session, error) { + terminal, err := xpty.New() + if err != nil { + return nil, fmt.Errorf("opening pty: %w", err) + } + + // Size before starting so the child reads the right geometry from its + // first prompt onward rather than drawing once at the wrong width. + cols, rows := size(opts.Cols, opts.Rows) + if err := terminal.Resize(int(cols), int(rows)); err != nil { + _ = terminal.Close() + return nil, fmt.Errorf("sizing pty: %w", err) + } + + argv := opts.Command + if len(argv) == 0 { + argv = shellFor(runtime.GOOS, os.Getenv) + } + + cmd := terminal.Command(argv[0], argv[1:]...) + cmd.Dir = opts.Cwd + cmd.Env = environ(opts.Env) + if err := cmd.Start(); err != nil { + _ = terminal.Close() + return nil, fmt.Errorf("starting %q: %w", argv[0], err) + } + + return &session{ + pty: terminal, + cmd: cmd, + done: make(chan struct{}), + scrollback: newRing(scrollbackBytes), + }, nil +} + +// run pumps output until the child exits, then publishes the exit status. It +// owns the session's lifetime and returns only once the child is reaped. +func (s *session) run() { + defer close(s.done) + + drained := make(chan struct{}) + go func() { + defer close(drained) + s.pump() + }() + + waitErr := s.cmd.Wait() + + // The child is gone, but output it wrote before exiting may still be in + // the pipe. Give the pump a moment to see EOF on its own so that last + // screenful reaches consumers ahead of the exit event; close the master + // only if something downstream is holding the slave end open. + select { + case <-drained: + case <-time.After(drainGrace): + } + _ = s.pty.Close() + <-drained + + s.finish(exitStatus(s.cmd, waitErr)) +} + +// pump reads the PTY master until it fails, which is how a closed or +// hung-up terminal reports that there is nothing more to read. +func (s *session) pump() { + buf := make([]byte, readChunkBytes) + for { + n, err := s.pty.Read(buf) + if n > 0 { + s.broadcast(buf[:n]) + } + if err != nil { + return + } + } +} + +// broadcast records a chunk in the scrollback and offers it to every attached +// consumer. A consumer whose queue is full has the chunk dropped: the child +// must never wait on a reader. +func (s *session) broadcast(chunk []byte) { + s.mu.Lock() + defer s.mu.Unlock() + + s.scrollback.write(chunk) + if len(s.consumers) == 0 { + return + } + + // pump reuses its read buffer, so consumers get a copy. One copy is + // shared across them because nothing downstream may modify it. + out := bytes.Clone(chunk) + for _, c := range s.consumers { + select { + case c.chunks <- out: + default: + } + } +} + +// finish publishes the exit status to every attached consumer and marks the +// session as ended, so later attaches report the status instead of waiting for +// output that will never come. +func (s *session) finish(e Exit) { + s.mu.Lock() + defer s.mu.Unlock() + + s.exited, s.exit = true, e + for _, c := range s.consumers { + close(c.chunks) + c.exited <- e + close(c.exited) + } + s.consumers = nil +} + +// attach registers a consumer and returns its view of the session. The +// scrollback snapshot and the registration happen under one lock, so no chunk +// is both replayed and delivered, and none is missed between the two. +func (s *session) attach() Attachment { + s.mu.Lock() + defer s.mu.Unlock() + + c := &consumer{ + chunks: make(chan []byte, consumerQueue), + exited: make(chan Exit, 1), + } + if s.exited { + close(c.chunks) + c.exited <- s.exit + close(c.exited) + } else { + s.consumers = append(s.consumers, c) + } + + return Attachment{Replay: s.scrollback.snapshot(), Chunks: c.chunks, Exited: c.exited} +} + +// write sends input to the child. +func (s *session) write(p []byte) error { + if _, err := s.pty.Write(p); err != nil { + return fmt.Errorf("writing to pty: %w", err) + } + return nil +} + +// resize changes the window size the child sees. +func (s *session) resize(cols, rows uint16) error { + if err := s.pty.Resize(int(cols), int(rows)); err != nil { + return fmt.Errorf("resizing pty: %w", err) + } + return nil +} + +// kill terminates the child and returns once it has been reaped, so a caller +// that returns has left no process behind. +// +// A hangup goes first: that is what closing a terminal window does, and it +// gives the shell its chance to run exit traps. A shell that is still alive +// after killGrace gets SIGKILL. Signal errors are not reported because the +// only ordinary cause is the child having already exited, which is the +// outcome being asked for. +func (s *session) kill() { + if proc := s.cmd.Process; proc != nil { + _ = hangup(proc) + select { + case <-s.done: + return + case <-time.After(killGrace): + } + _ = forceKill(proc) + } + <-s.done +} + +// exitStatus reads the child's status off the finished command. A child killed +// by a signal has no exit code of its own, and ExitCode reports -1 for it, +// which is the value Exit documents for that case. +func exitStatus(cmd *xpty.Cmd, waitErr error) Exit { + if cmd.ProcessState != nil { + return Exit{Code: cmd.ProcessState.ExitCode()} + } + if waitErr != nil { + return Exit{Code: -1} + } + return Exit{Code: 0} +} diff --git a/package_budget_test.go b/package_budget_test.go index 50875ca..3b9e0d6 100644 --- a/package_budget_test.go +++ b/package_budget_test.go @@ -50,6 +50,10 @@ var structuralPins = map[string]packagePin{ loc: 150, exported: 2, why: "link-time build identity; a dependency root importing nothing first-party", }, + "internal/pty": { + loc: 750, exported: 7, + why: "PTY service: session lifecycle, scrollback and platform termination for the embedded terminal", + }, } // locCeilingNote explains why the LOC ceilings carry headroom while every other @@ -58,19 +62,26 @@ var structuralPins = map[string]packagePin{ // A LOC ceiling set to a package's exact current line count is not a ratchet, // it is a freeze: one more line of doc comment in buildinfo.go would fail the // build. The number has to represent the size at which a package stops being -// readable in one sitting, and m6t cannot measure that yet — the backend -// services that will be the real packages (git, pty, kube, helm; DESIGN.md -// §3.2) land in #2 and #5. So these figures are seeded as policy: +// readable in one sitting. +// +// internal/pty is the first ceiling here set from a real measurement rather +// than policy. It landed with #2 at 644 lines across six files, and 750 is +// that plus room for the follow-up fixes a new service attracts — not enough +// room for a second service to move in alongside it. The stream server that +// consumes it (#3) is its own package, so this number should hold. +// +// The rest are still seeded as policy, because the services that would let +// them be measured (git, kube, helm; DESIGN.md §3.2) land in #5: // // - 200 for internal/app and 150 for buildinfo — roughly 3x and 2x today's // size, so ordinary work does not trip the gate while a package that // doubles again arrives in review as a decomposition question. // - 60 for the root: main.go does one thing and must keep doing only that. // -// Re-pin against real measurements once #2 and #5 land. No other ceiling here +// Re-pin those against real measurements once #5 lands. No other ceiling here // needs the caveat: counts of packages, files and exported names do not grow // through ordinary editing. -const locCeilingNote = "LOC ceilings are policy-seeded; re-pin after #2/#5 (see locCeilingNote)" +const locCeilingNote = "internal/pty is measured; the other LOC ceilings are policy-seeded pending #5 (see locCeilingNote)" // maxFilesPerPackage stops a package from escaping its LOC budget by fanning // the same code across many small files. diff --git a/package_graph_test.go b/package_graph_test.go index b6c1fe8..e9dd4ef 100644 --- a/package_graph_test.go +++ b/package_graph_test.go @@ -49,12 +49,21 @@ func TestNoDeadPackages(t *testing.T) { // reviewer gets to ask whether the new dependency belongs. func TestImportGraphIsPinned(t *testing.T) { // The composition root imports the binding layer; the binding layer reads - // build identity. buildinfo imports nothing first-party — it is a leaf, and - // depguard pins that independently. + // build identity and composes the backend services. buildinfo imports + // nothing first-party — it is a leaf, and depguard pins that + // independently. + // + // internal/app -> internal/pty is the first service edge (#2). It belongs + // because the binding layer is where services are composed: the App owns + // the Manager so that quitting the app can end the PTY sessions, which + // nothing else is positioned to do. The edge runs one way only — pty + // imports no first-party package at all, which is what keeps it usable + // from the stream server (#3) without dragging the Wails layer in. want := map[string][]string{ rootPackageDir: {"internal/app"}, - "internal/app": {"internal/buildinfo"}, + "internal/app": {"internal/buildinfo", "internal/pty"}, "internal/buildinfo": {}, + "internal/pty": {}, } graph := firstPartyImports(t) From 2062a9d9d1db221c36c9beea5937a7d7ed0709aa Mon Sep 17 00:00:00 2001 From: cjimti Date: Mon, 3 Aug 2026 10:53:23 -0700 Subject: [PATCH 2/2] Resolve the shell against PATH before starting a session go-pty resolves a bare command name relative to Cmd.Dir on Windows, so a session created with a Cwd looked for the shell inside that directory and failed with "file does not exist". This was not confined to tests. The Windows default shell is the bare name "powershell.exe" and every project terminal sets Cwd to the project root, so no terminal could have opened on Windows at all. start() now calls exec.LookPath first, which also turns "the shell is not installed" into an error at Create rather than a session that exists and immediately dies. exec.LookPath does not trip gosec G204. Caught by the Windows CI smoke test added in the previous commit. --- internal/pty/pty_test.go | 39 +++++++++++++++++++++++++++++++++++++++ internal/pty/session.go | 15 ++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/internal/pty/pty_test.go b/internal/pty/pty_test.go index c3eae3d..85a9a7b 100644 --- a/internal/pty/pty_test.go +++ b/internal/pty/pty_test.go @@ -331,6 +331,45 @@ func TestCreateRunsTheChildInTheRequestedDirectory(t *testing.T) { readUntil(t, attached, filepath.Base(resolved)) } +// A bare command name must resolve against PATH even when the session has a +// working directory, because every project terminal has one and the Windows +// default shell is the bare name "powershell.exe". +// +// This is a regression test. go-pty resolves a bare name relative to Cmd.Dir +// on Windows, so before start() called exec.LookPath, creating a session with +// a Cwd looked for the shell inside the project directory and failed with +// "file does not exist" — meaning no terminal could ever open on Windows. +func TestCreateResolvesABareCommandNameAgainstPathNotTheWorkingDirectory(t *testing.T) { + bare := "sh" + if runtime.GOOS == windowsGOOS { + bare = "cmd.exe" + } + + m := New() + // An empty directory: anything resolved relative to it cannot exist. + id, err := m.Create(Options{Cwd: t.TempDir(), Command: []string{bare}}) + if err != nil { + t.Fatalf("Create with a bare command name and a Cwd: %v", err) + } + t.Cleanup(func() { _ = m.Kill(id) }) + + if _, err := m.Attach(id); err != nil { + t.Fatalf("Attach: %v", err) + } +} + +func TestCreateReportsAShellThatIsNotOnPath(t *testing.T) { + m := New() + + _, err := m.Create(Options{Command: []string{"m6t-definitely-not-on-path"}}) + if err == nil { + t.Fatal("Create succeeded for a binary that is not on PATH") + } + if !strings.Contains(err.Error(), "m6t-definitely-not-on-path") { + t.Errorf("error %q does not name the binary that could not be found", err) + } +} + func TestCreateExportsTheCallersEnvironment(t *testing.T) { m := New() id, err := m.Create(Options{ diff --git a/internal/pty/session.go b/internal/pty/session.go index 07c226f..ca312fd 100644 --- a/internal/pty/session.go +++ b/internal/pty/session.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "os" + "os/exec" "runtime" "sync" "time" @@ -55,7 +56,19 @@ func start(opts Options) (*session, error) { argv = shellFor(runtime.GOOS, os.Getenv) } - cmd := terminal.Command(argv[0], argv[1:]...) + // Resolve against PATH before handing the name over. go-pty resolves a + // bare name relative to Cmd.Dir on Windows, so a session started with a + // Cwd — which every project terminal is — would look for powershell.exe + // inside the project directory and fail. Resolving here also turns "the + // shell is not installed" into an error at Create rather than a session + // that exists and immediately dies. + binary, err := exec.LookPath(argv[0]) + if err != nil { + _ = terminal.Close() + return nil, fmt.Errorf("locating %q: %w", argv[0], err) + } + + cmd := terminal.Command(binary, argv[1:]...) cmd.Dir = opts.Cwd cmd.Env = environ(opts.Env) if err := cmd.Start(); err != nil {