Repository navigation
Add the PTY service behind the embedded terminal - #23
Merged
Merged
Conversation
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
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.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2
What this does
Adds
internal/pty, the backend service that owns real PTY sessions running the user's real shell (DESIGN.md §3.2, §8), and composes it into the app so quitting m6t cannot orphan a shell.The package is transport-agnostic by construction: it takes an argv slice and a window size and returns an identifier. It knows nothing about WebSockets, terminal tabs or the Wails bridge, which is what lets the stream server (#3) and the UI (#4) land without touching process lifecycle.
Design notes worth reviewing
Ownership contract. A session outlives its child. On exit it records the status, notifies consumers and stops producing, but stays registered so a consumer can still attach and replay the final scrollback — the "your shell exited" state a terminal tab shows.
KillandShutdownare the only things that drop a session. The cost is that a tab which is never closed holds its 256KB ring; that cleanup obligation lands on #3/#4.Never block the child. Output always goes to the ring. Each attached consumer has a 64-chunk queue, and a consumer that falls behind has chunks dropped, not queued — back-pressure onto the PTY would let a stalled WebSocket freeze the user's terminal. Dropping loses bytes mid-stream, so
Attachment.Chunksdocuments that a consumer wanting rendering fidelity should redraw from a freshAttachrather than assume continuity. Worth a look from whoever picks up #3.Process groups, not processes. go-pty starts the child in its own session, so the shell is a process-group leader. Termination signals
-pgid, not the pid: signalling only the shell would reap it and orphan theclaudeorvimit started. That is what makes "no zombie processes" true rather than aspirational.Ordering of output vs. exit.
run()waits for the child, gives the pumpdrainGraceto see EOF on its own, and only then publishes the exit event — so a consumer never sees "exited" before the last screenful the child wrote.Deviation from the issue (please confirm)
The issue specifies creack/pty on Unix and aymanbagabas/go-pty on Windows. This uses go-pty on both platforms.
Reason: go-pty wraps creack/pty on Unix anyway (it is in the dependency tree either way), and it owns process creation internally. The two-library split requires calling
exec.Command(shell, args...)directly, which tripsgosecG204 "subprocess launched with variable" — I verified this fires against the pinned gosec 2.28.0. Clearing it would have needed either a repo-wide G204 exclusion (a real regression: G204 is exactly the rule guarding thegit/kubectl/helmargv paths in #5) or a#nosecannotation, which CLAUDE.md reserves for maintainer sign-off. Using go-pty means no suppression exists to review.Net effect: one dependency instead of two, and the only genuinely platform-specific code left is
hangup_*.go.New dependencies, all on the allowlist:
aymanbagabas/go-pty(MIT),creack/pty(MIT),u-root/u-root(BSD-3-Clause).How it was verified
make verify— all checks passed, run against the exact commit content.coverage-reportpatch-coverageinternal/ptydead-codeAcceptance criteria from the issue:
sh -c 'echo hello; sleep 60', output read, resize succeeds, kill observed via exit event, child reaped (ProcessStatepopulated — no zombie)exit 7assertsCode == 7,exit 3assertsCode == 3ErrNoSuchSessionfromAttach/Write/Resize/KillGOOS=windowsvet + test-binary build) and is smoke-tested in CIAdversarial review — what it caught
The SIGKILL-escalation test was passing for the wrong reason. It killed ~148µs after spawn, before
shhad installedtrap "" HUP, so SIGHUP arrived at default disposition and the child died instantly — the escalation path never executed. A probe confirmed the implementation is correct (the child survives 4s once the trap is actually in place). The test now blocks on aTRAP-INSTALLEDmarker before killing, and asserts the kill takes at leastkillGrace. It would have been flaky-green in CI otherwise.Also fixed during review:
Attachment.Chunksshares one backing array across all consumers of a session, which was an undocumented aliasing contract. Now stated explicitly.Structural ratchets raised (3)
Each is justified in the diff, at the line that raises it:
maxAppFields1 → 2 — the PTY service arrives as a single*pty.Managerhandle, which is the one-handle-per-service case the ceiling's own comment describes.maxAppMethodsis unchanged at 1: no new Wails-bound API.internal/app → internal/ptyandinternal/pty → {}. The service imports no first-party package, which is what keeps it usable from Loopback stream server #3 without dragging the Wails layer along.structuralPins— newinternal/ptyentry (654 LOC / ceiling 750, 7 exported). This is the first LOC ceiling set from a real measurement rather than policy, solocCeilingNotewas updated to say so;internal/appandbuildinforemain policy-seeded pending Project registry and tabs #5.CI change
The Test job runs on ubuntu only, so the ConPTY path would otherwise reach a release having passed every gate without ever executing. Added a PTY smoke-test step to the existing Build matrix, so
go test ./internal/pty/...now runs on ubuntu, macOS and Windows. Parity holds: it is the same suitemake testruns on a maintainer's host.Known limitation
The Windows path is compile-verified and cross-vetted locally, but I have no Windows machine here — this PR's Windows CI job is the first real execution of the ConPTY path. That job is the one to watch on this PR.
Checklist
os/execuse at all. go-pty owns process creation, and.semgrep/go-security.ymlalready names the PTY service as the one legitimate place a shell is spawned.