-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Allow running commands in background automatically #8607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in a mirror - twice the bugs, half the excuses.
| // kilocode_change start- If runInBackgroundRequested, automatically continue the process | ||
| if (runInBackgroundRequested && !completed) { | ||
| completed = true | ||
| process.continue() | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Logic flaw in runInBackground implementation.
The current implementation has several issues:
-
Only triggers on output: This code is inside the
onLinecallback, so it only executes when the command produces output. Commands that don't produce immediate output won't auto-continue. -
Premature completion flag: Setting
completed = truehere happens before the command actually completes (which is signaled byonCompletedcallback). This could interfere with the exit code handling logic at lines 334-361. -
Variable shadowing confusion: Line 181 declares
let runInBackground = runInBackgroundRequested, but this variable serves a different purpose - it tracks whether the user chose to run in background via the UI prompt (line 212). The current implementation conflates these two concepts.
Suggested fix:
The auto-continue logic should be placed where the user interaction prompt happens (around line 211), not in the onLine callback:
// kilocode_change start
if (runInBackgroundRequested) {
// Auto-continue without user interaction
runInBackground = true
process.continue()
return
}
// kilocode_change end
try {
const { response, text, images } = await task.ask("command_output", "")
runInBackground = true
// ... rest of existing logic
}This ensures:
- Auto-continue happens regardless of output
- The
completedflag is only set when the command actually completes - The two
runInBackgroundconcepts remain separate and clear
640466c to
b8c112e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in a mirror - twice the bugs, half the excuses.
| expect(mockTerminal.runCommand).toHaveBeenCalledWith("npm run dev", expect.any(Object)) | ||
| expect(mockProcess.continue).toHaveBeenCalled() | ||
|
|
||
| // The task.ask method should not be called since runInBackground skips user interaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Test doesn't verify the key behavior. The comment says "task.ask method should not be called" but there's no assertion to verify this. Add an assertion to ensure task.ask is never called when runInBackground is true:
| // The task.ask method should not be called since runInBackground skips user interaction | |
| // The task.ask method should not be called since runInBackground skips user interaction | |
| expect(mockTask.ask).not.toHaveBeenCalled() | |
| }) |
ad840b7 to
8788338
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No new issues found - all concerns already addressed in existing comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in a mirror - twice the bugs, half the excuses.
No new issues found - all concerns already addressed in existing comments.
8788338 to
01183cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code again? The existential dread of recursive self-analysis intensifies.
ad01db6 to
ee05573
Compare
src/core/tools/executeCommandTool.ts
Outdated
| // kilocode_change start- If run_in_backgroundRequested, automatically continue the process | ||
| if (runInBackgroundRequested && !backgroundProcessStarted) { | ||
| backgroundProcessStarted = true | ||
| process.continue() | ||
| return | ||
| } | ||
| // kilocode_change end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto-continue only triggers when command produces output. Commands that don't produce immediate output (or produce no output) won't auto-continue, defeating the purpose of runInBackground. For commands like npm run dev that may not output immediately, users will still see the interactive prompt.
The auto-continue should happen immediately when runInBackgroundRequested is true, not wait for output. Consider moving this logic before the try-catch at line 211 to ensure it runs regardless of output timing.
|
I updated this after discussing this with @hannesrudolph. I added more code in order to help the agent manage the running terminal processes. I couldn't think of a better way to do this other than adding a new tool, I thought about maybe adding a new parameter to the At first, I tried capturing the PIDs of the processes, but it's difficult to get the PID of the built-in VS Code terminal commands. You end up getting the shell process ID, which cannot be killed using the regular kill command, which is what the agent tries to do if you don't give it a new tool. So by giving it a tool, we can kill the process similar to the kill button that exists in the UI today. Essentially, this PR gives the agent the ability to click its own buttons in order to continue processes in the background and to kill things later. Test plan: Testing prompt: |
ee05573 to
7299b67
Compare
|
|
||
| if (!targetTerminal.busy && !targetTerminal.process) { | ||
| return `Terminal ${terminalId} is not running any process.` | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent null checking for process. The condition at line 74 checks !targetTerminal.busy && !targetTerminal.process, which returns early when process is null. However, at line 85 in the ExecaTerminal branch, the code calls targetTerminal.process.abort() without verifying process exists. This creates a potential null pointer exception if targetTerminal.busy is true but targetTerminal.process is null.
The early return condition should be if (!targetTerminal.process) to ensure process exists before attempting to use it in either branch.
- Add new `terminal_kill` tool for terminating terminal processes - Enhance `execute_command` tool with `run_in_background` parameter for long-running commands - Update tool descriptions, handlers, and tests accordingly
7299b67 to
9dea0b5
Compare
Add terminalId validation Co-authored-by: roomote[bot] <219738659+roomote[bot]@users.noreply.github.com>
|
Sorry for the delay- I'm going to close this PR while I iterate so I don't cause unnecessary noise and my the Roo bot work too hard. It'll be back soon! :) |
Adds a
runInBackgroundparameter to theexecute_commandtool. This allows commands, such as dev servers or monitoring processes, to be executed in the background without requiring user interaction or blocking further execution.We've had a bunch of users request this feature:
Kilo-Org/kilocode#2917
Kilo-Org/kilocode#2196
The change includes updates to the tool definition, implementation, and corresponding tests.
Important
Adds
runInBackgroundparameter toexecute_commandtool for background command execution, with tests and documentation updates.runInBackgroundparameter toexecute_commandtool inexecuteCommandTool.tsto allow commands to run in the background.runInBackgroundtofalseif not specified.runInBackgroundinexecuteCommandTimeout.integration.spec.tsandexecuteCommandTool.spec.tsto verify background execution behavior.execute-command.tsto includerunInBackgroundparameter in the tool description and usage examples.This description was created by
for 640466c. You can customize this summary. It will automatically update as commits are pushed.