kolu-tui: list/snapshot your terminals from the shell (R-4 Phase 1) - #1084
Merged
Conversation
… (R-4 Phase 1)
Phase 1 of the kolu-tui plan: kolu-server now serves its in-process pty-host
router over a unix socket (serveOverStdio), and a new CLI (@kolu/pty-tui)
connects via stdioLink to list/snapshot the live PTYs — the *raw*, terminal-side
client of the same pty-host the browser drives. The web path is byte-identical:
one PTY host, two transports.
@kolu/pty-host:
- createInProcessPtyHost returns {router, client} from one host — directLink
for the web path AND the same router for the socket, so they can never drift.
- getPtyHostSocketPath: the shared default $XDG_RUNTIME_DIR/kolu/pty-host.sock
(tmpdir fallback), one resolver both server and CLI compute identically.
- servePtyHostOverUnixSocket: the socket link the package already promised
("reused over a socket by the surviving daemon"); a net.Socket Duplex IS the
stdio transport. Stale-vs-live handling so a second server can't hijack a live
one's socket.
kolu-server:
- ptyHost.ts instantiates the pty-host once; index.ts adds the listener
(--pty-host-socket override) and cleans up the socket on exit. local.ts now
consumes the shared client — no behavior change.
@kolu/pty-tui (new):
- `kolu-tui list [--json]` and `kolu-tui snapshot <id>`; honest
ECONNREFUSED/ENOENT error when no server is up; --socket override. Packaged
as `nix run github:juspay/kolu#kolu-tui`.
Tests: a real unix-socket round-trip (net.Server + net.Socket + serveOverStdio
+ stdioLink against the actual pty-host router) and pure render-helper coverage.
Docs: README "Terminal UI (beta)" section + architecture rows; website
sneak-preview section.
Hickey finding: the client-only shorthand has no production callers (all code uses createInProcessPtyHost(deps).client) and its presence implies a second, host-discarding way to build a client — a footgun in the two-transport world. Removed from @kolu/pty-host's public export; the function stays as the internal helper inProcessPtyHost.test.ts already imports directly.
…nstruction Hickey flagged serveOverSocket.ts braiding socket lifecycle with the implementSurface-fragment -> contract-router wrap, and proposed moving the wrap to the call site. Lowy cross-validation vetoed the call site (it would fragment the wrap across kolu-server AND the future Phase-B daemon, and leak transport internals into the orchestration layer). Synthesis: hoist the wrap into createInProcessPtyHost as a `servedRouter` field — done once, beside the contract it references, in the package both serving call sites share. serveOverSocket.ts becomes pure socket-transport (no `implement`/`ptyHostSurface` import, no cast); the single fragment->wire `any` now lives at the one host-construction site.
Hickey cross-validation flagged `snapshot <id>` reconstituting one artifact (screen state + identity) from two round-trips: terminalAttach.get for the data, then a second terminal.list purely to decorate the stderr trailer with pid/cwd. The trailer is now derived from the snapshot already in hand (id + line count), so it's a single read — no contract change, scrollback on stdout stays clean.
kolu-tui already treats `help` as an alias for --help (args.command === "help"); the HELP text now lists it so the documented usage matches the behaviour.
The socket listener logged on install ('pty-host socket listening') but not on
close(); add a teardown log so the listener's lifecycle is traceable in the
server log (install + retire), per the watcher-lifecycle-logs rule.
Member
Author
Hickey/Lowy AnalysisPost-implement structural review of
Hickey rationaleThe new Lowy rationaleZero "Fix" findings — the §1 prior-encapsulation survey found each new home correctly placed: Cross-validation (the interesting part)Running each lens over the other's recommendations caught real cross-effects:
Commits: |
Member
Author
EvidenceCaptured on an ephemeral pu box ( 1. Shipped binary runs — 2. Honest error, no server — 3. PR-built server boots — 4. list (empty) — server up, no terminals: 5. Spawn one real terminal over the pty-host socket (Phase 1 6. list (populated) — 7. list --json: 8. snapshot <id> — live shell scrollback on stdout (ANSI-stripped here for readability; the raw stream is genuine VT, e.g. Teardown: server killed, Every step ran against the nix-packaged binaries on an off-machine box, exactly like CI. No output fabricated. |
Member
Author
|
| Step | Status | Duration | Verification |
|---|---|---|---|
| hickey+lowy | ✓ | 20m 27s | hickey 3 findings (1 applied, 2 via cross-validated synthesis); lowy 0 fix; cross-validation vetoed call-site wrap + Connection hoist (No-op), surfaced any-cast + double-RPC; 3 commits |
| police | ✓ | 7m 50s | 3 passes: rules 1 (watcher-lifecycle-logs), fact-check 0, elegance 1 (help alias); 2 commits |
| test | ✓ | 1m 3s | e2e smoke+terminal 15/15 (web path unaffected by local.ts refactor); new behaviour covered by socket round-trip + render unit tests + live CLI e2e |
| create-pr | ✓ | — | draft PR #1084 + hickey/lowy analysis comment (step-start was missed, so it's absent from the timing total) |
| ci | ✓ | 11m 43s | justci all 22 nodes green on HEAD 5e1a17fd (both platforms); 20 required contexts pass |
| evidence | ✓ | 5m 4s | ## Evidence — nix-packaged kolu-tui on a pu box: help, honest error, list/--json/snapshot of a real PTY |
| Total | ~49m |
Slowest step: hickey+lowy (20m 27s)
Optimization suggestions
hickey+lowywas 42% of the run. The cross-validation pass earned its keep here (it vetoed two of Hickey's three first-pass recommendations and reshaped the router-wrap fix into theservedRoutersynthesis), so it's worth keeping — but the fullvitest runre-verification after each review commit overlapped with the latertest/cisteps. Targeted typechecks per review-commit + one suite at the end would trim several minutes.- The first CI box landed with broken egress (
000/timeout). The probe-first guard in.agency/do.mdcaught it before a wasted ~12m run — destroy+recreate cost ~1m. The guard worked; no change needed. - For any re-run,
--from ci-onlyre-runs just CI against HEAD — the draft PR and its comments persist — skipping the ~30m of review/police/test that already landed as commits.
Workflow completed.
`terminal.list` carried only id/pid/cwd/lastActivity, so the title lived on a separate tap. Enrich the list entry with `title` (OSC 0/2) + `foregroundProcess` (additive · optional, contract 2.1) so a one-shot `list` shows a CMD column (title, else the foreground command's basename) without per-row tap fetches, and `--json` carries the full metadata. README + website list mock updated.
…eality items Phase 1 row gets a shipped pill; the 'Contract reality check' panel's socket and list-metadata items are now resolved (the socket exists at the stable path; the list entry carries title + foregroundProcess). Spawn-command + tap-naming items remain for Phases 2-3.
Member
Author
Evidence — video
▶ HD: https://juspay.github.io/video-evidence/evidence.html?repo=juspay/kolu&v=kolu-tui-list.mp4 The nix-packaged |
master's #1091 standardized the workspace on vitest ^4.1.0; pty-tui (added on this branch) carried ^4.1.2, which made pnpm pull a second vitest (4.1.8) into the merged lockfile. Pin to ^4.1.0 so it resolves to the single locked version — no duplicate, and the fetchPnpmDeps hash stays valid.
…list # Conflicts: # default.nix
… bind The socket listener awaited at the end of index.ts rejected on a listen error, so an EADDRINUSE — which the e2e harness hits routinely, since its parallel workers boot many servers sharing the default socket path and race for it — became an unhandledRejection that killed the whole server process (darwin e2e: 'Server did not become healthy'). The socket is an additive convenience for kolu-tui and the web path is independent of it, so servePtyHostOverUnixSocket now swallows every bind failure (live peer, lost race, unwritable dir) into a no-op listener with a warning, never a rejection. Regression test covers the already-served path.
…list # Conflicts: # packages/pty-host/src/index.ts # packages/server/src/index.ts # packages/server/src/terminalBackend/local.ts # pnpm-lock.yaml # website/src/pages/index.astro
Master's homepage rewrite into numbered guide sections dropped the TUI sneak-preview section during the master merge. Re-add it as a deep how-to block in the Power features section (06): kolu-tui list / snapshot, the unix-socket transport, and the nix run invocation.
Member
Author
🧪 CI metrics — leased pool boxThe x86_64-linux lane ran on
Pool status (8 boxes)
Posted by |
…list # Conflicts: # packages/server/src/index.ts
…nds the server On macOS (and non-systemd Linux) there is no $XDG_RUNTIME_DIR, so the socket path fell back to os.tmpdir() — which honours $TMPDIR. A launchd-spawned kolu-server gets a private /var/folders/.../T while a `nix run` CLI gets /tmp, so the two computed DIFFERENT socket paths and never met (the reported "no pty-host socket at /tmp/kolu/pty-host.sock"). Use a fixed, $TMPDIR-independent per-user dir /tmp/kolu-$UID/ (the tmux convention) instead: /tmp is identical in every process on Linux and macOS, and the -$UID suffix keeps it private. New socketPath.test.ts pins the invariant (incl. a $TMPDIR-independence case reproducing the bug). Also folds in correctness/security hardening found while reviewing the path: - serveOverSocket: the additive socket's serveOverStdio() promise REJECTS on a peer reset mid-frame; `void`-ing it let an unhandledRejection crash kolu-server (process.exit(1)) — exactly what the listener promises it won't. Now .catch()-ed. Plus isPrivateOwnedDir(): refuse to serve the full PTY surface from a dir we don't own 0700 (the stable /tmp path could be pre-created by another local user; mkdir's mode is a no-op on an existing dir). - kolu-tui snapshot: dump terminal.getScreenText (rendered text) instead of the terminalAttach first frame (serialized VT escapes), so `snapshot | grep` works. - kolu-tui: handshake system.version + isPtyHostContractCompatible before any command, for an honest "restart your server" on a contract mismatch. - render: sanitizeCell() strips control bytes from attacker-influenceable title/cwd before painting the human table (JSON output stays raw).
When services.kolu.enable is true the module now adds kolu-tui to home.packages, so the CLI is on PATH next to the running server with no extra config. A new services.kolu.tuiPackage option holds the package; the flake's homeManagerModules.default defaults it (via mkDefault) to this flake's matching kolu-tui build, so it ships automatically — set it to null to opt out, or override to pin a build. The example flake's NixOS VM test now asserts `kolu-tui list` succeeds against the running server over $XDG_RUNTIME_DIR/kolu/pty-host.sock — end-to-end proof of both the install and the R-4 Phase 1 CLI on Linux.
Addressed all four CODEX findings in the worktree at /home/srid/code/kolu/.worktrees/modest-runner. F1 was a genuine data-loss bug and is now fixed with a socket-only unlink guard plus a regression test; F2/F3/F4 were valid minors and are all fixed. Verified: pty-host (62) and pty-tui (9) unit tests pass, pty-host/pty-tui/kolu-server typechecks pass, biome format + lint clean on all touched files.
codex (round 1) findings:
- [F1 · major] `isSocketLive()` collapses every connection error to `false`, and the caller then unconditionally `rmSync`s `socketPath`. If `--pty-host-socket` points at an existing regular file in a private directory, or connect fails for `ENOTSOCK`/`EACCES`/other non-stale cases, kolu will delete that file before binding. That is a data-loss bug, not just stale-socket cleanup. (packages/pty-host/src/serveOverSocket.ts:115)
- [F2 · minor] The package root no longer re-exports `createInProcessPtyHostClient`, even though the function still exists and `package.json` only exposes the root subpath. That is a source API regression for any package-root consumer, and the relative imports in this package's tests will not catch it. (packages/pty-host/src/index.ts:37)
- [F3 · minor] `kolu-tui --help` documents the default socket only as `$XDG_RUNTIME_DIR/kolu/pty-host.sock`, but the actual default on macOS/non-systemd is `/tmp/kolu-$UID/pty-host.sock`. The design doc also says "tmpdir() fallback" at `docs/plans/remote-terminals.pty-daemon.tui.html:275`, which contradicts the code's deliberate non-`os.tmpdir()` fallback. (packages/pty-tui/src/main.ts:31)
- [F4 · minor] The contract changed `terminal.list` to carry `title` and `foregroundProcess`, and `kolu-tui list` depends on those fields, but the host/contract tests still only assert id/pid/cwd behavior. A regression that dropped the new metadata at the surface boundary would not be caught. (packages/pty-host/src/inProcessPtyHost.test.ts:92)
claude:
- F1 fixed: Agreed — real footgun. `--pty-host-socket` is an arbitrary user path, and `isSocketLive()` returning false for ENOTSOCK on a regular file would lead the caller to rmSync the user's data. Added `isRemovableStaleSocket(path)` in serveOverSocket.ts (lstatSync(path).isSocket()): only a socket inode (a dead peer's stale file) or ENOENT is removable; a regular file/dir/symlink → warn + return the no-op listener, never unlink. Used lstat (not stat) so a symlink is classified by itself, not followed. Wired the guard immediately before the `rmSync` at the former line 115. Added a falsifiability test in serveOverSocket.test.ts ('refuses to delete an existing regular file at the socket path') that writes a regular file, points the socket at it, and asserts the file and its contents survive both the call and listener.close().
- F2 fixed: Conceded — the root export dropped `createInProcessPtyHostClient` while the function lived on, reachable only via the package's own relative test imports (so the narrow typecheck never caught the dead export). Rather than re-export an API nothing consumes, I took CODEX's second option and removed the helper outright: it is fully superseded by `createInProcessPtyHost` (which `server/src/ptyHost.ts` already uses to build the single shared host). Updated inProcessPtyHost.test.ts to construct via `createInProcessPtyHost(...).client` (and renamed the two describe blocks + header accordingly), and fixed the now-stale doc comment in server/src/terminalBackend/local.ts:13 that referenced the removed symbol. grep confirms zero remaining `createInProcessPtyHostClient` references in packages/. (The two mentions in docs/plans/remote-terminals.pty-daemon.html are historical Phase-A/B planning prose untouched by this branch, so out of scope for this finding.)
- F3 fixed: Agreed — the help text and plan note understated the real default. Updated kolu-tui's HELP in pty-tui/src/main.ts so --socket documents BOTH defaults ($XDG_RUNTIME_DIR/kolu/pty-host.sock on systemd Linux, else /tmp/kolu-$UID/pty-host.sock). Also corrected the tui design doc (docs/plans/remote-terminals.pty-daemon.tui.html:275) which said 'tmpdir() fallback' — replaced with the accurate fixed $TMPDIR-independent /tmp/kolu-$UID/ fallback and the one-line reason it is deliberately NOT os.tmpdir() (launch-context divergence). Chose documenting both literals over rendering getPtyHostSocketPath() since the help is a static string and both branches are now explicit.
- F4 fixed: Agreed — the contract added title/foregroundProcess to terminal.list and kolu-tui list depends on them, but no test asserted they survive the surface boundary. Added a contract-level test in inProcessPtyHost.test.ts ('surfaces title + foregroundProcess on terminal.list') that spawns a real shell through the contract, writes an OSC 2 title sequence followed by `sleep 5` (so the shell prompt doesn't clobber the title before the read and foregroundProcess reflects a live process), polls `client.surface.terminal.list({})` until the async title tap propagates, and asserts entry.title === the set title and typeof entry.foregroundProcess === 'string'. Covers the in-process client path; the socket round-trip in serveOverSocket.test.ts already exercises terminal.list over a real net.Socket. Verified green (7/7 in that file).
Committed by the codex<->claude debate (round 1); not pushed or merged.
Round 2: only F1 remained open (codex held firm; F2/F3/F4 already resolved last round). I conceded codex's remaining point and closed the gap. The round-1 fix stopped data loss for regular files/dirs/symlinks, but isSocketLive() still collapsed every connect error to false, so a real socket inode that failed the probe for a non-stale reason (EACCES/EPERM/etc.) would still be rmSync'd. I replaced the boolean probe + post-hoc inode check with a three-way verdict probeSocket(): SocketProbe — live (never touch), stale (ECONNREFUSED or ENOENT → free to bind), unknown (any other connect error → could-not-prove-dead → no-op, never delete). The caller only reaches rmSync on a stale verdict AND isSocketInodeOrAbsent() confirming a socket/absent inode. Added a sharper falsifiability test that binds a real socket, chmod 0o000's the socket file (verified empirically this yields EACCES on connect while lstat still sees a socket), and asserts the inode survives — a case the round-1 logic would have unlinked. pty-host 63/63 tests pass (+1), tsc clean, biome format+lint clean.
codex (round 2) findings:
- [F1 · major] The fix prevents deletion of regular files, dirs, and symlinks, but `isSocketLive()` still turns every connect error into `false`. The caller then removes the path whenever `lstatSync(path).isSocket()` is true, including unexpected socket errors such as `EACCES`. That still violates the original requirement to remove only stale sockets, not arbitrary socket paths that merely failed the probe for another reason. (packages/pty-host/src/serveOverSocket.ts:59)
- [F2 · minor] Previously the package root dropped `createInProcessPtyHostClient` while the helper still existed. Claude removed the helper and migrated package tests/current code to `createInProcessPtyHost(...).client`, so there is no dangling live API to export. (packages/pty-host/src/index.ts:37)
- [F3 · minor] The help text and TUI design doc now document both socket defaults and correctly describe the `/tmp/kolu-$UID` fallback as `$TMPDIR`-independent rather than `os.tmpdir()`. (packages/pty-tui/src/main.ts:31)
- [F4 · minor] The new contract-level test drives a title through the client and asserts `terminal.list` exposes `title` plus a present `foregroundProcess`, covering the metadata surface boundary that `kolu-tui list` depends on. (packages/pty-host/src/inProcessPtyHost.test.ts:113)
claude:
- F1 fixed: Conceded codex's holdout. The non-stale socket-error path was real: isSocketLive() returned false for EACCES/EPERM, then isRemovableStaleSocket() saw a socket inode (lstat succeeds when only the socket FILE's perms, not the parent dir, are stripped) and the caller rmSync'd a socket it never proved dead. Fix in packages/pty-host/src/serveOverSocket.ts: introduced a discriminated SocketProbe ('live' | 'stale' | 'unknown') and probeSocket() that maps ECONNREFUSED+ENOENT -> stale (free to bind) and EVERY other connect error -> unknown (degrade to no-op, never delete). Caller (servePtyHostOverUnixSocket) now: live -> noop; unknown -> warn+noop (new branch); stale -> only then check isSocketInodeOrAbsent() (renamed from isRemovableStaleSocket) before rmSync. So a socket that fails the probe for a non-stale reason is preserved by the unknown short-circuit. Note: ENOENT had to stay in the stale set (it is the common fresh-start case) or nothing would ever bind — caught and fixed when the round-trip tests regressed to ENOENT. Added falsifiability test 'refuses to delete a real socket inode it could not probe (EACCES, not stale)' in serveOverSocket.test.ts: binds a real net.Server socket, chmod 0o000 on the socket file (empirically yields EACCES on connect, ECONNREFUSED avoided), points --pty-host-socket at it, asserts the inode survives bind+close. This case is unlinked under round-1 logic, preserved now. Guarded with getuid()===0 skip (root bypasses unix perm checks).
Committed by the codex<->claude debate (round 2); not pushed or merged.
Member
Author
Codex ⇄ Claude debate✅ Consensus after 3 round(s) · codex reviewed at Round 1codex — approved: The core socket/TUI shape is reasonable, and narrow typechecks pass for Findings:
claude — Addressed all four CODEX findings in the worktree at /home/srid/code/kolu/.worktrees/modest-runner. F1 was a genuine data-loss bug and is now fixed with a socket-only unlink guard plus a regression test; F2/F3/F4 were valid minors and are all fixed. Verified: pty-host (62) and pty-tui (9) unit tests pass, pty-host/pty-tui/kolu-server typechecks pass, biome format + lint clean on all touched files.
commit: Round 2codex — approved: Claude fixed F2, F3, and F4 adequately. F1 is materially improved for regular files/symlinks/directories, but it still collapses every socket connect error into "not live" and then unlinks any socket inode, so the non-stale socket-error part of the original finding remains open. Findings:
codex on the rebuttal: F1: holding firm. The regular-file data-loss path is fixed, but the original finding also called out EACCES/other non-stale probe errors. The current implementation discards the probe error code, so any socket inode after any connect failure is removed. F2: accepted. Removing the helper is a valid resolution for the live API issue; I'm not keeping this open on the historical planning prose. F3 and F4: accepted as fixed. claude — Round 2: only F1 remained open (codex held firm; F2/F3/F4 already resolved last round). I conceded codex's remaining point and closed the gap. The round-1 fix stopped data loss for regular files/dirs/symlinks, but isSocketLive() still collapsed every connect error to false, so a real socket inode that failed the probe for a non-stale reason (EACCES/EPERM/etc.) would still be rmSync'd. I replaced the boolean probe + post-hoc inode check with a three-way verdict probeSocket(): SocketProbe — live (never touch), stale (ECONNREFUSED or ENOENT → free to bind), unknown (any other connect error → could-not-prove-dead → no-op, never delete). The caller only reaches rmSync on a stale verdict AND isSocketInodeOrAbsent() confirming a socket/absent inode. Added a sharper falsifiability test that binds a real socket, chmod 0o000's the socket file (verified empirically this yields EACCES on connect while lstat still sees a socket), and asserts the inode survives — a case the round-1 logic would have unlinked. pty-host 63/63 tests pass (+1), tsc clean, biome format+lint clean.
commit: Round 3codex — approved: The remaining F1 concern is now addressed in the current worktree: the socket probe preserves the distinction between live, stale, and unknown, and deletion is gated behind both a stale probe verdict and an inode check. F2, F3, and F4 remain resolved from the prior round. I did not run tests because this review round is read-only, but I verified the relevant code and regression coverage by inspection. Findings:
codex on the rebuttal: F1: accepted. The new claude — (no author turn this round) (no author turn — the debate ended this round) |
…ocket: asymmetric naming for the same axis Renamed the kolu-tui CLI override flag from --socket to --pty-host-socket to match the server flag, syncing the resolver doc comment. Agreed by the lowy ⇄ hickey lens debate (finding lowy-3, raised by lowy). Not pushed or merged.
…rap seam to the consumer module Typed createInProcessPtyHost's servedRouter as Router<any, any> instead of bare any, encoding the producer/consumer contract at the serving boundary. Agreed by the lowy ⇄ hickey lens debate (finding lowy-4, raised by lowy). Not pushed or merged.
…+ columnify Two pieces of hand-written slop, swapped for the libraries the rest of the repo would reach for (per the "prefer external libraries over hand-rolled code" convention): - CLI: node:util parseArgs (a hand-rolled command/positional switch + a hand-maintained HELP string) → cleye, the SAME parser kolu-server uses. Real `list` / `snapshot <id>` subcommands, a shared `--pty-host-socket` flag, and auto-generated `--help` / `--version` / per-subcommand help — no hand-kept usage text to drift. - Table: the bespoke width()/padEnd/padStart/line() column math in render.ts → columnify (borderless `docker ps`-style aligned columns). formatList just builds row objects and hands them over; trailing pad is trimmed per line. All 9 render tests pass unchanged (columnify's output matches the prior layout); pty-tui + pty-host typecheck and unit suites green; `nix build .#kolu-tui` runs end-to-end (deps resolve through the workspace closure). pnpm-lock + the fetchPnpmDeps hash regenerated for the two new deps.
The shell CLI isn't ready to announce as a user-facing feature yet, so pull the README "Terminal UI (beta)" section and the website Power-features sneak-preview. The package still ships and the architecture/reference docs (and the design plan) keep describing it for contributors — this only removes the "here's a feature you can use" framing.
Migrate `remote-terminals.pty-daemon.tui.html` (the kolu-tui design) into the Atlas as `pty-daemon-tui.mdx` — flat slug, frontmatter (kind: feature, status: accepted), a D2 of the in-process server / two-transport architecture, the subcommand + phasing tables, Terminal mocks for the flow, and the headless test. Renders to docs/atlas/dist/pty-daemon-tui.html (check-sync green). Delete the legacy HTML and repoint every back reference at the Atlas note: - docs/plans/README.md — move tui to the "migrated" list - docs/plans/remote-terminals.pty-daemon.html — companion link → dist HTML - packages/pty-tui/src/main.ts, packages/surface/example/mini-ci/src/common/surface.ts — header comments - packages/surface/README.md, packages/surface/example/mini-ci/README.md — links
…ty-tui
Promote the kolu-tui transport work into @kolu/surface so the link
family covers local IPC, and fix the serveOverStdio crash footgun at
its source:
- New `@kolu/surface/unix-socket`: `serveOverUnixSocket` (outcome-based,
never-rejecting socket serving with the stale-probe + inode-guard +
dir-privacy hardening) and `getRuntimeSocketPath` (the XDG //tmp-$UID
rendezvous convention). New `@kolu/surface/links/unix-socket`:
`unixSocketLink`, the dialing client half.
- `serveOverStdio` now resolves with `{reason: "end"|"error"}` instead
of rejecting on a read-stream error — a rejecting serve promise was an
unhandled-rejection crash for multi-peer hosts (it bit kolu-server
twice); pinned by peer-server.test.ts.
- `isContractVersionCompatible` in `@kolu/surface/define` — the generic
major.minor handshake predicate; `isPtyHostContractCompatible`
delegates to it.
- pty-host's socketPath.ts + serveOverSocket.ts become thin kolu-voiced
wrappers (outcome → operator log copy); pty-tui dials via
unixSocketLink. Transport hardening tests move to surface
(genericized); pty-host keeps the contract round-trip pins.
…e docs/plans Migrate the legacy remote-terminals monolith family to compressed Atlas notes capturing current state: - remote-terminals.html (208 KB) → atlas/remote-terminals.mdx — phases at a glance (R-1/R-1.5/R-1.6 shipped · R-4 in progress · R-2/R-3 next), volatility axes, the six prototype lessons, per-phase shipped records, and the #994 retros. - remote-terminals.pty-daemon.html → atlas/pty-daemon.mdx — the R-4 plan of record: the hazard-phased A1/A2/B decomposition (A1 #1055 + A2 #1063 landed, B next), the #1034 postmortem + hard constraints, and the carry-forward design notes. - remote-terminals.pty-daemon.chrome-bar.html → atlas/pty-daemon-chrome-bar.mdx — the srv·pty rail, updated to reflect that A2 shipped it (status: implemented, IdentityRail.tsx). Back references updated: pty-daemon-tui.mdx gains parents+links, ghostex-vs-remote-terminals.mdx and second-brain.mdx repointed (the plans-migration milestone is done), code-comment pointers in pty-host / local.ts / mini-ci README repointed, and the docs/** rule now says docs/plans is retired (regenerated via apm). dist/ rebuilt; atlas::check-sync green.
Addressed all three CODEX findings in packages/surface (plus a tightened pty-host test). F1 and F2 were real hardening/contract gaps in the unix-socket transport; F3 was a real regex anchoring gap. All agreed and fixed, with regression tests. surface unix-socket tests: 15 passed; pty-host contract tests: 8 passed. Ran `just fmt`.
codex (round 1) findings:
- [F1 · major] The directory privacy check uses `statSync`, which follows symlinks. That means another local user can pre-create the stable `/tmp/<app>-$UID` rendezvous directory as a symlink to any existing owner-private directory for the victim. The check then passes, but the path component in `/tmp` is still attacker-owned and can be replaced later, letting the attacker redirect future clients to a different socket. This undercuts the security invariant the comments rely on for the full-control PTY socket. (packages/surface/src/unix-socket.ts:87)
- [F2 · minor] The `not-a-socket` outcome is not reliably returned for non-socket paths. The code classifies any non-`ECONNREFUSED`/`ENOENT` probe error as `probe-failed` before checking the inode, while the comments explicitly call out `ENOTSOCK` on regular files as an unknown probe error. On platforms that return `ENOTSOCK`, a regular file is reported as an unexpected probe failure instead of the machine-readable `not-a-socket` outcome. (packages/surface/src/unix-socket.ts:222)
- [F3 · minor] `isContractVersionCompatible` parses versions with an unanchored `/^(\d+)\.(\d+)/` regex, so malformed strings like `2.1garbage` are accepted as compatible even though the doc says only patch/prerelease suffixes are tolerated. Since this is now the shared process-boundary handshake helper, accepting invalid version strings weakens the fail-fast behavior it is meant to provide. (packages/surface/src/define.ts:672)
claude:
- F1 fixed: Agreed — `isPrivateOwnedDir` (unix-socket.ts:84-97) used `statSync`, which follows symlinks, so an attacker-owned `/tmp/<app>-$UID` symlink pointing at any owner-private dir would pass the privacy check while leaving the attacker owning the /tmp path component (swappable later to redirect clients). Switched to `lstatSync` and now require `st.isDirectory()` in addition to uid-owned + no group/other bits, so any symlink (or non-dir inode) is rejected outright. Dropped the now-unused `statSync` import. Added a regression test in unix-socket.test.ts that pre-creates the socket dir as a symlink to a genuinely 0700 owner-private dir and asserts the `dir-not-private` refusal with no socket created. mkdirSync(recursive) is a no-op against the pre-existing symlink target so it doesn't throw before the check runs.
- F2 fixed: Agreed — when `probeSocket` returns `{kind:"unknown"}` (which the comments note includes ENOTSOCK on a regular file), serve refused with the opaque `probe-failed` before any inode classification, so a regular file at the path could surface as `probe-failed` instead of the machine-readable `not-a-socket`. Added `isNonSocketInode(path)` (lstat-based) and, in the `unknown` branch (unix-socket.ts:~230), return `{kind:"not-a-socket"}` when lstat proves a non-socket inode; a truly unclassifiable inode (ENOENT or lstat failure) stays `probe-failed`. Tightened the existing regular-file test from `outcome.kind !== "listening"` to assert the exact `{kind:"not-a-socket"}` outcome, covering both the ENOTSOCK-via-lstat route and the stale-then-inode-guard route.
- F3 fixed: Agreed — `isContractVersionCompatible` (define.ts:671) parsed with the unanchored `/^(\d+)\.(\d+)/`, so `2.1garbage` parsed as 2.1 instead of being rejected, weakening the fail-fast handshake. Replaced with the anchored grammar `^(\d+)\.(\d+)(?:\.\d+)?(?:-[0-9A-Za-z.-]+)?$` (major.minor with an optional patch/prerelease suffix only). Added regression cases in ptyHostSurface.test.ts (the thin wrapper that calls this helper) asserting `2.1garbage`, `2.0garbage`, `2.0.1.2`, and `2.0 ` are all incompatible; the existing valid-suffix cases (`2.0.1`, `2.0.0-rc1`) still pass.
Committed by the codex<->claude debate (round 1); not pushed or merged.
Member
Author
Codex ⇄ Claude debate✅ Consensus after 2 round(s) · codex reviewed at Round 1codex — approved: The extraction is directionally sound, and the socket transport has useful safety tests. I found one real local-IPC hardening gap plus a couple of smaller API/contract precision issues in the new shared surface code. I did not run the socket tests because this review environment is read-only and those tests create temp files/sockets. Findings:
claude — Addressed all three CODEX findings in packages/surface (plus a tightened pty-host test). F1 and F2 were real hardening/contract gaps in the unix-socket transport; F3 was a real regex anchoring gap. All agreed and fixed, with regression tests. surface unix-socket tests: 15 passed; pty-host contract tests: 8 passed. Ran
commit: Round 2codex — approved: Verified the current fixes for F1-F3 against the changed files. The Unix socket directory check now rejects symlinks via Findings:
codex on the rebuttal: No disputes from CLAUDE. I agree with all three fixes after inspection and mark F1, F2, and F3 resolved. claude — (no author turn this round) (no author turn — the debate ended this round) |
…nverted lstat predicates Collapsed the two inverted lstat helpers (isSocketInodeOrAbsent, isNonSocketInode) in unix-socket.ts into one classifyInode helper returning "socket"|"absent"|"other", with both serve-flow branches reading classifyInode(socketPath) === "other". Agreed by the lowy ⇄ hickey lens debate (finding hickey-1, raised by hickey). Not pushed or merged.
…s the outcome union the wrapper already destructures
Replaced refusalWarning with describeRefusal returning {msg, ctx} per outcome kind, folding the wrapper's per-kind ctx if-cascade into one exhaustive switch.
Agreed by the lowy ⇄ hickey lens debate (finding hickey-3, raised by hickey). Not pushed or merged.
Member
Author
⚖️ Lowy ⇄ Hickey lens debate✅ Consensus after 1 round(s) · lowy + hickey · base Independent findings: lowy=4, hickey=5 Applied (2)
Agreed — no change (7)
|
The wrapper added no pty-host semantics over @kolu/surface/define's isContractVersionCompatible and had a single caller — kolu-tui now imports the generic predicate directly. The duplicated generic version-grammar tests left ptyHostSurface.test.ts with the pty-host self-compatibility pin staying put.
Member
Author
👮 Code-policeReviewed the surface-upstreaming diff (base Fixed (1):
Checked, not violations:
Verified after the fix: pty-host 53/53, pty-tui 9/9, |
…list # Conflicts: # docs/atlas/dist/index.html
machinectl shell forwards its stdin to the session PTY, and the nixos test driver's stdin pipe never EOFs — so the kolu-tui attempt never returned even after the CLI exited, and wait_until_succeeds (which bounds only the retry loop, not one attempt) hung the whole lane. Redirect stdin from /dev/null (machinectl now returns the moment kolu-tui exits) and bound each attempt with an in-guest `timeout 30` so no future wedge can hang the lane again. Verified: the VM test passes locally in 23s.
Member
Author
🧪 CI metrics — leased pool boxThe x86_64-linux lane ran on
Pool status (8 boxes)
Posted by |
Member
Author
🧪 CI metrics — leased pool boxThe x86_64-linux lane ran on
Pool status (8 boxes)
Posted by |
srid
marked this pull request as ready for review
June 9, 2026 21:49
srid
added a commit
to srid/drishti
that referenced
this pull request
Jun 9, 2026
srid
added a commit
that referenced
this pull request
Jun 9, 2026
PR #1084 (master) added serveOverUnixSocket / unixSocketLink — the hardened version of what coordinator/socket.ts hand-rolled (probe, stale reclaim, per-connection serveOverStdio). odu now consumes them, keeping its checkout-scoped .ci/odu.sock path and translating the library's outcomes (already-served = the one-run-per-checkout lock; a dial failure = no run in progress). .ci is tightened to 0700 — the library refuses to serve a full-control router from a world-readable directory. Also adopts serveOverStdio's new settled ServeOverStdioEnd result in the runner.
srid
added a commit
to srid/drishti
that referenced
this pull request
Jun 9, 2026
…ing) (#57) * chore(kolu): track @kolu/surface's unix-socket upstreaming (juspay/kolu#1084) Bump the kolu pin to the r4-phase1-kolu-tui-list branch head, which upstreams the unix-socket transport into @kolu/surface and changes serveOverStdio's contract: it now resolves with a ServeOverStdioEnd ({reason: "end" | "error"}) instead of rejecting on a read-stream error (a peer reset is an ordinary lifecycle event, not an unhandled-rejection crash). The agent's injectable `Serve` type widens its resolution to `unknown` accordingly — the agent only awaits serving's end, not its value, and test fakes may still resolve void. Mirror PR per kolu's surface-sharing rule; the pin moves back to kolu master once juspay/kolu#1084 merges. * chore(kolu): pin to master 4f8c3ce now that juspay/kolu#1084 has merged
srid
added a commit
that referenced
this pull request
Jun 11, 2026
**Every Atlas note re-audited against current master and GitHub state; 93 confirmed staleness items fixed across 22 notes.** Driven by a two-stage agent workflow: one auditor per note checked every factual claim (status pills, PR states via `gh`, code cites against the working tree), then an adversarial verifier independently re-checked each finding before any edit — 13 suggested fixes were corrected or rejected at that stage. ### The load-bearing corrections - **`remote-terminals` / `pty-daemon-tui`** — the R-4 row now credits kolu-tui Phases 0–2 (#1073 / #1084 / #1255, the last merged today); `list --json` dropped from the Phase 3 row (it shipped in Phase 1); the attach loop, `requirePty` NOT_FOUND nicety, and package-size figure recast from plan tense to shipped history. *Next in remote-terminals remains pty-daemon **Phase B** — both notes already said so correctly.* - **`anyforge`** — un-parented from `remote-terminals` (multi-forge is not part of that feature — it was misfiled at birth); phase 0b (#1257) marked shipped; the pre-extraction code claims (`startGitHubPrProvider`, the kolu-common→kolu-github wire coupling, the schemas-header promotion note) recast to past tense with cites re-pointed. - **Everything else** — stale "todo/next" pills for work that shipped (#1093, #1155, #1162, #1190, #1191, #1199, #1212, #1216, #1219, #1231 …), dead cites to moved/deleted files (`iframePreviewNav.ts`, `.claude/rules/workflow.md`, drifted line pins), and internal contradictions left by partial past updates. `herdr-vs-kolu` alone had 14. > **Bug found along the way:** three notes' frontmatter `description:` contained ` #NNNN` as an unquoted YAML scalar — YAML treats whitespace+`#` as a comment start, so the rendered meta descriptions were silently truncated mid-sentence. Those descriptions are now quoted (`mini-ci-vs-justci`, `nix-typecheck-gate`, `pty-daemon`). _Eight notes audited clean with zero findings (`pty-daemon`, `surface-connection`, `surface-mcp`, `correctness-review`, `ghostex-vs-remote-terminals`, `md-preview-relative-links`, `md-preview-wikilinks`, `pty-daemon-chrome-bar`)._ `dist/` regenerated via `just atlas::build`; `check-sync` green locally. _Generated by an ultracode audit workflow on Claude Code (model `claude-fable-5`)._
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
1 participant
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.
kolu-server now serves its in-process pty-host over a unix socket, and a new
kolu-tuiCLI lists and snapshots your live terminals from the shell — no browser. It's the raw, terminal-side client of the same pty-host the browser drives over the full contract, and R-4 Phase 1 of the kolu-tui plan. The web path is byte-identical: one PTY host, two transports.The CLI (
@kolu/pty-tui)Read-only this phase —
attach/spawn/killare later phases. The CLI comes and goes; kolu-server keeps owning the PTYs. Same CLI framework as kolu-server (cleye), table rendering via columnify.kolu-tui list [--json]cmd= the OSC title, else the foreground command);--jsoncarries the full entry incl. raw title + foreground processkolu-tui snapshot <id>--pty-host-socket <path>points at a non-default server; an unreachable socket is an honest one-line error (ECONNREFUSED/ENOENT→ "is kolu-server running?"), never a silent hang. Packaged asnix run github:juspay/kolu#kolu-tuiand auto-installed by the home-manager module. See the Evidence — video comment for a recording.Upstreamed into
@kolu/surface(mirror: srid/drishti#57)The transport work was generic, so it lives in the surface library — completing the link family (websocket · stdio · direct · unix-socket):
@kolu/surface/unix-socket—serveOverUnixSocket(outcome-based, never-rejecting socket serving: dir-privacy gate, live-peer probe, stale-inode clearing that refuses to unlink anything not proven a dead socket) +getRuntimeSocketPath(the$XDG_RUNTIME_DIR/<app>//tmp/<app>-$UIDrendezvous convention).@kolu/surface/links/unix-socket—unixSocketLink, the dialing client half.serveOverStdiono longer rejects when a peer's read stream errors — it resolves with{ reason: "end" | "error" }. A rejecting serve promise was an unhandled-rejection crash footgun for multi-peer hosts (it bit kolu-server twice during this PR's review); now the no-crash path is the default for every consumer.isContractVersionCompatiblein@kolu/surface/define— the genericmajor.minorhandshake predicate;isPtyHostContractCompatibledelegates.kolu's
socketPath.ts/serveOverSocket.tsare now thin wrappers that map transport outcomes to kolu-voiced operator log lines. Per the surface-sharing rule, srid/drishti#57 adapts drishti and pins it to this branch to prove API compatibility.How it fits together
@kolu/pty-hostgainscreateInProcessPtyHost— builds the host once and returns itsclient(the no-wiredirectLinkweb client) and aservedRouter(the contract-wrapped form serving needs), so one host backs both transports and can never be instantiated twice.getPtyHostSocketPathis the single resolver server and CLI share.EADDRINUSErace when parallel servers share the default socket, an unwritable runtime dir) degrades to a logged no-op, never a rejection. (This one bit the e2e harness, where many servers share$XDG_RUNTIME_DIR; caught in CI, hardened here.)list: theterminal.listentry was enriched withtitle(OSC 0/2) +foregroundProcess(additive · optional, contract 2.1), so a one-shotlistshows thecmdcolumn without per-row tap subscriptions.kolu-serverinstantiates the pty-host once (ptyHost.ts) and adds one additive socket listener inindex.ts;local.tsnow consumes the shared client. Nothing about the web path changes.Tests & docs
@kolu/surface'sunix-socket.test.ts+ the pty-host contract round-trip inserveOverSocket.test.ts), the never-rejects regression pin forserveOverStdio, data-loss refusal tests (regular file / unprobeable socket), rendezvous-path$TMPDIR-independence pins, render-helper coverage, and e2esmoke+terminalconfirming the web path is unaffected.packages/surface/README.mdsynced (the link family + unix-socket transport reference). The kolu-tui announcements were removed from README/website — it isn't ready for users yet; contributor-facing architecture rows remain.docs/plans/*.htmlwere ported to Atlas notes (pty-daemon-tui,pty-daemon,remote-terminals,pty-daemon-chrome-bar);docs/plans/is retired.Notes
/be-reviewgauntlet (codex-debate → lens-debate → code-police) ran on the pre-upstreaming shape — see the review comments.Try it locally
🤖 Generated with Claude Code
Generated by
/do+/beon Claude Code (modelclaude-fable-5).