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 3 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCommand abort state is added to terminal completion data, documented in exit details, tested on successful completion, and rendered as a dedicated “aborted by the user” message. ChangesCommand abort status
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 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 |
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
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.
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