fix(terminal): tell model when command was aborted by the user#880
fix(terminal): tell model when command was aborted by the user#880umi008 wants to merge 8 commits into
Conversation
When the user clicks Abort on a running shell command, the terminal previously emitted shell_execution_complete with exitCode: 0 and no indication of user intervention. The model would see a successful exit code or SIGKILL and suspect the Linux OOM-killer, leading to incorrect diagnosis and wrong follow-up actions. Now the ExitCodeDetails interface carries an aborted: true flag when the user explicitly stopped the process. formatExitStatus() checks this first and returns 'Command was aborted by the user.' so the model immediately knows what happened and doesn't speculate about OOM-killer. Closes Zoo-Code-Org#833
|
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:
📝 WalkthroughWalkthroughCommand abort state is added to terminal completion data, documented in exit details, tested across successful and error paths, and rendered as a dedicated “aborted by the user” message. ChangesCommand abort status
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ExecaTerminalProcess
participant shell_execution_complete
participant ExecuteCommandTool
ExecaTerminalProcess->>shell_execution_complete: Emit exitCode, signalName, and aborted
shell_execution_complete->>ExecuteCommandTool: Provide ExitCodeDetails
ExecuteCommandTool->>ExecuteCommandTool: Format aborted status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/integrations/terminal/ExecaTerminalProcess.ts (1)
134-144: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftError-path emissions are missing the
abortedflag, causing a race condition that undermines the PR's objective.When
abort()sendsSIGKILL, the subprocess stream can throw anExecaError(withsignal: "SIGKILL") before thefor awaitloop at line 89 checksthis.aborted. Execution then falls into the catch block, which emitsshell_execution_completewithoutaborted: this.aborted. Downstream,formatExitStatuswould then return"Process terminated by signal SIGKILL"— the exact ambiguous message this PR aims to eliminate.Both error paths (lines 137 and 143) must propagate
aborted: this.abortedso the abort flag survives regardless of which path executes.🐛 Proposed fix: include `aborted` in error-path emissions
} catch (error) { if (error instanceof ExecaError) { console.error(`[ExecaTerminalProcess#run] shell execution error: ${error.message}`) - this.emit("shell_execution_complete", { exitCode: error.exitCode ?? 0, signalName: error.signal }) + this.emit("shell_execution_complete", { exitCode: error.exitCode ?? 0, signalName: error.signal, aborted: this.aborted }) } else { console.error( `[ExecaTerminalProcess#run] shell execution error: ${error instanceof Error ? error.message : String(error)}`, ) - this.emit("shell_execution_complete", { exitCode: 1 }) + this.emit("shell_execution_complete", { exitCode: 1, aborted: this.aborted }) }🤖 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 `@src/integrations/terminal/ExecaTerminalProcess.ts` around lines 134 - 144, Include aborted: this.aborted in both shell_execution_complete payloads within ExecaTerminalProcess#run’s catch block, covering both ExecaError and generic error paths so abort state is propagated consistently.
🤖 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.
Outside diff comments:
In `@src/integrations/terminal/ExecaTerminalProcess.ts`:
- Around line 134-144: Include aborted: this.aborted in both
shell_execution_complete payloads within ExecaTerminalProcess#run’s catch block,
covering both ExecaError and generic error paths so abort state is propagated
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e181b82-ca9e-4335-bf62-b2f42ca6ec27
📒 Files selected for processing (3)
src/core/tools/ExecuteCommandTool.tssrc/integrations/terminal/ExecaTerminalProcess.tssrc/integrations/terminal/types.ts
The shell_execution_complete emission now includes aborted: false for normal process exits. Update the test expectation to match.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
3c9ca0a to
5f21e21
Compare
github-actions
Bot
added
the
awaiting-review
When abort() sends SIGKILL and the subprocess throws ExecaError before the for-await loop checks this.aborted, the catch block was emitting shell_execution_complete without aborted: this.aborted. This caused formatExitStatus to return 'Process terminated by signal SIGKILL' — the ambiguous message this PR was designed to eliminate. Both error paths now propagate aborted: this.aborted.
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Nice! Thanks for taking this on, this has bugged me for a while.
Since we're touching these files, can we also increase coverage so that the codecov plugin is happy as well.
- Export formatExitStatus from ExecuteCommandTool for testing - Add tests for formatExitStatus: aborted=true returns user message, aborted=false/signal/undefined all produce correct output - Add ExecaTerminalProcess error-path tests: both ExecaError and generic error catch paths assert aborted: false in emission - Rename existing test to reflect aborted: false assertion Increases codecov patch coverage from 20% to 100% Addresses PR Zoo-Code-Org#880 review comment from edelauna
- Add test that calls abort() during execution and verifies shell_execution_complete includes aborted: true - Fix docstring wording: abort() is also called by timeout and task teardown paths, not just user button press Addresses PR Zoo-Code-Org#880 review comments from edelauna (2 threads)
|
addressed all review feedback from @edelauna:
both threads replied inline |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts`:
- Around line 149-198: Add a regression test in the “error-path abort emission”
suite that exercises the user-abort flow through ExecaTerminalProcess’s existing
abort/kill mechanism, then verifies shell_execution_complete emits aborted:
true. Keep the test focused on actual abort propagation rather than generic or
ExecaError failures, and reuse the suite’s existing terminalProcess and
event-spy setup.
🪄 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 Plus
Run ID: 2726ad0d-2a6d-4b52-93c2-5161bf81cdb5
📒 Files selected for processing (3)
src/core/tools/ExecuteCommandTool.tssrc/core/tools/__tests__/executeCommandTool.spec.tssrc/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/tools/ExecuteCommandTool.ts
…enerators add eslint-disable-next-line comments before the async generator functions that throw immediately (no yield needed) to pass the CI compile/lint check
…ck-types compliance
github-actions
Bot
added
awaiting-author
2 participants
Related GitHub Issue
Closes: #833
Description
When the user clicked the Abort button on a running shell command,
ExecaTerminalProcessemittedshell_execution_completewith{ exitCode: 0 }regardless of whether the process completed normally or was killed by the user. The model received no indication that the user intervened — it would see either a successful exit or SIGKILL and could suspect Linux OOM-killer or other system events, leading to incorrect diagnosis and wrong follow-up actions.Fix:
abortedfield toExitCodeDetailsinterfaceExecaTerminalProcess.run()emits{ exitCode: 0, aborted: this.aborted }so the flag reflects whetherabort()was calledformatExitStatus()checksabortedfirst and returns"Command was aborted by the user."— clear, unambiguous, and takes priority over exit code or signal infoTest Procedure
sleep 60)Command executed in terminal... Command was aborted by the user.Pre-Submission Checklist
abortedis optional so all existing callers work unchangedScreenshots / Videos
N/A — backend message formatting, no UI changes.
Documentation Updates
Additional Notes
The VSCode terminal path (
TerminalProcess) sends Ctrl+C instead of SIGKILL when aborting, which correctly produces SIGINT in the exit details — that path was not affected by this bug. The fix targets the execa-based fallback path specifically.Summary by CodeRabbit
abortedflag in completion event payloads.abortedon success and added abort/error scenarios.