refactor(agents): make the server own subagent execution - #190
Closed
Waishnav wants to merge 6 commits into
Closed
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI 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)
📝 WalkthroughWalkthroughThe PR moves local agent execution into a server-managed ChangesLocal agent execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AgentsCLI
participant DevSpaceServer
participant LocalAgentControlServer
participant LocalAgentManager
participant Provider
User->>DevSpaceServer: start devspace serve
DevSpaceServer->>LocalAgentControlServer: start agent control
User->>AgentsCLI: devspace agents run
AgentsCLI->>LocalAgentControlServer: submit run command
LocalAgentControlServer->>LocalAgentManager: enqueue command
LocalAgentManager->>Provider: execute queued turn
Provider-->>LocalAgentManager: return response and session data
LocalAgentManager-->>AgentsCLI: return agent record
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Waishnav
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/local-agent-control.ts (1)
108-132: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a client timeout to
requestControl.The client resolves only on
endand rejects only onerror. If the server accepts the connection and never responds,devspace agents runhangs with no output. Add a socket timeout so the CLI fails with a clear message.♻️ Proposed fix
const socket = createConnection(address); socket.setEncoding("utf8"); + socket.setTimeout(30_000, () => { + socket.destroy(); + reject(new Error("DevSpace subagent runtime did not respond in time.")); + });🤖 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/local-agent-control.ts` around lines 108 - 132, Update requestControl to configure a client-side timeout on the socket after createConnection. On timeout, destroy the socket and reject with a clear error stating that the DevSpace subagent runtime did not respond in time, while preserving the existing error, data, end, and connect handling.
🤖 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 `@docs/configuration.md`:
- Around line 156-158: Update the `devspace agents run` documentation to
explicitly require setting `DEVSPACE_SUBAGENTS=1` before starting `devspace
serve`, while preserving the guidance about running the agent command from
another terminal.
In `@src/local-agent-control.ts`:
- Around line 64-77: Update handleConnection to register a socket error handler
so peer disconnects or write failures do not become uncaught exceptions. Define
and use MAX_CONTROL_REQUEST_BYTES to reject or destroy connections once the
accumulated request exceeds the limit, stop processing input after the first
line, and ensure parse/handle failures cleanly destroy or close the socket
instead of leaving it paused.
- Around line 53-62: Track active client sockets in local-agent-control by
adding each socket in handleConnection and removing it on the socket’s close
event. During close(), destroy all tracked sockets before awaiting
server.close(), so idle connections cannot keep shutdown blocked.
In `@src/local-agent-manager.ts`:
- Around line 73-88: Update enqueue so the existing-agent lookup only matches
agent IDs, not provider_session_id values; use the store API or predicate that
explicitly targets the agent ID field before calling prepareExistingAgent or
createAgent. Preserve the subsequent scheduling and status update flow.
- Around line 139-153: Update the queue-tail handling around next in the local
agent manager so the promise stored in queue.tail always has a rejection
handler, preventing unhandled rejections from the status update or executeTurn
callback. Preserve the rejection details by recording the failure in the agent
record, while keeping queue.pending decrement and queue cleanup in the existing
finally flow.
- Around line 114-131: The local-agent manager must enforce allowed-root
containment for workspace paths at both creation and execution boundaries. In
src/local-agent-manager.ts:114-131, validate command.workspaceRoot before
loadLocalAgentProfiles or store.create, and validate stored agent records before
provider execution; keep src/cli.ts:352-360 unchanged and rely on server-side
validation.
In `@src/server.ts`:
- Around line 1700-1703: Update the startup flow around localAgentControl and
startAgentControl so any rejected startup closes the server before propagating
the failure. Ensure close() runs before callers exit or register shutdown
handlers, preserving normal startup and shutdown behavior.
---
Nitpick comments:
In `@src/local-agent-control.ts`:
- Around line 108-132: Update requestControl to configure a client-side timeout
on the socket after createConnection. On timeout, destroy the socket and reject
with a clear error stating that the DevSpace subagent runtime did not respond in
time, while preserving the existing error, data, end, and connect handling.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ad03e84-b539-46f9-8a76-3f849a33ecc5
📒 Files selected for processing (8)
docs/configuration.mdpackage.jsonsrc/cli.tssrc/local-agent-control.test.tssrc/local-agent-control.tssrc/local-agent-manager.test.tssrc/local-agent-manager.tssrc/server.ts
Greptile SummaryThe PR moves subagent execution into the long-lived DevSpace server and introduces local IPC plus per-agent turn serialization.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking IPC hardening issue around unbounded incomplete-request buffering. The server-owned runtime and serialized execution paths are coherent, but the new control socket retains unlimited input from a client that never sends a newline. Files Needing Attention: src/local-agent-control.ts
|
| Filename | Overview |
|---|---|
| src/local-agent-control.ts | Introduces local IPC request handling; request buffering is not bounded for clients that omit the newline terminator. |
| src/local-agent-manager.ts | Introduces server-owned subagent queues, serialized provider execution, and durable session updates. |
| src/server.ts | Integrates control-server startup and manager shutdown into the DevSpace server lifecycle. |
| src/cli.ts | Replaces detached subagent workers with requests to the running server. |
| src/local-agent-control.test.ts | Covers a successful control request but not oversized or unterminated request handling. |
| src/local-agent-manager.test.ts | Verifies serialization and provider-session continuation across queued turns. |
Sequence Diagram
sequenceDiagram
participant CLI as devspace agents run
participant IPC as LocalAgentControlServer
participant Manager as LocalAgentManager
participant Store as LocalAgentStore
participant Provider as Agent Provider
CLI->>IPC: run command over local socket
IPC->>Manager: enqueue(command)
Manager->>Store: create/update running record
Manager-->>IPC: record
IPC-->>CLI: record
Manager->>Provider: execute serialized turn
Provider-->>Manager: response and session ID
Manager->>Store: persist idle/error state
Reviews (1): Last reviewed commit: "test(agents): cover serialized server ex..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Owner
Author
[GPT-5.6-SOL] RESPONDING ON BEHALF OF WAISHNAVAlso addressed the review nitpick about client hangs in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/local-agent-store.test.ts (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover exact local-ID resolution.
Line [27] tests only the unique-prefix query. The exact-ID query in
src/local-agent-store.tsLines [156]-[159] is a separate branch. Add an exact-ID assertion before the prefix assertion.Proposed test addition
+ assert.equal(store.getByAgentId(created.id)?.id, created.id); assert.equal(store.getByAgentId(created.id.slice(0, 7))?.id, created.id);🤖 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/local-agent-store.test.ts` at line 27, Add an assertion in the test covering the created agent to call getByAgentId with created.id exactly and verify it returns the created agent ID, placing it before the existing shortened-prefix assertion so both resolution branches are covered.
🤖 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/local-agent-store.ts`:
- Around line 161-169: Update the local-agent lookup method containing the
matches query and rowToLocalAgentRecord so matches.length > 1 produces an
explicit ambiguous result or throws instead of returning undefined. Preserve
undefined only for no matches, and update the local-agent-manager flow around
createAgent so ambiguous prefixes cannot create a new agent. Add a regression
test covering multiple agents with the same prefix.
---
Nitpick comments:
In `@src/local-agent-store.test.ts`:
- Line 27: Add an assertion in the test covering the created agent to call
getByAgentId with created.id exactly and verify it returns the created agent ID,
placing it before the existing shortened-prefix assertion so both resolution
branches are covered.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a8ac993-b409-4540-965c-b01726346590
📒 Files selected for processing (8)
docs/configuration.mdsrc/local-agent-control.test.tssrc/local-agent-control.tssrc/local-agent-manager.test.tssrc/local-agent-manager.tssrc/local-agent-store.test.tssrc/local-agent-store.tssrc/server.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/local-agent-control.test.ts
- src/local-agent-manager.test.ts
- docs/configuration.md
- src/local-agent-manager.ts
- src/server.ts
- src/local-agent-control.ts
This was referenced Aug 12, 2026
Owner
Author
|
Closing this Sol stack PR in favor of the Luna-based implementation. |
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.
Subagent turns currently run from short-lived detached workers, which makes provider lifetime and per-agent turn ownership hard to reason about and prevents later runtime reuse. This moves execution under the running DevSpace server and introduces a single LocalAgentManager boundary for starting, continuing, persisting, and serializing logical agent turns. Durable session state stays in LocalAgentStore; live execution remains server-owned and disposable.\n\nThis is the bottom layer of the runtime-efficiency stack. The existing
devspace agentscommand surface stays intact, but agent execution now requires the DevSpace server to be running so later layers can safely reuse expensive provider runtimes.Summary by CodeRabbit
New Features
Documentation
devspace servebefore running agent commands.