feat(desktop): v2 workspace setup script execution#3359
Conversation
Add initialCommand support to createTerminalSessionInternal and ensureSession tRPC. Commands are queued behind shellReadyPromise (OSC 133;A detection from Phase 1) and written to the PTY only after the shell is ready. Update v2 preset execution to create sessions with commands on the host-service before adding panes. Remove initialCommand from TerminalPaneData and delete the renderer-side WebSocket delivery effect — command delivery is now entirely host-service-side.
During v2 workspace creation, if .superset/setup.sh exists and
runSetupScript is enabled, the host-service now creates a terminal
session with the setup command via createTerminalSessionInternal.
The command runs after shell readiness (OSC 133;A), and output
buffers until the renderer connects.
Returns terminals array [{id, role, label}] instead of initialCommands.
The pending page pre-populates the workspace pane layout with terminal
panes referencing the host-provided terminalIds before navigating, so
TerminalPane mounts and attaches to already-running sessions.
|
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:
📝 WalkthroughWalkthroughReplaced OSC 777 readiness signaling with OSC 133 semantic prompt markers across shells; added a shared OSC 133 scanner and per-session readiness gating; routed initialCommand through host-side session creation; persisted terminal descriptors and pre-populated pane layouts in the renderer; updated tests and docs. Changes
Sequence Diagram(s)mermaid Frontend->>Router: create workspace (runSetupScript: true) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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 |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 16 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/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.ts:67">
P2: `executePreset` now returns a rejecting Promise, but callers invoke it as `void` without error handling. Wrap the async flow in an internal `try/catch` (or ensure callers await/catch) to avoid unhandled promise rejections.
(Based on your team's feedback about handling async errors explicitly.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts:845">
P3: This `not.toContain("777")` check is flaky because the temp path embedded in the init-command can legitimately include `777`.</violation>
</file>
<file name="packages/host-service/src/terminal/terminal.ts">
<violation number="1" location="packages/host-service/src/terminal/terminal.ts:175">
P2: On prefix mismatch, the current character is flushed to output without re-testing it against position 0 of `OSC_133_A`. If `ch` is `\x1b` (which is `OSC_133_A[0]`), a valid marker that immediately follows a stale ESC byte will be missed. After resetting, check whether `ch` starts a new match and hold it instead of flushing.</violation>
</file>
<file name="packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts">
<violation number="1" location="packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts:484">
P2: Handle the `{ error }` result from `createTerminalSessionInternal` instead of silently skipping setup terminal creation; otherwise setup-script failures are invisible and hard to diagnose.
(Based on your team's feedback about handling async/error failures explicitly and avoiding silent failure paths.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryThis PR delivers three major improvements for the desktop terminal experience:
The architecture is sound: sessions are created host-side and the renderer simply attaches, eliminating the renderer-side Key observations:
Confidence Score: 4/5Safe to merge; core shell readiness detection and preset race-condition fix are correct, with only minor robustness and dead-code cleanup items remaining. The primary features (OSC 133 shell readiness detection, initialCommand queueing, v2 setup script execution) all work correctly. The shellReadyState machine is idempotent, the byte scanner handles cross-chunk partial matches properly, and the TerminalPane attach-to-existing-session flow is sound. The two flagged items are P2: the bash DEBUG trap produces spurious 133;C/D markers in subsequent prompts (harmless for current xterm.js integration since it doesn't render OSC 133 zones) and there's a missing .catch() on the shellReadyPromise callback. The initialCommands dead-code field is harmless. No blocking issues. apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts (bash DEBUG trap), packages/host-service/src/terminal/terminal.ts (missing .catch) Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer
participant HS as HostService
participant PTY as PTY
R->>HS: workspaceCreation.create
HS->>PTY: spawn shell
HS->>HS: queue initialCommand behind shellReadyPromise
HS-->>R: workspace plus terminals array
PTY->>HS: onData shell ready marker detected
HS->>HS: resolveShellReady promise settles
HS->>PTY: pty.write initialCommand
R->>R: buildSetupPaneLayout from terminals
R->>HS: ensureSession returns existing session
R->>HS: WebSocket attaches and replays buffer
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.ts (1)
66-169:⚠️ Potential issue | 🔴 CriticalType mismatch:
executePresetis now async but callers expect sync.The implementation at line 67 declares
executePresetasasync (preset: V2TerminalPresetRow) => { ... }, but consumers (useWorkspaceHotkeys,V2PresetsBar,V2PresetBarItem) all declare it with type(preset: V2TerminalPresetRow) => void. Call sites invoke it without awaiting:
useWorkspaceHotkeys.ts:250—if (preset) executePreset(preset);V2PresetBarItem.tsx:93, 112—onClick={() => onExecutePreset(preset)}and context menu selectionThis silently discards the returned promise, causing:
- Unhandled rejections — if
ensureSession.mutateAsync()fails, the error is never caught- Race conditions — rapid hotkey presses or button clicks could interleave
addTab/addPanemutations before prior awaits completeUpdate consumer type signatures to
(preset: V2TerminalPresetRow) => Promise<void>and handle errors. For fire-and-forget hotkey invocations, add explicit.catch()or void operators.
🧹 Nitpick comments (2)
packages/host-service/src/terminal/env.test.ts (1)
237-246: Assert the full fish marker set here too.The production init command now includes
133;A,133;C, and133;D, but this test only guards the ready marker. Adding the other two here keepspackages/host-service/src/terminal/shell-launch.tsaligned withapps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts.Suggested assertion update
expect(args[0]).toBe("-l"); expect(args[1]).toBe("--init-command"); expect(args[2]).toContain("_superset_bin"); expect(args[2]).toContain("133;A"); + expect(args[2]).toContain("133;C"); + expect(args[2]).toContain("133;D"); + expect(args[2]).not.toContain("777");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/terminal/env.test.ts` around lines 237 - 246, Update the fish unit test to assert the full init-command marker trio instead of only the ready marker: inside the "fish uses init-command" test that calls getShellLaunchArgs, keep the existing checks for args[0] and args[1] but change the args[2] assertions to verify it contains all three markers "133;A", "133;C", and "133;D" (matching the init command built in shell-launch.ts) so the test aligns with apps/desktop's agent-setup/shell-wrappers.test.ts.packages/host-service/src/terminal/terminal.ts (1)
162-194: Consider handling ST terminator for broader OSC 133 compatibility.The scanner only recognizes BEL (
\x07) as the OSC string terminator. Per the FinalTerm spec, ST (\x1b\\) is also valid. While the shell wrappers likely emit BEL consistently, terminals configured differently would timeout and degrade gracefully—acceptable fallback behavior.If broader compatibility becomes needed, you could extend the scanner to also match
\x1b\\after the133;Aprefix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/terminal/terminal.ts` around lines 162 - 194, The scanner scanForShellReady currently only treats BEL ('\x07') as the OSC terminator; update it to also recognize the ST terminator sequence ESC '\\' (0x1b followed by '\\') after matching the OSC_133_A prefix: when session.oscMatchPos >= OSC_133_A.length and you encounter '\x1b', peek the next character in data (i+1) and if it is '\\' consume both chars, clear session.oscHeldBytes, reset session.oscMatchPos, call resolveShellReady(session,"ready"), append the remainder data.slice(i+2) to output and break; if the peek fails (end of buffer or not '\\') continue accumulating into session.oscHeldBytes as before. Ensure state resets mirror the existing BEL-path behavior.
🤖 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 289-318: Wrap existing PROMPT_COMMAND and any existing DEBUG trap
instead of appending/replacing: save the current PROMPT_COMMAND (array or
scalar) and replace it with a wrapper that calls __superset_semantic_precmd
first (so it captures the original command exit code in ret) and then invokes
the saved/original PROMPT_COMMAND handlers; likewise, capture the current DEBUG
trap (via trap -p DEBUG or reading $BASH_COMMAND) and install a new DEBUG trap
that calls __superset_bash_preexec and then calls the original DEBUG
trap/handler so you don't clobber user tooling. Ensure you reference and
preserve __superset_semantic_precmd, PROMPT_COMMAND, __superset_bash_preexec and
the DEBUG trap when composing the wrappers.
- Around line 225-240: The precmd/preexec hooks currently capture the exit
status too late and directly reassign shell hook arrays unsafely; modify
__superset_semantic_preexec to capture the user's command exit status into a
dedicated variable (e.g., __superset_semantic_last_ret) before other precmd
hooks run and change __superset_semantic_precmd to emit the OSC 133;D;<ret>
using that captured variable separate from the final A emission, and wrap the
array modifications for precmd_functions and preexec_functions in the same
defensive pattern used in buildZshPrecmdHook (append with redirection
suppression like 2>/dev/null || true) so readonly hook arrays do not break
startup while preserving existing hook names
(__superset_semantic_precmd_executing, __superset_semantic_precmd,
__superset_semantic_preexec, precmd_functions, preexec_functions).
---
Nitpick comments:
In `@packages/host-service/src/terminal/env.test.ts`:
- Around line 237-246: Update the fish unit test to assert the full init-command
marker trio instead of only the ready marker: inside the "fish uses
init-command" test that calls getShellLaunchArgs, keep the existing checks for
args[0] and args[1] but change the args[2] assertions to verify it contains all
three markers "133;A", "133;C", and "133;D" (matching the init command built in
shell-launch.ts) so the test aligns with apps/desktop's
agent-setup/shell-wrappers.test.ts.
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 162-194: The scanner scanForShellReady currently only treats BEL
('\x07') as the OSC terminator; update it to also recognize the ST terminator
sequence ESC '\\' (0x1b followed by '\\') after matching the OSC_133_A prefix:
when session.oscMatchPos >= OSC_133_A.length and you encounter '\x1b', peek the
next character in data (i+1) and if it is '\\' consume both chars, clear
session.oscHeldBytes, reset session.oscMatchPos, call
resolveShellReady(session,"ready"), append the remainder data.slice(i+2) to
output and break; if the peek fails (end of buffer or not '\\') continue
accumulating into session.oscHeldBytes as before. Ensure state resets mirror the
existing BEL-path behavior.
🪄 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: a08e55c2-e198-4f79-88dd-70a8c535abfe
📒 Files selected for processing (16)
apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.tsapps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildSetupPaneLayout.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useSubmitWorkspace/useSubmitWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.tsdocs/V2_WORKSPACE_SETUP_SCRIPTS.mdpackages/host-service/src/terminal/env.test.tspackages/host-service/src/terminal/shell-launch.tspackages/host-service/src/terminal/terminal.tspackages/host-service/src/trpc/router/terminal/terminal.tspackages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.ts
Extract the shell readiness scanner (character-by-character OSC 133;A matching) into packages/shared/src/shell-ready-scanner.ts. Both v1 desktop (session.ts) and v2 host-service (terminal.ts) now import from the shared module. V1 switches from the custom OSC 777 marker to the standard OSC 133 protocol, matching the shell wrappers updated in the first commit. All 19 v1 shell-ready tests pass with the new marker.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/terminal-host/session-shell-ready.test.ts (1)
10-11: Add one OSC 133-with-params case.
packages/shared/src/shell-ready-scanner.tsalso accepts optional params before the BEL terminator, but every test here still uses the bare\x1b]133;A\x07form. A single;cl=...case would cover that shared-parser path too.➕ Suggested test case
+ it("handles OSC 133 markers with params", () => { + const { session, proc } = createTestSession("/bin/zsh"); + spawnAndReady(session, proc); + + sendData(proc, "\x1b]133;A;cl=m;aid=123\x07prompt$ "); + + session.write("test\n"); + expect(getWrittenData(proc)).toEqual(["test\n"]); + });🤖 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 10 - 11, Add a new test in session-shell-ready.test.ts that covers the OSC 133 variant with params (e.g. "\x1b]133;A;cl=some-value\x07") so the shared parser in packages/shared/src/shell-ready-scanner.ts that accepts optional params before the BEL is exercised; create a new constant or inline marker that appends a ";cl=..." param to SHELL_READY_MARKER and add an assertion mirroring the existing bare-marker test (use the same test helper/expectations as the existing SHELL_READY_MARKER case).
🤖 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 10-11: Add a new test in session-shell-ready.test.ts that covers
the OSC 133 variant with params (e.g. "\x1b]133;A;cl=some-value\x07") so the
shared parser in packages/shared/src/shell-ready-scanner.ts that accepts
optional params before the BEL is exercised; create a new constant or inline
marker that appends a ";cl=..." param to SHELL_READY_MARKER and add an assertion
mirroring the existing bare-marker test (use the same test helper/expectations
as the existing SHELL_READY_MARKER case).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56a61988-be28-40c1-941e-475ebb1efd57
📒 Files selected for processing (5)
apps/desktop/src/main/terminal-host/session-shell-ready.test.tsapps/desktop/src/main/terminal-host/session.tspackages/host-service/src/terminal/terminal.tspackages/shared/package.jsonpackages/shared/src/shell-ready-scanner.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/host-service/src/terminal/terminal.ts
We only consume 133;A for shell readiness detection. Emitting 133;C and 133;D via DEBUG trap (bash) and preexec hooks (zsh/fish) brought complexity we don't need — the bash DEBUG trap was also fundamentally buggy, firing within PROMPT_COMMAND and producing spurious 133;C emissions inside precmd. Simplify to a single precmd hook per shell that emits 133;A. Matches kitty's bash approach. If we ever need command-finished tracking, we can add bash-preexec or PS0 (Bash 4.4+) at that point.
…atch - Setup terminal creation failures now surface via the `warnings` array instead of being silently dropped. - Scanner re-tests the current character after a mismatch — a valid OSC 133;A marker that immediately follows a stale ESC byte is no longer lost.
executePreset is async and can reject if ensureSession fails (network, host down). Callers invoke as fire-and-forget — without a try/catch, errors become unhandled promise rejections and the user sees the preset silently not open. Wrap the body in try/catch with a toast.
* Shell readiness
* feat(host-service): host-side initial command delivery via ensureSession
Add initialCommand support to createTerminalSessionInternal and
ensureSession tRPC. Commands are queued behind shellReadyPromise
(OSC 133;A detection from Phase 1) and written to the PTY only after
the shell is ready.
Update v2 preset execution to create sessions with commands on the
host-service before adding panes. Remove initialCommand from
TerminalPaneData and delete the renderer-side WebSocket delivery
effect — command delivery is now entirely host-service-side.
* feat(desktop): create setup terminal during workspace creation
During v2 workspace creation, if .superset/setup.sh exists and
runSetupScript is enabled, the host-service now creates a terminal
session with the setup command via createTerminalSessionInternal.
The command runs after shell readiness (OSC 133;A), and output
buffers until the renderer connects.
Returns terminals array [{id, role, label}] instead of initialCommands.
The pending page pre-populates the workspace pane layout with terminal
panes referencing the host-provided terminalIds before navigating, so
TerminalPane mounts and attaches to already-running sessions.
* refactor: extract OSC 133 scanner into @superset/shared, update v1
Extract the shell readiness scanner (character-by-character OSC 133;A
matching) into packages/shared/src/shell-ready-scanner.ts. Both v1
desktop (session.ts) and v2 host-service (terminal.ts) now import
from the shared module.
V1 switches from the custom OSC 777 marker to the standard OSC 133
protocol, matching the shell wrappers updated in the first commit.
All 19 v1 shell-ready tests pass with the new marker.
* Lint
* chore: remove dead SHELL_READY_MARKER constant, clean up stale comment
* Lint
* chore: remove dead initialCommands field from pendingWorkspaceSchema
* refactor(shell-wrappers): emit only OSC 133;A, drop C/D
We only consume 133;A for shell readiness detection. Emitting 133;C
and 133;D via DEBUG trap (bash) and preexec hooks (zsh/fish) brought
complexity we don't need — the bash DEBUG trap was also fundamentally
buggy, firing within PROMPT_COMMAND and producing spurious 133;C
emissions inside precmd.
Simplify to a single precmd hook per shell that emits 133;A. Matches
kitty's bash approach. If we ever need command-finished tracking, we
can add bash-preexec or PS0 (Bash 4.4+) at that point.
* fix: surface setup terminal errors and re-test marker on scanner mismatch
- Setup terminal creation failures now surface via the `warnings` array
instead of being silently dropped.
- Scanner re-tests the current character after a mismatch — a valid
OSC 133;A marker that immediately follows a stale ESC byte is no
longer lost.
* Lint
* fix(preset): show toast on executePreset failure
executePreset is async and can reject if ensureSession fails (network,
host down). Callers invoke as fire-and-forget — without a try/catch,
errors become unhandled promise rejections and the user sees the
preset silently not open. Wrap the body in try/catch with a toast.
1 participant
Summary
133;A(prompt ready),133;C(command start),133;D(command finished + exit code) instead of our custom OSC 777 marker.initialCommandsupport tocreateTerminalSessionInternalandensureSessiontRPC — commands are queued behindshellReadyPromiseand written to the PTY only after the shell is ready. Fixes the preset race condition where commands fired before shell init completed.await ensureSession({ initialCommand })before adding panes.initialCommandremoved fromTerminalPaneDataand the renderer-side WebSocket delivery effect deleted..superset/setup.shexists, the host-service creates a terminal session with the setup command. Returnsterminals: [{ id, role, label }]. The pending page pre-populates the workspace pane layout before navigating, soTerminalPaneattaches to already-running sessions with buffered output.Test plan
.superset/setup.sh+runSetupScriptenabled — workspace opens with "Workspace Setup" terminal showing setup outputbun test packages/host-service/src/terminal/env.test.tspassesbun test apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.tspassesbun run typecheckpassesSummary by cubic
Runs v2 workspace setup scripts and preset commands by creating host-side terminal sessions that queue initial commands until the shell is ready (OSC 133;A). A “Workspace Setup” terminal is pre-started, panes attach to buffered output, and setup errors return as warnings.
New Features
initialCommandincreateTerminalSessionInternalandterminal.ensureSession; commands wait for shell readiness with a 15s timeout fallback..superset/setup.shexists andrunSetupScriptis enabled, a setup terminal starts and the API returnsterminals: [{ id, role, label }]; the renderer pre-builds the pane layout so panes attach to running sessions.Refactors
@superset/sharedand switched v1 desktop and host-service to it.initialCommandfromTerminalPaneDataand the renderer-side delivery effect; presets now callawait terminal.ensureSession({ initialCommand })before adding panes.initialCommandstoterminalswithbuildSetupPaneLayoutand docs added; preset execution now shows a toast if session creation fails.Written for commit 3909cf8. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation