-
Notifications
You must be signed in to change notification settings - Fork 953
feat(pty-daemon): standalone PTY daemon package (Phase 1, skeleton) #3886
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Kitenite
wants to merge
2
commits into
main
Choose a base branch
from
elastic-lens
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| # @superset/pty-daemon | ||
|
|
||
| Long-lived PTY-owning process for the v2 desktop terminal. host-service is a | ||
| client over a Unix socket; routine host-service upgrades don't touch shells. | ||
|
|
||
| Implements [Phase 1 of the daemon plan](../../apps/desktop/plans/20260429-pty-daemon-implementation.md). | ||
| This package is **standalone**: it does not import from `@superset/host-service` | ||
| or any other workspace package. Host-service consumes only the protocol types | ||
| via `@superset/pty-daemon/protocol`. | ||
|
|
||
| ## Runtime | ||
|
|
||
| **Production: Node ≥ 20** (Electron's bundled Node), via | ||
| `process.execPath` — exactly the same pattern as `host-service` already | ||
| uses today (`packages/host-service/build.ts` → `dist/host-service.js`, | ||
| spawned by `apps/desktop/src/main/lib/host-service-coordinator.ts`). | ||
| Bun is the build tool, not a runtime. **No new runtime in the desktop | ||
| app bundle.** | ||
|
|
||
| **Why not Bun at runtime:** verified during development that node-pty | ||
| 1.1's master fd handling is incompatible with Bun 1.3 (`tty.ReadStream` | ||
| closes immediately, alternate `fs.createReadStream(null, { fd })` | ||
| returns EAGAIN with no recovery). The daemon needs a runtime where | ||
| node-pty actually works. | ||
|
|
||
| **Dev:** unit tests run under Bun (`bun test`) for speed; integration | ||
| tests run under Node (`bun run test:integration`) since they touch real | ||
| PTYs. The daemon binary itself runs under Node in both dev and prod. | ||
|
|
||
| ## Layout | ||
|
|
||
| ``` | ||
| src/ | ||
| ├── main.ts # Node entrypoint: argv → Server.listen() | ||
| ├── index.ts # Public exports for host-service consumers | ||
| ├── protocol/ # Wire schemas + length-prefixed framing | ||
| │ ├── version.ts # CURRENT_PROTOCOL_VERSION + supported list | ||
| │ ├── messages.ts # ClientMessage / ServerMessage unions | ||
| │ ├── framing.ts # encodeFrame / FrameDecoder (4-byte BE prefix) | ||
| │ └── index.ts | ||
| ├── Pty/ # node-pty thin wrapper with dim validation | ||
| │ ├── Pty.ts | ||
| │ └── index.ts | ||
| ├── SessionStore/ # in-memory map + 64KB ring buffer per session | ||
| │ ├── SessionStore.ts | ||
| │ └── index.ts | ||
| ├── handlers/ # pure functions: open/input/resize/close/list/subscribe | ||
| │ ├── handlers.ts | ||
| │ └── index.ts | ||
| └── Server/ # AF_UNIX SOCK_STREAM accept loop, handshake, dispatch | ||
| ├── Server.ts | ||
| └── index.ts | ||
|
|
||
| test/ | ||
| ├── helpers/ | ||
| │ └── client.ts # reusable DaemonClient: connect, send, waitFor, collect | ||
| ├── integration.test.ts # smoke / happy-path (3 tests) | ||
| └── control-plane.test.ts # exhaustive control-plane coverage (25 tests) | ||
|
|
||
| build.ts # Bun bundler → dist/pty-daemon.js (target: node) | ||
| ``` | ||
|
|
||
| ## Design notes | ||
|
|
||
| - **Stateless from the client's perspective.** Every protocol call carries | ||
| full context. No client tracking, no session tombstones, no business | ||
| rules. Single design principle from | ||
| [the implementation plan](../../apps/desktop/plans/20260429-pty-daemon-implementation.md#the-single-design-principle). | ||
| - **Auth boundary = Unix socket file mode 0600.** No in-band tokens. The | ||
| daemon trusts whoever can open the socket. | ||
| - **Buffer is in-memory only.** Survives host-service restarts (because the | ||
| daemon does), but never persisted to disk. No SQLite, no scrollback files. | ||
| v1's `HistoryManager` is explicitly out of scope. | ||
| - **Protocol versioned from day one.** Handshake (`hello` / `hello-ack`) | ||
| picks the highest mutually supported version. | ||
|
|
||
| ## Testing | ||
|
|
||
| ```sh | ||
| bun test # 24 unit tests (protocol framing, handlers, SessionStore, Pty validation) | ||
| bun run test:integration # 28 integration tests under node --test: | ||
| # - test/integration.test.ts (smoke / happy-path, 3 tests) | ||
| # - test/control-plane.test.ts (every usage pattern, 25 tests) | ||
| bun run typecheck # tsc --noEmit | ||
| bun run build:daemon # bundle src/main.ts → dist/pty-daemon.js (target: node) | ||
| ``` | ||
|
|
||
| **Control-plane coverage** (`test/control-plane.test.ts`): | ||
|
|
||
| - Handshake: rejects non-hello first, picks highest mutual protocol, rejects unsupported, rejects duplicate hello. | ||
| - Session lifecycle: invalid dims, duplicate ids, ENOENT on missing, instant-exit shells, SIGKILL on hung shells. | ||
| - I/O patterns: resize during running shell, burst output (200 lines), multi-byte UTF-8 (🚀). | ||
| - Multi-client fan-out: two subscribers see same output, unsubscribe stops further delivery, dropped subscriber doesn't crash daemon. | ||
| - Detach + reattach (the headline feature): late subscriber gets replay, full reattach cycle continues live after disconnect. | ||
| - list reflects active sessions with cols/rows/alive. | ||
| - Hostile input: malformed frames disconnect cleanly, oversized frames are rejected, input on exited session returns EEXITED. | ||
| - Concurrency: 20 sessions in parallel from one connection, 10 connections opening sessions in parallel. | ||
| - Server shutdown: in-flight clients disconnect cleanly, owned PTYs are killed. | ||
| - Framing: tolerates split frames across multiple TCP chunks. | ||
|
|
||
| Why two runners? `bun test` is fast for pure-JS work. node-pty doesn't work | ||
| under Bun, so anything that spawns a real PTY runs under Node. | ||
|
|
||
| ## Running locally | ||
|
|
||
| ```sh | ||
| bun run start --socket=/tmp/pty-daemon.sock | ||
| ``` | ||
|
|
||
| Logs go to stderr; stdout stays empty (so the daemon can later be supervised | ||
| by host-service with stdout reserved for protocol or kept dark). | ||
|
|
||
| ## Out of scope (Phase 1) | ||
|
|
||
| - Host-service integration (DaemonClient, terminal.ts refactor, manifest | ||
| adoption) — separate PR. | ||
| - Daemon-upgrade handoff via `child_process.spawn` `stdio` fd inheritance | ||
| — separate PR (Phase 2 of the plan). | ||
| - Windows ConPTY — not in v1 protocol; defer until Windows users justify it. | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| /** | ||
| * Bundles the pty-daemon entry point into a single JS file executable by a | ||
| * standalone Node.js runtime (matches packages/host-service/build.ts). Native | ||
| * addons (node-pty) are marked external and resolved from the desktop app's | ||
| * lib/native/ at runtime. | ||
| * | ||
| * Production: Electron spawns the daemon via process.execPath (its bundled | ||
| * Node), exactly like host-service. No Bun in the production bundle. | ||
| */ | ||
| import { existsSync, mkdirSync } from "node:fs"; | ||
|
|
||
| const outdir = "dist"; | ||
| if (!existsSync(outdir)) { | ||
| mkdirSync(outdir, { recursive: true }); | ||
| } | ||
|
|
||
| const result = await Bun.build({ | ||
| entrypoints: ["src/main.ts"], | ||
| target: "node", | ||
| outdir, | ||
| naming: "pty-daemon.js", | ||
| format: "esm", | ||
| external: ["node-pty"], | ||
| }); | ||
|
|
||
| if (!result.success) { | ||
| console.error("[pty-daemon] build failed:"); | ||
| for (const log of result.logs) { | ||
| console.error(log); | ||
| } | ||
| process.exit(1); | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| { | ||
| "name": "@superset/pty-daemon", | ||
| "version": "0.1.0", | ||
| "private": true, | ||
| "type": "module", | ||
| "exports": { | ||
| ".": { | ||
| "types": "./src/index.ts", | ||
| "default": "./src/index.ts" | ||
| }, | ||
| "./protocol": { | ||
| "types": "./src/protocol/index.ts", | ||
| "default": "./src/protocol/index.ts" | ||
| } | ||
| }, | ||
| "bin": { | ||
| "pty-daemon": "./src/main.ts" | ||
| }, | ||
| "engines": { | ||
| "node": ">=20" | ||
| }, | ||
| "scripts": { | ||
| "clean": "git clean -xdf .cache .turbo dist node_modules", | ||
| "start": "node --experimental-strip-types src/main.ts", | ||
| "build:daemon": "bun run build.ts", | ||
| "typecheck": "tsc --noEmit --emitDeclarationOnly false", | ||
| "test": "bun test src/protocol src/SessionStore src/handlers src/Pty/Pty.test.ts", | ||
| "test:integration": "node --experimental-strip-types --test test/integration.test.ts test/control-plane.test.ts" | ||
| }, | ||
| "dependencies": { | ||
| "node-pty": "1.1.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@superset/typescript": "workspace:*", | ||
| "@types/node": "^24.9.1", | ||
| "bun-types": "^1.3.1", | ||
| "typescript": "^5.9.3" | ||
| } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { spawn } from "./Pty.ts"; | ||
|
|
||
| // node-pty's runtime requires Node (Bun's tty.ReadStream handling is | ||
| // incompatible with the master fd setup). The daemon ships running under | ||
| // node; integration spawn tests live in test/integration.ts and run via | ||
| // `npm run test:integration`. Here we only cover the synchronous validation | ||
| // logic that doesn't require spawning a real PTY. | ||
|
|
||
| describe("Pty wrapper (validation only — spawn behavior tested under node)", () => { | ||
| test("rejects invalid spawn dims (cols)", () => { | ||
| expect(() => | ||
| spawn({ | ||
| meta: { shell: "/bin/sh", argv: [], cols: 0, rows: 24 }, | ||
| }), | ||
| ).toThrow(/invalid cols/); | ||
| }); | ||
|
|
||
| test("rejects invalid spawn dims (rows)", () => { | ||
| expect(() => | ||
| spawn({ | ||
| meta: { shell: "/bin/sh", argv: [], cols: 80, rows: 0 }, | ||
| }), | ||
| ).toThrow(/invalid rows/); | ||
| }); | ||
|
|
||
| test("rejects non-integer dims", () => { | ||
| expect(() => | ||
| spawn({ | ||
| meta: { shell: "/bin/sh", argv: [], cols: 80.5, rows: 24 }, | ||
| }), | ||
| ).toThrow(/invalid cols/); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import * as nodePty from "node-pty"; | ||
| import type { SessionMeta } from "../protocol/index.ts"; | ||
|
|
||
| export type PtyOnData = (data: Buffer) => void; | ||
| export type PtyOnExit = (info: { | ||
| code: number | null; | ||
| signal: number | null; | ||
| }) => void; | ||
|
|
||
| export interface Pty { | ||
| readonly pid: number; | ||
| readonly meta: SessionMeta; | ||
| write(data: Buffer): void; | ||
| resize(cols: number, rows: number): void; | ||
| kill(signal?: NodeJS.Signals): void; | ||
| onData(cb: PtyOnData): void; | ||
| onExit(cb: PtyOnExit): void; | ||
| } | ||
|
|
||
| export interface SpawnOptions { | ||
| meta: SessionMeta; | ||
| } | ||
|
|
||
| class NodePtyAdapter implements Pty { | ||
| readonly pid: number; | ||
| meta: SessionMeta; | ||
| private term: nodePty.IPty; | ||
|
|
||
| constructor(term: nodePty.IPty, meta: SessionMeta) { | ||
| this.term = term; | ||
| this.pid = term.pid; | ||
| this.meta = meta; | ||
| } | ||
|
|
||
| write(data: Buffer): void { | ||
| // node-pty's write accepts strings or buffers; pass buffer to keep bytes intact. | ||
| this.term.write(data as unknown as string); | ||
| } | ||
|
|
||
| resize(cols: number, rows: number): void { | ||
| validateDims(cols, rows); | ||
| this.term.resize(cols, rows); | ||
| this.meta = { ...this.meta, cols, rows }; | ||
| } | ||
|
|
||
| kill(signal?: NodeJS.Signals): void { | ||
| this.term.kill(signal); | ||
| } | ||
|
|
||
| onData(cb: PtyOnData): void { | ||
| this.term.onData((d) => { | ||
| cb(typeof d === "string" ? Buffer.from(d, "utf8") : d); | ||
| }); | ||
| } | ||
|
|
||
| onExit(cb: PtyOnExit): void { | ||
| this.term.onExit(({ exitCode, signal }) => { | ||
| cb({ code: exitCode ?? null, signal: signal ?? null }); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| function validateDims(cols: number, rows: number): void { | ||
| if (!Number.isInteger(cols) || cols <= 0) { | ||
| throw new Error(`invalid cols: ${cols}`); | ||
| } | ||
| if (!Number.isInteger(rows) || rows <= 0) { | ||
| throw new Error(`invalid rows: ${rows}`); | ||
| } | ||
| } | ||
|
|
||
| export function spawn({ meta }: SpawnOptions): Pty { | ||
| validateDims(meta.cols, meta.rows); | ||
| const term = nodePty.spawn(meta.shell, meta.argv, { | ||
| name: "xterm-256color", | ||
| cols: meta.cols, | ||
| rows: meta.rows, | ||
| cwd: meta.cwd, | ||
| env: meta.env, | ||
| // node-pty's encoding defaults to utf8; we want raw bytes for fidelity. | ||
| encoding: null, | ||
| }); | ||
| return new NodePtyAdapter(term, meta); | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| export type { Pty, PtyOnData, PtyOnExit, SpawnOptions } from "./Pty.ts"; | ||
| export { spawn } from "./Pty.ts"; |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a language tag to the fenced code block to satisfy markdownlint.
Line 20 should specify a fence language (
textis sufficient) to resolve MD040.Suggested patch
Verify each finding against the current code and only fix it if needed.
In
@packages/pty-daemon/README.mdaround lines 20 - 44, The README.md code fenceshowing the src/ and test/ tree lacks a language tag which triggers markdownlint
MD040; update the fenced block around the directory listing to include a
language tag (e.g., change the opening
totext) so the block becomes alanguage-tagged fenced code block in the README (the block containing the lines
starting with "src/" and ending with "test/ integration.test.ts").