Add shell input to the composer - #163
Merged
Merged
Conversation
Typing `!` as the first character in a thread's composer arms shell
input: border turns red, placeholder explains the mode, and submitting
runs the typed command as a worker-side subprocess instead of a chat
message. Output streams live to watching peers and persists as a new
`shell_execution` conversation entry (command, stdout/stderr-tagged
lines, exit code, start/complete times) rendered via the validated
"Variant A" design from the earlier prototype (accordion header with a
live status dot, inline stop control, and chevron; right-aligned to
read as user-authored).
Key design points carried over from the grilling session:
- No approval gate — the user typing and submitting the command is the
authorization (see docs/adr/0024-shell-input-has-no-approval-gate.md).
- Shell input runs concurrently with an agent turn in the same thread;
ChatChunkSchema's turnId is now optional with a new shellExecutionId
field, so a shell execution's chunks are never mistaken for an open
turn by code that gates/queues real chat turns (use-thread-turns.ts)
or builds synthetic Turn records (fold.ts).
- executeShellInput is fire-and-forget (returns { shellExecutionId }
immediately); output arrives async over the existing chat-chunk
broadcast, reusing the per-thread seq/sub stamping in queue/bus.ts.
- cancelShellExecution is a dedicated RPC with its own per-row stop
control, distinct from the composer's turn-scoped Stop button, since
a turn and a shell execution can be running at the same time.
- CYRUS_SHELL_INPUT_TIMEOUT_MS (default 60s) bounds how long a spawned
command may run before it's killed and reported as timed out.
Backspacing the composer back to empty exits shell input with no
dedicated key handler needed: shell input is armed reactively
(`plainText.startsWith("!")`), so deleting the `!` already flips it
off.
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds thread-scoped shell input and execution. It defines shell events and APIs, streams process output, stores shell executions separately from turns, adds cancellation and timeout handling, and renders terminal output in the chat feed. ChangesShell execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Composer
participant ThreadWorkspace
participant Controller
participant ShellProcess
participant ConversationFeed
User->>Composer: Enter !command
Composer->>ThreadWorkspace: executeShellInput(command)
ThreadWorkspace->>Controller: executeShellInput(threadId, command)
Controller->>ShellProcess: spawn sh -c
ShellProcess-->>Controller: stdout/stderr lines
Controller-->>ConversationFeed: publish shell start, line, and end events
ConversationFeed-->>User: render shell execution row
User->>ConversationFeed: cancel running execution
ConversationFeed->>Controller: cancelShellExecution(threadId, shellExecutionId)
Controller->>ShellProcess: terminate process
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
apps/web/src/components/chat/feed/shell-execution-row.tsx (1)
23-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
cancelledstatus color with the summary line.Line 23 treats every non-
exitedstatus as failed, so acancelledexecution shows a red dot.shellSummaryLinerenders the same status withtext-muted-foreground(lines 55-59). The two indicators disagree for a user-initiated stop.🎨 Proposed change
- const failed = execution.status !== "exited" || execution.exitCode !== 0; + if (execution.status === "cancelled") { + return <span className="size-2 shrink-0 rounded-full bg-muted-foreground" />; + } + const failed = execution.status !== "exited" || execution.exitCode !== 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/chat/feed/shell-execution-row.tsx` around lines 23 - 30, Update the failed-status logic in the shell execution row so cancelled executions use the same neutral styling as shellSummaryLine, while preserving red for actual failures and green for successful exits. Adjust the class selection around the failed indicator without changing unrelated execution statuses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/handlers/controller/shell.ts`:
- Around line 97-103: Update executeShellInput’s Bun.spawn launch path to use
the supported shell executable and argument conventions for the current worker
platform instead of always invoking "sh". Preserve cwd, stdout, and stderr
handling, reject unsupported worker OS values with the existing error result,
and add coverage for command launching and shell output on every supported
platform.
- Around line 118-153: Update the shell execution cancellation path around kill
and subprocess.exited to terminate the entire spawned process tree using the
supported strategy for each platform, rather than only calling
subprocess.kill(). Preserve pipe draining and await the tree’s exit before
finish persists shell_execution_end. Add an integration test using a cancellable
background child command that verifies both child termination and persistence of
the terminal event.
In `@apps/web/src/components/chat/feed/shell-execution-row.tsx`:
- Around line 96-103: Add disclosure semantics to both accordion toggle buttons
in the shell execution row, including the controls near the command and output
sections: set aria-expanded from the open state and aria-controls to the shared
outputId. Assign the matching outputId to the TerminalOutput wrapper so each
button references the expanded output container.
- Around line 108-119: Update the stop button handler in the running-state
branch of the shell execution row to await and inspect the Result returned by
cancelShellExecution. On failure, surface the contained error through the
component’s existing toast or notification mechanism while leaving the execution
state and button enabled so the user can retry.
In `@apps/web/src/components/chat/feed/terminal-output.tsx`:
- Around line 128-151: Keep the rendered tree stable in the wrappedChildren and
content paths by rendering ItemIndexContext.Provider around every child and
SequenceContext.Provider around the <pre> unconditionally. Pass the existing
null-resolving contextValue when sequence is disabled, and remove the
conditional branches that return unwrapped children or bare content.
In `@apps/web/src/hooks/chat/use-composer-editor.ts`:
- Around line 249-253: Move the shellInputArmed branch in the composer execution
flow before the hasAgents and hasAgentSelected guards. Ensure executeShellInput
can run with only threadId and command, without requiring an agent selection,
while preserving the existing command and onExecuteShell validation.
- Around line 207-214: Update restorePlainText to also persist the restored
command by calling setDraft with the same text content after restoring the
editor state. Preserve the existing originatingThreadId guard and local state
updates.
In `@shared/utils/src/conversations/fold.ts`:
- Around line 278-289: Update the shell execution start handling around the
state.shellExecutions.set call to merge start metadata into an existing record
instead of replacing it. Preserve existing lines, status, exitCode, and
completedAt when a fallback record from applyShellExecutionLine already exists,
while using the start event’s metadata for the execution identity and start
fields; add a test covering output arriving before shell_execution_start.
---
Nitpick comments:
In `@apps/web/src/components/chat/feed/shell-execution-row.tsx`:
- Around line 23-30: Update the failed-status logic in the shell execution row
so cancelled executions use the same neutral styling as shellSummaryLine, while
preserving red for actual failures and green for successful exits. Adjust the
class selection around the failed indicator without changing unrelated execution
statuses.
🪄 Autofix
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 Plus
Run ID: 2b96ab93-392f-417c-8b10-4ad3759550d0
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
CONTEXT.mdapps/cli/src/handlers/controller/index.tsapps/cli/src/handlers/controller/shell.test.tsapps/cli/src/handlers/controller/shell.tsapps/cli/src/lib/env.tsapps/cli/src/queue/bus.test.tsapps/cli/src/queue/bus.tsapps/web/package.jsonapps/web/src/components/chat/composer/index.tsxapps/web/src/components/chat/feed/feed-entry-view.tsxapps/web/src/components/chat/feed/shell-execution-row.tsxapps/web/src/components/chat/feed/terminal-output.tsxapps/web/src/components/chat/main/draft-workspace.tsxapps/web/src/components/chat/main/thread-workspace.tsxapps/web/src/hooks/chat/use-composer-editor.tsapps/web/src/types/composer.tsdocs/adr/0024-shell-input-has-no-approval-gate.mdshared/connections/src/contracts/controller.tsshared/hooks/src/conversation/conversation-cache.test.tsshared/hooks/src/conversation/conversation-cache.tsshared/hooks/src/conversation/use-shell-execution.tsshared/hooks/src/conversation/use-thread-conversation.tsshared/hooks/src/conversation/use-thread-turns.tsshared/schemas/src/rtc/chat.tsshared/schemas/src/view/index.tsshared/utils/src/conversations/fold.test.tsshared/utils/src/conversations/fold.tsshared/utils/src/conversations/thread-feed.test.tsshared/utils/src/conversations/thread-feed.ts
- New --terminal / --terminal-foreground CSS variables (#4af262): the composer border and primary send button now use terminal green while shell input is armed, instead of red (which read as an error state). - Typing `!` as the first character now arms shell input and consumes the `!` itself rather than leaving it visible — the box shows just the command. Since shell input is no longer derivable from plainText once armed, it's real state now, with Backspace-on-empty-composer as the explicit disarm trigger (new KEY_BACKSPACE_COMMAND handling in composer-prompt-editor.tsx). - ShellExecutionRow: dropped the "Process exited with code 0" summary line (redundant with the green status dot; nonzero-exit/timeout/ cancelled/spawn_error summaries still show, since the dot alone can't say *why*), the $ icon is terminal green, and the inline stop control is red with a brighter hover state. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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 (2)
apps/web/src/hooks/chat/use-composer-editor.ts (2)
218-231: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle rejected submission actions.
If
action()rejects,onFailureandsetSending(false)do not run. The composer stays disabled after its input and draft were cleared.Wrap the await in
try/catch/finally. Restore the input for both an errorResultand a rejected promise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/hooks/chat/use-composer-editor.ts` around lines 218 - 231, Update runSubmission so rejected action() promises are caught, invoking onFailure with originatingThreadId and restoring the cleared composer input; also handle error Results the same way. Use finally to always call setSending(false), including when action() rejects.
349-363: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisarm shell mode when the input becomes empty.
After the final character is deleted,
nextis empty butshellInputArmedremains true. Text typed afterward is submitted as a shell command instead of a chat message.Set
shellInputArmedtofalsewhen shell input transitions to empty. Re-arm it inrestorePlainTextwhen a shell submission fails.Proposed fix
const restorePlainText = useCallback( (text: string, originatingThreadId: string) => { if (originatingThreadId !== threadIdRef.current) return; + setShellInputArmed(true); editorRef.current?.setPlainText(text); setPlainText(text); setHasContent(true); }, [] ); function handlePlainTextChange(next: string) { + if (shellInputArmed && next === "") { + setShellInputArmed(false); + } + if (onExecuteShell && !shellInputArmed && next.startsWith("!")) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/hooks/chat/use-composer-editor.ts` around lines 349 - 363, Update handlePlainTextChange so an armed shell input is disarmed when next becomes empty, while preserving the existing “!” arming behavior. Also update restorePlainText to re-arm shell input after a failed shell submission, ensuring subsequent typing is handled as shell input only in that failure-recovery path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/components/chat/composer/primary-action.tsx`:
- Around line 65-70: Update the busy-path action control in the component
containing shellInputArmed so an armed shell composer retains shell-specific
styling and the appropriate shell action label while an agent turn is active.
Mirror the conditional styling currently used by the idle control and update the
busy-path accessible label/text, preserving the existing non-shell busy
behavior.
In `@apps/web/src/index.css`:
- Around line 237-239: Define a darker terminal-border token alongside
--terminal in the theme variables, and update the composer’s border-terminal
usage to reference it. Keep --terminal unchanged for the filled primary action
and retain --terminal-foreground for its text.
---
Outside diff comments:
In `@apps/web/src/hooks/chat/use-composer-editor.ts`:
- Around line 218-231: Update runSubmission so rejected action() promises are
caught, invoking onFailure with originatingThreadId and restoring the cleared
composer input; also handle error Results the same way. Use finally to always
call setSending(false), including when action() rejects.
- Around line 349-363: Update handlePlainTextChange so an armed shell input is
disarmed when next becomes empty, while preserving the existing “!” arming
behavior. Also update restorePlainText to re-arm shell input after a failed
shell submission, ensuring subsequent typing is handled as shell input only in
that failure-recovery path.
🪄 Autofix
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 Plus
Run ID: fd41dc3c-3dcf-4f5e-9f47-b6de6cd4db3c
📒 Files selected for processing (6)
apps/web/src/components/chat/composer/composer-prompt-editor.tsxapps/web/src/components/chat/composer/index.tsxapps/web/src/components/chat/composer/primary-action.tsxapps/web/src/components/chat/feed/shell-execution-row.tsxapps/web/src/hooks/chat/use-composer-editor.tsapps/web/src/index.css
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/components/chat/feed/shell-execution-row.tsx
- apps/web/src/components/chat/composer/index.tsx
Drop the redundant summary line since the status dot already conveys outcome; move exit-code detail into a tooltip instead. Hide the accordion/chevron when a command produced no output, toast timeout and spawn errors instead of rendering an extra row state, and drop the status dot entirely for cancelled commands since there's no outcome to convey.
Owner
Author
|
Re the two coderabbit findings on |
…, dark-mode contrast - Remove aria-expanded/aria-controls gap on the shell row accordion - Surface cancelShellExecution failures via toast instead of discarding the result - Keep TerminalOutput's provider tree stable across the running->done transition so completed output no longer flashes/re-animates - Persist the restored shell command to the draft store on a failed submission - Decouple shell submission from agent-selection gates (no agent required to run a shell command), including composer canSend and the busy-path "Add to queue" control - Disarm shell input whenever the composer empties by any means, not just single-character backspace; drop the now-dead KEY_BACKSPACE_COMMAND wiring it relied on - Merge a late shell_execution_start into an existing fallback record instead of clobbering accumulated output - Dim the terminal-green accent for dark mode only via a new --terminal-border token; light mode keeps the original color Also drop a few stray comments per direct feedback.
soorya-u
commented
Aug 9, 2026
Comment on lines
+23
to
+24
| const ItemIndexContext = createContext<number | null>(null); | ||
| const useItemIndex = () => useContext(ItemIndexContext); |
Owner
Author
There was a problem hiding this comment.
why do we need this? cant we use useId
Owner
Author
|
Re `terminal-output.tsx` — `ItemIndexContext`/`useItemIndex` (now in `components/ui/terminal-output.tsx`): can't swap this for `useId()`. `useId()` returns an opaque, unique string per component instance — it has no ordering. The sequencing logic here needs to know each `AnimatedSpan`'s ordinal position among its siblings (`sequence.activeIndex === itemIndex`) so it can animate them in order, one completing before the next starts. An opaque ID can't answer "am I the 3rd item" the way a numeric index can. Left as-is; happy to reconsider if there's a different concern I'm missing. |
…lacement - Add a shell error module in shared/errors (ShellSpawnFailedError, ShellStreamFailedError) instead of ad-hoc toError, mirroring the turn/connection modules - Refine use-shell-execution's error conversion to extract a real message from unknown causes instead of blindly stringifying - Move shell domain logic (pumpLines, runShellExecution, active-execution tracking) out of the oRPC handler into apps/cli/src/shell/, mirroring the git.ts/git/ split; the handler is now a thin oRPC wrapper - for(;;) -> while(true) in pumpLines - Move useShellFailureToast out of the row component into hooks/chat - Move terminal-output.tsx into components/ui (it's a trimmed magicui Terminal, not feed-specific) - Simplify fold.ts's applyEvent to check shellExecutionId once instead of in each of the three shell-event branches
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/src/components/chat/composer/primary-action.tsx (1)
76-81:⚠️ Potential issue | 🟡 MinorKeep the idle shell label consistent.
When
shellInputArmedis true, the terminal styling is active, but the button still exposes"Send message"at Line 75. Use"Run command"and"Running command"whilesendingis true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/chat/composer/primary-action.tsx` around lines 76 - 81, Update the button label logic near the primary action’s `shellInputArmed` styling so shell mode uses “Run command” when idle and “Running command” when `sending` is true, while preserving the existing “Send message” labels for normal chat mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/shell/run.ts`:
- Around line 136-160: Update the streamed.isErr() handling around pumpLines so
a stream failure terminates the subprocess and awaits subprocess.exited before
cleanup. Keep timeoutHandle active and retain the shellExecutionId in
activeShellExecutions until the child has exited, then clear and remove them
before calling finish. Add coverage that makes one output stream fail while the
command remains running.
- Around line 64-73: Update the shell output handling around emitLine and the
related lines 80–101 flow to enforce a bounded byte or line limit before
appending to lines or publishing through context.eventBus. Once the limit is
reached, stop buffering and emitting further output, and record a terminal
truncation/output-limit state so callers can distinguish capped output from
normal completion.
In `@apps/web/src/hooks/chat/use-shell-failure-toast.ts`:
- Around line 13-18: Update the status handling in useShellFailureToast to show
a failure toast when execution.status is "exited" and execution.exitCode is
non-zero, matching ShellStatusDot’s failure condition. Preserve the existing
timeout and spawn_error notifications, and include the command in the new toast
message.
---
Duplicate comments:
In `@apps/web/src/components/chat/composer/primary-action.tsx`:
- Around line 76-81: Update the button label logic near the primary action’s
`shellInputArmed` styling so shell mode uses “Run command” when idle and
“Running command” when `sending` is true, while preserving the existing “Send
message” labels for normal chat mode.
🪄 Autofix
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 Plus
Run ID: 089826b3-dbca-4c39-93db-1feba94163e5
📒 Files selected for processing (16)
apps/cli/src/handlers/controller/shell.tsapps/cli/src/shell/run.test.tsapps/cli/src/shell/run.tsapps/web/src/components/chat/composer/index.tsxapps/web/src/components/chat/composer/primary-action.tsxapps/web/src/components/chat/feed/shell-execution-row.tsxapps/web/src/components/ui/terminal-output.tsxapps/web/src/hooks/chat/use-composer-editor.tsapps/web/src/hooks/chat/use-shell-failure-toast.tsapps/web/src/index.cssshared/errors/src/common.tsshared/errors/src/shell.tsshared/hooks/src/conversation/use-shell-execution.tsshared/utils/src/conversations/fold.test.tsshared/utils/src/conversations/fold.tsshared/utils/src/conversations/thread-feed.ts
💤 Files with no reviewable changes (1)
- shared/utils/src/conversations/thread-feed.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/src/index.css
- shared/utils/src/conversations/fold.test.ts
- apps/web/src/components/chat/composer/index.tsx
- shared/utils/src/conversations/fold.ts
- apps/web/src/hooks/chat/use-composer-editor.ts
Comment on lines
+64
to
+73
| const lines: ShellExecutionLine[] = []; | ||
|
|
||
| function emitLine(line: ShellExecutionLine): void { | ||
| lines.push(line); | ||
| context.eventBus.publish({ | ||
| threadId, | ||
| shellExecutionId, | ||
| seq: 0, | ||
| event: { type: "shell_execution_line", lines: [line] }, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound shell output before buffering and persistence.
lines has no byte or line limit. A command such as yes can exhaust worker memory and flood the event bus well before the 60-second timeout. Define an output limit, expose a truncation or output-limit terminal state, and stop buffering after the limit.
Also applies to: 80-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/cli/src/shell/run.ts` around lines 64 - 73, Update the shell output
handling around emitLine and the related lines 80–101 flow to enforce a bounded
byte or line limit before appending to lines or publishing through
context.eventBus. Once the limit is reached, stop buffering and emitting further
output, and record a terminal truncation/output-limit state so callers can
distinguish capped output from normal completion.
Comment on lines
+136
to
+160
| const streamed = await Result.tryPromise({ | ||
| try: async () => { | ||
| await Promise.all([ | ||
| pumpLines(subprocess.stdout, "stdout", emitLine), | ||
| pumpLines(subprocess.stderr, "stderr", emitLine), | ||
| ]); | ||
| return await subprocess.exited; | ||
| }, | ||
| catch: (cause) => shellStreamFailed(shellErrorMessageFromUnknown(cause)), | ||
| }); | ||
|
|
||
| clearTimeout(timeoutHandle); | ||
| activeShellExecutions.delete(shellExecutionId); | ||
|
|
||
| if (streamed.isErr()) { | ||
| log.error({ | ||
| kind: "shell_execution_stream_failed", | ||
| error: streamed.error, | ||
| shellExecutionId, | ||
| threadId, | ||
| }); | ||
| await finish(killedFor ?? "exited", null); | ||
| return; | ||
| } | ||
| await finish(killedFor ?? "exited", killedFor ? null : streamed.value); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep process cleanup active after a stream failure.
If either pumpLines call fails, Promise.all rejects before subprocess.exited is awaited. The code then clears timeoutHandle and removes the active execution entry. The child process can continue without timeout enforcement or cancellation support.
On this error path, terminate the subprocess, wait for its exit, and only then clear the timeout and remove the active entry. Add a test that forces one output stream to fail while the command remains running.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/cli/src/shell/run.ts` around lines 136 - 160, Update the
streamed.isErr() handling around pumpLines so a stream failure terminates the
subprocess and awaits subprocess.exited before cleanup. Keep timeoutHandle
active and retain the shellExecutionId in activeShellExecutions until the child
has exited, then clear and remove them before calling finish. Add coverage that
makes one output stream fail while the command remains running.
Comment on lines
+13
to
+18
| if (execution.status === "timeout") { | ||
| toast.error(`Command timed out: ${execution.command}`); | ||
| } else if (execution.status === "spawn_error") { | ||
| toast.error(`Failed to start command: ${execution.command}`); | ||
| } | ||
| }, [execution.status, execution.command]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Notify when an exited command fails.
ShellStatusDot marks an exited execution with a non-zero exit code as failed. This hook only notifies for timeouts and spawn errors. Add a failure toast for non-zero exit codes.
Proposed fix
if (execution.status === "timeout") {
toast.error(`Command timed out: ${execution.command}`);
} else if (execution.status === "spawn_error") {
toast.error(`Failed to start command: ${execution.command}`);
+ } else if (execution.status === "exited" && execution.exitCode !== 0) {
+ toast.error(
+ `Command exited with code ${execution.exitCode}: ${execution.command}`
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (execution.status === "timeout") { | |
| toast.error(`Command timed out: ${execution.command}`); | |
| } else if (execution.status === "spawn_error") { | |
| toast.error(`Failed to start command: ${execution.command}`); | |
| } | |
| }, [execution.status, execution.command]); | |
| if (execution.status === "timeout") { | |
| toast.error(`Command timed out: ${execution.command}`); | |
| } else if (execution.status === "spawn_error") { | |
| toast.error(`Failed to start command: ${execution.command}`); | |
| } else if (execution.status === "exited" && execution.exitCode !== 0) { | |
| toast.error( | |
| `Command exited with code ${execution.exitCode}: ${execution.command}` | |
| ); | |
| } | |
| }, [execution.status, execution.command]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/hooks/chat/use-shell-failure-toast.ts` around lines 13 - 18,
Update the status handling in useShellFailureToast to show a failure toast when
execution.status is "exited" and execution.exitCode is non-zero, matching
ShellStatusDot’s failure condition. Preserve the existing timeout and
spawn_error notifications, and include the command in the new toast message.
This branch was successfully deployed
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
!as the first character in a thread's composer arms shell input: border turns red, placeholder explains the mode, and submitting runs the command as a worker-side subprocess (Bun.spawn) instead of a chat message.shell_executionconversation entry (command, stdout/stderr-tagged lines, exit code, start/complete times), rendered as an accordion row with a live status dot, inline stop control, and chevron.docs/adr/0024-shell-input-has-no-approval-gate.md).ChatChunkSchema.turnIdis now optional with a newshellExecutionIdfield, so a shell execution's chunks are never mistaken for an open turn by code that gates/queues chat turns or builds syntheticTurnrecords.executeShellInputis fire-and-forget ({ shellExecutionId }returned immediately); output arrives async over the existing chat-chunk broadcast, reusing the per-thread seq/sub stamping already inqueue/bus.ts.cancelShellExecutionis a dedicated RPC with its own per-row stop control, distinct from the composer's turn-scoped Stop button, since a turn and a shell execution can run at the same time.CYRUS_SHELL_INPUT_TIMEOUT_MS(default 60s) bounds how long a spawned command may run before it's killed and reported as timed out.plainText.startsWith("!")), so deleting the!already flips it off.Closes #161.
Test plan
bun run check:types— clean across all 17 packagesbun run test:unit— 154 vitest + 130 bun:test, 0 failuresfold.ts(shell execution folding incl. the "no fake turn" invariant),thread-feed.ts(feed ordering),bus.ts(live fan-out + no replay-buffer leak),conversation-cache.ts(line merging/pruning),shell.ts'spumpLines(UTF-8 boundary handling)/code-reviewrun againsted64428...HEAD— spec axis clean, standards-axis findings (naming collision, misplaced shared component, complexity, try/catch vsResult) all fixed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
!-prefixed commands.Documentation