Repository navigation
fix(desktop): gate terminal stdin on shell readiness to prevent preset command loss - #2469
Conversation
|
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:
📝 WalkthroughWalkthroughThis pull request introduces shell readiness detection for terminal sessions. It adds marker emission logic to shell wrappers (zsh, bash, fish), exports a marker constant, implements marker detection and input buffering in session handling, and includes comprehensive tests for buffering and marker detection behavior. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Session
participant PTY as PTY/Shell
participant Buffer as Input Buffer
Client->>Session: Start terminal session
activate Session
Session->>PTY: Spawn shell process
activate PTY
Note over Session: Shell readiness state = "pending"
Note over Session: Start timeout (SHELL_READY_TIMEOUT_MS)
Client->>Session: Write input
Session->>Buffer: Queue write (pending state)
Note over Buffer: Input buffered, not yet sent
PTY->>Session: Emit startup output + marker
activate Session
Session->>Session: Detect SHELL_READY_MARKER in data
Session->>Session: Strip marker bytes from output
Note over Session: Shell readiness state = "ready"
deactivate Session
Session->>PTY: Flush queued input buffer to shell
Buffer->>PTY: Send buffered writes
PTY->>Client: Return shell output (marker stripped)
deactivate PTY
deactivate Session
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable sequence diagrams in the walkthrough.Disable the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 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/terminal-host/session.ts">
<violation number="1" location="apps/desktop/src/main/terminal-host/session.ts:299">
P2: Shell-ready marker detection is chunk-fragile: if the marker is split across PTY data frames, readiness is never detected and session creation waits for timeout.</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts:278">
P2: Handle array-valued `PROMPT_COMMAND` in Bash 5.1+; this scalar assignment can discard existing prompt hooks.</violation>
</file>
<file name="apps/desktop/src/main/terminal-host/terminal-host.ts">
<violation number="1" location="apps/desktop/src/main/terminal-host/terminal-host.ts:159">
P2: This unconditionally waits for a shell-ready marker that some supported shells (`sh`/`ksh`) never emit, causing a guaranteed 15s startup delay for those sessions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/terminal-host/session.ts`:
- Around line 293-312: The current PtySubprocessIpcType.Data handler only
searches the decoded chunk for SHELL_READY_MARKER and misses markers split
across frames; modify the handler (in session.ts within the Data case) to keep a
small carry buffer on the session (e.g., this._pendingMarkerBuffer) and operate
on Buffers instead of the decoded string: prepend the new payload to the pending
buffer, search that combined Buffer for the SHELL_READY_MARKER sequence, remove
the marker bytes when found (set this.shellReadyResolved and call
this.shellReadyResolve as before), forward the remaining bytes to clients as
UTF-8, and if not found keep only the trailing N-1 bytes of the combined buffer
(N = marker length) to avoid unbounded growth so future frames can complete a
split marker.
In `@apps/desktop/src/main/terminal-host/terminal-host.ts`:
- Around line 153-169: Register the newly spawned Session in this.sessions
before awaiting session.waitForShellReady() (or use a separate pending map) so
concurrent createOrAttach/kill/disconnect can see it; then after the
promiseWithTimeout resolves/rejects re-check session.isAlive (or call a helper)
and if the session already exited (Session.handleSubprocessExit set it dead)
remove it from this.sessions and treat it as non-new. Specifically, move the
this.sessions.set(sessionId, session) to just before calling
promiseWithTimeout(session.waitForShellReady(), SHELL_READY_TIMEOUT_MS) (or add
an inFlight map entry), and after waiting re-evaluate session.isAlive and adjust
isNew/cleanup accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 745a4d51-6739-46f9-8f93-2275f60236b5
📒 Files selected for processing (4)
apps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/main/terminal-host/pty-subprocess-ipc.tsapps/desktop/src/main/terminal-host/session.tsapps/desktop/src/main/terminal-host/terminal-host.ts
There was a problem hiding this comment.
2 issues found across 3 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/terminal-host/session.ts">
<violation number="1" location="apps/desktop/src/main/terminal-host/session.ts:310">
P1: When the shell-ready marker is split across PTY frames, slicing with `this.shellReadyCarry.length` can drop prompt/output bytes and corrupt the OSC sequence handling.</violation>
</file>
<file name="apps/desktop/src/main/terminal-host/terminal-host.ts">
<violation number="1" location="apps/desktop/src/main/terminal-host/terminal-host.ts:178">
P2: The post-wait liveness check uses `isAlive`, which misses sessions that became terminating during initialization. This can return a non-attachable session after a concurrent kill.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/terminal-host/session.ts`:
- Around line 299-329: The carry-buffer logic around SHELL_READY_MARKER is
corrupting output because partial-marker bytes from this.shellReadyCarry are
being forwarded and later assumed already-sent; fix by never forwarding the
carry itself and instead forward only the safe prefix of combined data that
cannot be part of a split marker: when marker not found, set maxCarry =
SHELL_READY_MARKER.length - 1, set this.shellReadyCarry =
combined.slice(-maxCarry) and set data = combined.slice(0, combined.length -
maxCarry) (so potential marker tail is withheld); when marker found, compute
stripped = combined.slice(0, markerIndex) + combined.slice(markerIndex +
SHELL_READY_MARKER.length), set data = stripped (do not slice by
this.shellReadyCarry.length), clear this.shellReadyCarry, set
this.shellReadyResolved and call this.shellReadyResolve as before; keep the
existing early-break if data.length === 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 788c460c-80c0-45d5-8d1d-d41408152c67
📒 Files selected for processing (3)
apps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/main/terminal-host/session.tsapps/desktop/src/main/terminal-host/terminal-host.ts
6bd85c7 to
2073451
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/main/terminal-host/session.ts (1)
303-303: Minor: The marker is 27 bytes, not 30.The comment states "the marker is 30 bytes" but
SHELL_READY_MARKERis 27 bytes (\x1b+]777;superset-shell-ready+\x07).📝 Suggested fix
- // still detected (the marker is 30 bytes; a single printf usually + // still detected (the marker is 27 bytes; a single printf usually🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.ts` at line 303, The inline comment incorrectly states the marker is 30 bytes; update the comment near SHELL_READY_MARKER in session.ts to say the marker is 27 bytes (it consists of '\x1b' + ']777;superset-shell-ready' + '\x07'), so adjust the sentence "the marker is 30 bytes" to "the marker is 27 bytes" (or reword to compute length from SHELL_READY_MARKER) to keep the comment accurate.apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts (1)
270-292: Misleading comment: bash hook is prepended, not appended.The comment on lines 270-272 states the hook "fires AFTER direnv and other hooks complete," but the implementation prepends
_superset_shell_readyto PROMPT_COMMAND (lines 288, 291), causing it to fire BEFORE other hooks in the chain.This doesn't break functionality—the marker still fires when the first prompt is about to display (after RC files are sourced), which is the key requirement. However, the comment is inaccurate.
📝 Suggested comment fix
-# One-shot shell-ready marker for preset command timing. -# Uses PROMPT_COMMAND so it fires AFTER direnv and other hooks complete. -# Supports both scalar and array PROMPT_COMMAND (Bash 5.1+). +# One-shot shell-ready marker for preset command timing. +# Uses PROMPT_COMMAND to fire when the first prompt is about to display +# (after RC files are sourced). Supports both scalar and array PROMPT_COMMAND (Bash 5.1+).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts` around lines 270 - 292, Update the misleading comment above the _superset_shell_ready function to reflect that the hook is prepended to PROMPT_COMMAND and therefore runs before (not after) other PROMPT_COMMAND hooks like direnv; specifically mention that we insert "_superset_shell_ready" at the front of PROMPT_COMMAND (see PROMPT_COMMAND=("_superset_shell_ready" "${PROMPT_COMMAND[@]}") and the non-array branch) so the marker fires on the first prompt before other hooks, and adjust wording to say it ensures the marker fires when the first prompt is about to display rather than claiming it runs after other hooks complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts`:
- Around line 270-292: Update the misleading comment above the
_superset_shell_ready function to reflect that the hook is prepended to
PROMPT_COMMAND and therefore runs before (not after) other PROMPT_COMMAND hooks
like direnv; specifically mention that we insert "_superset_shell_ready" at the
front of PROMPT_COMMAND (see PROMPT_COMMAND=("_superset_shell_ready"
"${PROMPT_COMMAND[@]}") and the non-array branch) so the marker fires on the
first prompt before other hooks, and adjust wording to say it ensures the marker
fires when the first prompt is about to display rather than claiming it runs
after other hooks complete.
In `@apps/desktop/src/main/terminal-host/session.ts`:
- Line 303: The inline comment incorrectly states the marker is 30 bytes; update
the comment near SHELL_READY_MARKER in session.ts to say the marker is 27 bytes
(it consists of '\x1b' + ']777;superset-shell-ready' + '\x07'), so adjust the
sentence "the marker is 30 bytes" to "the marker is 27 bytes" (or reword to
compute length from SHELL_READY_MARKER) to keep the comment accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe86ae77-c335-4ef3-90b6-fac1bcf1c70b
📒 Files selected for processing (4)
apps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/main/terminal-host/pty-subprocess-ipc.tsapps/desktop/src/main/terminal-host/session.tsapps/desktop/src/main/terminal-host/terminal-host.ts
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
Review agent take:
Findings
-
apps/desktop/src/main/terminal-host/terminal-host.ts:150
createOrAttach()is carrying shell-init latency. That fixes preset timing, but it puts the wait on the wrong boundary: terminal creation/attach now blocks on prompt readiness. The terminal should be attachable as soon as the PTY is live; only stdin delivery needs to wait for shell readiness. -
apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts:267
The bash path prepends_superset_shell_readytoPROMPT_COMMAND, so the marker can fire before existing prompt hooks. Ifdirenv/devenv/condawork is inPROMPT_COMMAND, this can still signal readiness too early. -
apps/desktop/src/main/terminal-host/session.ts:296
The marker stripping logic is still chunk-sensitive. A split marker can leak prefix bytes into xterm/output because the current carry approach forwards bytes before it knows whether they belong to the marker.
Correct architecture
- Keep the shell-emitted marker. That is the right detector for “interactive shell is actually ready for input.” PTY spawn alone cannot tell us that.
- Move readiness ownership into
Session, notTerminalHost.createOrAttach(). - Attach immediately once the PTY is spawn-ready.
- Buffer writes in
Sessionwhile readiness is pending for shells that advertise the marker. - Resolve that pending state on marker, timeout, unsupported shell, or subprocess exit, then flush or drop the queued writes in order.
- Leave the renderer sequencing alone for now. If the host owns the pre-ready input queue,
await createOrAttach(); write(...)becomes safe without stalling session attach. - Optional follow-up: make preset launch atomic by allowing
createOrAttachto carry initial input, but that is a cleanup on top of the host-side queue, not the core fix.
Plan
- Introduce session-scoped shell readiness state:
pending | ready | timed_out | unsupported | exitedplus a small pre-ready stdin queue. - Start the readiness timeout when the session spawns, not inside
createOrAttach(). - Remove the blocking
waitForShellReady()fromTerminalHost.createOrAttach(); register the session and attach/snapshot immediately after PTY readiness. - Gate
Session.write()on shell readiness for new interactive shells, then flush queued writes in FIFO order when readiness resolves. - Fix bash hook ordering so
_superset_shell_readyruns after existingPROMPT_COMMANDhooks instead of before them. - Replace the current carry-based marker stripping with safe-prefix buffering or a small byte-level state machine so split markers never leak into terminal output.
- Add focused coverage for:
- preset write during delayed shell init
- marker split across PTY frames
- bash scalar and array
PROMPT_COMMAND - kill/exit before readiness resolves
- unsupported shells falling back without blocking attach
I’d reframe the PR around host-side input gating rather than blocking session creation. That keeps the UX responsive and fixes the actual race at the write boundary.
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/terminal-host/session.ts">
<violation number="1" location="apps/desktop/src/main/terminal-host/session.ts:308">
P1: Split-frame shell-ready marker stripping can leak an unterminated OSC prefix to the terminal stream, causing subsequent output to be consumed until a later terminator appears.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/main/terminal-host/session.ts (1)
294-319:⚠️ Potential issue | 🔴 CriticalCarry-buffer handling still corrupts output when marker spans frames.
Line 317 stores carry but Line 321 can still forward bytes that may be part of the marker tail. Then Line 308 slices only current-frame data assuming carry was already safe, which can leak partial OSC bytes and drop real output after the marker.
🐛 Proposed fix
if (!this.shellReadyResolved) { const combined = this.shellReadyCarry + data; const markerIndex = combined.indexOf(SHELL_READY_MARKER); if (markerIndex !== -1) { - // Strip only marker bytes belonging to current frame - // (carry bytes were already forwarded last time) - const carryLen = this.shellReadyCarry.length; - const markerEnd = markerIndex + SHELL_READY_MARKER.length; - const dataStripStart = Math.max(0, markerIndex - carryLen); - const dataStripEnd = markerEnd - carryLen; - data = data.slice(0, dataStripStart) + data.slice(dataStripEnd); + // Strip marker from full combined stream (carry + current frame). + data = + combined.slice(0, markerIndex) + + combined.slice(markerIndex + SHELL_READY_MARKER.length); this.shellReadyCarry = ""; this.shellReadyResolved = true; if (this.shellReadyResolve) { this.shellReadyResolve(); this.shellReadyResolve = null; } } else { const maxCarry = SHELL_READY_MARKER.length - 1; - this.shellReadyCarry = combined.slice(-maxCarry); + // Forward only the safe prefix; keep possible marker tail in carry. + const safeLength = Math.max(0, combined.length - maxCarry); + data = safeLength > 0 ? combined.slice(0, safeLength) : ""; + this.shellReadyCarry = combined.slice(safeLength); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.ts` around lines 294 - 319, The carry-handling currently can forward bytes that may be part of a marker spanning frames; update the logic in the session terminal data handler (variables: shellReadyCarry, shellReadyResolved, shellReadyResolve, SHELL_READY_MARKER, payload/data) so that while shellReadyResolved is false you never forward the trailing up-to-(SHELL_READY_MARKER.length-1) bytes of the combined carry+data (they must be kept in shellReadyCarry), and only forward the safe prefix; when the marker is found in combined, remove the marker from the combined stream and forward the safe bytes (excluding the retained carry tail), set shellReadyResolved and resolve shellReadyResolve; otherwise set shellReadyCarry to the last maxCarry bytes of combined and do not forward those bytes. Ensure all slicing uses the combined buffer/string so you don't accidentally forward bytes that might belong to the marker.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts (1)
221-221: Avoid hardcoding the shell-ready OSC marker in multiple snippets.The same marker literal is embedded in zsh/bash/fish wrappers. Please source it from a single constant (or shared helper) to avoid drift from
apps/desktop/src/main/terminal-host/pty-subprocess-ipc.ts.Also applies to: 270-270, 324-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts` at line 221, Extract the OSC marker literal into a single exported constant (e.g., SHELL_READY_OSC or SHELL_READY_OSC_MARKER) in the module that already defines it (the code in pty-subprocess-ipc.ts), export that constant, then import and use that constant in shell-wrappers.ts to replace the hardcoded '\\033]777;superset-shell-ready\\007' occurrences inside the zsh/bash/fish wrapper strings (the printf usages in shell-wrappers.ts at the three places noted) so all snippets reference the shared constant rather than duplicating the literal.
🤖 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/lib/agent-setup/shell-wrappers.ts`:
- Around line 283-288: The current logic prepends _superset_shell_ready to
PROMPT_COMMAND (both array and scalar branches), causing it to run before
existing hooks; change both branches to append the hook so _superset_shell_ready
runs after existing PROMPT_COMMAND hooks. For the array branch that currently
assigns PROMPT_COMMAND=("_superset_shell_ready" "${PROMPT_COMMAND[@]}"), update
it to append the symbol _superset_shell_ready to the existing array (use
PROMPT_COMMAND+= or equivalent) and for the scalar branch that currently builds
_superset_orig_prompt_cmd and sets
PROMPT_COMMAND="_superset_shell_ready;${_superset_orig_prompt_cmd}", swap the
order so the existing command runs first and then _superset_shell_ready is
appended (preserve any quoting/escaping used for _superset_orig_prompt_cmd).
---
Duplicate comments:
In `@apps/desktop/src/main/terminal-host/session.ts`:
- Around line 294-319: The carry-handling currently can forward bytes that may
be part of a marker spanning frames; update the logic in the session terminal
data handler (variables: shellReadyCarry, shellReadyResolved, shellReadyResolve,
SHELL_READY_MARKER, payload/data) so that while shellReadyResolved is false you
never forward the trailing up-to-(SHELL_READY_MARKER.length-1) bytes of the
combined carry+data (they must be kept in shellReadyCarry), and only forward the
safe prefix; when the marker is found in combined, remove the marker from the
combined stream and forward the safe bytes (excluding the retained carry tail),
set shellReadyResolved and resolve shellReadyResolve; otherwise set
shellReadyCarry to the last maxCarry bytes of combined and do not forward those
bytes. Ensure all slicing uses the combined buffer/string so you don't
accidentally forward bytes that might belong to the marker.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts`:
- Line 221: Extract the OSC marker literal into a single exported constant
(e.g., SHELL_READY_OSC or SHELL_READY_OSC_MARKER) in the module that already
defines it (the code in pty-subprocess-ipc.ts), export that constant, then
import and use that constant in shell-wrappers.ts to replace the hardcoded
'\\033]777;superset-shell-ready\\007' occurrences inside the zsh/bash/fish
wrapper strings (the printf usages in shell-wrappers.ts at the three places
noted) so all snippets reference the shared constant rather than duplicating the
literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37a49fed-5b9e-4637-a8c9-be2d68bbabc1
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.tsapps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/main/terminal-host/pty-subprocess-ipc.tsapps/desktop/src/main/terminal-host/session-lifecycle.test.tsapps/desktop/src/main/terminal-host/session.tsapps/desktop/src/main/terminal-host/terminal-host.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/terminal-host/pty-subprocess-ipc.ts
c0e5e51 to
cf92810
Compare
Roshvan
changed the title
cf92810 to
5adab42
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/main/terminal-host/session-shell-ready.test.ts (2)
178-185: Remove unused_broadcastedDatavariable.The
_broadcastedDatavariable is declared and reset increateTrackedSessionbut never actually used in any test assertions. This appears to be leftover scaffolding.Simplify by removing unused tracking
describe("Session shell-ready: marker detection", () => { - let _broadcastedData: string[]; - - function createTrackedSession(shell: string) { - const result = createTestSession(shell); - _broadcastedData = []; - return result; - } + // Use createTestSession directly - no special tracking needed it("strips marker from single data frame", () => { - const { session, proc } = createTrackedSession("/bin/zsh"); + const { session, proc } = createTestSession("/bin/zsh");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session-shell-ready.test.ts` around lines 178 - 185, Remove the unused tracking variable by deleting the declaration and resets for _broadcastedData and any related references in the test; specifically remove the top-level let _broadcastedData: string[]; and the line that sets _broadcastedData = []; inside createTrackedSession (function name createTrackedSession and variable _broadcastedData identify the locations to change), leaving createTrackedSession to simply return createTestSession(shell) and updating any tests that referenced _broadcastedData to use actual assertions or remove those unused assertions.
161-175: Test title is slightly misleading.The test is titled "flushes buffered writes on timeout" but actually tests exit behavior. While the result is equivalent (both resolve shell readiness and flush the queue), the title suggests timer-based timeout testing.
Consider renaming to "flushes buffered writes on subprocess exit" for clarity, or add a separate test using timer mocks to test the actual 15s timeout path.
Suggested rename
- it("flushes buffered writes on timeout", async () => { + it("flushes buffered writes on subprocess exit", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session-shell-ready.test.ts` around lines 161 - 175, Rename the test title to accurately reflect that it exercises subprocess exit behavior (e.g., change the it description in session-shell-ready.test.ts from "flushes buffered writes on timeout" to "flushes buffered writes on subprocess exit") so it's clear this uses sendExit/proc.emit to resolve readiness; alternatively add a new test that uses Jest's fake timers to simulate the actual 15s timeout path and verify buffered writes are flushed there (use createTestSession, spawnAndReady, session.write, getWrittenData and the timeout resolution path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/terminal-host/session-shell-ready.test.ts`:
- Around line 178-185: Remove the unused tracking variable by deleting the
declaration and resets for _broadcastedData and any related references in the
test; specifically remove the top-level let _broadcastedData: string[]; and the
line that sets _broadcastedData = []; inside createTrackedSession (function name
createTrackedSession and variable _broadcastedData identify the locations to
change), leaving createTrackedSession to simply return createTestSession(shell)
and updating any tests that referenced _broadcastedData to use actual assertions
or remove those unused assertions.
- Around line 161-175: Rename the test title to accurately reflect that it
exercises subprocess exit behavior (e.g., change the it description in
session-shell-ready.test.ts from "flushes buffered writes on timeout" to
"flushes buffered writes on subprocess exit") so it's clear this uses
sendExit/proc.emit to resolve readiness; alternatively add a new test that uses
Jest's fake timers to simulate the actual 15s timeout path and verify buffered
writes are flushed there (use createTestSession, spawnAndReady, session.write,
getWrittenData and the timeout resolution path).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b2ae59c-46f4-4996-9d21-163de8ed316a
📒 Files selected for processing (4)
apps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/main/terminal-host/pty-subprocess-ipc.tsapps/desktop/src/main/terminal-host/session-shell-ready.test.tsapps/desktop/src/main/terminal-host/session.ts
71bc3aa to
66f4c49
Compare
…t command loss
When a preset launches a terminal and sends a command (e.g. `claude`),
the command was lost because the shell hadn't finished initializing.
Tools like direnv, devenv, nvm, conda, and oh-my-zsh hook into shell
startup and take non-trivial time to load. The preset command arrived
during that initialization window and was swallowed.
Solution: Session-owned shell readiness state machine that gates
`write()` on shell initialization status. A one-shot OSC marker
(\033]777;superset-shell-ready\007) is emitted from shell prompt hooks
once the shell is fully initialized:
- zsh: precmd hook in .zlogin (last in precmd_functions)
- bash: PROMPT_COMMAND hook (appended, supports Bash 5.1+ arrays)
- fish: --on-event fish_prompt handler
Session detects the marker via a byte-level state machine that
withholds potential marker bytes from terminal output until confirmed,
preventing any leakage to xterm even across PTY data frame boundaries.
During shell init:
- User commands are buffered in a pre-ready stdin queue
- Terminal protocol responses (DA, DSR) are dropped — they are stale
responses that would appear as typed text (e.g. ?62;4;9;22c)
- Headless emulator responses are dropped (renderer handles queries
once a client attaches)
- All buffered writes flush in FIFO order once readiness resolves
createOrAttach() returns immediately — the terminal attaches and
renders right away. Only stdin delivery is gated, not session creation.
This follows the same pattern used by VS Code (OSC 133), iTerm2 (OSC
1337), and Warp (OSC 9001) for shell integration.
66f4c49 to
2096641
Compare
|
@Kitenite — Addressed all your feedback. Here's a walkthrough of the implementation and how it maps to your plan. The ProblemWhen you open a terminal in Superset and a preset tries to run a command (like What Was Wrong With the First ApproachThe initial fix blocked
Your review correctly identified this: the terminal should render immediately. Only the command delivery needs to wait. How It Works Now1. Shell-side: One-shot ready markerOur shell wrappers now inject a tiny invisible signal — an OSC escape sequence — that fires right before the first prompt appears: Same pattern VS Code (OSC 133), iTerm2 (OSC 1337), and Warp (OSC 9001) use for shell integration. We use 777 to avoid conflicts. Why a prompt hook? Because Why last in the hook chain? direnv and other tools also register prompt hooks. We append ours at the end so it fires AFTER everything else finishes. If we prepended it, the marker would fire before direnv was done loading. 2. Session-side: State machine + write gatingInstead of blocking session creation, the How
When the marker arrives (or the timeout fires), all buffered commands flush to the PTY in order. Why drop escape sequences? During shell init, zsh queries terminal capabilities. The renderer's xterm responds with something like 3. Marker detection: Byte-level state machineWe scan PTY output one character at a time, matching against the marker. Bytes that might be part of the marker are held back. Full match → discard held bytes and resolve readiness. Mismatch → flush held bytes as regular output. Zero marker bytes ever leak to xterm, even across chunk boundaries. 4. Headless emulator DA responsesThe headless emulator responds to terminal queries when no renderer client is attached. During init we drop these — they'd pollute the stdin queue and appear as typed text once flushed. After a client attaches the renderer handles all queries. Mapping to Your Plan
Files Changed
User ExperienceBefore: Terminal opens, hangs for up to 15s (or command lost entirely). Happy to adjust further — let me know what you think or if there's another way you'd like to tackle this. Happy to pivot. |
|
@kiet this also blocks anyone trying to do Cmd+N or new workspace from firing off with Claude, since it will try to spin up and break. |
Summary
claude) being lost when the shell is still initializing (direnv, devenv, nvm, conda, oh-my-zsh, etc.)write()— buffering stdin while the shell is pending, flushing in FIFO order once readycreateOrAttach()returns immediately — terminal attaches and renders as soon as PTY spawns, no blockingProblem
When a preset launches a terminal and sends a command, the command gets lost because the shell hasn't finished initializing. The PTY
SpawnedIPC frame fires immediately afterpty.spawn()— before the shell has sourced RC files or run hook functions.This affects any project where shell initialization takes non-trivial time:
.envrc, evaluates Nix expressions, exports env vars.nvmrc, switches Node version, rehashes PATH.tool-versions, shims PATH entriesArchitecture
Approach: Session-owned stdin gating with OSC shell-ready marker
This follows the same pattern used by major terminal emulators for shell integration:
OSC 133sequences from precmd/PROMPT_COMMANDOSC 1337andOSC 133for shell integrationOSC 9001proprietary sequences from shell hooksWe use
OSC 777(private-use range, won't conflict with existing protocols).How it works
1. Shell wrappers (
shell-wrappers.ts) inject a one-shot marker hook at the end of shell init:precmdfunction in.zlogin— placed last inprecmd_functionsso it fires after direnv and all other hooksPROMPT_COMMANDfunction in rcfile — appended (not prepended) so it runs after existing hooks. Handles both scalar and arrayPROMPT_COMMAND(Bash 5.1+). Guards against emptyPROMPT_COMMAND.--on-event fish_prompthandler, self-removing viafunctions -e2. Session (
session.ts) owns shell readiness with a state machine:pending | ready | timed_out | unsupportedwrite()buffers user commands inpreReadyStdinQueuewhilepending, passes through otherwise\x1b) are dropped during init — they are stale DA/DSR responses from shell startup queries that would appear as typed text (e.g.?62;4;9;22c)resolveShellReady()transitions state, clears timeout, flushes queue in FIFO orderspawn(), not in terminal-hostunsupported— writes pass through immediately, no timeout3. TerminalHost (
terminal-host.ts) — unchanged:createOrAttach()returns immediately after PTY spawn — no blocking wait4. Renderer (
launch-command.ts) — no changes neededEdge cases handled
unsupported, no buffering, no timeouthandleSubprocessExit→resolveShellReady('timed_out')→ queue flusheddeclare -p, both forms handledFiles changed
apps/desktop/src/main/terminal-host/pty-subprocess-ipc.tsSHELL_READY_MARKERconstant with TSDocapps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/main/terminal-host/session.tsapps/desktop/src/main/terminal-host/session-shell-ready.test.tsTest coverage (19 tests)
Test plan
bun run typecheckpasses.envrc+ direnv — terminal renders immediately, preset command runs after promptPR History
Initial implementation
Blocked
createOrAttach()withwaitForShellReady()— terminal wouldn't render until shell was ready (up to 15s). Used a carry-buffer approach for marker detection.Review feedback from @Kitenite
Identified that blocking
createOrAttach()was the wrong boundary — the terminal should attach immediately, and only stdin delivery should wait. Recommended:Refactor to address feedback
Rewrote the implementation:
createOrAttach()— returns immediatelyShellReadyStateenum +preReadyStdinQueuewrite()gates on readiness;resolveShellReady()flushes queuespawn()where it belongsDA response fix
Discovered that headless emulator DA responses and renderer xterm protocol responses were reaching the shell during init, causing
?62;4;9;22cto appear as typed text. Fixed by:\x1b-prefixed) during pending state — they are stale responses to shell startup queriesTest suite
Added 19 focused unit tests covering all cases requested in review: write buffering, marker detection (including cross-frame splits), kill/exit before readiness, unsupported shell fallback, and DA response dropping.