Repository navigation
fix(host-service): stop gating preset launches on OSC 133;A marker - #4963
Conversation
queueInitialCommand was holding the preset command behind shellReadyPromise, so any user whose shell never emits OSC 133;A (stale wrapper install, exec'd shell, .zshrc blocking on stdin, broken precmd hook) waited the full 15 s SHELL_READY_TIMEOUT_MS before their preset started. Write the initial command immediately: the PTY's stdin buffer holds bytes until the shell reads them, so the command runs as soon as rc files finish. Also drop the timeout from 15 s to 3 s — with initialCommand no longer gated, the only remaining job of the timeout is flushing partial OSC 133;A prefix bytes the scanner holds back from output, which doesn't need 15 s. Adds a reproduction test that spawns /bin/bash without the Superset wrapper on disk (the most common stale-install symptom). Before the fix the test took 15 257 ms; after, 142 ms.
|
Ready to review this PR? Stage has broken it down into 3 individual chapters for you:
Chapters generated by Stage for commit 5df3c3e on May 28, 2026 2:16am UTC. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReduce OSC 133;A scanner timeout from 15s to 3s and write CreateTerminalSessionOptions.initialCommand immediately to the PTY (if the session is live). Add an adoption test that forces /bin/bash and verifies the initialCommand creates a sentinel file quickly (measured <5s; up to 10s allowed). ChangesShell-readiness timeout and immediate initialCommand execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Greptile SummaryThis PR removes the
Confidence Score: 4/5Safe to merge; the core write-path change is minimal and well-understood, with the main risk being cosmetic echo artifacts that were already present in v1. The production code change is a two-line simplification with no new state and no behavioral regression — bytes written before the shell's read loop are safely buffered by the kernel PTY layer, matching v1's documented approach. The new regression test validates the fix end-to-end but has a marginal flakiness window: it asserts elapsed < 3000 ms after a waitFor that itself has a 3000 ms timeout, so on a slow host the session-creation overhead alone could push elapsed past the threshold. The timing assertion in terminal.adoption.node-test.ts (lines 192–198) deserves a second look to decouple the waitFor timeout from the promptness threshold.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/terminal/terminal.ts | Removes the shellReadyPromise gate from queueInitialCommand and reduces SHELL_READY_TIMEOUT_MS from 15s to 3s; change is well-scoped and the remaining redundant exited guard is harmless. |
| packages/host-service/src/terminal/terminal.adoption.node-test.ts | New regression test for the promptness fix; the elapsed < 3000 assertion shares the same boundary as the waitFor timeout, creating a marginal flakiness window on slow CI. |
Sequence Diagram
sequenceDiagram
participant Renderer
participant HostService
participant PTY
participant Shell
Renderer->>HostService: createTerminalSession(initialCommand)
HostService->>PTY: spawn shell
HostService->>PTY: pty.write(cmd) immediately
PTY-->>Shell: bytes buffered in kernel stdin buffer
Shell->>Shell: source rc files
Shell->>PTY: enters read loop
PTY->>Shell: cmd delivered from buffer
Shell->>Shell: execute cmd
alt Wrapper present
Shell->>HostService: shell-ready marker in output
HostService->>HostService: resolveShellReady ready, clear timeout
else Timeout after 3s
HostService->>HostService: resolveShellReady timed_out
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/host-service/src/terminal/terminal.adoption.node-test.ts:192-198
**Timing assertion shares its boundary with `waitFor`'s timeout**
`elapsed` is measured after `await waitFor(..., 3000)`, which itself can block for up to 3000 ms. On a slow CI host where bash startup + `echo` takes, say, 2 800 ms, `createTerminalSessionInternal` might add another 200 ms before `waitFor` even starts polling — pushing `elapsed` past the 3 000 ms threshold and causing a spurious failure. Splitting the `waitFor` budget (e.g., 5 000 ms) from the timing threshold (e.g., 3 000 ms from the moment `waitFor` begins) would decouple the two checks and eliminate this flakiness window.
### Issue 2 of 2
packages/host-service/src/terminal/terminal.ts:802-804
**Redundant `exited` guard after removing the async gap**
The outer guard at line 792 (`if (session.initialCommandQueued || session.exited) return`) and the inner `if (!session.exited)` wrap were both needed in the old code because the `.then()` callback introduced an async interval during which `session.exited` could flip. Now that the write is synchronous there is no scheduling gap between the two checks, so the inner guard is unreachable in any new-failure scenario. It is harmless, but it may mislead future readers into thinking a race still exists here.
Reviews (1): Last reviewed commit: "fix(host-service): stop gating preset la..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await waitFor(() => fs.existsSync(sentinelFile), 3000); | ||
| const elapsed = Date.now() - start; | ||
| console.log(`[repro] initialCommand executed in ${elapsed}ms`); | ||
| assert.ok( | ||
| elapsed < 3000, | ||
| `expected initialCommand to run promptly, took ${elapsed}ms`, | ||
| ); |
There was a problem hiding this comment.
waitFor's timeout
elapsed is measured after await waitFor(..., 3000), which itself can block for up to 3000 ms. On a slow CI host where bash startup + echo takes, say, 2 800 ms, createTerminalSessionInternal might add another 200 ms before waitFor even starts polling — pushing elapsed past the 3 000 ms threshold and causing a spurious failure. Splitting the waitFor budget (e.g., 5 000 ms) from the timing threshold (e.g., 3 000 ms from the moment waitFor begins) would decouple the two checks and eliminate this flakiness window.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/terminal/terminal.adoption.node-test.ts
Line: 192-198
Comment:
**Timing assertion shares its boundary with `waitFor`'s timeout**
`elapsed` is measured after `await waitFor(..., 3000)`, which itself can block for up to 3000 ms. On a slow CI host where bash startup + `echo` takes, say, 2 800 ms, `createTerminalSessionInternal` might add another 200 ms before `waitFor` even starts polling — pushing `elapsed` past the 3 000 ms threshold and causing a spurious failure. Splitting the `waitFor` budget (e.g., 5 000 ms) from the timing threshold (e.g., 3 000 ms from the moment `waitFor` begins) would decouple the two checks and eliminate this flakiness window.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if (!session.exited) { | ||
| session.pty.write(cmd); | ||
| } |
There was a problem hiding this comment.
exited guard after removing the async gap
The outer guard at line 792 (if (session.initialCommandQueued || session.exited) return) and the inner if (!session.exited) wrap were both needed in the old code because the .then() callback introduced an async interval during which session.exited could flip. Now that the write is synchronous there is no scheduling gap between the two checks, so the inner guard is unreachable in any new-failure scenario. It is harmless, but it may mislead future readers into thinking a race still exists here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/terminal/terminal.ts
Line: 802-804
Comment:
**Redundant `exited` guard after removing the async gap**
The outer guard at line 792 (`if (session.initialCommandQueued || session.exited) return`) and the inner `if (!session.exited)` wrap were both needed in the old code because the `.then()` callback introduced an async interval during which `session.exited` could flip. Now that the write is synchronous there is no scheduling gap between the two checks, so the inner guard is unreachable in any new-failure scenario. It is harmless, but it may mislead future readers into thinking a race still exists here.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
- queueInitialCommand: drop redundant `if (!session.exited)` — outer guard at the top of the function already returns early when exited, and the write is now synchronous so there is no async gap to revisit. - adoption test: decouple the 3 s timing assertion from the waitFor budget it sat next to. waitFor now has 10 s of headroom; the timing assertion uses 5 s — still well under the pre-fix 15 s stall and gives CI room.
…e dropped forever (#5927) PR #5774 reintroduced marker-gating of initialCommand (regressing #4963) and removed the timeout fallback, so any shell whose OSC 133;A marker never reaches the scanner (exec'd shells, ZDOTDIR overrides) left preset and agent launches queued behind a promise that never resolves — an empty terminal with no command run. Keep #5774's first-prompt wait (real fix for direnv/devenv startup hooks consuming PTY input) but bound it at 15s, matching the v1 stack: on expiry the readiness promise resolves as timed_out and the queued command writes anyway. Also restore the flush of partially-matched marker bytes the scanner withholds, and clear the timer on every pending-state exit. Claude-Session: https://claude.ai/code/session_01KEmzD2DhH34tRA6GUuRgri
…ite (#6026) * fix(host-service): send initialCommand Enter as a separate delayed write Agent and preset launches intermittently ended up typed at the prompt but never executed: the workspace was created, the terminal opened, and the full launch command sat in the shell's edit buffer with no agent running. The OSC 133;A shell-ready marker fires from precmd, which runs before the prompt is drawn and before the line editor starts reading input. Writing `command\n` in that gap lets shell plugin init (vi-mode, syntax-highlighting — worst under first-create load) flush the PTY input queue mid-read: the command text survives into the editor's buffer while the trailing newline is lost. Reproduced live via CDP on a first workspace create after app boot; a single synthetic Enter keypress launched the stuck agent, confirming only the newline was missing. Strip trailing newlines from the command text and send Enter as its own write 500ms later — as `\r`, what a real Enter key sends, bound to accept-line in every keymap. Exactly one Enter is ever sent, so a double run is impossible, and if startup eats the text itself the stray `\r` is a no-op at an empty prompt — no failure mode is worse than before. Residual hole in the #4963 → #5774 → #5927 marker-gating series. Claude-Session: https://claude.ai/code/session_01CwWxQTQW2zLeN9hhRkQNxT * test(host-service): expect split command/Enter writes in setup terminal test The setup-scripts integration test asserted the old single `cmd\n` write; initialCommand now arrives as the command text plus a separate delayed `\r`. Assert both writes and their ordering. Claude-Session: https://claude.ai/code/session_01CwWxQTQW2zLeN9hhRkQNxT * style(host-service): format setup-scripts test assertion Claude-Session: https://claude.ai/code/session_01CwWxQTQW2zLeN9hhRkQNxT
Summary
queueInitialCommandno longer waits onshellReadyPromise— the preset command is written to the PTY immediately. Users whose shell never emits the OSC 133;A marker (stale wrapper, exec'd shell, blocking.zshrc, broken precmd hook) no longer hit a guaranteed 15 s stall on every preset send.SHELL_READY_TIMEOUT_MSdrops from 15 s to 3 s. WithinitialCommandno longer gated, the only remaining job of the timeout is flushing the scanner's held partial-marker bytes, which doesn't need 15 s.Why / Context
Users have been reporting that sending a preset in the v2 workspace takes ~15 s before anything happens. Traced to
queueInitialCommand(packages/host-service/src/terminal/terminal.ts), which deferredpty.write(cmd)behindshellReadyPromise.then(...). That promise resolves either when the OSC 133;A "prompt start" marker arrives or whenSHELL_READY_TIMEOUT_MS(15 s) fires.The marker comes from our zsh/bash wrapper rc files via a
precmdhook (apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts). Several real-world cases cause it to never fire:getShellBootstrapEnvonly setsZDOTDIRto~/.superset/zshif.zshrcexists there. IfsetupAgentHookshasn't run / failed silently, zsh sources the user's plain dotfiles and ourprecmdhook is never registered.exec tmux/exec screenin user.zshrc— our hook is installed in.zlogin, which runs after.zshrc. Anexecin.zshrcreplaces the process before.zloginruns..zshrcblocking on stdin —ssh-add,fnm/asdf/miseprompts,conda initlicense,gh auth. Shell sits at the keyboard;precmdnever fires.All three look identical from the renderer: send preset → 15 s of nothing → command finally appears.
How It Works
PTY line discipline buffers bytes written to stdin until the slave (the shell) reads them. Writing
claude\nimmediately at session create just puts it in that buffer; the shell consumes it once it finishes sourcing rc files and enters its read loop. v1's preset path achieves the same outcome by baking the command into shell argv (zsh -lc "<cmd>") and explicitly did not gate writes — the #3478 comment inapps/desktop/src/main/terminal-host/session.ts:870documents that gating was previously removed because it froze workspaces.The OSC 133;A scanner still strips the marker from output and holds partial-prefix bytes back across chunks (job preserved). Adoption still treats adopted sessions as
ready(the prompt has already happened in the previous host-service lifetime).Manual QA Checklist
Terminal Features
bun dev~/.superset/zsh/.zloginis missing or pre-fix-vintage — should now launch promptly instead of stalling.zshrc(oh-my-zsh + nvm + conda init) — preset still lands, no double-promptWorkspace
Testing
bun run lint— cleanbun run typecheck(host-service) — cleanpackages/host-service/src/terminal/terminal.adoption.node-test.ts→initialCommand runs promptly even when OSC 133;A never fires. Spawns/bin/bash(inSHELLS_WITH_READY_MARKER) with no Superset wrapper on disk so the marker never arrives. Before the change the suite took 15 257 ms (the full timeout); after, 142 ms.terminal.adoption.node-test.tssuite (13 tests) passes — including the adoption-ordering test that confirms an adopted session does not re-fireinitialCommand.Design Decisions
zsh -lc "<cmd>"so the shell exits when the preset exits. v2 keeps an interactive shell alive after the preset for the "Run" tab and similar flows; switching to argv-based would change that behavior. Dropping the gate matches v1's write policy ([bug] Workspace freezes on missing Node version prompt (no way to answer y/N) #3478) without changing v2's process-lifecycle behavior.initialCommandno longer dependent on it, the timeout only governs how long the scanner withholds a partial OSC 133;A prefix from output. 3 s is overkill for that and bounded enough that the worst case is invisible.Known Limitations
.zshrcconsumes stdin viaread(ssh-add passphrase, fnm install prompt, conda license, gpg passphrase), the preset command bytes can be consumed by that read instead of landing at the zle prompt. This was already true under the old behavior — users in this situation saw their preset command typed into the passphrase prompt anyway, just 15 s later. The principled long-term fix is to switch to argv-based preset launch (v1's pattern); not done here to keep the change minimal.Follow-ups
resolveShellReady(state,shellName, elapsed) so we can confirm the distribution of marker-fire vs timeout in production.createTerminalSessionInternal— check~/.superset/zsh/.zlogincontains__superset_prompt_markbefore each spawn; re-invokecreateZshWrapperif not.Summary by cubic
Presets now start immediately by writing the initial command to the PTY without waiting for the OSC 133;A marker, removing the 15s stall. The shell-ready timeout drops to 3s and only flushes partial marker bytes held by the output scanner.
Bug Fixes
queueInitialCommandonshellReadyPromiseso presets run even when wrappers are stale, shellsexec, or.zshrcblocks on stdin.SHELL_READY_TIMEOUT_MSfrom 15s to 3s; scanner still strips OSC 133;A and flushes partial prefixes.Refactors
queueInitialCommandby removing a redundant exit guard; relaxed the regression test timing (5s assert, 10s waitFor) to reduce CI flakiness.Written for commit 5df3c3e. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
Bug Fixes
Tests