feat(desktop): fresh Mach context PTY spawn + daemon-owned lifecycle#3575
Open
Haknt wants to merge 27 commits intosuperset-sh:mainfrom
Open
feat(desktop): fresh Mach context PTY spawn + daemon-owned lifecycle#3575Haknt wants to merge 27 commits intosuperset-sh:mainfrom
Haknt wants to merge 27 commits intosuperset-sh:mainfrom
Conversation
This was referenced Apr 19, 2026
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a macOS "fresh-spawn" subsystem: an Electron-hosted UDS spawn server, client/session API, NDJSON protocol and handlers, a Changes
Sequence Diagram(s)sequenceDiagram
participant ElectronMain as Electron Main
participant TerminalHost as Terminal-Host Daemon
participant SpawnServer as Spawn Server (UDS)
participant PTYSubprocess as PTY Subprocess
ElectronMain->>TerminalHost: startServer()
TerminalHost->>SpawnServer: startFreshSpawnServer()
SpawnServer->>SpawnServer: Create UDS socket\nGenerate auth token
Note over TerminalHost: User opens terminal tab
TerminalHost->>SpawnServer: openSpawnSession() (UDS + token)
SpawnServer->>SpawnServer: Verify token & dispatch
SpawnServer->>PTYSubprocess: spawn PTY subprocess (node-pty)
SpawnServer->>TerminalHost: {type:"ok", pid} handshake
TerminalHost->>SpawnServer: NDJSON stdin frames
SpawnServer->>PTYSubprocess: write to PTY stdin
PTYSubprocess->>SpawnServer: NDJSON stdout/stderr frames
SpawnServer->>TerminalHost: framed stdout/stderr (base64)
PTYSubprocess->>SpawnServer: NDJSON exit frame
SpawnServer->>TerminalHost: {type:"exit", code, signal}
sequenceDiagram
participant Zsh as zsh Shell
participant FreshExecHook as zsh Hook
participant FreshExecBin as fresh-exec Binary
participant SpawnServer as Spawn Server
participant RemotePTY as Remote PTY Process
Zsh->>Zsh: user types `gh auth status`
Zsh->>FreshExecHook: function override intercepts
FreshExecHook->>FreshExecBin: exec fresh-exec gh auth status
FreshExecBin->>SpawnServer: sendSpawnRequest(fresh-exec + token)
SpawnServer->>RemotePTY: handleFreshExec spawn in fresh context
RemotePTY->>SpawnServer: NDJSON stdout/stderr frames
SpawnServer->>FreshExecBin: stream frames (handshake + NDJSON)
FreshExecBin->>Zsh: bridge stdout/stderr/stdin
RemotePTY->>SpawnServer: exit frame
SpawnServer->>FreshExecBin: exit status
FreshExecBin->>Zsh: exit with remote status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR introduces a daemon-owned fresh-Mach-context PTY spawn server to fix stale Mach bootstrap context failures for Go-based CLIs ( Key architectural choices validated by the review:
Issues found:
Confidence Score: 4/5Safe to merge; all findings are P2 suggestions with no primary user path broken and no data-loss or security risk at the threat model in play. The architecture is well-reasoned, thoroughly tested (65 unit tests, 41 terminal-host tests, live four-assertion integration test), and the core protocol, auth, and fallback paths are correct. The issues found — missing
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/fresh-spawn/spawn-server.ts | New UDS server: clean NDJSON protocol, token auth, idle timeout, handler dispatch; stale socket cleanup on startup is correct |
| apps/desktop/src/main/fresh-spawn/spawn-session.ts | ChildProcess-compatible SpawnSession wrapper; missing close handler in handshake phase causes up to 2 s stall on abrupt server close |
| apps/desktop/src/main/fresh-spawn/spawn-client.ts | Single-shot spawn request helper; missing close event handler means promise hangs until full timeout on graceful-but-empty server close |
| apps/desktop/src/main/fresh-spawn/auth.ts | Token generation (0600 file) and timing-safe verification; early-return on length mismatch comment is misleading but practically unexploitable for fixed-length tokens |
| apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts | Spawns pty-subprocess.js as daemon child; bidirectional NDJSON frame forwarding, SIGTERM→SIGKILL grace period on client disconnect, finalize-on-both-closed pattern is correct |
| apps/desktop/src/main/fresh-spawn/handlers/fresh-exec.ts | node-pty based interactive command handler; signal reported as numeric string ("15") rather than name ("SIGTERM"), inconsistent with spawn-pty-subprocess handler |
| apps/desktop/src/main/terminal-host/session.ts | spawn() correctly made async, trySpawnViaFreshServer fallback wiring is sound; all call sites in terminal-host.ts and tests updated to await |
| apps/desktop/src/main/terminal-host/signal-handlers.ts | SIGTERM and SIGHUP now no-ops (daemon survival semantics); well-documented, SIGKILL and shutdown RPC remain as kill paths |
| apps/desktop/src/resources/shell-hooks/zsh-fresh-exec.zsh | eval with env-var-derived command names; SUPERSET_FRESH_EXEC_ACTIVE recursion guard is correct, but no identifier sanitization before eval |
| apps/desktop/src/main/fresh-spawn/types.ts | Zod-validated discriminated unions for request/response/stream frames; schema is thorough and well-typed |
| apps/desktop/src/main/fresh-spawn/lifecycle.ts | Idempotent server start/stop with graceful error swallowing; module-level singleton is appropriate for the daemon entry point |
| apps/desktop/src/main/fresh-spawn/fresh-exec.ts | CLI bridge binary with clean fallback to direct exec; SIGWINCH forwarding and raw-mode stdin handling are correct |
| apps/desktop/src/main/terminal-host/fresh-spawn-integration.ts | Thin glue layer; platform guard, existence checks, and error swallowing for graceful fallback are all correct |
| apps/desktop/src/main/terminal-host/index.ts | Server started at daemon boot, stopped before teardown; __dirname-relative pty-subprocess.js path resolution is correct for both dev and packaged builds |
Sequence Diagram
sequenceDiagram
participant D as Terminal-Host Daemon
participant SS as Spawn Server (UDS)
participant S as session.ts
participant SP as pty-subprocess.js
participant FE as fresh-exec CLI
Note over D: startFreshSpawnServer() at daemon boot
D->>SS: startSpawnServer({ socketPath, tokenPath, subprocessScriptPath })
SS-->>D: listening on ~/.superset/fresh-spawn.sock
Note over S: session.spawn() called (async)
S->>S: trySpawnViaFreshServer()
S->>SS: connect UDS + {type:spawn-pty-subprocess, token, env}
SS->>SS: verifyToken()
SS->>SP: child_process.spawn(electronPath, [pty-subprocess.js])
SS-->>S: {type:ok, pid}
Note over S: SpawnSession returned — handshake done
loop I/O streaming (NDJSON frames)
SP-->>SS: stdout/stderr bytes
SS-->>S: {type:stdout, data:base64}
S-->>SS: {type:stdin, data:base64}
SS-->>SP: stdin write
end
SP-->>SS: process exit
SS-->>S: {type:exit, code, signal}
SS->>SS: client.end()
Note over S: fallback (non-macOS or server down)
S->>SP: child_process.spawn(electronPath, [pty-subprocess.js])
Note over FE: whitelisted CLI typed in stale terminal
FE->>SS: connect UDS + {type:fresh-exec, command:gh, token, env}
SS->>SS: verifyToken()
SS->>SP: node-pty.spawn(gh, args)
SS-->>FE: {type:ok, pid}
loop PTY I/O
SP-->>SS: onData (PTY merged stream)
SS-->>FE: {type:stdout, data:base64}
FE-->>SS: {type:stdin/resize/signal}
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/fresh-spawn/spawn-client.ts
Line: 62-78
Comment:
**Missing `close` handler — promise hangs until timeout on server-side crash**
`spawn-client.ts` listens for `"error"` and `"data"` but not `"close"`. If the server accepts the connection and then closes it without writing a response (e.g. crashes between `net.createServer` accept and the first `writeResponse` call), Node.js emits `"close"` on the client socket **without** emitting `"error"`. The promise then hangs for the full `timeoutMs` (default 5 s) before the fallback triggers.
Adding a `"close"` listener that calls `settle(() => reject(...))` would reject immediately in that scenario, keeping the fresh-spawn fallback sub-millisecond:
```typescript
client.once("close", () => {
settle(() =>
reject(new Error("server closed connection before sending a response")),
);
});
```
Place this alongside the existing `client.once("error", ...)` registration.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/main/fresh-spawn/spawn-session.ts
Line: 116-131
Comment:
**Missing `close` handler during handshake phase — 2 s stall on abrupt server close**
During the `onHandshakeData` phase, `client.once("error", ...)` handles connection-refused / ECONNRESET errors, but a **graceful** server-side `client.end()` before writing the `{type:"ok"}` line (e.g. from a mid-startup server crash) emits `"close"` without `"error"`. The promise then blocks until the `handshakeTimeoutMs` timer fires — 2 000 ms in `trySpawnViaFreshServer`, which is the hot path for every PTY session creation.
Add a `"close"` listener in the handshake phase:
```typescript
client.once("close", () => {
if (!handshakeDone) {
clearTimeout(timer);
client.destroy();
reject(new Error("server closed connection before handshake completed"));
}
});
```
(Remove this listener—or make it a no-op—once `handshakeDone` is set.)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/main/fresh-spawn/auth.ts
Line: 24-29
Comment:
**Timing oracle comment is misleading — early return on length mismatch leaks token length**
The docstring says "Returns false on length mismatch **without** timing leak" but the early return on `received.length !== expected.length` is itself a timing oracle: an attacker who can measure response times can distinguish "wrong length" (fast path) from "correct length, wrong bytes" (slow `timingSafeEqual` path) and thereby determine the expected token length.
In this codebase the token is always `crypto.randomBytes(32).toString("base64url")` (43 chars), so the length is a public constant and the practical exploit impact is negligible. Still, the comment should be accurate:
```typescript
/**
* Constant-time token comparison.
*
* Note: a length mismatch short-circuits before `timingSafeEqual`, leaking
* whether the token is the right length. This is acceptable here because
* token length is a fixed constant (43 chars). Do not reuse this function
* for variable-length tokens.
*/
export function verifyToken(received: string, expected: string): boolean {
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/resources/shell-hooks/zsh-fresh-exec.zsh
Line: 39-46
Comment:
**`eval` with env-var-derived content — injection risk if `SUPERSET_FRESH_EXEC_COMMANDS` is poisoned**
The hook splits `$SUPERSET_FRESH_EXEC_COMMANDS` via `${(z)...}` (zsh word-splitting) and passes each word directly into an `eval "function ${_superset_cmd}() { ... }"`. Zsh's `(z)` flag splits on shell quoting boundaries, not just whitespace — so a poisoned env var containing shell metacharacters (e.g. `gh); curl evil.example.com; function noop`) could inject arbitrary shell code into the `eval`.
In practice, Superset controls `SUPERSET_FRESH_EXEC_COMMANDS` (set in `env.ts` from the hardcoded `FRESH_EXEC_WHITELIST`), so the attack surface is limited to a local privilege escalation by an attacker who can already set arbitrary env vars. Still, the `eval` makes the hook fragile if the whitelist is ever sourced from an external config or if users manually source the hook file.
At minimum, add a guard that skips tokens that don't look like simple command identifiers (`^[a-zA-Z0-9_-]+$`) before passing them to `eval`:
```zsh
for _superset_cmd in ${(z)SUPERSET_FRESH_EXEC_COMMANDS}; do
[[ $_superset_cmd =~ ^[a-zA-Z0-9_-]+$ ]] || continue
eval "function ${_superset_cmd}() { ... }"
done
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/main/fresh-spawn/handlers/fresh-exec.ts
Line: 165-171
Comment:
**Signal reported as numeric string `"15"` — inconsistent with `spawn-pty-subprocess` which uses signal names `"SIGTERM"`**
`node-pty` delivers `signal` as a number on POSIX, so `String(signal)` produces `"15"`, `"9"`, etc. By contrast, `handlers/spawn-pty-subprocess.ts` forwards the `child_process` string signal name (`"SIGTERM"`, `"SIGKILL"`, etc.) directly.
`fresh-exec.ts`'s `bridgeSocketToStdio` correctly handles numeric strings via `Number.parseInt(exit.signal, 10)`, so the client-facing exit-code computation is correct. But any other consumer of a `SpawnSession` backed by `fresh-exec` will see a raw number string instead of a POSIX signal name.
Consider converting using Node's `os.constants.signals` reverse-map:
```typescript
import * as os from "node:os";
const sigNumToName: Record<number, string> = Object.fromEntries(
Object.entries(os.constants.signals).map(([name, num]) => [num, name]),
);
// In onExit:
signal: typeof signal === "number" ? (sigNumToName[signal] ?? String(signal)) : null,
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(daemon): ignore SIGTERM in addition ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+62
to
+78
| } catch { | ||
| // already destroyed — ignore | ||
| } | ||
| fn(); | ||
| }; | ||
|
|
||
| const timer = setTimeout(() => { | ||
| settle(() => | ||
| reject(new Error(`spawn request timeout after ${timeoutMs}ms`)), | ||
| ); | ||
| }, timeoutMs); | ||
|
|
||
| client.once("error", (err) => { | ||
| settle(() => reject(err)); | ||
| }); | ||
|
|
||
| client.once("connect", () => { |
There was a problem hiding this comment.
close handler — promise hangs until timeout on server-side crash
spawn-client.ts listens for "error" and "data" but not "close". If the server accepts the connection and then closes it without writing a response (e.g. crashes between net.createServer accept and the first writeResponse call), Node.js emits "close" on the client socket without emitting "error". The promise then hangs for the full timeoutMs (default 5 s) before the fallback triggers.
Adding a "close" listener that calls settle(() => reject(...)) would reject immediately in that scenario, keeping the fresh-spawn fallback sub-millisecond:
client.once("close", () => {
settle(() =>
reject(new Error("server closed connection before sending a response")),
);
});Place this alongside the existing client.once("error", ...) registration.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/fresh-spawn/spawn-client.ts
Line: 62-78
Comment:
**Missing `close` handler — promise hangs until timeout on server-side crash**
`spawn-client.ts` listens for `"error"` and `"data"` but not `"close"`. If the server accepts the connection and then closes it without writing a response (e.g. crashes between `net.createServer` accept and the first `writeResponse` call), Node.js emits `"close"` on the client socket **without** emitting `"error"`. The promise then hangs for the full `timeoutMs` (default 5 s) before the fallback triggers.
Adding a `"close"` listener that calls `settle(() => reject(...))` would reject immediately in that scenario, keeping the fresh-spawn fallback sub-millisecond:
```typescript
client.once("close", () => {
settle(() =>
reject(new Error("server closed connection before sending a response")),
);
});
```
Place this alongside the existing `client.once("error", ...)` registration.
How can I resolve this? If you propose a fix, please make it concise.
Author
There was a problem hiding this comment.
Deferred as low-urgency: sendSpawnRequest is a single-shot probe used for fire-and-forget health checks. The 5s default timeout is acceptable for its callers (which don't gate anything user-visible on this). Happy to revisit if this becomes a user-facing latency concern.
Comment on lines
+116
to
+131
| } | ||
|
|
||
| const line = buffer.slice(0, newlineIdx); | ||
| const remainder = buffer.slice(newlineIdx + 1); | ||
| buffer = ""; | ||
|
|
||
| let parsed: unknown; | ||
| try { | ||
| parsed = JSON.parse(line); | ||
| } catch (err) { | ||
| clearTimeout(timer); | ||
| client.destroy(); | ||
| reject( | ||
| new Error( | ||
| `invalid handshake response JSON: ${ | ||
| err instanceof Error ? err.message : String(err) |
There was a problem hiding this comment.
close handler during handshake phase — 2 s stall on abrupt server close
During the onHandshakeData phase, client.once("error", ...) handles connection-refused / ECONNRESET errors, but a graceful server-side client.end() before writing the {type:"ok"} line (e.g. from a mid-startup server crash) emits "close" without "error". The promise then blocks until the handshakeTimeoutMs timer fires — 2 000 ms in trySpawnViaFreshServer, which is the hot path for every PTY session creation.
Add a "close" listener in the handshake phase:
client.once("close", () => {
if (!handshakeDone) {
clearTimeout(timer);
client.destroy();
reject(new Error("server closed connection before handshake completed"));
}
});(Remove this listener—or make it a no-op—once handshakeDone is set.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/fresh-spawn/spawn-session.ts
Line: 116-131
Comment:
**Missing `close` handler during handshake phase — 2 s stall on abrupt server close**
During the `onHandshakeData` phase, `client.once("error", ...)` handles connection-refused / ECONNRESET errors, but a **graceful** server-side `client.end()` before writing the `{type:"ok"}` line (e.g. from a mid-startup server crash) emits `"close"` without `"error"`. The promise then blocks until the `handshakeTimeoutMs` timer fires — 2 000 ms in `trySpawnViaFreshServer`, which is the hot path for every PTY session creation.
Add a `"close"` listener in the handshake phase:
```typescript
client.once("close", () => {
if (!handshakeDone) {
clearTimeout(timer);
client.destroy();
reject(new Error("server closed connection before handshake completed"));
}
});
```
(Remove this listener—or make it a no-op—once `handshakeDone` is set.)
How can I resolve this? If you propose a fix, please make it concise.
Author
There was a problem hiding this comment.
Similar deferral as above: the handshake path does block PTY creation, but the scenario (server closes after accept() but before writeResponse) is a crash/bug condition rather than normal flow. The 5s handshake timeout caps the worst case. Will revisit if this surfaces in practice.
Comment on lines
+24
to
+29
| * Constant-time token comparison. Returns false on length mismatch | ||
| * without timing leak. | ||
| */ | ||
| export function verifyToken(received: string, expected: string): boolean { | ||
| if (received.length !== expected.length) return false; | ||
| return crypto.timingSafeEqual(Buffer.from(received), Buffer.from(expected)); |
There was a problem hiding this comment.
The docstring says "Returns false on length mismatch without timing leak" but the early return on received.length !== expected.length is itself a timing oracle: an attacker who can measure response times can distinguish "wrong length" (fast path) from "correct length, wrong bytes" (slow timingSafeEqual path) and thereby determine the expected token length.
In this codebase the token is always crypto.randomBytes(32).toString("base64url") (43 chars), so the length is a public constant and the practical exploit impact is negligible. Still, the comment should be accurate:
/**
* Constant-time token comparison.
*
* Note: a length mismatch short-circuits before `timingSafeEqual`, leaking
* whether the token is the right length. This is acceptable here because
* token length is a fixed constant (43 chars). Do not reuse this function
* for variable-length tokens.
*/
export function verifyToken(received: string, expected: string): boolean {Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/fresh-spawn/auth.ts
Line: 24-29
Comment:
**Timing oracle comment is misleading — early return on length mismatch leaks token length**
The docstring says "Returns false on length mismatch **without** timing leak" but the early return on `received.length !== expected.length` is itself a timing oracle: an attacker who can measure response times can distinguish "wrong length" (fast path) from "correct length, wrong bytes" (slow `timingSafeEqual` path) and thereby determine the expected token length.
In this codebase the token is always `crypto.randomBytes(32).toString("base64url")` (43 chars), so the length is a public constant and the practical exploit impact is negligible. Still, the comment should be accurate:
```typescript
/**
* Constant-time token comparison.
*
* Note: a length mismatch short-circuits before `timingSafeEqual`, leaking
* whether the token is the right length. This is acceptable here because
* token length is a fixed constant (43 chars). Do not reuse this function
* for variable-length tokens.
*/
export function verifyToken(received: string, expected: string): boolean {
```
How can I resolve this? If you propose a fix, please make it concise.
Author
There was a problem hiding this comment.
Noted — the docstring will be updated in a follow-up doc pass alongside design.md. The new byte-length pre-check in 03b12386a is not a timing leak because both buffers are guaranteed same-length before the compare.
Comment on lines
+39
to
+46
| function ${_superset_cmd}() { | ||
| if [[ -x \"\$SUPERSET_FRESH_EXEC_BIN\" ]]; then | ||
| SUPERSET_FRESH_EXEC_ACTIVE=1 \"\$SUPERSET_FRESH_EXEC_BIN\" ${_superset_cmd} \"\$@\" | ||
| else | ||
| command ${_superset_cmd} \"\$@\" | ||
| fi | ||
| } | ||
| " |
There was a problem hiding this comment.
eval with env-var-derived content — injection risk if SUPERSET_FRESH_EXEC_COMMANDS is poisoned
The hook splits $SUPERSET_FRESH_EXEC_COMMANDS via ${(z)...} (zsh word-splitting) and passes each word directly into an eval "function ${_superset_cmd}() { ... }". Zsh's (z) flag splits on shell quoting boundaries, not just whitespace — so a poisoned env var containing shell metacharacters (e.g. gh); curl evil.example.com; function noop) could inject arbitrary shell code into the eval.
In practice, Superset controls SUPERSET_FRESH_EXEC_COMMANDS (set in env.ts from the hardcoded FRESH_EXEC_WHITELIST), so the attack surface is limited to a local privilege escalation by an attacker who can already set arbitrary env vars. Still, the eval makes the hook fragile if the whitelist is ever sourced from an external config or if users manually source the hook file.
At minimum, add a guard that skips tokens that don't look like simple command identifiers (^[a-zA-Z0-9_-]+$) before passing them to eval:
for _superset_cmd in ${(z)SUPERSET_FRESH_EXEC_COMMANDS}; do
[[ $_superset_cmd =~ ^[a-zA-Z0-9_-]+$ ]] || continue
eval "function ${_superset_cmd}() { ... }"
donePrompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/resources/shell-hooks/zsh-fresh-exec.zsh
Line: 39-46
Comment:
**`eval` with env-var-derived content — injection risk if `SUPERSET_FRESH_EXEC_COMMANDS` is poisoned**
The hook splits `$SUPERSET_FRESH_EXEC_COMMANDS` via `${(z)...}` (zsh word-splitting) and passes each word directly into an `eval "function ${_superset_cmd}() { ... }"`. Zsh's `(z)` flag splits on shell quoting boundaries, not just whitespace — so a poisoned env var containing shell metacharacters (e.g. `gh); curl evil.example.com; function noop`) could inject arbitrary shell code into the `eval`.
In practice, Superset controls `SUPERSET_FRESH_EXEC_COMMANDS` (set in `env.ts` from the hardcoded `FRESH_EXEC_WHITELIST`), so the attack surface is limited to a local privilege escalation by an attacker who can already set arbitrary env vars. Still, the `eval` makes the hook fragile if the whitelist is ever sourced from an external config or if users manually source the hook file.
At minimum, add a guard that skips tokens that don't look like simple command identifiers (`^[a-zA-Z0-9_-]+$`) before passing them to `eval`:
```zsh
for _superset_cmd in ${(z)SUPERSET_FRESH_EXEC_COMMANDS}; do
[[ $_superset_cmd =~ ^[a-zA-Z0-9_-]+$ ]] || continue
eval "function ${_superset_cmd}() { ... }"
done
```
How can I resolve this? If you propose a fix, please make it concise.
Author
There was a problem hiding this comment.
Addressed in 03b12386a. Added [[ $_superset_cmd =~ ^[A-Za-z_][A-Za-z0-9_-]*$ ]] || continue before the eval so entries that don't match a POSIX identifier shape are dropped — eliminates the env-derived injection surface.
Comment on lines
+165
to
+171
| // ===================================================================== | ||
|
|
||
| let buffer = options.initialBuffer ?? ""; | ||
| const drainBuffer = (): void => { | ||
| let newlineIdx: number; | ||
| // biome-ignore lint/suspicious/noAssignInExpressions: NDJSON line extractor | ||
| while ((newlineIdx = buffer.indexOf("\n")) !== -1) { |
There was a problem hiding this comment.
"15" — inconsistent with spawn-pty-subprocess which uses signal names "SIGTERM"
node-pty delivers signal as a number on POSIX, so String(signal) produces "15", "9", etc. By contrast, handlers/spawn-pty-subprocess.ts forwards the child_process string signal name ("SIGTERM", "SIGKILL", etc.) directly.
fresh-exec.ts's bridgeSocketToStdio correctly handles numeric strings via Number.parseInt(exit.signal, 10), so the client-facing exit-code computation is correct. But any other consumer of a SpawnSession backed by fresh-exec will see a raw number string instead of a POSIX signal name.
Consider converting using Node's os.constants.signals reverse-map:
import * as os from "node:os";
const sigNumToName: Record<number, string> = Object.fromEntries(
Object.entries(os.constants.signals).map(([name, num]) => [num, name]),
);
// In onExit:
signal: typeof signal === "number" ? (sigNumToName[signal] ?? String(signal)) : null,Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/fresh-spawn/handlers/fresh-exec.ts
Line: 165-171
Comment:
**Signal reported as numeric string `"15"` — inconsistent with `spawn-pty-subprocess` which uses signal names `"SIGTERM"`**
`node-pty` delivers `signal` as a number on POSIX, so `String(signal)` produces `"15"`, `"9"`, etc. By contrast, `handlers/spawn-pty-subprocess.ts` forwards the `child_process` string signal name (`"SIGTERM"`, `"SIGKILL"`, etc.) directly.
`fresh-exec.ts`'s `bridgeSocketToStdio` correctly handles numeric strings via `Number.parseInt(exit.signal, 10)`, so the client-facing exit-code computation is correct. But any other consumer of a `SpawnSession` backed by `fresh-exec` will see a raw number string instead of a POSIX signal name.
Consider converting using Node's `os.constants.signals` reverse-map:
```typescript
import * as os from "node:os";
const sigNumToName: Record<number, string> = Object.fromEntries(
Object.entries(os.constants.signals).map(([name, num]) => [num, name]),
);
// In onExit:
signal: typeof signal === "number" ? (sigNumToName[signal] ?? String(signal)) : null,
```
How can I resolve this? If you propose a fix, please make it concise.
Author
There was a problem hiding this comment.
Deferred: bridgeSocketToStdio in fresh-exec.ts consumes the signal via Number.parseInt, so the inconsistency is currently contained to internal usage. Will unify on POSIX names (via os.constants.signals reverse lookup) in a follow-up if external consumers start relying on the string form.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/terminal-host/terminal-host.ts (1)
124-155:⚠️ Potential issue | 🟠 MajorDispose the new session when awaited spawn fails.
Now that
session.spawn()is awaited, failures happen before the session is stored inthis.sessions; the current catch releases the semaphore but can leave any partially-created subprocess/session resources dangling.🧹 Proposed cleanup
} catch (error) { releaseSpawnOnce(); + if (shouldDisposeIfCanceled && session) { + try { + await session.dispose(); + } catch { + // Preserve the original spawn error. + } + this.sessions.delete(sessionId); + } throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/terminal-host.ts` around lines 124 - 155, The try/catch can leave a partially-created session running if session.spawn() throws before the session is stored in this.sessions; after creating the session via createSession(request) but before rethrowing in the catch, ensure you clean up the created session: if a local session variable exists and shouldDisposeIfCanceled is true, call the session's disposal method (e.g., session.dispose() or session.kill() depending on the Session API) and unregister any listeners (or rely on session.onExit/handleSessionExit) before rethrowing; keep the existing releaseSpawnOnce() calls but add this guarded session cleanup in the catch block so no subprocess/session resources are leaked.apps/desktop/src/main/lib/terminal/env.ts (1)
515-560:⚠️ Potential issue | 🟠 MajorClear inherited
SUPERSET_FRESH_EXEC_*vars before applying the macOS gate.
baseEnvcan pass through parentSUPERSET_*values, so a launched environment can pre-populateSUPERSET_FRESH_EXEC_HOOK_PATH/COMMANDS/BINon non-darwin or when assets are missing. Clear this reserved namespace first, then set only app-resolved values.Proposed fix
delete terminalEnv.GOOGLE_API_KEY; + for (const key of Object.keys(terminalEnv)) { + if (key.startsWith("SUPERSET_FRESH_EXEC_")) { + delete terminalEnv[key]; + } + } // Electron child processes can't access macOS Keychain for TLS cert verification, // causing "x509: OSStatus -26276" in Go binaries like `gh`. File-based fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/terminal/env.ts` around lines 515 - 560, terminalEnv inherits potential SUPERSET_FRESH_EXEC_* values from baseEnv which can leak into non-macOS or missing-asset cases; before the macOS gate in the block that checks os.platform() === "darwin", explicitly delete terminalEnv.SUPERSET_FRESH_EXEC_BIN, terminalEnv.SUPERSET_FRESH_EXEC_COMMANDS, and terminalEnv.SUPERSET_FRESH_EXEC_HOOK_PATH so the reserved namespace is cleared, then proceed to call getFreshExecPaths() and set SUPSERSET_FRESH_EXEC_* only when freshExec.bin && freshExec.hook are present (function/variable references: terminalEnv, baseEnv, SUPSERSET_FRESH_EXEC_BIN/COMMANDS/HOOK_PATH, getFreshExecPaths, FRESH_EXEC_WHITELIST).
🟡 Minor comments (8)
apps/desktop/package.json-190-190 (1)
190-190:⚠️ Potential issue | 🟡 MinorRemove
node-unix-socketfrom production dependencies.This package is imported only in
apps/desktop/src/main/fresh-spawn/spike/fd-passing-spike.tsand serves no purpose in the production codebase since the FD-passing approach was rejected in favor of pure I/O forwarding. Move it todevDependenciesif the spike code should remain, or remove it entirely if the spike is archival.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/package.json` at line 190, The package "node-unix-socket" is listed in production dependencies but is only used by the FD-passing spike (fd-passing-spike.ts) and should not ship; remove "node-unix-socket" from the dependencies list in package.json and either add it to devDependencies (if you want to keep apps/desktop/src/main/fresh-spawn/spike/fd-passing-spike.ts for local experimentation) or delete the dep entirely if the spike will be archived/removed; ensure package-lock/yarn lock is updated after the change.apps/desktop/plans/20260417-1500-fresh-mach-context-spawn.md-17-29 (1)
17-29:⚠️ Potential issue | 🟡 MinorRefresh the stale FD-passing/Electron-owned architecture text.
The plan now conflicts with the final PR design: the top update mentions I/O forwarding, but later tasks and the PR template still say to pass FDs via SCM_RIGHTS and spawn Electron-owned children. Update these sections to describe daemon-hosted lifecycle + framed I/O forwarding so future workers do not resurrect the rejected architecture.
Also applies to: 963-964, 1124-1127, 1688-1689, 2426-2452
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/plans/20260417-1500-fresh-mach-context-spawn.md` around lines 17 - 29, Update the plan and PR template text to replace any references to passing FDs via SCM_RIGHTS or spawning Electron-owned child processes with the daemon-hosted lifecycle and NDJSON-framed I/O forwarding over UDS; specifically edit the high-level description and the task list entries for Task 8 (spawn-pty-subprocess handler), Task 9 (ChildProcess adapter), Task 13 (fresh-exec handler) and Task 14 (PTY bridging) to state that Electron holds local pipes and forwards stdin/stdout/stderr as framed NDJSON over UDS to a daemon-owned process, and ensure the PR template no longer mentions FD-passing/SCM_RIGHTS or Electron-owned children so future reviewers implement the daemon-hosted design.apps/desktop/src/main/fresh-spawn/handlers/fresh-exec.test.ts-94-115 (1)
94-115:⚠️ Potential issue | 🟡 MinorWait for the spawned process to die before ending the test.
Line 114 triggers async cleanup, but the test exits immediately, so
sleep 5can leak into teardown or the next test if the disconnect kill path is delayed. Reuse the polling helper from the disconnect test or make this command exit immediately after theokframe is verified.🧪 Proposed test cleanup
expect(firstFrame.type).toBe("ok"); expect(typeof firstFrame.pid).toBe("number"); - expect((firstFrame.pid as number) > 0).toBe(true); + const pid = firstFrame.pid as number; + expect(pid > 0).toBe(true); // Clean up: disconnecting triggers the handler's SIGTERM → SIGKILL // path on the spawned PTY. client.destroy(); + await waitForProcessExit(pid); }, 10000);Then share the helper with the second test:
async function waitForProcessExit(pid: number): Promise<void> { for (let i = 0; i < 40; i++) { await new Promise((resolve) => setTimeout(resolve, 100)); try { process.kill(pid, 0); } catch { return; } } throw new Error(`Process ${pid} did not exit`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/fresh-spawn/handlers/fresh-exec.test.ts` around lines 94 - 115, The test "returns ok+pid for a valid fresh-exec request" currently disconnects the client immediately and can leak the spawned sleep process; after verifying firstFrame.type and firstFrame.pid, wait for the spawned process to exit before finishing the test (reuse or add a helper like waitForProcessExit(pid: number)) by calling it with the PID from firstFrame, then destroy the client; update the test around sendRequest / firstFrame / client to poll/process.kill(0) until the PID is gone to ensure no leakage.apps/desktop/docs/fresh-mach-context-design.md-14-14 (1)
14-14:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks.
markdownlintflags these fences as missing language tags. Usetext,mermaid,console, or another appropriate identifier.Also applies to: 53-53, 64-64, 82-82, 97-97, 241-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/docs/fresh-mach-context-design.md` at line 14, Update every fenced code block that currently uses bare triple backticks (```) by adding an appropriate language identifier: replace ``` with ```text for plain text, ```mermaid for diagrams, ```console or ```bash for shell examples, etc.; locate the bare fences (the ``` blocks) and change each to the correct fenced-language form so markdownlint no longer flags them.apps/desktop/src/main/fresh-spawn/fresh-exec.ts-305-310 (1)
305-310:⚠️ Potential issue | 🟡 MinorUse
osConstants.signalsto map signal names to their numeric values.Node.js reports signals as strings (e.g.,
"SIGINT"), but the current code tries to parse them as numbers, which returnsNaN, causing a fallback to143for every signal. This breaks shell-compatible exit codes—Ctrl-C should report130(128 + 2 for SIGINT), not143. UseosConstants.signals[signal]to map signal names portably across platforms.♻️ Proposed signal mapper
import { spawn } from "node:child_process"; import * as fs from "node:fs"; import * as net from "node:net"; +import { constants as osConstants } from "node:os"; @@ interface BridgeExitInfo { code: number | null; signal: string | null; } + +function signalExitCode(signal: string): number { + const signum = + osConstants.signals[signal as keyof typeof osConstants.signals]; + return 128 + (typeof signum === "number" ? signum : 15); +} @@ child.on("exit", (code, signal) => { if (signal !== null) { - // bash semantics: 128 + signal number. Node gives signal as a name - // string; we don't have a portable signal->number mapping here, so - // use 128 + 15 (SIGTERM) as the generic fallback. - resolve(128 + 15); + resolve(signalExitCode(signal)); return; } @@ if (exit.signal !== null) { - const signum = Number.parseInt(exit.signal, 10); - if (Number.isFinite(signum) && signum > 0) { - return 128 + signum; - } - return 128 + 15; // SIGTERM default + return signalExitCode(exit.signal); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/fresh-spawn/fresh-exec.ts` around lines 305 - 310, The exit handler for child.on("exit") currently always falls back to 128+15 because signal strings are not converted to numbers; change it to look up the numeric value using Node's os.constants.signals (e.g., os.constants.signals[signal]) inside the handler, compute resolve(128 + signalNumber) when found, and only use the 128+15 fallback if the lookup is undefined; ensure you import or reference os.constants as osConstants (or similar) and keep the existing resolve(...) call flow in the child.on("exit") callback.apps/desktop/src/main/fresh-spawn/types.ts-93-104 (1)
93-104:⚠️ Potential issue | 🟡 MinorUDS path may exceed
sun_pathlimit on macOS.On Darwin,
struct sockaddr_un.sun_pathis 104 bytes. For a home directory like/Users/<username>,~/.superset/fresh-spawn.sockfits comfortably (~50 chars), but corporate environments with FileVault home redirects or non-standard home paths (/private/var/folders/..., NFS-mounted homes, managed-user paths) can push this past the limit, andlisten(socketPath)will fail withENAMETOOLONG.Consider falling back to
$TMPDIR(typically/private/var/folders/xx/yy/T/on macOS, ~40 chars + filename) oros.tmpdir()when the home-based path would exceed ~100 bytes. At minimum, surface a clearer error instartSpawnServerif the path is too long so the direct-spawn fallback kicks in gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/fresh-spawn/types.ts` around lines 93 - 104, The DEFAULT_SOCKET_PATH (and similarly DEFAULT_TOKEN_PATH) built from os.homedir() can exceed Darwin's sockaddr_un.sun_path limit and cause listen() to fail with ENAMETOOLONG; update the code that computes DEFAULT_SOCKET_PATH (and places used by startSpawnServer/fallback logic) to detect the byte length of the resolved socket path and, if it would exceed ~100 bytes, fall back to os.tmpdir() (or $TMPDIR) with FRESH_SPAWN_DIR + filename, and ensure startSpawnServer surfaces a clear, descriptive error when path length is too long so the direct-spawn fallback is attempted; reference DEFAULT_SOCKET_PATH, DEFAULT_TOKEN_PATH, FRESH_SPAWN_DIR, and startSpawnServer in your change.apps/desktop/src/main/fresh-spawn/spawn-server.ts-185-196 (1)
185-196:⚠️ Potential issue | 🟡 MinorUnlink the socket file when chmod fails.
On a successful
listen, the socket file exists on disk. IfchmodSyncthrows, youserver.close()but leave the socket file behind, leading to a stale file that the next startup'sunlinkSyncneeds to clean up — inconsistent withclose()semantics at line 202-210 which does unlink.♻️ Proposed fix
try { fs.chmodSync(options.socketPath, 0o700); } catch (err) { - server.close(); - reject(err); - return; + server.close(() => { + try { fs.unlinkSync(options.socketPath); } catch { /* ignore */ } + reject(err); + }); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/fresh-spawn/spawn-server.ts` around lines 185 - 196, When chmodSync(options.socketPath) throws in the try/catch inside the server.listen flow, the code currently calls server.close() but leaves the socket file on disk; update the catch block in spawn-server.ts so that after server.close() you also remove the socket file (e.g. call fs.unlinkSync(options.socketPath) or fs.unlink with safe existence check) and swallow any unlink error before calling reject(err), mirroring the cleanup done in the server close/unlink logic used elsewhere (reference: the catch block around fs.chmodSync and the server.close() call, and the unlink logic around server close at lines handling "close"/unlink).apps/desktop/src/main/fresh-spawn/handlers/fresh-exec.ts-259-265 (1)
259-265:⚠️ Potential issue | 🟡 MinorPass Buffer directly to node-pty instead of converting to UTF-8 string.
Line 261 converts the base64 data to a UTF-8 string before writing to the PTY, which silently corrupts any non-UTF-8 bytes by replacing them with U+FFFD. Since
node-pty'sIPty.write()acceptsstring | Buffer, pass the Buffer directly to preserve binary data:ptyProcess.write(Buffer.from(frame.data, "base64"));This matches the pattern already used in
spawn-pty-subprocess.tsfor stdin handling and is necessary for tools likekubectlandterraformthat may process binary input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/fresh-spawn/handlers/fresh-exec.ts` around lines 259 - 265, In the "stdin" case in fresh-exec.ts, stop converting the base64 payload to a UTF-8 string (which corrupts non-UTF8 bytes) and pass the Buffer directly to ptyProcess.write; replace the Buffer.from(frame.data, "base64").toString("utf8") usage with passing Buffer.from(frame.data, "base64") into ptyProcess.write, keeping the existing try/catch around the write in case the PTY closed.
🧹 Nitpick comments (5)
apps/desktop/src/shared/fresh-spawn-whitelist.test.ts (1)
9-12: Optional: explicit locale-independent sort comparator.
Array.prototype.sort()with no comparator sorts by UTF-16 code units, which happens to coincide with alphabetical order for the current lowercase+digit+-/_charset, but a future entry containing uppercase would silently reorder. Consider[...FRESH_EXEC_WHITELIST].sort((a, b) => a.localeCompare(b, "en"))or enforcing the charset here too. Non-blocking given the regex check on line 25 already bans uppercase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/fresh-spawn-whitelist.test.ts` around lines 9 - 12, The test for alphabetical order uses default Array.prototype.sort(), which can be locale-dependent and may reorder if uppercase entries are added; update the sort call in the test that references FRESH_EXEC_WHITELIST to use an explicit comparator such as [...FRESH_EXEC_WHITELIST].sort((a, b) => a.localeCompare(b, "en")) (or another explicit locale/option) so the comparison is deterministic and locale-independent while still asserting equality against the original array.apps/desktop/src/main/fresh-spawn/spawn-session.test.ts (1)
55-64: Wait for stdout deterministically instead of sleeping.The 200 ms delay can make this integration test flaky on slow CI. Attach a bounded
datapromise before writing to stdin.Proposed test refactor
- const received: Buffer[] = []; - session.stdout.on("data", (chunk: Buffer) => received.push(chunk)); + const stdoutPromise = new Promise<Buffer>((resolve, reject) => { + const timeout = setTimeout( + () => reject(new Error("timed out waiting for echo")), + 2000, + ); + session.stdout.once("data", (chunk: Buffer) => { + clearTimeout(timeout); + resolve(chunk); + }); + }); session.stdin.write("hello\n"); - // Wait for echo - await new Promise<void>((r) => setTimeout(r, 200)); - - const got = Buffer.concat(received).toString("utf8"); + const got = (await stdoutPromise).toString("utf8"); expect(got).toBe("hello\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/fresh-spawn/spawn-session.test.ts` around lines 55 - 64, The test currently sleeps 200ms which is flaky; instead attach a bounded Promise that resolves on the next session.stdout "data" event before writing to session.stdin so the test waits deterministically. Specifically, create a Promise (or use events.once) that listens for session.stdout "data" (and rejects/timeout after a short bound), start that listener, then call session.stdin.write("hello\n"), await the data promise, then assert by concatenating received; reference the existing received array and session.stdout/session.stdin and ensure the promise times out to avoid hanging tests.apps/desktop/src/main/terminal-host/fresh-spawn-integration.test.ts (1)
89-94: Await the session exit instead of using a fixed delay.A 200 ms sleep may be too short on loaded CI and can leave the spawned child/server state racing teardown.
Proposed cleanup refactor
expect(session).not.toBeNull(); expect(session?.pid).toBeGreaterThan(0); - // Clean up: kill it so child doesn't linger - session?.kill("SIGTERM"); - await new Promise((r) => setTimeout(r, 200)); + const exitPromise = new Promise<void>((resolve) => { + session?.once("exit", () => resolve()); + }); + session?.kill("SIGTERM"); + await exitPromise; }, 10000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/fresh-spawn-integration.test.ts` around lines 89 - 94, Replace the fixed 200ms sleep with an explicit wait for the spawned session to exit: after calling session?.kill("SIGTERM"), listen for the session exit event (e.g., session.once('exit', ...) or equivalent on the session object) and await that promise so the test only proceeds once the child has actually terminated; ensure you still assert session and session?.pid before killing and handle the case where the exit event may not fire (optional timeout/fail) to avoid flakiness.apps/desktop/src/main/fresh-spawn/spawn-session.ts (1)
85-90: Handshakeonce('error')listener survives after success.On a successful handshake the listener at line 85 is never removed (it only no-ops when
handshakeDoneis true). AftercreateSessionattaches its ownclient.on('error')at line 277, both fire on any future socket error — harmless but a minor listener leak and potential source of confusion. Considerclient.off('error', ...)alongsideclient.off('data', onHandshakeData)at line 157.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/fresh-spawn/spawn-session.ts` around lines 85 - 90, The handshake error listener attached with client.once("error", ...) can persist after a successful handshake; change it to a named function (e.g., onHandshakeError) instead of an inline callback and remove it when the handshake completes (alongside the existing client.off('data', onHandshakeData) call) so it cannot fire later and conflict with the post-handshake client.on('error') in createSession; ensure you still clear the timer and reject on handshake failure inside that named handler.apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts (1)
121-133: Unbounded forwarding ignores socket backpressure.
child.stdout/stderrdataevents are consumed as fast as the child produces them and re-emitted viaclient.write(...)without checking the return value. If the remote consumer is slow, Node's internal socket buffer grows without bound — a noisy child can balloon daemon memory.Consider pausing the child streams when
client.writereturnsfalseand resuming onclient.on('drain'). Same pattern applies toptyProcess.onDatainfresh-exec.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts` around lines 121 - 133, The stdout/stderr handlers in spawn-pty-subprocess.ts call writeFrame(client, ...) unconditionally and ignore socket backpressure; modify the handlers for child.stdout and child.stderr to check the boolean return of client.write (via your writeFrame path) and call child.pause() when write returns false, then attach a one-time or persistent client.on('drain') to resume the specific child stream; mirror the same pattern for ptyProcess.onData in fresh-exec.ts (pause the pty source when write returns false and resume on drain) to prevent unbounded memory growth when the remote is slow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/docs/fresh-mach-context-design.md`:
- Around line 39-115: Update the design doc to reflect the implemented
architecture: replace the "Electron main (FRESH ctx)" spawn model and SCM_RIGHTS
FD-passing flow with the actual "daemon-hosted fresh-spawn" design that uses
NDJSON I/O forwarding and explicitly rejects FD passing; update all diagrams
(formerly showing "Electron main → spawn-server", "pty-subprocess",
"terminal-host", "fresh-exec") and the terminal flows to show daemon as the
spawner, show NDJSON streams instead of SCM_RIGHTS FD transfer, adjust the
fallback note to describe daemon vs. terminal-host behavior, and revise the
protocol section to document NDJSON I/O framing and the deliberate rejection of
SCM_RIGHTS/FD passing (also apply the same edits to the later section referenced
in the comment).
In `@apps/desktop/src/main/fresh-spawn/auth.ts`:
- Around line 11-12: The token write currently uses fs.writeFileSync(tokenPath,
token, { mode: 0o600 }) which only guarantees permissions on creation; after
overwriting an existing file you must enforce secure perms explicitly—after
fs.writeFileSync(...) call add fs.chmodSync(tokenPath, 0o600) (ensure tokenPath
is the same variable used above) so the token file is always set to 0o600 even
when replaced; keep the existing fs.mkdirSync(...) behavior unchanged.
- Around line 27-29: In verifyToken, the pre-check uses string .length (UTF-16
code units) which can differ from actual UTF-8 byte length and causes
crypto.timingSafeEqual to throw; fix by creating buffers for received and
expected (e.g., Buffer.from(..., 'utf8')), compare their .length (or
Buffer.byteLength) and only call crypto.timingSafeEqual when the two buffer
lengths match, then pass the buffers to timingSafeEqual; reference verifyToken,
received, expected, and the timingSafeEqual call.
In `@apps/desktop/src/main/fresh-spawn/fresh-exec.ts`:
- Around line 232-283: The current close handler resolves with lastExit even
when no "exit" frame was received (lastExit remains { code: null, signal: null
}), which reports success for interrupted connections; change the
client.once("close") behavior to detect missing exit (lastExit.code === null &&
lastExit.signal === null) and treat it as a bridge failure by rejecting the
surrounding Promise (or returning a non-zero exit) instead of resolve(lastExit).
Apply the same guard to the other close handler referenced around lines 356-368
so both close paths use the missing-exit check; reference symbols: lastExit,
handleFrameLine, processBuffer, client.once("close") (and the other close
handler) when making the change.
In `@apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts`:
- Around line 72-87: The spawned ChildProcess from spawn(...) (the child
variable in spawn-pty-subprocess.ts) lacks an 'error' event listener, so add a
child.on('error', (err) => { ... }) handler immediately after creating child to
catch async spawn failures (ENOENT/EACCES), log or surface the error, and ensure
the process is cleaned up (child.kill(...) or similar) and the caller is
informed (reject/throw or propagate the error) instead of letting an unhandled
exception crash the daemon; place the listener before returning from the handler
and keep the existing stdin/stdout/stderr/pid checks intact.
In `@apps/desktop/src/main/fresh-spawn/lifecycle.ts`:
- Around line 32-42: Concurrent calls to startFreshSpawnServer can race because
they both check `instance` before awaiting `startSpawnServer`; add an in-flight
sentinel (e.g. a module-scoped `starting: Promise<void> | null` or boolean and
promise pair) and set it immediately when entering the startup path,
return/await on that sentinel for subsequent callers, and clear it on
success/failure; specifically, in the function that currently checks `if
(instance)` set `starting` before calling `startSpawnServer({...
DEFAULT_SOCKET_PATH, DEFAULT_TOKEN_PATH, subprocessScriptPath:
options.subprocessScriptPath })`, have other callers await `starting` if
present, and ensure `starting` is cleared (and `instance` assigned) after the
awaited `startSpawnServer` completes or on error.
In `@apps/desktop/src/main/fresh-spawn/paths.ts`:
- Around line 21-35: resolveFreshExecBinaryPath currently returns whatever path
fs.existsSync sees, which may be an in-archive app.asar path that external
shells cannot execute; update resolveFreshExecBinaryPath to prefer the physical
unpacked file by detecting when candidate contains ".asar" and checking a
corresponding ".asar.unpacked" variant first (e.g., replace the first ".asar"
segment with ".asar.unpacked"), returning that unpacked path if fs.existsSync
reports it, otherwise fall back to the original candidate if it exists, else
null; keep references to candidate, fs.existsSync, path.join and fresh-exec.js
when making the change so the logic is easy to find and verify.
In `@apps/desktop/src/main/fresh-spawn/spawn-server.ts`:
- Around line 73-93: The handshake in client.on("data") accumulates `buffer`
without a size cap, allowing OOM if no newline is sent; add a MAX_LINE_BYTES
constant (match spawn-session.ts, e.g. 1 MiB) and enforce it in the
client.on("data") handler by checking the buffer size before/after appending
chunk and returning a JSON error (use writeResponse with an appropriate code
like "E_LINE_TOO_LONG") and closing the socket when exceeded; update the handler
that parses JSON (the try/catch around JSON.parse) to only run after the newline
and after the size check to prevent unbounded growth.
In `@apps/desktop/src/main/fresh-spawn/spawn-session.ts`:
- Around line 194-208: The stdin Writable's final() currently doesn't notify the
server, causing remote processes to hang; add a new client→server frame type
(e.g. "stdin-close") to the protocol in types.ts, emit that frame from the
Writable.final() in spawn-session.ts (call writeClientFrame(client, { type:
"stdin-close" })) and update the server/handler that processes incoming frames
to treat "stdin-close" by calling child.stdin.end() or closing the PTY master
write side (or equivalent) so the remote process receives EOF. Ensure any
corresponding switch/case or dispatcher that handles "stdin" frames also handles
"stdin-close" to avoid unhandled-frame errors.
- Around line 247-265: The current size-guard checks the entire accumulated
`buffer` before `processBuffer()` drains complete newline-terminated frames,
causing valid batched frames to be rejected; change the `client.on("data", ...)`
handler to call `processBuffer()` first (so completed lines are removed) and
then perform the `MAX_LINE_BYTES` check against the remaining `buffer` (the tail
residue) and only then synthesize the error, call `client.destroy()` and
`safeEmitError(emitter, err)` if the residue exceeds the limit; keep references
to `buffer`, `MAX_LINE_BYTES`, `processBuffer()`, `client.destroy()`, and
`safeEmitError(emitter, err)` so the change is localized.
In `@apps/desktop/src/main/terminal-host/index.ts`:
- Around line 787-805: startFreshSpawnServer is being started inside the
long-lived terminal-host (daemon) which lets the fresh-spawn UDS keep the
daemon's stale bootstrap context; instead, ensure the spawn authority is owned
by the current fresh Electron/helper process by either moving the
startFreshSpawnServer call out of the persistent daemon into the fresh
Electron/helper process (invoke startFreshSpawnServer from that process and pass
subprocessScriptPath: join(__dirname, "pty-subprocess.js")), or implement an
explicit rotation API (e.g. rotateFreshSpawnAuthority or
restartFreshSpawnServer) that cleanly stops the existing server in the daemon
and restarts it under the current process on relaunch so new PTYs inherit the
fresh context.
In `@apps/desktop/src/resources/shell-hooks/zsh-fresh-exec.zsh`:
- Around line 36-46: The loop currently interpolates unsanitized command names
into eval, creating a shell injection risk; before calling eval to define the
wrapper function for each _superset_cmd from SUPERSET_FRESH_EXEC_COMMANDS,
validate _superset_cmd against a safe identifier pattern (e.g. match POSIX
function-name rules like ^[A-Za-z_][A-Za-z0-9_]*$) and skip or fallback if it
fails validation; apply this check around the loop that uses eval (symbols:
SUPERSET_FRESH_EXEC_COMMANDS, _superset_cmd, eval, SUPERSET_FRESH_EXEC_BIN,
SUPERSET_FRESH_EXEC_ACTIVE) so only validated names are interpolated.
---
Outside diff comments:
In `@apps/desktop/src/main/lib/terminal/env.ts`:
- Around line 515-560: terminalEnv inherits potential SUPERSET_FRESH_EXEC_*
values from baseEnv which can leak into non-macOS or missing-asset cases; before
the macOS gate in the block that checks os.platform() === "darwin", explicitly
delete terminalEnv.SUPERSET_FRESH_EXEC_BIN,
terminalEnv.SUPERSET_FRESH_EXEC_COMMANDS, and
terminalEnv.SUPERSET_FRESH_EXEC_HOOK_PATH so the reserved namespace is cleared,
then proceed to call getFreshExecPaths() and set SUPSERSET_FRESH_EXEC_* only
when freshExec.bin && freshExec.hook are present (function/variable references:
terminalEnv, baseEnv, SUPSERSET_FRESH_EXEC_BIN/COMMANDS/HOOK_PATH,
getFreshExecPaths, FRESH_EXEC_WHITELIST).
In `@apps/desktop/src/main/terminal-host/terminal-host.ts`:
- Around line 124-155: The try/catch can leave a partially-created session
running if session.spawn() throws before the session is stored in this.sessions;
after creating the session via createSession(request) but before rethrowing in
the catch, ensure you clean up the created session: if a local session variable
exists and shouldDisposeIfCanceled is true, call the session's disposal method
(e.g., session.dispose() or session.kill() depending on the Session API) and
unregister any listeners (or rely on session.onExit/handleSessionExit) before
rethrowing; keep the existing releaseSpawnOnce() calls but add this guarded
session cleanup in the catch block so no subprocess/session resources are
leaked.
---
Minor comments:
In `@apps/desktop/docs/fresh-mach-context-design.md`:
- Line 14: Update every fenced code block that currently uses bare triple
backticks (```) by adding an appropriate language identifier: replace ``` with
```text for plain text, ```mermaid for diagrams, ```console or ```bash for shell
examples, etc.; locate the bare fences (the ``` blocks) and change each to the
correct fenced-language form so markdownlint no longer flags them.
In `@apps/desktop/package.json`:
- Line 190: The package "node-unix-socket" is listed in production dependencies
but is only used by the FD-passing spike (fd-passing-spike.ts) and should not
ship; remove "node-unix-socket" from the dependencies list in package.json and
either add it to devDependencies (if you want to keep
apps/desktop/src/main/fresh-spawn/spike/fd-passing-spike.ts for local
experimentation) or delete the dep entirely if the spike will be
archived/removed; ensure package-lock/yarn lock is updated after the change.
In `@apps/desktop/plans/20260417-1500-fresh-mach-context-spawn.md`:
- Around line 17-29: Update the plan and PR template text to replace any
references to passing FDs via SCM_RIGHTS or spawning Electron-owned child
processes with the daemon-hosted lifecycle and NDJSON-framed I/O forwarding over
UDS; specifically edit the high-level description and the task list entries for
Task 8 (spawn-pty-subprocess handler), Task 9 (ChildProcess adapter), Task 13
(fresh-exec handler) and Task 14 (PTY bridging) to state that Electron holds
local pipes and forwards stdin/stdout/stderr as framed NDJSON over UDS to a
daemon-owned process, and ensure the PR template no longer mentions
FD-passing/SCM_RIGHTS or Electron-owned children so future reviewers implement
the daemon-hosted design.
In `@apps/desktop/src/main/fresh-spawn/fresh-exec.ts`:
- Around line 305-310: The exit handler for child.on("exit") currently always
falls back to 128+15 because signal strings are not converted to numbers; change
it to look up the numeric value using Node's os.constants.signals (e.g.,
os.constants.signals[signal]) inside the handler, compute resolve(128 +
signalNumber) when found, and only use the 128+15 fallback if the lookup is
undefined; ensure you import or reference os.constants as osConstants (or
similar) and keep the existing resolve(...) call flow in the child.on("exit")
callback.
In `@apps/desktop/src/main/fresh-spawn/handlers/fresh-exec.test.ts`:
- Around line 94-115: The test "returns ok+pid for a valid fresh-exec request"
currently disconnects the client immediately and can leak the spawned sleep
process; after verifying firstFrame.type and firstFrame.pid, wait for the
spawned process to exit before finishing the test (reuse or add a helper like
waitForProcessExit(pid: number)) by calling it with the PID from firstFrame,
then destroy the client; update the test around sendRequest / firstFrame /
client to poll/process.kill(0) until the PID is gone to ensure no leakage.
In `@apps/desktop/src/main/fresh-spawn/handlers/fresh-exec.ts`:
- Around line 259-265: In the "stdin" case in fresh-exec.ts, stop converting the
base64 payload to a UTF-8 string (which corrupts non-UTF8 bytes) and pass the
Buffer directly to ptyProcess.write; replace the Buffer.from(frame.data,
"base64").toString("utf8") usage with passing Buffer.from(frame.data, "base64")
into ptyProcess.write, keeping the existing try/catch around the write in case
the PTY closed.
In `@apps/desktop/src/main/fresh-spawn/spawn-server.ts`:
- Around line 185-196: When chmodSync(options.socketPath) throws in the
try/catch inside the server.listen flow, the code currently calls server.close()
but leaves the socket file on disk; update the catch block in spawn-server.ts so
that after server.close() you also remove the socket file (e.g. call
fs.unlinkSync(options.socketPath) or fs.unlink with safe existence check) and
swallow any unlink error before calling reject(err), mirroring the cleanup done
in the server close/unlink logic used elsewhere (reference: the catch block
around fs.chmodSync and the server.close() call, and the unlink logic around
server close at lines handling "close"/unlink).
In `@apps/desktop/src/main/fresh-spawn/types.ts`:
- Around line 93-104: The DEFAULT_SOCKET_PATH (and similarly DEFAULT_TOKEN_PATH)
built from os.homedir() can exceed Darwin's sockaddr_un.sun_path limit and cause
listen() to fail with ENAMETOOLONG; update the code that computes
DEFAULT_SOCKET_PATH (and places used by startSpawnServer/fallback logic) to
detect the byte length of the resolved socket path and, if it would exceed ~100
bytes, fall back to os.tmpdir() (or $TMPDIR) with FRESH_SPAWN_DIR + filename,
and ensure startSpawnServer surfaces a clear, descriptive error when path length
is too long so the direct-spawn fallback is attempted; reference
DEFAULT_SOCKET_PATH, DEFAULT_TOKEN_PATH, FRESH_SPAWN_DIR, and startSpawnServer
in your change.
---
Nitpick comments:
In `@apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts`:
- Around line 121-133: The stdout/stderr handlers in spawn-pty-subprocess.ts
call writeFrame(client, ...) unconditionally and ignore socket backpressure;
modify the handlers for child.stdout and child.stderr to check the boolean
return of client.write (via your writeFrame path) and call child.pause() when
write returns false, then attach a one-time or persistent client.on('drain') to
resume the specific child stream; mirror the same pattern for ptyProcess.onData
in fresh-exec.ts (pause the pty source when write returns false and resume on
drain) to prevent unbounded memory growth when the remote is slow.
In `@apps/desktop/src/main/fresh-spawn/spawn-session.test.ts`:
- Around line 55-64: The test currently sleeps 200ms which is flaky; instead
attach a bounded Promise that resolves on the next session.stdout "data" event
before writing to session.stdin so the test waits deterministically.
Specifically, create a Promise (or use events.once) that listens for
session.stdout "data" (and rejects/timeout after a short bound), start that
listener, then call session.stdin.write("hello\n"), await the data promise, then
assert by concatenating received; reference the existing received array and
session.stdout/session.stdin and ensure the promise times out to avoid hanging
tests.
In `@apps/desktop/src/main/fresh-spawn/spawn-session.ts`:
- Around line 85-90: The handshake error listener attached with
client.once("error", ...) can persist after a successful handshake; change it to
a named function (e.g., onHandshakeError) instead of an inline callback and
remove it when the handshake completes (alongside the existing
client.off('data', onHandshakeData) call) so it cannot fire later and conflict
with the post-handshake client.on('error') in createSession; ensure you still
clear the timer and reject on handshake failure inside that named handler.
In `@apps/desktop/src/main/terminal-host/fresh-spawn-integration.test.ts`:
- Around line 89-94: Replace the fixed 200ms sleep with an explicit wait for the
spawned session to exit: after calling session?.kill("SIGTERM"), listen for the
session exit event (e.g., session.once('exit', ...) or equivalent on the session
object) and await that promise so the test only proceeds once the child has
actually terminated; ensure you still assert session and session?.pid before
killing and handle the case where the exit event may not fire (optional
timeout/fail) to avoid flakiness.
In `@apps/desktop/src/shared/fresh-spawn-whitelist.test.ts`:
- Around line 9-12: The test for alphabetical order uses default
Array.prototype.sort(), which can be locale-dependent and may reorder if
uppercase entries are added; update the sort call in the test that references
FRESH_EXEC_WHITELIST to use an explicit comparator such as
[...FRESH_EXEC_WHITELIST].sort((a, b) => a.localeCompare(b, "en")) (or another
explicit locale/option) so the comparison is deterministic and
locale-independent while still asserting equality against the original array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 298283fe-3bfb-49f1-97d3-3ea807cc3e14
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
apps/desktop/docs/fresh-mach-context-design.mdapps/desktop/electron-builder.tsapps/desktop/electron.vite.config.tsapps/desktop/package.jsonapps/desktop/plans/20260417-1500-fresh-mach-context-spawn.mdapps/desktop/src/main/fresh-spawn/auth.test.tsapps/desktop/src/main/fresh-spawn/auth.tsapps/desktop/src/main/fresh-spawn/fresh-exec.test.tsapps/desktop/src/main/fresh-spawn/fresh-exec.tsapps/desktop/src/main/fresh-spawn/handlers/fresh-exec.test.tsapps/desktop/src/main/fresh-spawn/handlers/fresh-exec.tsapps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.test.tsapps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.tsapps/desktop/src/main/fresh-spawn/lifecycle.test.tsapps/desktop/src/main/fresh-spawn/lifecycle.tsapps/desktop/src/main/fresh-spawn/paths.test.tsapps/desktop/src/main/fresh-spawn/paths.tsapps/desktop/src/main/fresh-spawn/spawn-client.test.tsapps/desktop/src/main/fresh-spawn/spawn-client.tsapps/desktop/src/main/fresh-spawn/spawn-server.test.tsapps/desktop/src/main/fresh-spawn/spawn-server.tsapps/desktop/src/main/fresh-spawn/spawn-session.test.tsapps/desktop/src/main/fresh-spawn/spawn-session.tsapps/desktop/src/main/fresh-spawn/spike/fd-passing-spike.test.tsapps/desktop/src/main/fresh-spawn/spike/fd-passing-spike.tsapps/desktop/src/main/fresh-spawn/types.test.tsapps/desktop/src/main/fresh-spawn/types.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/agent-setup/shell-wrappers.test.tsapps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/main/lib/terminal/env.test.tsapps/desktop/src/main/lib/terminal/env.tsapps/desktop/src/main/terminal-host/fresh-spawn-integration.test.tsapps/desktop/src/main/terminal-host/fresh-spawn-integration.tsapps/desktop/src/main/terminal-host/index.tsapps/desktop/src/main/terminal-host/session-shell-ready.test.tsapps/desktop/src/main/terminal-host/session.test.tsapps/desktop/src/main/terminal-host/session.tsapps/desktop/src/main/terminal-host/signal-handlers.tsapps/desktop/src/main/terminal-host/terminal-host.test.tsapps/desktop/src/main/terminal-host/terminal-host.tsapps/desktop/src/resources/shell-hooks/zsh-fresh-exec.zshapps/desktop/src/shared/fresh-spawn-whitelist.test.tsapps/desktop/src/shared/fresh-spawn-whitelist.tsapps/desktop/vite/helpers.ts
Comment on lines
+39
to
+115
| ## 2. Çözüm: Spawn'ı Electron Main'e Delegate Et | ||
|
|
||
| ### Anahtar İçgörü | ||
|
|
||
| **Electron main process her Superset açılışında yeniden doğar.** Bu demektir ki: | ||
| - Her Superset açılışında Electron main **fresh Mach bootstrap context** alır | ||
| - Bu context çağdaş, canlı kullanıcı oturumunun kimliğini taşır | ||
| - Fresh Electron main'den spawn edilen her child **fresh context inherit eder** | ||
|
|
||
| O zaman: pty-subprocess'leri terminal-host yerine Electron main doğursun. Terminal-host sadece sessionları yönetsin, fork yapmasın. | ||
|
|
||
| ### Mimari | ||
|
|
||
| **Önce (stale chain):** | ||
| ``` | ||
| terminal-host (STALE ctx) | ||
| ↓ fork/exec | ||
| pty-subprocess (STALE ctx inherit) | ||
| ↓ fork | ||
| zsh (STALE) | ||
| ↓ exec | ||
| gh ❌ | ||
| ``` | ||
|
|
||
| **Sonra (fresh chain):** | ||
| ``` | ||
| Electron main (FRESH ctx — her restart'ta yenilenir) | ||
| ↓ IPC-triggered spawn | ||
| pty-subprocess (FRESH ctx ✅) | ||
| │ | ||
| │ stdin/stdout FD'leri UDS üzerinden pass edilir | ||
| ▼ | ||
| terminal-host (STALE ctx — ama fork etmiyor!) | ||
| │ | ||
| │ sadece I/O forwarding yapar, ChildProcess referansı tutar | ||
| ▼ | ||
| zsh (FRESH) | ||
| ↓ exec | ||
| gh ✅ | ||
| ``` | ||
|
|
||
| ### Yeni Terminal Akışı | ||
|
|
||
| ``` | ||
| 1. Kullanıcı "yeni terminal" → Electron renderer | ||
| 2. Electron renderer → terminal-host "bana session aç" | ||
| 3. terminal-host → Electron main UDS (spawn-server): "pty-subprocess spawn et" | ||
| 4. Electron main: fresh child_process.spawn(pty-subprocess.js) | ||
| - Child fresh context alır | ||
| 5. Electron main: child'ın stdin/stdout/stderr FD'lerini UDS üzerinden SCM_RIGHTS ile terminal-host'a pass eder | ||
| 6. terminal-host: FD'leri alır, Session class'ına bağlar, I/O akışı başlar | ||
| 7. Kullanıcıya "session hazır" | ||
| ``` | ||
|
|
||
| ### Eski Terminal Akışı (Shell Wrapper ile) | ||
|
|
||
| Eski session'ların içindeki zsh stale. Onları düzeltemeyiz. Ama zsh'in içindeki **komutları** fresh context'te çalıştırabiliriz. | ||
|
|
||
| ``` | ||
| 1. Kullanıcı eski terminalde: gh auth login | ||
| 2. zsh preexec hook devreye girer | ||
| 3. Hook: gh whitelist'te → komutu yakalar | ||
| 4. Hook: komutu `fresh-exec gh auth login` ile replace eder | ||
| 5. fresh-exec → Electron main UDS: "bu komutu fresh ctx'te çalıştır" | ||
| 6. Electron main: fresh child_process.spawn(gh, [auth, login], { stdio: "pipe" }) | ||
| - gh fresh context alır, trustd'ye ulaşır | ||
| 7. Electron main: gh'nin stdin/stdout/stderr'ını fresh-exec'e UDS üzerinden pipe'lar | ||
| 8. fresh-exec: kendi TTY'sine (eski terminal) yönlendirir | ||
| 9. gh çalışır ✅ | ||
| ``` | ||
|
|
||
| ### Neden Çalışır? | ||
|
|
||
| **Fork inheritance kritik:** Mach bootstrap port **fork anında snapshot** olarak geçer. Electron main fresh'se, ondan fork edilen her şey fresh. Terminal-host'un stale olması önemli değil — o artık fork etmiyor, sadece FD'ler üzerinden I/O yönetiyor. | ||
|
|
||
| **Elektron kapalıyken?** Fallback: terminal-host yine eski stale spawn yapar. Degradation, crash değil. | ||
|
|
Contributor
There was a problem hiding this comment.
Update the design doc to match the implemented architecture.
This still describes Electron-main-owned spawning plus SCM_RIGHTS FD passing, but the PR implements daemon-hosted fresh-spawn with NDJSON I/O forwarding and explicitly rejected FD passing. Please refresh the diagrams, terminal flow, fallback notes, and protocol section so the doc does not preserve the obsolete design.
Also applies to: 160-208
🧰 Tools
🪛 LanguageTool
[grammar] ~95-~95: Ensure spelling is correct
Context: ...ession'ların içindeki zsh stale. Onları düzeltemeyiz. Ama zsh'in içindeki komutları fres...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.0)
[warning] 53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 64-64: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 82-82: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/docs/fresh-mach-context-design.md` around lines 39 - 115, Update
the design doc to reflect the implemented architecture: replace the "Electron
main (FRESH ctx)" spawn model and SCM_RIGHTS FD-passing flow with the actual
"daemon-hosted fresh-spawn" design that uses NDJSON I/O forwarding and
explicitly rejects FD passing; update all diagrams (formerly showing "Electron
main → spawn-server", "pty-subprocess", "terminal-host", "fresh-exec") and the
terminal flows to show daemon as the spawner, show NDJSON streams instead of
SCM_RIGHTS FD transfer, adjust the fallback note to describe daemon vs.
terminal-host behavior, and revise the protocol section to document NDJSON I/O
framing and the deliberate rejection of SCM_RIGHTS/FD passing (also apply the
same edits to the later section referenced in the comment).
Author
There was a problem hiding this comment.
Noted — the design doc will be refreshed in a follow-up doc pass. The 'Post-Spike Architecture Update' section at the top of the plan (apps/desktop/plans/20260417-1500-fresh-mach-context-spawn.md) already documents the pivot away from SCM_RIGHTS; I'll propagate the same into fresh-mach-context-design.md to avoid stale guidance for future readers.
Contributor
There was a problem hiding this comment.
@Haknt, understood — thanks for the context. The plan file serving as the interim source of truth for the pivot makes sense. Would you like me to open a GitHub issue to track the fresh-mach-context-design.md refresh so it doesn't get lost across PRs?
🧠 Learnings used
Learnt from: Haknt
Repo: superset-sh/superset PR: 3575
File: apps/desktop/src/main/terminal-host/index.ts:787-805
Timestamp: 2026-04-19T14:17:37.090Z
Learning: In `apps/desktop/src/main/terminal-host/index.ts` (superset-sh/superset desktop app), the fresh-spawn UDS server is intentionally hosted inside the long-lived terminal-host daemon rather than in the Electron main process. The daemon is spawned exactly once by Electron main at first launch (inheriting a fresh macOS Mach bootstrap context at that moment), then survives Electron quit via `detached: true` + `unref()` + SIGHUP/SIGTERM suppression (added in PR `#3574`). On subsequent Electron restarts, the main process re-attaches to the existing daemon via the pid-file liveness check rather than spawning a new one. This means the daemon's Mach context is established once (fresh) and never becomes stale through normal operation; all PTY children of the spawn server inherit that same clean context for the daemon's entire lifetime. Do not flag this placement as a stale-context bug.
Learnt from: Haknt
Repo: superset-sh/superset PR: 3575
File: apps/desktop/src/main/fresh-spawn/spawn-client.ts:40-42
Timestamp: 2026-04-19T14:17:34.111Z
Learning: In `apps/desktop/src/main/fresh-spawn/spawn-client.ts`, `sendSpawnRequest` is a single-shot probe utility (used for health checks and tests). It intentionally destroys the socket after reading the first handshake response (`SpawnResponse`). The actual production streaming path for spawned PTYs goes through `openSpawnSession` in `apps/desktop/src/main/fresh-spawn/spawn-session.ts`, which keeps the UDS socket open after the handshake and exposes a full bidirectional `SpawnSession` EventEmitter. Do not flag `sendSpawnRequest` for closing the socket prematurely — this is intentional design.
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/CLAUDE.md:0-0
Timestamp: 2025-11-24T21:32:21.725Z
Learning: Applies to apps/desktop/**/AGENTS.md : Document agent responsibilities, capabilities, and interaction patterns in AGENTS.md
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-18T22:44:26.758Z
Learning: Applies to **/*_PLAN.md : Place implementation plans in `plans/` (cross-cutting) or `apps/<app>/plans/` (app-scoped); move shipped plans to `plans/done/`. Place architecture/reference docs in `<app>/docs/`. Never create `*_PLAN.md` at an app root or inside `src/`.
Comment on lines
+194
to
+208
| const stdin = new Writable({ | ||
| write(chunk: Buffer | string, _encoding, callback) { | ||
| const buf = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as string); | ||
| writeClientFrame(client, { | ||
| type: "stdin", | ||
| data: buf.toString("base64"), | ||
| }); | ||
| callback(); | ||
| }, | ||
| final(callback) { | ||
| // End of stdin: just stop writing. UDS stream stays open for | ||
| // further frames (signal, resize, incoming stdout). | ||
| callback(); | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
stdin EOF is not propagated to the server — programs that read until EOF will hang.
The Writable's final() callback runs when the consumer calls stdin.end(), but it only completes locally; no frame is written to signal "no more stdin" to the child on the server side. Any command that reads stdin to completion (piped input, cat, sort, gh auth login --with-token < file, CLIs accepting stdin configs) will block forever.
The fresh-spawn protocol in types.ts has no stdin-close / stdin-eof frame — this is a gap in both the schema and the handler. A minimal fix: add a client→server frame like {type:"stdin-close"}, and have both handlers call child.stdin.end() / close the PTY master's write side on receipt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/fresh-spawn/spawn-session.ts` around lines 194 - 208,
The stdin Writable's final() currently doesn't notify the server, causing remote
processes to hang; add a new client→server frame type (e.g. "stdin-close") to
the protocol in types.ts, emit that frame from the Writable.final() in
spawn-session.ts (call writeClientFrame(client, { type: "stdin-close" })) and
update the server/handler that processes incoming frames to treat "stdin-close"
by calling child.stdin.end() or closing the PTY master write side (or
equivalent) so the remote process receives EOF. Ensure any corresponding
switch/case or dispatcher that handles "stdin" frames also handles "stdin-close"
to avoid unhandled-frame errors.
Author
There was a problem hiding this comment.
Deferred: the PTY use case doesn't rely on stdin EOF (TTY programs use ^D/^C explicitly), and the current users of SpawnSession are all PTY-backed. Adding a stdin-close frame type + server-side handling is a protocol change worth doing in a standalone PR once a concrete consumer needs non-PTY pipe semantics.
Contributor
There was a problem hiding this comment.
@Haknt, understood — that's a reasonable deferral. PTY sessions handle EOF via ^D/^C in-band, so there's no blocking issue for the current consumers. I'll track the design intent as a learning.
Would you like me to open a follow-up GitHub issue to track the stdin-close frame / non-PTY pipe semantics work for when a concrete consumer needs it?
(¬_¬)ノ
✏️ Learnings added
Learnt from: Haknt
Repo: superset-sh/superset PR: 3575
File: apps/desktop/src/main/fresh-spawn/spawn-session.ts:194-208
Timestamp: 2026-04-19T14:31:47.672Z
Learning: In `apps/desktop/src/main/fresh-spawn/spawn-session.ts`, the `stdin` Writable's `final()` does not send a `stdin-close` frame to the server — this is intentional. All current `SpawnSession` consumers are PTY-backed; PTY programs use `^D`/`^C` for EOF rather than relying on stdin pipe closure. Adding a `stdin-close` client→server frame type and corresponding server-side `child.stdin.end()` handling is deferred to a future standalone PR once a non-PTY pipe consumer exists.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Haknt
Repo: superset-sh/superset PR: 3575
File: apps/desktop/src/main/fresh-spawn/spawn-client.ts:40-42
Timestamp: 2026-04-19T14:17:34.111Z
Learning: In `apps/desktop/src/main/fresh-spawn/spawn-client.ts`, `sendSpawnRequest` is a single-shot probe utility (used for health checks and tests). It intentionally destroys the socket after reading the first handshake response (`SpawnResponse`). The actual production streaming path for spawned PTYs goes through `openSpawnSession` in `apps/desktop/src/main/fresh-spawn/spawn-session.ts`, which keeps the UDS socket open after the handshake and exposes a full bidirectional `SpawnSession` EventEmitter. Do not flag `sendSpawnRequest` for closing the socket prematurely — this is intentional design.
Learnt from: Haknt
Repo: superset-sh/superset PR: 3575
File: apps/desktop/src/main/terminal-host/index.ts:787-805
Timestamp: 2026-04-19T14:17:37.090Z
Learning: In `apps/desktop/src/main/terminal-host/index.ts` (superset-sh/superset desktop app), the fresh-spawn UDS server is intentionally hosted inside the long-lived terminal-host daemon rather than in the Electron main process. The daemon is spawned exactly once by Electron main at first launch (inheriting a fresh macOS Mach bootstrap context at that moment), then survives Electron quit via `detached: true` + `unref()` + SIGHUP/SIGTERM suppression (added in PR `#3574`). On subsequent Electron restarts, the main process re-attaches to the existing daemon via the pid-file liveness check rather than spawning a new one. This means the daemon's Mach context is established once (fresh) and never becomes stale through normal operation; all PTY children of the spawn server inherit that same clean context for the daemon's entire lifetime. Do not flag this placement as a stale-context bug.
Learnt from: AviPeltz
Repo: superset-sh/superset PR: 2810
File: packages/host-service/src/serve.ts:17-20
Timestamp: 2026-03-24T20:56:09.986Z
Learning: In `packages/host-service/src/serve.ts`, the full session token is intentionally printed to stdout (including `Authorization: Bearer <token>`) because this file is the dev/standalone entry point only. In production Electron, the token is generated in `apps/desktop/src/main/host-service/index.ts` and distributed to the renderer via IPC — never logged to stdout.
Learnt from: AviPeltz
Repo: superset-sh/superset PR: 2266
File: apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useFileLinkClick.ts:35-35
Timestamp: 2026-03-09T02:17:17.939Z
Learning: In the superset-sh/superset desktop app (apps/desktop), Windows is not supported. When reviewing code for path handling in this codebase, do not flag Windows-specific path issues (e.g., drive-letter paths like C:\ or backslash separators). This guideline applies to all TypeScript files under apps/desktop and helps avoid false positives related to Windows-specific path handling in a non-Windows-supported environment.
Learnt from: DogaOztuzun
Repo: superset-sh/superset PR: 2431
File: apps/desktop/src/main/index.ts:0-0
Timestamp: 2026-03-12T19:53:56.860Z
Learning: In Electron apps, do not rely on app code to enable --no-sandbox. For Linux sandbox issues, set --no-sandbox at the launcher/OS level (e.g., pass as a CLI flag to the Electron binary or via the launcher or packaging configuration) before Chromium initializes. Avoid calling app.commandLine.appendSwitch('no-sandbox') from the main process, as Chromium's sandbox check runs during process initialization. Document and centralize this in the launcher/packaging workflow (reference AppImage-specific notes in `#2429`).
Comment on lines
+787
to
+805
| // Start the fresh-spawn UDS server inside the daemon. | ||
| // | ||
| // Hosting the server here (instead of in Electron main) means PTYs | ||
| // spawned through it live under the detached + unref'd daemon process, | ||
| // so they survive Electron app quit — satisfying the "terminal survives | ||
| // app restart" promise. Fresh Mach bootstrap context is still achieved: | ||
| // the daemon was itself spawned by Electron main at launch (fresh | ||
| // context at that moment), so its children inherit a clean context. | ||
| // | ||
| // The pty-subprocess.js bundle is emitted by electron-vite into the | ||
| // same `dist/main/` directory as this entry file, so resolving it | ||
| // relative to `__dirname` works in both dev and packaged builds. | ||
| // | ||
| // Errors are swallowed inside startFreshSpawnServer (it only logs); a | ||
| // failed start degrades cleanly because client code falls back to | ||
| // direct spawn when the socket is missing. | ||
| await startFreshSpawnServer({ | ||
| subprocessScriptPath: join(__dirname, "pty-subprocess.js"), | ||
| }); |
Contributor
There was a problem hiding this comment.
Fresh-spawn still inherits the persistent daemon’s stale context.
Hosting the spawn server inside the long-lived terminal-host means new PTYs/fresh-exec commands remain children of that same daemon. If the daemon survived Electron quit, Fast User Switching, or the stale-context event this PR targets, Line 793’s “fresh context at that moment” assumption no longer holds for later relaunches.
Consider making the fresh-spawn authority belong to the current fresh Electron/helper process, or explicitly rotating only the spawn authority on relaunch while preserving existing PTY ownership.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/terminal-host/index.ts` around lines 787 - 805,
startFreshSpawnServer is being started inside the long-lived terminal-host
(daemon) which lets the fresh-spawn UDS keep the daemon's stale bootstrap
context; instead, ensure the spawn authority is owned by the current fresh
Electron/helper process by either moving the startFreshSpawnServer call out of
the persistent daemon into the fresh Electron/helper process (invoke
startFreshSpawnServer from that process and pass subprocessScriptPath:
join(__dirname, "pty-subprocess.js")), or implement an explicit rotation API
(e.g. rotateFreshSpawnAuthority or restartFreshSpawnServer) that cleanly stops
the existing server in the daemon and restarts it under the current process on
relaunch so new PTYs inherit the fresh context.
Author
There was a problem hiding this comment.
By design: the daemon is spawned by Electron main at first launch (fresh Mach context at that moment) and then survives app quit thanks to detached: true + unref() + the SIGHUP fix in PR #3574. Its children always inherit that same fresh context. The daemon is not re-launched on subsequent app restarts — Electron main re-attaches via the pid-file liveness check, so the fresh context persists across the entire daemon lifetime.
Contributor
There was a problem hiding this comment.
@Haknt, understood — thanks for the clarification. The invariant makes sense: the daemon is started exactly once by the fresh Electron main process, inheriting a clean Mach bootstrap context at that moment, and it then persists indefinitely (surviving Electron quits via detached/unref + the SIGHUP hardening in #3574). Because Electron main re-attaches to the existing daemon on subsequent launches rather than spawning a new one, the daemon's Mach context never becomes stale through normal operation. All PTY children of the spawn server therefore inherit that same original clean context for the daemon's entire lifetime. I'll withdraw my concern.
✏️ Learnings added
Learnt from: Haknt
Repo: superset-sh/superset PR: 3575
File: apps/desktop/src/main/terminal-host/index.ts:787-805
Timestamp: 2026-04-19T14:17:37.090Z
Learning: In `apps/desktop/src/main/terminal-host/index.ts` (superset-sh/superset desktop app), the fresh-spawn UDS server is intentionally hosted inside the long-lived terminal-host daemon rather than in the Electron main process. The daemon is spawned exactly once by Electron main at first launch (inheriting a fresh macOS Mach bootstrap context at that moment), then survives Electron quit via `detached: true` + `unref()` + SIGHUP/SIGTERM suppression (added in PR `#3574`). On subsequent Electron restarts, the main process re-attaches to the existing daemon via the pid-file liveness check rather than spawning a new one. This means the daemon's Mach context is established once (fresh) and never becomes stale through normal operation; all PTY children of the spawn server inherit that same clean context for the daemon's entire lifetime. Do not flag this placement as a stale-context bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: AviPeltz
Repo: superset-sh/superset PR: 2810
File: packages/host-service/src/serve.ts:17-20
Timestamp: 2026-03-24T20:56:09.986Z
Learning: In `packages/host-service/src/serve.ts`, the full session token is intentionally printed to stdout (including `Authorization: Bearer <token>`) because this file is the dev/standalone entry point only. In production Electron, the token is generated in `apps/desktop/src/main/host-service/index.ts` and distributed to the renderer via IPC — never logged to stdout.
Learnt from: kirankunigiri
Repo: superset-sh/superset PR: 2511
File: apps/desktop/src/renderer/stores/tabs/store.ts:2092-2100
Timestamp: 2026-03-16T08:41:29.886Z
Learning: In superset-sh/superset (apps/desktop), the Zustand store merge function in `apps/desktop/src/renderer/stores/tabs/store.ts` intentionally coerces `pane.workspaceRun.state === "running"` to `"stopped-by-exit"` during hydration. This is by design: on a full app restart the daemon is dead, so pessimistic hydration is correct. On renderer-only refresh, `useTerminalLifecycle` remounts within milliseconds and calls `terminal.getSession` to correct the state if the session survived. The brief "stopped-by-exit" flash is acceptable and preferable to stale green badges that could cause incorrect toggle behavior (e.g. attempting to stop a dead process).
Learnt from: rajinsyed
Repo: superset-sh/superset PR: 2722
File: apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx:50-55
Timestamp: 2026-03-22T05:00:07.808Z
Learning: In `apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx`, `workspaceTabs` keeps **all** workspace `TabView` wrappers mounted (toggling `display: none` for inactive tabs) to prevent Electron `<webview>` reparenting and page reloads. However, `TabPane` conditionally renders `<Terminal>` only when the tab is active (`activeTabIds[workspaceId] === tabId`), so terminal stream subscriptions and xterm instances are still freed on tab switch. The net result is a hybrid keepalive: browser/webview panes survive tab switches; terminal panes unmount and remount as before. Do not flag background webview keepalive as a bug, and do not flag terminal conditional rendering as incorrect.
Learnt from: DogaOztuzun
Repo: superset-sh/superset PR: 2431
File: apps/desktop/src/main/index.ts:0-0
Timestamp: 2026-03-12T19:53:56.860Z
Learning: In Electron apps, do not rely on app code to enable --no-sandbox. For Linux sandbox issues, set --no-sandbox at the launcher/OS level (e.g., pass as a CLI flag to the Electron binary or via the launcher or packaging configuration) before Chromium initializes. Avoid calling app.commandLine.appendSwitch('no-sandbox') from the main process, as Chromium's sandbox check runs during process initialization. Document and centralize this in the launcher/packaging workflow (reference AppImage-specific notes in `#2429`).
Learnt from: AviPeltz
Repo: superset-sh/superset PR: 2266
File: apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useFileLinkClick.ts:35-35
Timestamp: 2026-03-09T02:17:17.939Z
Learning: In the superset-sh/superset desktop app (apps/desktop), Windows is not supported. When reviewing code for path handling in this codebase, do not flag Windows-specific path issues (e.g., drive-letter paths like C:\ or backslash separators). This guideline applies to all TypeScript files under apps/desktop and helps avoid false positives related to Windows-specific path handling in a non-Windows-supported environment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
16 issues found across 46 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/fresh-spawn/handlers/fresh-exec.ts">
<violation number="1" location="apps/desktop/src/main/fresh-spawn/handlers/fresh-exec.ts:232">
P1: PTY streaming ignores socket backpressure (`write()` result and `drain`), allowing unbounded buffered output growth under slow readers.</violation>
</file>
<file name="apps/desktop/docs/fresh-mach-context-design.md">
<violation number="1" location="apps/desktop/docs/fresh-mach-context-design.md:174">
P2: The newly added design doc contains stale architecture guidance (SCM_RIGHTS/node-unix-socket FD passing) that conflicts with the documented post-spike implementation direction (UDS NDJSON I/O forwarding), creating actionable maintenance risk.</violation>
</file>
<file name="apps/desktop/plans/20260417-1500-fresh-mach-context-spawn.md">
<violation number="1" location="apps/desktop/plans/20260417-1500-fresh-mach-context-spawn.md:704">
P2: Protocol parsing assumes one `data` event equals one full JSON message; for stream sockets this is unsafe and can produce parse failures or dropped frames.</violation>
<violation number="2" location="apps/desktop/plans/20260417-1500-fresh-mach-context-spawn.md:1379">
P1: FD handoff protocol is broken: client closes the response socket before SCM_RIGHTS descriptors can be received, then attempts FD receive via a separate socket path.</violation>
</file>
<file name="apps/desktop/src/main/fresh-spawn/spawn-client.ts">
<violation number="1" location="apps/desktop/src/main/fresh-spawn/spawn-client.ts:83">
P2: Missing `end`/`close` handling can leave requests waiting until timeout when the server closes gracefully without sending a response line.</violation>
</file>
<file name="apps/desktop/src/main/fresh-spawn/spawn-server.ts">
<violation number="1" location="apps/desktop/src/main/fresh-spawn/spawn-server.ts:181">
P1: The server removes its only `error` listener after startup, so later `net.Server` errors can become unhandled and crash the daemon process.</violation>
</file>
<file name="apps/desktop/src/main/fresh-spawn/spawn-session.ts">
<violation number="1" location="apps/desktop/src/main/fresh-spawn/spawn-session.ts:201">
P2: `stdin` reports writes as successful even when socket writes fail, which can silently drop terminal input.</violation>
</file>
<file name="apps/desktop/src/main/lib/terminal/env.ts">
<violation number="1" location="apps/desktop/src/main/lib/terminal/env.ts:206">
P2: Fresh-exec path lookup negatively caches missing assets forever, so transient startup misses can permanently disable fresh-exec env injection until process restart.</violation>
</file>
<file name="apps/desktop/src/main/fresh-spawn/fresh-exec.ts">
<violation number="1" location="apps/desktop/src/main/fresh-spawn/fresh-exec.ts:50">
P2: `parseFreshExecArgv` uses POSIX-only path splitting, so Windows-style argv paths can be misparsed and select the wrong command index.</violation>
<violation number="2" location="apps/desktop/src/main/fresh-spawn/fresh-exec.ts:310">
P2: Fallback signal handling always returns 143, masking real signal exit codes like SIGINT (130).</violation>
</file>
<file name="apps/desktop/src/main/terminal-host/signal-handlers.ts">
<violation number="1" location="apps/desktop/src/main/terminal-host/signal-handlers.ts:140">
P2: Ignoring SIGTERM/SIGHUP unconditionally breaks existing daemon stop paths that still send SIGTERM, so daemon restart/cleanup may fail silently.</violation>
</file>
<file name="apps/desktop/src/main/fresh-spawn/auth.ts">
<violation number="1" location="apps/desktop/src/main/fresh-spawn/auth.ts:12">
P1: `writeFileSync(..., { mode: 0o600 })` does not harden permissions on existing token files, so insecure prior file mode can persist.</violation>
<violation number="2" location="apps/desktop/src/main/fresh-spawn/auth.ts:28">
P2: `verifyToken` can throw because it checks string length instead of buffer byte length before `timingSafeEqual`.</violation>
</file>
<file name="apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts">
<violation number="1" location="apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts:72">
P1: Missing `child` `'error'` event handling leaves a crash path for spawn/kill failures in daemon process management.</violation>
<violation number="2" location="apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts:144">
P1: Closing the client socket on `exit` can drop trailing stdout/stderr frames; wait for child stdio `close` before ending the stream.</violation>
<violation number="3" location="apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts:219">
P1: Socket backpressure is ignored when forwarding child output, which can cause unbounded buffering and memory growth under heavy output.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| | { type: "error"; message: string; code: string }, | ||
| ): void { | ||
| try { | ||
| client.write(`${JSON.stringify(frame)}\n`); |
Contributor
There was a problem hiding this comment.
P1: PTY streaming ignores socket backpressure (write() result and drain), allowing unbounded buffered output growth under slow readers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/fresh-spawn/handlers/fresh-exec.ts, line 232:
<comment>PTY streaming ignores socket backpressure (`write()` result and `drain`), allowing unbounded buffered output growth under slow readers.</comment>
<file context>
@@ -0,0 +1,281 @@
+ | { type: "error"; message: string; code: string },
+): void {
+ try {
+ client.write(`${JSON.stringify(frame)}\n`);
+ } catch {
+ // socket may be destroyed; ignore
</file context>
Author
There was a problem hiding this comment.
Reviewed, deferring: for local UDS loopback (same machine, terminal reader always the same Superset renderer) kernel buffer exhaustion is not a realistic concern; the existing writeFrame catch handles socket destruction. Explicit drain/backpressure plumbing here would add non-trivial complexity for negligible practical benefit in this specific use case.
| */ | ||
| export function parseFreshExecArgv(argv: string[]): ParsedArgv { | ||
| const freshExecIdx = argv.findIndex((a) => { | ||
| const base = a.split("/").pop() ?? a; |
Contributor
There was a problem hiding this comment.
P2: parseFreshExecArgv uses POSIX-only path splitting, so Windows-style argv paths can be misparsed and select the wrong command index.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/fresh-spawn/fresh-exec.ts, line 50:
<comment>`parseFreshExecArgv` uses POSIX-only path splitting, so Windows-style argv paths can be misparsed and select the wrong command index.</comment>
<file context>
@@ -0,0 +1,388 @@
+ */
+export function parseFreshExecArgv(argv: string[]): ParsedArgv {
+ const freshExecIdx = argv.findIndex((a) => {
+ const base = a.split("/").pop() ?? a;
+ return (
+ base === "fresh-exec" ||
</file context>
Suggested change
| const base = a.split("/").pop() ?? a; | |
| const base = a.split(/[\\/]/).pop() ?? a; |
Author
There was a problem hiding this comment.
Scope-gated: this handler is macOS-only (guarded by process.platform !== 'darwin' in lifecycle.ts), so Windows-style path splitting is not reachable. If Windows support is added later, switch to path.basename(a) — leaving a note in the follow-ups section of the plan doc.
| // bash semantics: 128 + signal number. Node gives signal as a name | ||
| // string; we don't have a portable signal->number mapping here, so | ||
| // use 128 + 15 (SIGTERM) as the generic fallback. | ||
| resolve(128 + 15); |
Contributor
There was a problem hiding this comment.
P2: Fallback signal handling always returns 143, masking real signal exit codes like SIGINT (130).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/fresh-spawn/fresh-exec.ts, line 310:
<comment>Fallback signal handling always returns 143, masking real signal exit codes like SIGINT (130).</comment>
<file context>
@@ -0,0 +1,388 @@
+ // bash semantics: 128 + signal number. Node gives signal as a name
+ // string; we don't have a portable signal->number mapping here, so
+ // use 128 + 15 (SIGTERM) as the generic fallback.
+ resolve(128 + 15);
+ return;
+ }
</file context>
Author
There was a problem hiding this comment.
Noted — the 128 + 15 fallback is conservative but imprecise. A mapping from signal name to number via os.constants.signals would be exact; deferring as a quality-of-life improvement rather than a correctness bug (the main() consumer currently treats any non-zero as failure).
| stopServerErrorMessage: "Error during stopServer in SIGTERM shutdown", | ||
| timeoutMessage: "Forced exit after SIGTERM shutdown timeout", | ||
| }); | ||
| log("info", "Received SIGTERM; ignoring (daemon survival semantics)"); |
Contributor
There was a problem hiding this comment.
P2: Ignoring SIGTERM/SIGHUP unconditionally breaks existing daemon stop paths that still send SIGTERM, so daemon restart/cleanup may fail silently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/terminal-host/signal-handlers.ts, line 140:
<comment>Ignoring SIGTERM/SIGHUP unconditionally breaks existing daemon stop paths that still send SIGTERM, so daemon restart/cleanup may fail silently.</comment>
<file context>
@@ -119,21 +120,27 @@ export function setupTerminalHostSignalHandlers({
- stopServerErrorMessage: "Error during stopServer in SIGTERM shutdown",
- timeoutMessage: "Forced exit after SIGTERM shutdown timeout",
- });
+ log("info", "Received SIGTERM; ignoring (daemon survival semantics)");
});
process.on("SIGHUP", () => {
</file context>
Author
There was a problem hiding this comment.
Same response as on PR #3574: reclassifying SIGTERM as an app-quit signal (rather than a graceful-stop signal) is a deliberate tradeoff. Dev workflows that used kill <pid> should migrate to kill -9 or the shutdown RPC. Happy to add a CHANGELOG/README entry making this explicit.
Haknt
added a commit
to Haknt/superset
that referenced
this pull request
Apr 19, 2026
…y, correctness Addresses actionable review feedback from coderabbitai, greptile-apps, and cubic-dev-ai on PR superset-sh#3575. Security and crash-safety fixes first, then correctness gaps. ## Security * `auth.ts:verifyToken` — compare by byte length, not string length. `crypto.timingSafeEqual` requires equal-length byte buffers; two strings with equal `.length` can produce buffers of different byte lengths under UTF-8 encoding, throwing ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH and crashing the daemon on any adversarial input. Base64url tokens never trigger this via the normal path, but fuzz/adversarial input over the UDS socket can. * `auth.ts:generateTokenFile` — `fs.chmodSync(tokenPath, 0o600)` after `writeFileSync`. The `mode` option to writeFileSync only applies on file creation; if the token path already exists with looser permissions, the token is left world-readable. * `zsh-fresh-exec.zsh` — validate command identifiers against `^[A-Za-z_][A-Za-z0-9_-]*$` before `eval`. The env var SUPERSET_FRESH_EXEC_COMMANDS is inherited by user `.zshrc`, direnv, asdf, etc., which can rewrite arbitrary env — a poisoned entry like "a;rm -rf ~" would otherwise execute through eval. ## Crash safety * `spawn-server.ts` — permanent error listener after startup. The temporary listener was removed on "listening" with no replacement; any post-startup server error (EMFILE, socket permission changes, etc.) would propagate as unhandled and kill the daemon. * `spawn-server.ts` — cap handshake buffer at 1 MiB. A peer that streams bytes without ever sending a newline would otherwise OOM the daemon within the 5s idle window. Matches the existing client-side cap in spawn-session.ts. * `spawn-pty-subprocess.ts` — handle `error` event on spawned child. ChildProcess can emit `error` asynchronously (ENOENT on the binary, EACCES on the subprocess script); without a listener it crashes the daemon. Translate into a synthetic exit frame so the client unblocks cleanly. ## Correctness * `lifecycle.ts:startFreshSpawnServer` — use an in-flight promise sentinel so two concurrent awaited calls don't both pass the null-instance check and race to bind the same socket (EADDRINUSE on the second, clobbered token file). * `spawn-session.ts` client data handler — drain complete lines BEFORE applying the MAX_LINE_BYTES cap. The old order rejected valid bursts of small frames whose aggregate size exceeded 1 MiB. * `fresh-exec.ts:bridgeSocketToStdio` — reject, not resolve, when the socket closes before an `exit` frame arrives. The old behavior made abrupt server disconnects look like a clean exit code 0. See PR superset-sh#3575 review comments for full discussion.
Author
|
Addressed actionable review feedback in the latest push (9 changes across 7 files): Security
Crash safety
Correctness
I also posted responses to the 4 reviews I believe are incorrect or target doc/pseudocode (spawn-client probe vs streaming session confusion, daemon-context FUD, two plan-doc pseudocode reports). Typecheck passes, 106 fresh-spawn + terminal-host tests green. |
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts">
<violation number="1" location="apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts:100">
P2: Synthetic `exit` emission in `error` handler can be followed by unguarded `exit` handler emission, causing duplicate `{type:"exit"}` frames.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/desktop/src/main/fresh-spawn/spawn-session.ts (1)
193-208:⚠️ Potential issue | 🟠 Major
stdinEOF still not propagated to the server — commands that read stdin to completion will hang.The
final()callback at lines 203–207 acknowledges locally but never tells the server that the writable side is done. Any consumer pattern likespawnSession.stdin.end(payload)(e.g.gh auth login --with-token < token, pipedcat/sort, CLIs accepting stdin-delivered configs) will block indefinitely because the PTY/child on the server side never sees EOF on its stdin.The protocol in
types.tshas nostdin-closeframe, so this is a schema gap as well as a handler gap. A minimal fix is to add a{type:"stdin-close"}client→server frame, emit it fromfinal()here, and have bothhandleSpawnPtySubprocessandhandleFreshExecrespond by endingchild.stdin/ closing the PTY master's write side.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/fresh-spawn/spawn-session.ts` around lines 193 - 208, The Writable stream `stdin`'s final() currently only calls callback() and never notifies the server, so EOF isn't propagated; add a new client frame type `{type:"stdin-close"}` in the protocol and call writeClientFrame(client, { type: "stdin-close" }) from `stdin.final()` before invoking the callback; then update the server-side handlers `handleSpawnPtySubprocess` and `handleFreshExec` to handle the `stdin-close` frame by ending/closing the child's stdin or PTY master write side (e.g., call child.stdin.end() or close the PTY write) so remote processes receive EOF.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts`:
- Around line 72-108: The spawn error listener must be attached immediately
after creating the ChildProcess to catch async 'error' events before any
synchronous validation; move the child.on("error", ...) block to directly follow
the spawn(...) call (before the if (!child.stdin || !child.stdout ||
!child.stderr || child.pid == null) check) so ENOENT/EACCES are handled and
produce the same synthetic exit frame via writeFrame(client, { type: "exit", ...
}), client.end(), and tryFinalize().
In `@apps/desktop/src/resources/shell-hooks/zsh-fresh-exec.zsh`:
- Around line 4-7: The top-of-file architecture comment incorrectly states the
fresh-spawn UDS server runs in the Electron main process; update the comment in
zsh-fresh-exec.zsh to state the fresh-spawn server is hosted inside the
long-lived terminal-host daemon (not Electron main) and that fresh-exec proxies
commands through that daemon to avoid stale Mach bootstrap context; specifically
edit the sentence mentioning "fresh-spawn server running in Superset's Electron
main process" to reference "terminal-host daemon" and keep the rest of the
explanation about bypassing stale bootstrap context intact.
- Around line 24-26: The comment incorrectly states that `\<name>` bypasses the
function override; remove the `\<name>` bypass instruction and update the
comment in zsh-fresh-exec.zsh to only document `command <name>` as the proper
way to run the real binary (keep the surrounding explanation about stale
context/TLS commands failing and debugging usefulness intact), ensuring the text
no longer claims backslash-quoting bypasses function resolution.
---
Duplicate comments:
In `@apps/desktop/src/main/fresh-spawn/spawn-session.ts`:
- Around line 193-208: The Writable stream `stdin`'s final() currently only
calls callback() and never notifies the server, so EOF isn't propagated; add a
new client frame type `{type:"stdin-close"}` in the protocol and call
writeClientFrame(client, { type: "stdin-close" }) from `stdin.final()` before
invoking the callback; then update the server-side handlers
`handleSpawnPtySubprocess` and `handleFreshExec` to handle the `stdin-close`
frame by ending/closing the child's stdin or PTY master write side (e.g., call
child.stdin.end() or close the PTY write) so remote processes receive EOF.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46bed254-90db-4ce4-8a63-c51db4690429
📒 Files selected for processing (7)
apps/desktop/src/main/fresh-spawn/auth.tsapps/desktop/src/main/fresh-spawn/fresh-exec.tsapps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.tsapps/desktop/src/main/fresh-spawn/lifecycle.tsapps/desktop/src/main/fresh-spawn/spawn-server.tsapps/desktop/src/main/fresh-spawn/spawn-session.tsapps/desktop/src/resources/shell-hooks/zsh-fresh-exec.zsh
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/main/fresh-spawn/auth.ts
- apps/desktop/src/main/fresh-spawn/lifecycle.ts
- apps/desktop/src/main/fresh-spawn/fresh-exec.ts
Haknt
added a commit
to Haknt/superset
that referenced
this pull request
Apr 19, 2026
Addresses the new actionable findings posted after the previous fix round on PR superset-sh#3575: - **cubic (spawn-pty-subprocess.ts:100) — duplicate exit frames.** The async `child.on('error')` handler emits a synthetic exit frame, but the subsequent `child.once('exit')` handler used to emit again unconditionally. Gated the exit handler behind the `childExited` flag so we emit exactly one exit frame per session. - **coderabbitai (spawn-pty-subprocess.ts:108) — error listener attached too late.** Node emits `error` asynchronously on ENOENT/ EACCES, which could fire before the synchronous pid/stdin validation block ran. Moved the lifecycle state + `child.on('error')` listener to run immediately after `spawn()`, before the validation, so early errors are always captured. - **coderabbitai (zsh-fresh-exec.zsh:4-7) — stale architecture comment.** The header referenced "Electron main" but PR superset-sh#3575 moved the fresh-spawn server into the terminal-host daemon. Rewrote the header to describe the daemon-owned architecture. - **coderabbitai (zsh-fresh-exec.zsh:24) — incorrect bypass instruction.** The docstring claimed `\<name>` bypasses the function override; in zsh, backslash quoting only suppresses alias expansion — functions are still resolved after quote removal. Updated the bypass note to document only `command <name>` as the working bypass and call out the backslash pitfall.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts (1)
149-161: Consider applying backpressure from child stdout/stderr to the UDS socket.
child.stdout/child.stderrare drained as fast as the child produces data and re-framed intoclient.writewithout honoring the socket's return value. If the client reads slowly (or stalls), Node will buffer the outgoing data inside the socket's internal write buffer, which can grow without bound for a chatty subprocess. Pausing the child streams whenclient.writereturnsfalseand resuming ondraingives you natural backpressure and bounds memory.const pipeWithBackpressure = (src: Readable, type: "stdout" | "stderr") => { src.on("data", (chunk: Buffer) => { const ok = client.write(`${JSON.stringify({ type, data: chunk.toString("base64") })}\n`); if (!ok) src.pause(); }); }; client.on("drain", () => { child.stdout?.resume(); child.stderr?.resume(); });Not urgent given typical interactive PTY workloads, but worth addressing before long-running high-throughput sessions (e.g.
tail -fon a verbose log,yes) can stress the daemon.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts` around lines 149 - 161, The child stdout/stderr handlers currently push frames via writeFrame(client, ...) without honoring the UDS socket backpressure; change them to check the client.write return value and pause the corresponding Readable (child.stdout / child.stderr) when write returns false, and resume both streams in a client.on("drain") handler; implement this by replacing the inline data handlers with a small helper (e.g., pipeWithBackpressure(src, "stdout"|"stderr") that calls writeFrame and pauses src when writeFrame/client.write returns false) and add client.on("drain", () => { child.stdout?.resume(); child.stderr?.resume(); }) so high-throughput subprocesses won't build unbounded socket buffers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts`:
- Around line 190-205: The streaming NDJSON loop using buffer and drainBuffer
has no maximum line length and can OOM if a client sends bytes without a
newline; modify drainBuffer (and the client.on("data") flow) to enforce a
max-line cap (e.g. 1 MiB) after concatenating chunk and after each drain
attempt: if buffer.length exceeds the cap with no newline present, call
client.destroy() (or client.end/destroy as appropriate) so the existing close
handler tears down the child; ensure you reference the existing symbols (buffer,
drainBuffer, client.on("data"), handleIncomingFrame, child) when applying the
check so long lines are rejected consistently post-handshake.
---
Nitpick comments:
In `@apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts`:
- Around line 149-161: The child stdout/stderr handlers currently push frames
via writeFrame(client, ...) without honoring the UDS socket backpressure; change
them to check the client.write return value and pause the corresponding Readable
(child.stdout / child.stderr) when write returns false, and resume both streams
in a client.on("drain") handler; implement this by replacing the inline data
handlers with a small helper (e.g., pipeWithBackpressure(src, "stdout"|"stderr")
that calls writeFrame and pauses src when writeFrame/client.write returns false)
and add client.on("drain", () => { child.stdout?.resume();
child.stderr?.resume(); }) so high-throughput subprocesses won't build unbounded
socket buffers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fed1a341-c5cb-4790-9064-10a71e402f45
📒 Files selected for processing (2)
apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.tsapps/desktop/src/resources/shell-hooks/zsh-fresh-exec.zsh
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/resources/shell-hooks/zsh-fresh-exec.zsh
Haknt
added a commit
to Haknt/superset
that referenced
this pull request
Apr 19, 2026
…hook Credit to @coderabbitai on PR superset-sh#3575 for flagging this with a clean repro and a concrete proposed fix. The original path resolvers probed `__dirname`-relative candidates for both fresh-exec.js and zsh-fresh-exec.zsh and trusted Electron's asar-patched `fs.existsSync` to return truthy values. That is correct for consumers running inside Electron main, but silently breaks the feature in packaged production builds because: 1. Both files are asarUnpack'd per `electron-builder.ts`, so they physically live at `.../app.asar.unpacked/...` on disk. 2. `__dirname` inside the main process remains `.../app.asar/...`. 3. `fs.existsSync(.../app.asar/...)` returns true from main because Electron intercepts fs. 4. The returned path is exported to the terminal as `SUPERSET_FRESH_EXEC_BIN` / `SUPERSET_FRESH_EXEC_HOOK_PATH`. 5. zsh reads these env vars and tests `[[ -x ... ]]` / `source ...`, neither of which goes through Electron's fs patch — the asar-interior path does not exist on the real filesystem, the guard in the hook fails, and the feature goes inert. Fix: when a candidate path contains `.../app.asar/...`, probe the `.../app.asar.unpacked/...` twin first and only fall back to the original candidate if the unpack twin is missing. Dev mode (no asar segment in the path) is unchanged. Unit tests cover packaged and dev layouts. Verified in the packaged build: $ ls /Applications/Superset.app/Contents/Resources/app.asar.unpacked/dist/main/fresh-exec.js (exists) $ ls /Applications/Superset.app/Contents/Resources/app.asar/dist/main/fresh-exec.js (Electron sees it via asar fs patch, zsh does not)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts`:
- Around line 149-160: The stdout/stderr handlers currently call
writeFrame(client, ...) which ends up calling client.write(...) without checking
its boolean return, allowing unbounded buffering; update the data handlers for
child.stdout and child.stderr (and the similar handlers around lines 277-282) to
check the boolean result of client.write: if writeFrame/client.write returns
false, call child.pause() to apply backpressure and set client.once('drain',
...) to call child.resume() (and remove the drain listener after firing) so the
child process is paused while the socket is congested and resumed when writable
again.
- Around line 298-305: The code ignores write() return values causing unbounded
buffering; update handleIncomingFrame (case "stdin") to check
child.stdin.write(...)’s boolean result and, if false, call client.pause() and
attach a one-time child.stdin.once("drain", ...) to call client.resume();
similarly, update the stdout/stderr write paths and writeFrame where
client.write(...) is used to check its return value and, if false, pause the
child (child.pause()/child.stdout.pause() as appropriate) and resume on
client.once("drain", ...). Use the existing symbols child.stdin,
child.stdout/stderr, handleIncomingFrame, writeFrame, and client; ensure you
remove swallowed exceptions only after handling backpressure and clean up drain
listeners to avoid leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4be80002-822d-44ce-bf2c-65ca2af563ea
📒 Files selected for processing (1)
apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts">
<violation number="1" location="apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts:322">
P2: Backpressure handling can register multiple pending `drain` listeners for stdin writes, causing listener churn and potential `MaxListenersExceededWarning` under bursty input.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ); | ||
| if (!flushed) { | ||
| client.pause(); | ||
| child.stdin.once("drain", () => client.resume()); |
Contributor
There was a problem hiding this comment.
P2: Backpressure handling can register multiple pending drain listeners for stdin writes, causing listener churn and potential MaxListenersExceededWarning under bursty input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/fresh-spawn/handlers/spawn-pty-subprocess.ts, line 322:
<comment>Backpressure handling can register multiple pending `drain` listeners for stdin writes, causing listener churn and potential `MaxListenersExceededWarning` under bursty input.</comment>
<file context>
@@ -298,7 +314,13 @@ function handleIncomingFrame(line: string, child: ChildProcess): void {
+ );
+ if (!flushed) {
+ client.pause();
+ child.stdin.once("drain", () => client.resume());
+ }
} catch {
</file context>
Adds the implementation plan and architecture design for resolving issue superset-sh#2570 (stale macOS bootstrap context in terminal-host daemon). The approach delegates PTY spawn to the Electron main process (always fresh context) rather than killing running sessions on every restart (as proposed in superset-sh#2571). Refs superset-sh#2570
Uses node-unix-socket for SCM_RIGHTS FD transfer. Round-trip test verifies writable FD sent from sender side reaches receiver side and writes land in the expected file. Finding: node-unix-socket@0.2.7 does not expose any SCM_RIGHTS FD passing API (no sendFd/recvFd on Seqpacket or Dgram surfaces) and its SOCK_SEQPACKET transport throws "Protocol not supported" on macOS. Test codifies the gap and keeps the intended round-trip assertion in an it.skip() so it can be re-enabled once a native addon (Task 3.6) lands. Refs superset-sh#2570
node-unix-socket does not support SCM_RIGHTS FD passing and seqpacket is unavailable on macOS. Rather than write a native N-API addon, we switched to Electron-hosted child processes with I/O forwarding over UDS NDJSON frames — reusing the existing pty-subprocess-ipc framing. Refs superset-sh#2570
Zod discriminated union for SpawnRequest (spawn-pty-subprocess | fresh-exec) and SpawnResponse (ok | error). Tests cover happy paths and validation failures (missing token, missing pty dimensions, bad pid). Refs superset-sh#2570
Replaces process.env.HOME interpolation which could silently produce "undefined/.superset/..." paths when HOME is unset. Adds FRESH_SPAWN_DIR constant to avoid duplicating the directory name. Refs superset-sh#2570
256-bit random token generated at server startup, stored in ~/.superset/fresh-spawn.token with owner-only read. Constant-time comparison prevents timing attacks on the token check. Refs superset-sh#2570
Starts server on configurable socket path, generates auth token, validates request schema, checks token with constant-time compare. Actual spawn handlers are TODO (Tasks 8, 13) and return E_TODO. Refs superset-sh#2570
Prevents resource exhaustion from clients that connect but never send a request. 5s default, configurable via options for tests. Refs superset-sh#2570
Reads auth token from disk, connects to UDS server, writes single NDJSON request, awaits response, validates schema, returns parsed SpawnResponse. Default 5s timeout, configurable. Integration-tested against server skeleton. Refs superset-sh#2570
ServerToClientStreamFrame: stdout | stderr | exit ClientToServerStreamFrame: stdin | resize | signal Base64 used for data payloads to keep NDJSON line-safe. Enables the streaming phase that follows the initial SpawnResponse. Refs superset-sh#2570
Server spawns fresh child in Electron main context, keeps UDS connection
open, and bidirectionally forwards I/O as NDJSON StreamFrames:
- child stdout/stderr -> {type:"stdout"|"stderr", data:base64}
- child exit -> {type:"exit", code, signal}
- client {type:"stdin", data} -> child.stdin
- client {type:"signal", name} -> child.kill
- client {type:"resize", ...} -> no-op (Task 13 honors it for PTYs)
- client disconnect -> SIGTERM then SIGKILL to child
Idle timeout disabled once streaming begins. Target script path is a
server-side config option (SpawnServerOptions.subprocessScriptPath), not
part of the authenticated RPC payload, so clients cannot spawn arbitrary
scripts. Pipelined frames arriving in the same TCP chunk as the auth line
are forwarded into the handler via initialBuffer. Fresh-exec still E_TODO.
Refs superset-sh#2570
openSpawnSession() performs the handshake against the fresh-spawn server, then returns a ChildProcess-like object wrapping the UDS connection. stdin -> client-to-server stdin frames, stdout/stderr from server frames, kill sends signal frames, resize forwards resize frames (no-op for non-PTY spawn, used by fresh-exec later). Refs superset-sh#2570
…ror emit Addresses 3 review findings on SpawnSession: - Emit exit(null,null) when server socket closes without prior exit frame, preventing consumer hang. - Cap in-flight buffer at 1 MiB; destroy socket + emit error on overflow to prevent memory exhaustion from malformed streams. - Guard EventEmitter error emit with listenerCount check so unhandled errors don't crash the process. Refs superset-sh#2570
…lable Session.spawn() now tries fresh-spawn first (macOS only). On success, the resulting SpawnSession drops into place as the subprocess — its stdin/stdout/stderr streams are ChildProcess-compatible. Falls back to existing direct spawn if fresh-spawn is unavailable (non-macOS, socket missing, handshake failure). Fixes the stale Mach context problem while preserving existing behavior everywhere else. Refs superset-sh#2570
Wires startSpawnServer() into app.whenReady() and stops it on before-quit. macOS-only (no-op otherwise). Server spawns pty-subprocess.js in Electron main's fresh Mach context so terminal-host can request pty spawns without inheriting the daemon's stale bootstrap port. Refs superset-sh#2570
Server handles fresh-exec by allocating a PTY via node-pty (interactive commands like gh auth login need a real tty). Forwards I/O bidirectionally as NDJSON StreamFrames and honors resize frames (unlike spawn-pty-subprocess where resize is a no-op). Refs superset-sh#2570
Runs inside a stale terminal (invoked by shell wrapper). Connects to the fresh-spawn UDS server and bridges the fresh PTY's I/O to local stdin/ stdout/stderr. Raw mode stdin forwards keystrokes including Ctrl+C; SIGWINCH triggers a resize frame. Falls back to direct exec if the server is unreachable (non-macOS, server not running) — TLS tools will fail the same way they would without fresh-exec, no worse than baseline. Refs superset-sh#2570
Shared whitelist of Go-based CLIs (gh, terraform, kubectl, tofu, terragrunt) whose macOS TLS path goes through trustd. zsh hook function-overrides each entry to proxy through fresh-exec. Env-var-driven (SUPERSET_FRESH_EXEC_BIN, SUPERSET_FRESH_EXEC_COMMANDS) so the hook is a no-op outside managed Superset terminals. SUPERSET_FRESH_EXEC_ACTIVE breaks recursion on inner subprocess spawns. Refs superset-sh#2570
- Shell-wrappers.ts: append fresh-exec source block to managed .zshrc
- Terminal env builder: emit SUPERSET_FRESH_EXEC_{BIN,COMMANDS,HOOK_PATH}
on darwin so the hook activates only inside Superset terminals
- paths.ts: resolve fresh-exec.js binary + zsh-fresh-exec.zsh hook across
packaged and dev environments
- electron-builder.ts: asarUnpack shell-hooks/ and fresh-exec.js so zsh
can source them (zsh is not asar-aware)
- electron.vite.config.ts: emit fresh-exec.js as a sibling of index.js
so __dirname-relative path resolution matches at runtime
- vite/helpers.ts: include shell-hooks in RESOURCES_TO_COPY for dev mode
Refs superset-sh#2570
The previous default path assumed lifecycle.ts's __dirname would resolve to dist/main/, but rollup may place the lifecycle module in a chunk subdirectory depending on bundling decisions. Moving path resolution to index.ts ties the relative path to a known entry point: rollup emits main/index.js and main/pty-subprocess.js side by side, so the join is stable. Lifecycle signature now requires subprocessScriptPath explicitly — inversion of control. Tests updated accordingly. Refs superset-sh#2570
Moves fresh-spawn server out of Electron main and into the detached terminal-host daemon process. PTY subprocesses spawned via the server become daemon's children and survive Electron app quit, satisfying Superset's "terminal survives app restart" blog promise while preserving fresh Mach bootstrap context at daemon spawn time. Client-side call sites (session.ts, fresh-spawn-integration.ts) are unchanged -- same UDS socket path + token, different host process. Followup PR will add NSWorkspace FUS event refresh to handle long-lived daemon context staleness (separate scope).
…ntics Session survival testing on 2026-04-19 revealed the daemon was still being killed on Electron quit despite the SIGHUP no-op from PR superset-sh#1. Log showed "Terminal host disposed" → "Forced exit after SIGTERM shutdown timeout" → daemon restart, but no "Received SIGTERM" message — meaning stopServer was reached via a path that didn't log the signal. Extending the ignore to SIGTERM (in addition to SIGHUP) prevents macOS app-quit signal propagation from reaching shutdownOnce at all. Intentional daemon shutdown goes through the explicit `shutdown` RPC (calls stopServer() directly) or SIGKILL via killDaemonFromPidFile — neither of which pass through these handlers. Also adds signal diagnostic to Session subprocess exit log so future debugging can distinguish natural exits (code=0) from signal-killed (code=null signal=SIGTERM).
…y, correctness Addresses actionable review feedback from coderabbitai, greptile-apps, and cubic-dev-ai on PR superset-sh#3575. Security and crash-safety fixes first, then correctness gaps. ## Security * `auth.ts:verifyToken` — compare by byte length, not string length. `crypto.timingSafeEqual` requires equal-length byte buffers; two strings with equal `.length` can produce buffers of different byte lengths under UTF-8 encoding, throwing ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH and crashing the daemon on any adversarial input. Base64url tokens never trigger this via the normal path, but fuzz/adversarial input over the UDS socket can. * `auth.ts:generateTokenFile` — `fs.chmodSync(tokenPath, 0o600)` after `writeFileSync`. The `mode` option to writeFileSync only applies on file creation; if the token path already exists with looser permissions, the token is left world-readable. * `zsh-fresh-exec.zsh` — validate command identifiers against `^[A-Za-z_][A-Za-z0-9_-]*$` before `eval`. The env var SUPERSET_FRESH_EXEC_COMMANDS is inherited by user `.zshrc`, direnv, asdf, etc., which can rewrite arbitrary env — a poisoned entry like "a;rm -rf ~" would otherwise execute through eval. ## Crash safety * `spawn-server.ts` — permanent error listener after startup. The temporary listener was removed on "listening" with no replacement; any post-startup server error (EMFILE, socket permission changes, etc.) would propagate as unhandled and kill the daemon. * `spawn-server.ts` — cap handshake buffer at 1 MiB. A peer that streams bytes without ever sending a newline would otherwise OOM the daemon within the 5s idle window. Matches the existing client-side cap in spawn-session.ts. * `spawn-pty-subprocess.ts` — handle `error` event on spawned child. ChildProcess can emit `error` asynchronously (ENOENT on the binary, EACCES on the subprocess script); without a listener it crashes the daemon. Translate into a synthetic exit frame so the client unblocks cleanly. ## Correctness * `lifecycle.ts:startFreshSpawnServer` — use an in-flight promise sentinel so two concurrent awaited calls don't both pass the null-instance check and race to bind the same socket (EADDRINUSE on the second, clobbered token file). * `spawn-session.ts` client data handler — drain complete lines BEFORE applying the MAX_LINE_BYTES cap. The old order rejected valid bursts of small frames whose aggregate size exceeded 1 MiB. * `fresh-exec.ts:bridgeSocketToStdio` — reject, not resolve, when the socket closes before an `exit` frame arrives. The old behavior made abrupt server disconnects look like a clean exit code 0. See PR superset-sh#3575 review comments for full discussion.
Addresses the new actionable findings posted after the previous fix round on PR superset-sh#3575: - **cubic (spawn-pty-subprocess.ts:100) — duplicate exit frames.** The async `child.on('error')` handler emits a synthetic exit frame, but the subsequent `child.once('exit')` handler used to emit again unconditionally. Gated the exit handler behind the `childExited` flag so we emit exactly one exit frame per session. - **coderabbitai (spawn-pty-subprocess.ts:108) — error listener attached too late.** Node emits `error` asynchronously on ENOENT/ EACCES, which could fire before the synchronous pid/stdin validation block ran. Moved the lifecycle state + `child.on('error')` listener to run immediately after `spawn()`, before the validation, so early errors are always captured. - **coderabbitai (zsh-fresh-exec.zsh:4-7) — stale architecture comment.** The header referenced "Electron main" but PR superset-sh#3575 moved the fresh-spawn server into the terminal-host daemon. Rewrote the header to describe the daemon-owned architecture. - **coderabbitai (zsh-fresh-exec.zsh:24) — incorrect bypass instruction.** The docstring claimed `\<name>` bypasses the function override; in zsh, backslash quoting only suppresses alias expansion — functions are still resolved after quote removal. Updated the bypass note to document only `command <name>` as the working bypass and call out the backslash pitfall.
…hook Credit to @coderabbitai on PR superset-sh#3575 for flagging this with a clean repro and a concrete proposed fix. The original path resolvers probed `__dirname`-relative candidates for both fresh-exec.js and zsh-fresh-exec.zsh and trusted Electron's asar-patched `fs.existsSync` to return truthy values. That is correct for consumers running inside Electron main, but silently breaks the feature in packaged production builds because: 1. Both files are asarUnpack'd per `electron-builder.ts`, so they physically live at `.../app.asar.unpacked/...` on disk. 2. `__dirname` inside the main process remains `.../app.asar/...`. 3. `fs.existsSync(.../app.asar/...)` returns true from main because Electron intercepts fs. 4. The returned path is exported to the terminal as `SUPERSET_FRESH_EXEC_BIN` / `SUPERSET_FRESH_EXEC_HOOK_PATH`. 5. zsh reads these env vars and tests `[[ -x ... ]]` / `source ...`, neither of which goes through Electron's fs patch — the asar-interior path does not exist on the real filesystem, the guard in the hook fails, and the feature goes inert. Fix: when a candidate path contains `.../app.asar/...`, probe the `.../app.asar.unpacked/...` twin first and only fall back to the original candidate if the unpack twin is missing. Dev mode (no asar segment in the path) is unchanged. Unit tests cover packaged and dev layouts. Verified in the packaged build: $ ls /Applications/Superset.app/Contents/Resources/app.asar.unpacked/dist/main/fresh-exec.js (exists) $ ls /Applications/Superset.app/Contents/Resources/app.asar/dist/main/fresh-exec.js (Electron sees it via asar fs patch, zsh does not)
The handshake path enforces a 1 MiB line cap, but the streaming loop in spawn-pty-subprocess had no analogous bound. A client that sends bytes without a trailing newline could grow the buffer unbounded and OOM the long-lived daemon, taking every active PTY session down with it. Mirror the handshake cap: destroy the socket once the unconsumed buffer exceeds 1 MiB. The existing close handler tears the child down.
Both directions of the long-lived UDS connection were ignoring write() return values, letting buffers grow unbounded in the daemon whenever one side stopped consuming fast enough: - Server→client: child.stdout/stderr → client.write. If the client's read side is slow, the daemon's TCP-style send buffer would grow per PTY session (bad for commands that dump large output like cat/yes). - Client→server: client data → child.stdin.write. If the child is slow to read (blocked, waiting, compute-bound), the socket read buffer would grow unbounded. writeFrame() now returns the client.write boolean; the stdout/stderr 'data' handlers pause the respective child stream and resume on the socket's 'drain'. handleIncomingFrame threads the Socket through so the 'stdin' case can pause the client and resume on child.stdin 'drain'. The 1 MiB MAX_LINE_BYTES cap still protects against malformed (no newline) input; this commit addresses the orthogonal case where frames are well-formed but produced faster than the peer consumes.
Haknt
force-pushed
the
feat/fresh-spawn-in-daemon
branch
from
98618ea to
a3a939f
Compare
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.
Summary
On macOS, the long-lived terminal-host daemon accumulates a stale Mach bootstrap context over time. The symptom is that Go CLIs that reach out to
trustdfor TLS certificate validation —gh,terraform,kubectl,gcloud— fail mysteriously in terminals spawned from a session that predates a Fast User Switch or similar login-session transition.#2571 addressed this by killing and respawning the daemon on every Superset launch. That restores a fresh context but at the cost of killing every active PTY session — an unacceptable trade-off given Superset's "Terminal That (Almost) Never Dies" promise (blog post).
This PR takes a different path: the daemon remains long-lived, but PTY spawn is delegated to a dedicated UDS-based spawn server, which can be restarted independently (or its subprocesses can be spawned in a controlled, fresh-context way) without killing existing sessions.
Fixes #2570.
Architecture
A UDS server is started inside the daemon's
main().session.tsopens a UDS connection as a client and receives aSpawnSessionthat implements a ChildProcess-compatible surface (stdin/stdout/stderr/kill/exit), so the rest of Session wiring is unchanged. The directspawnProcess()fallback is preserved for non-macOS platforms and for handshake failures.Why I/O forwarding instead of FD passing: Task 3 spike validated that
node-unix-socket@0.2.7does not expose SCM_RIGHTS on macOS andSOCK_SEQPACKETis unavailable. A pure I/O forwarding approach over NDJSON-framed UDS streams is materially simpler and avoids a native addon. The full design doc lives atapps/desktop/plans/20260417-1500-fresh-mach-context-spawn.md, including the pivot rationale at lines 13–30.Shell wrapper path (Phase 2 of the plan): A
fresh-exechelper binary + zsh preexec hook + whitelist (gh,terraform,kubectl, etc.) is included. When a whitelisted command is typed in an existing stale-context shell, zsh routes it through the spawn server'sfresh-exechandler so it runs in a fresh context without touching the parent shell's context. This gives existing terminals the fresh-context benefit without forcing users to close them.Why the server lives in the daemon (not Electron main)
An earlier iteration hosted the spawn server in Electron main. That worked for the fresh-context requirement (Electron main is born fresh at every launch), but it violated the session-survival requirement: PTY subprocesses were children of Electron main, so they died when the user quit the app. The daemon-side placement here preserves both requirements:
NSWorkspaceSessionDidBecomeActiveNotification— out of scope here).What's in this PR
22 commits grouped into phases:
Verification
bun run typecheckclean (root, all 25 tasks).bun run lintclean.bun test apps/desktop/src/main/fresh-spawn/— 65 pass, 1 skip (platform-gated), 0 fail.bun test apps/desktop/src/main/terminal-host/— 41 pass, 0 fail.attached, notcreated).Session Contents Restoredbanner does not appear).Related PRs
Follow-ups
NSWorkspaceSessionDidBecomeActiveNotificationto refresh the spawn server on Fast User Switching events — closes the narrow window where the daemon's context might go stale mid-session.Summary by cubic
Delegates PTY spawn to a UDS server inside the terminal‑host daemon on macOS so each new terminal gets a fresh Mach bootstrap context without killing existing sessions. Adds a
fresh-exechelper and zsh hook so whitelisted CLIs run in a fresh context from older terminals; sessions survive app quit.New Features
fresh-exec+ zsh hook routesgh,kubectl,terraform,terragrunt, andtofuthrough a fresh PTY; supports resize; env‑gated and inert if paths are missing.fresh-exec.js; path resolvers detectapp.asar.unpackedso zsh can source/exec in packaged builds and work the same in dev.Bug Fixes
0600on the token file; validate zsh whitelist entries beforeeval.errorlistener early; emit exactly one exit frame on failures.exitframe as an error infresh-exec.Written for commit a3a939f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests