-
Notifications
You must be signed in to change notification settings - Fork 4k
feat(cli): add beta background process execution tools #9074
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
base: main
Are you sure you want to change the base?
Conversation
… [Continue](https://continue.dev) Co-Authored-By: Continue <noreply@continue.dev>
|
Keep this PR in a mergeable state → Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
2 similar comments
|
Keep this PR in a mergeable state → Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
|
Keep this PR in a mergeable state → Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
|
✅ Review Complete Code Review Summary |
|
📚 Documentation updates have been added in PR #9075 Created user-facing documentation for the beta background process execution feature, including:
The documentation maintains consistency with existing CLI docs and focuses on practical usage patterns. |
|
🤖 All Green agent started: View agent |
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.
1 issue found across 13 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="extensions/cli/src/services/BackgroundProcessService.ts">
<violation number="1" location="extensions/cli/src/services/BackgroundProcessService.ts:209">
P1: The error handler doesn't update process status to 'exited'. When a process fails to spawn (e.g., command not found), Node.js emits the 'error' event but may not emit the 'close' event. This leaves the process incorrectly marked as 'running' indefinitely, which could also cause issues with the `runningCount` check in `startProcess` since failed processes would still count against `maxProcesses`.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| this.emit("processExited", { id, exitCode: code }); | ||
| }); | ||
|
|
||
| child.on("error", (error) => { |
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.
P1: The error handler doesn't update process status to 'exited'. When a process fails to spawn (e.g., command not found), Node.js emits the 'error' event but may not emit the 'close' event. This leaves the process incorrectly marked as 'running' indefinitely, which could also cause issues with the runningCount check in startProcess since failed processes would still count against maxProcesses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At extensions/cli/src/services/BackgroundProcessService.ts, line 209:
<comment>The error handler doesn't update process status to 'exited'. When a process fails to spawn (e.g., command not found), Node.js emits the 'error' event but may not emit the 'close' event. This leaves the process incorrectly marked as 'running' indefinitely, which could also cause issues with the `runningCount` check in `startProcess` since failed processes would still count against `maxProcesses`.</comment>
<file context>
@@ -0,0 +1,344 @@
+ this.emit("processExited", { id, exitCode: code });
+ });
+
+ child.on("error", (error) => {
+ logger.debug(`Background process ${id} error: ${error.message}`);
+ processInfo.stderrBuffer.append(`Process error: ${error.message}`);
</file context>
✅ Addressed in f2cf8fc
When a process fails to spawn (e.g., command not found), Node.js emits the 'error' event but may not emit the 'close' event. This fix ensures the process is marked as 'exited' and properly cleaned up from the running count. Co-authored-by: nate <nate@continue.dev> Generated with [Continue](https://continue.dev) Co-Authored-By: Continue <noreply@continue.dev>
| this.emit("processExited", { id, exitCode: code }); | ||
| }); | ||
|
|
||
| child.on("error", (error) => { |
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.
Fixed - now updates process status to exited and emits processExited event
Tools importing serviceContainer at module level caused circular dependencies with ToolPermissionService. Changed to dynamic imports within run functions to break the cycle. Co-authored-by: nate <nate@continue.dev> Generated with [Continue](https://continue.dev) Co-Authored-By: Continue <noreply@continue.dev>
|
@cubic-dev-ai This issue has been fixed in commit f2cf8fc. The error handler now properly updates the process status to |
- Move CircularBuffer to separate file to fix max-classes-per-file - Fix import order in BackgroundProcessService, exit.ts - Remove unused catch variable in bashOutput.ts Co-authored-by: nate <nate@continue.dev> Generated with [Continue](https://continue.dev) Co-Authored-By: Continue <noreply@continue.dev>
The exit.ts module was importing serviceContainer at module level, which contributed to the circular dependency issue with services. Co-authored-by: nate <nate@continue.dev> Generated with [Continue](https://continue.dev) Co-Authored-By: Continue <noreply@continue.dev>
ESLint requires no empty lines within import groups. Co-authored-by: nate <nate@continue.dev> Generated with [Continue](https://continue.dev) Co-Authored-By: Continue <noreply@continue.dev>
Removed BackgroundProcessService type import since we use dynamic imports and don't need the type at module level. Co-authored-by: nate <nate@continue.dev> Generated with [Continue](https://continue.dev) Co-Authored-By: Continue <noreply@continue.dev>
TypeScript needs type information for the service instance. Using dynamic import with type assertion to avoid circular dependency. Co-authored-by: nate <nate@continue.dev> Generated with [Continue](https://continue.dev) Co-Authored-By: Continue <noreply@continue.dev>
ESLint unused-imports rule requires that imports only used as types be handled differently. Co-authored-by: nate <nate@continue.dev> Generated with [Continue](https://continue.dev) Co-Authored-By: Continue <noreply@continue.dev>
Simplify type handling by using any since we're in error handler context. Co-authored-by: nate <nate@continue.dev> Generated with [Continue](https://continue.dev) Co-Authored-By: Continue <noreply@continue.dev>
| import { Tool } from "./types.js"; | ||
|
|
||
| export const bashOutputTool: Tool = { | ||
| name: "BashOutput", |
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.
Feels like the name could be more descriptive, eg ReadBackgroundProcessOutput
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.
Could we just use https://www.npmjs.com/package/ringbufferjs ?
More descriptive name that clearly indicates the tool's purpose of reading output from background processes. Co-authored-by: nate <nate@continue.dev> Generated with [Continue](https://continue.dev) Co-Authored-By: Continue <noreply@continue.dev>
| run_in_background: { | ||
| type: "boolean", | ||
| description: | ||
| "Run command in background and return immediately. Use BashOutput tool to monitor output. Useful for long-running processes like dev servers.", |
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.
Probably should mention KillProcess and ListProcesses as well. Ideally just import the obj and use the name property to avoid drift.
Also wondering if it's worth providing an example flow in the description. In general wondering how well the agent will be able to follow the expected usage pattern here, were you able to test much? It feels reasonably complicated and there isn't much guidance for the agent to follow.
- Replace custom circular buffer implementation with battle-tested ringbufferjs package - Maintain same API interface (append, getLines, getTotalLinesWritten, clear) - Keep line truncation and incremental reading features - Reduce maintenance burden by using external package - All existing tests pass with new implementation Co-authored-by: nate <nate@continue.dev>
- Import tool names dynamically to avoid drift - Add comprehensive example workflow in run_in_background description - Enhance all tool descriptions with clear use cases - Document incremental reading behavior for ReadBackgroundProcessOutput - Add three common usage patterns to documentation: * Waiting for server to start * Monitoring build progress * Parallel process execution This addresses concerns about agent understanding the expected usage patterns for background process management tools. Co-authored-by: nate <nate@continue.dev>
Summary by cubic
Adds beta support for running terminal commands in the background with a new BackgroundProcessService and tools to monitor and manage them. Enables Bash run_in_background, incremental output reads, process listing, and cleanup on exit, gated behind --beta-persistent-terminal-tools.
New Features
Migration
Written for commit 74fd449. Summary will update automatically on new commits.