feat: run local agents through an on-demand daemon - #183
Conversation
|
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:
📝 WalkthroughWalkthroughThe change introduces ChangesLocal agent execution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant LocalAgentClient
participant LocalAgentDaemon
participant LocalAgentManager
participant LocalAgentRuntimePool
CLI->>LocalAgentClient: submit agent operation
LocalAgentClient->>LocalAgentDaemon: send JSON-line request
LocalAgentDaemon->>LocalAgentManager: dispatch operation
LocalAgentManager->>LocalAgentRuntimePool: execute provider turn
LocalAgentRuntimePool-->>LocalAgentManager: return runtime result
LocalAgentManager-->>LocalAgentDaemon: return agent record
LocalAgentDaemon-->>LocalAgentClient: send protocol response
LocalAgentClient-->>CLI: display result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Greptile SummaryThis PR moves subagent turn ownership from detached CLI workers into the long-lived server.
Confidence Score: 1/5The PR is not safe to merge until the control credential is kept off unencrypted network paths and agent workspace roots are constrained to the configured allowed roots. The new server control path can expose a replayable write-capable agent credential over HTTP and trusts requester-selected filesystem roots without applying the existing workspace boundary. Files Needing Attention: src/cli.ts, src/local-agent-control.ts, src/local-agent-manager.ts, src/server.ts
|
| Filename | Overview |
|---|---|
| src/cli.ts | Replaces detached workers with a control-endpoint request, but exposes the control credential when configured to contact a non-loopback host over HTTP. |
| src/local-agent-control.ts | Adds token-authenticated agent execution, but accepts arbitrary workspace roots without enforcing configured root restrictions. |
| src/local-agent-manager.ts | Adds durable turn orchestration and concurrency tracking; it propagates the control request's workspace directly into write-enabled runtime execution. |
| src/local-agent-runtime-pool.ts | Adds single-flight runtime acquisition, reuse, crash eviction, idle closure, and idempotent pool shutdown. |
| src/local-agent-store.ts | Adds startup reconciliation that marks interrupted starting or running turns as errors. |
| src/server.ts | Installs the control endpoint and manager lifecycle on the main listener, including shutdown cleanup. |
Sequence Diagram
sequenceDiagram
participant CLI
participant Server as DevSpace Server
participant Control as LocalAgentControl
participant Manager as LocalAgentManager
participant Pool as Runtime Pool
participant Runtime as Provider Runtime
CLI->>Server: "POST /__devspace/agents/run<br/>control token + target + prompt + workspaceRoot"
Server->>Control: handle(request)
Control->>Control: validate bearer token
Control->>Manager: start(input) or continue(id, prompt)
Manager->>Manager: "persist status = running"
Manager->>Pool: run(driver, context, input)
Pool->>Runtime: acquire/create and run turn
Runtime-->>Pool: provider session + final response
Pool-->>Manager: result
Manager->>Manager: "persist status = idle/error"
Manager-->>Control: current record
Control-->>CLI: 202 Accepted
Reviews (1): Last reviewed commit: "refactor: move subagent execution into s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function localAgentControlUrl(config: ReturnType<typeof loadConfig>): string { | ||
| const host = config.host === "0.0.0.0" || config.host === "::" ? "127.0.0.1" : config.host; | ||
| const formattedHost = host.includes(":") && !host.startsWith("[") ? `[${host}]` : host; | ||
| return `http://${formattedHost}:${config.port}${LOCAL_AGENT_CONTROL_PATH}`; |
There was a problem hiding this comment.
If the server uses a non-loopback host, this URL sends the agent-control bearer token over unencrypted HTTP, allowing a network observer to capture and replay it to start or continue write-enabled agents.
How this was verified: The configured host reaches a plain http:// fetch carrying the token, and the receiving route authorizes write-enabled agent execution with that token.
| function decodeControlRequest(value: unknown): LocalAgentControlRequest { | ||
| if (!isRecord(value)) throw new Error("Invalid local agent control request."); | ||
| const target = requiredString(value.target, "target"); | ||
| const prompt = requiredString(value.prompt, "prompt"); | ||
| const workspaceRoot = requiredString(value.workspaceRoot, "workspaceRoot"); | ||
| return { | ||
| target, | ||
| prompt, | ||
| workspaceRoot, |
There was a problem hiding this comment.
When a valid control-token holder supplies a workspaceRoot outside config.allowedRoots, this decoder accepts it and the manager passes it to a write-enabled runtime, allowing the agent to read or modify files outside the configured workspace boundary.
How this was verified: The request path validates only that workspaceRoot is nonempty, then persists it and forwards it to the runtime without calling the repository's allowed-path check.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce23beb451
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { createWorkspaceStore } from "./workspace-store.js"; | ||
| import { formatAgentsPath, WorkspaceRegistry } from "./workspaces.js"; | ||
| import { summarizeLocalAgentProfile } from "./local-agent-profiles.js"; | ||
| import { createLocalAgentDrivers } from "./local-agent-adapters.js"; |
There was a problem hiding this comment.
With this import, src/server.ts cannot even be loaded because src/local-agent-adapters.ts does not export createLocalAgentDrivers (I checked with rg createLocalAgentDrivers), and that adapter module also still imports the now-removed createCodexSdkLocalAgentRuntime from local-agent-runtime.ts. This means devspace serve fails during ESM module instantiation before the new subagent manager/control path can run.
AGENTS.md reference: AGENTS.md:L70-L84
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (14)
src/local-agent-runtime.ts (2)
37-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
releaseSessionhas no caller andLocalAgentRunResult.provideris widened tostring.Two contract observations:
releaseSession(providerSessionId)is part of the runtime interface, butLocalAgentRuntimePoolnever calls it. Every implementation must supply a stub. Either remove it until a caller exists, or document which component owns session release.LocalAgentRunResult.providerisstringwhileLocalAgentRuntime.providerandLocalAgentRuntimeContext.providerareLocalAgentProvider. Narrow the result field toLocalAgentProviderfor one consistent provider type across the seam.🤖 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-runtime.ts` around lines 37 - 43, Remove releaseSession from LocalAgentRuntime until a caller exists, eliminating the unused implementation requirement. Narrow LocalAgentRunResult.provider from string to LocalAgentProvider so it matches LocalAgentRuntime.provider and LocalAgentRuntimeContext.provider across the runtime contract.
21-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
workspaceto make the path meaning explicit.
LocalAgentRuntimeContext.workspacereceivesrecord.workspaceRootinsrc/local-agent-manager.tsline 187. The nameworkspacereads as the workspace concept, and the neighbouring handle in the domain isworkspaceId. UseworkspaceRootso the field states that it carries a filesystem root, not the opaque workspace handle.LocalAgentRunInput.workspacehas the same ambiguity.As per coding guidelines: "Use glossary terms precisely in schemas, types, documentation, and errors, including distinctions among workspace, allowed root, checkout, and worktree."
♻️ Proposed rename
export interface LocalAgentRuntimeContext { agentId: string; provider: LocalAgentProvider; - workspace: string; + workspaceRoot: string; providerSessionId?: string;🤖 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-runtime.ts` around lines 21 - 30, Rename LocalAgentRuntimeContext.workspace to workspaceRoot and update all construction and consumption sites, including the assignment from record.workspaceRoot in the local agent manager. Apply the same rename to LocalAgentRunInput.workspace and its references so filesystem-root semantics are explicit while preserving the existing workspaceId usage.Source: Coding guidelines
src/local-agent-store.ts (1)
199-209: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGive
reconcileActiveRunsan optional scope.The statement matches every
startingorrunningrow in the database. The store has no way to limit the reconciliation to one workspace or one owning process. Accept an optional workspace filter so the caller can restrict the blast radius. The unconditional caller issrc/local-agent-manager.tsline 76.🤖 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.ts` around lines 199 - 209, Update reconcileActiveRuns to accept an optional workspace filter and apply it to the SQL WHERE clause when provided, while preserving reconciliation of all active rows when omitted. Update the unconditional caller in local-agent-manager.ts to pass the appropriate workspace scope.src/local-agent-manager.ts (2)
252-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate the provider instead of casting it.
assertDrivercastsprovider as LocalAgentProviderto index the map.LocalAgentRecord.provideris typedstringinsrc/local-agent-store.ts. The cast hides the fact that a stored row can hold a value outside the union. The lookup still fails safely and throws, so this is a typing hygiene point: narrow with a runtime check and keep the union honest.🤖 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-manager.ts` around lines 252 - 256, Update assertDriver to validate and narrow the string provider at runtime before looking it up in this.drivers, rather than casting provider as LocalAgentProvider. Preserve the existing error for unsupported providers and keep the map lookup typed against the validated union.
146-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRegister the turn before you start it.
Line 162 calls
this.runTurn(...)and line 163 registers the promise inactiveTurns. The registration works today only becauserunTurnreaches its firstawaitbefore returning. If a future edit makes an early path inrunTurnsynchronous, thefinallyblock at line 226 can delete an entry that line 163 then re-adds, and the agent stays permanently marked as busy. Register a placeholder before you start the turn, or start the turn in a deferred step.🤖 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-manager.ts` around lines 146 - 166, Update begin so activeTurns is registered before invoking runTurn, using a placeholder promise or deferred start that preserves the existing turn tracking and cleanup behavior. Ensure runTurn’s completion cannot remove an entry that is added afterward, while retaining the current duplicate-running check and error swallowing.src/local-agent-manager.test.ts (1)
143-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that
close()releases the runtimes.The test calls
manager.close()twice and then removes the directory. It never checks that the pooled runtimes were closed.FakeRuntimealready tracksclosed. Add an assertion so a regression inLocalAgentRuntimePool.closefails this test.💚 Proposed addition
await manager.close(); await manager.close(); +for (const [agentId, runtime] of runtimes) { + assert.equal(runtime.closed, true, `runtime for ${agentId} was closed on shutdown`); +} await rm(root, { recursive: true, force: true });🤖 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-manager.test.ts` around lines 143 - 145, Add an assertion in the test around the repeated manager.close() calls to verify each tracked FakeRuntime has closed set, using the existing runtime collection and tracking rather than adding new state. Ensure the assertion runs after closing the manager and before removing the temporary directory.src/cli.test.ts (2)
101-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the request path and the control token.
The stub server accepts any path and any headers. The test therefore passes even if
localAgentControlUrlbuilds a wrong path or the CLI omits thex-devspace-control-tokenheader. Both are contract points introduced by this PR. Capture and assert them.💚 Proposed addition
const received: Record<string, unknown>[] = []; + const receivedMeta: { url?: string; token?: string }[] = []; const controlServer = createHttpServer((request, response) => { let body = ""; request.setEncoding("utf8"); request.on("data", (chunk: string) => { body += chunk; }); request.on("end", () => { received.push(JSON.parse(body) as Record<string, unknown>); + receivedMeta.push({ + url: request.url, + token: request.headers["x-devspace-control-token"] as string | undefined, + });assert.deepEqual(received, [{ target: "reviewer", prompt: "Review this", workspaceRoot: projectRoot, workspaceId: "ws_current", }]); + assert.deepEqual(receivedMeta, [{ + url: "/__devspace/agents/run", + token: controlToken, + }]);Also applies to: 143-148
🤖 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/cli.test.ts` around lines 101 - 119, Update the controlServer request handler in the CLI test to capture and assert the request URL/path and the x-devspace-control-token header, including the corresponding handler at the other covered location. Verify both values match the expected control endpoint and token before accepting the request, while preserving the existing body and response assertions.
127-141: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout, and note the verification scope.
execFileAsyncruns without atimeoutoption. If the CLI blocks onfetch, the child process never exits and the test hangs until the outer runner kills it. Set an explicit timeout so the failure is attributable.This test runs
src/cli.tsthrough the tsx loader. It does not exercise the packagednpxentry point, so the packaging path stays unverified by this test.As per coding guidelines: "Verify the actual user-consumption path, including packaged npm/npx usage... clearly state when only a narrower proxy was verified."
♻️ Proposed change
const result = await execFileAsync("node", ["--import", "tsx", "src/cli.ts", "agents", "run", "reviewer", "Review", "this"], { cwd: process.cwd(), + timeout: 30_000, env: {🤖 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/cli.test.ts` around lines 127 - 141, Update the execFileAsync invocation in the CLI test to include an explicit timeout so blocked child processes fail deterministically. Keep the existing tsx-based test flow, and document in the test’s verification scope that it exercises src/cli.ts only, not the packaged npx entry point.Source: Coding guidelines
src/local-agent-control.test.ts (2)
80-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
responseForwill need a socket once the handler checks the peer address.The fake
Requestsuppliesmethod,body, andheaderonly. If the handler adds a loopback check, as proposed onsrc/local-agent-control.tslines 52-60, this fake dereferencesreq.socketand throws. Add asocket: { remoteAddress: "127.0.0.1" }stub now so the fake matches the real request surface.🤖 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.test.ts` around lines 80 - 116, Update the responseFor test helper’s fake Request to include a socket stub with remoteAddress set to "127.0.0.1", alongside its existing method, body, and header properties, so handler peer-address checks can execute without throwing.
39-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the rejection paths.
The tests cover 401, 202, and 409. Three handler branches stay untested:
- A non-POST method must return 405.
- A missing
x-devspace-control-tokenheader must return 401.- A body without
target,prompt, orworkspaceRootmust return 400 with the field name.These branches are the request-validation contract of the endpoint.
🤖 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.test.ts` around lines 39 - 52, Add test cases alongside the existing authorization tests for the handler’s validation branches: verify a non-POST request returns 405, a request missing x-devspace-control-token returns 401, and a body missing target, prompt, or workspaceRoot returns 400 with the relevant field name in the response. Keep the existing 401, 202, and 409 assertions unchanged.src/local-agent-runtime.test.ts (1)
55-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the failure and eviction paths.
The tests cover single-flight creation, active-run protection, and idempotent close. Three pool behaviours stay untested:
createRuntimerejection, which must delete the entry and propagate the error.- A runtime that reports
isAlive() === false, which must trigger the recreate path inrunat lines 61-69 ofsrc/local-agent-runtime-pool.ts.- Actual idle eviction with a finite
idleTimeoutMs, which the current driver disables withNumber.POSITIVE_INFINITY.These paths carry the shutdown and crash-recovery logic, so they deserve direct assertions.
Note that
FakeRuntimeis a narrow proxy. No test in this file exercises a real provider runtime.As per coding guidelines: "Verify the actual user-consumption path... clearly state when only a narrower proxy was verified."
🤖 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-runtime.test.ts` around lines 55 - 88, Extend the tests around LocalAgentRuntimePool to cover createRuntime rejection, asserting the error propagates and the failed entry is removed; an existing pooled runtime whose isAlive() returns false, asserting run recreates it; and finite idleTimeoutMs eviction, asserting an idle runtime is closed and removed while active runtimes remain protected. Keep the existing FakeRuntime limitation explicit by distinguishing proxy coverage from real provider-runtime behavior.Source: Coding guidelines
src/local-agent-control.ts (1)
41-50: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueA second
LocalAgentControlon the samestateDirinvalidates the first token.The constructor always writes
agent-control.token, andclose()always removes it. Two instances that share astateDiroverwrite each other's token and delete a file the other instance still needs.src/local-agent-control.test.tslines 34-69 already show the behaviour. The server creates one instance today, so this is a robustness concern, not a live bug. Reject construction when a live token file exists, or store the token per instance.Also applies to: 76-80
🤖 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 41 - 50, The LocalAgentControl constructor currently overwrites the shared token file, while close() can remove another instance’s token. Update the constructor and related cleanup using the existing token-path handling to reject construction when a live token file already exists, preserving the current token creation behavior only when no active token is present.src/cli.ts (2)
421-425: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe control address is derived from local config, not from the running server.
localAgentControlUrlbuilds the URL fromconfig.hostandconfig.port. The CLI treats the token file as proof that the server runs, but it never learns the address the server actually bound. If the operator starts the server with onePORTand runs the CLI with a different environment, the request goes to the wrong address or to an unrelated process, while the token is still sent. Write the bound base URL next to the token at server start and read both here.🤖 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/cli.ts` around lines 421 - 425, The localAgentControlUrl function must use the server’s actual bound base URL rather than deriving host and port from loadConfig. At server startup, persist the bound base URL alongside the control token, then update the CLI read path to load both values and construct requests from the persisted URL, preserving IPv6/path handling and avoiding token use when the address metadata is unavailable.
347-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument asynchronous
agents runbehavior.Add the
runningstatus and the follow-updevspace agents show <id>flow toREADME.md, the CLI help insrc/cli.ts, and workflow documents that list these commands. The bundledsubagent-delegationskill already documents this behavior.🤖 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/cli.ts` around lines 347 - 352, Document the asynchronous behavior of runAgentsRun in README.md, the CLI help text in src/cli.ts, and workflow documents listing agent commands: explain that agents run reports a running status and direct users to follow up with devspace agents show <id>. Align the wording with the existing subagent-delegation skill documentation.Source: Coding guidelines
🤖 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/cli.ts`:
- Around line 369-374: Validate the successful `payload` returned by
`readJsonResponse` before casting it to `LocalAgentRecord` or passing it to
`formatAgentLine`; when it is missing or not an object, throw a clear error
instead of allowing `formatAgentLine` to dereference undefined. Preserve the
existing non-OK response error handling.
- Around line 354-368: Update the control request in the agents run flow around
fetch(localAgentControlUrl(config)) to attach an AbortSignal-based timeout,
ensuring stalled requests are aborted instead of blocking indefinitely. Catch
timeout failures and report a clear error that distinguishes the request timeout
from other fetch errors.
In `@src/local-agent-control.test.ts`:
- Around line 54-69: Use a distinct temporary state directory when constructing
busyControl, while keeping control on the original stateDir. Read the busy
control token from its own directory for the request, then close busyControl and
assert its token is removed separately; finally close control and assert the
original control token is removed.
In `@src/local-agent-control.ts`:
- Around line 52-60: Restrict LocalAgentControl.handle to loopback callers by
validating req.socket.remoteAddress before the token check, rejecting
non-loopback addresses. In src/local-agent-control.ts#L52-L60, preserve the
existing method and token responses for valid local requests; in
src/local-agent-control.test.ts#L80-L116, add the loopback socket stub to the
fake Request and a test asserting non-loopback requests are rejected.
- Around line 62-74: Separate the request’s agent handle from its
provider/profile target so LocalAgentStore.get and LocalAgentManager
continuation use the opaque agent identifier, while new runs use the requested
profile; update the request schema and callers accordingly. In the local-agent
manager, introduce and throw a typed conflict error for an already-running turn,
and have the request handler map status by error type rather than message text.
Replace raw manager error responses with safe client-facing messages, while
logging or preserving detailed errors internally, and ensure missing-driver and
store failures are not reported as generic 400 responses.
In `@src/local-agent-manager.ts`:
- Around line 230-250: Update buildRunInput to default writeMode to "read_only"
without reading it from LocalAgentProfile. Ensure every provider adapter
consumes input.writeMode and explicitly maps "read_only" to restrictive
permissions, including Claude’s permission mode and ACP’s allow option, while
preserving the existing behavior for other supported modes.
In `@src/local-agent-runtime-pool.ts`:
- Around line 107-114: Update the runtime creation flow in acquire() to re-check
this.closing after createPromise resolves and before retaining or returning the
new entry. If closing has started, immediately close the newly created runtime
through the existing closeEntry path and do not leave it in this.entries;
otherwise preserve the normal insertion and return behavior.
- Around line 76-88: Update the catch block around removeAndClose in the runtime
execution flow so a close failure is isolated and cannot replace the original
provider error or prevent the harness_runtime_crashed log. Handle any rejection
from removeAndClose, then continue logging and rethrow the caught error
unchanged.
In `@src/local-agent-store.ts`:
- Around line 199-209: The stale-run reconciliation must be scoped to the
current process rather than updating every active row. In
src/local-agent-store.ts lines 199-209, update
LocalAgentStore.reconcileActiveRuns to accept an optional workspace or owner
scope and include it in the WHERE clause; in src/local-agent-manager.ts lines
69-77, pass the scope for runs owned by that manager when invoking
reconcileActiveRuns, or alternatively enforce and document exclusive stateDir
ownership.
In `@src/server.ts`:
- Around line 59-61: Implement and export createLocalAgentDrivers in
local-agent-adapters.ts so it returns a readonly LocalAgentDriver[] compatible
with LocalAgentManager. Compose the available single-provider adapters into the
returned collection, and preserve the existing imports and usage in server.ts.
---
Nitpick comments:
In `@src/cli.test.ts`:
- Around line 101-119: Update the controlServer request handler in the CLI test
to capture and assert the request URL/path and the x-devspace-control-token
header, including the corresponding handler at the other covered location.
Verify both values match the expected control endpoint and token before
accepting the request, while preserving the existing body and response
assertions.
- Around line 127-141: Update the execFileAsync invocation in the CLI test to
include an explicit timeout so blocked child processes fail deterministically.
Keep the existing tsx-based test flow, and document in the test’s verification
scope that it exercises src/cli.ts only, not the packaged npx entry point.
In `@src/cli.ts`:
- Around line 421-425: The localAgentControlUrl function must use the server’s
actual bound base URL rather than deriving host and port from loadConfig. At
server startup, persist the bound base URL alongside the control token, then
update the CLI read path to load both values and construct requests from the
persisted URL, preserving IPv6/path handling and avoiding token use when the
address metadata is unavailable.
- Around line 347-352: Document the asynchronous behavior of runAgentsRun in
README.md, the CLI help text in src/cli.ts, and workflow documents listing agent
commands: explain that agents run reports a running status and direct users to
follow up with devspace agents show <id>. Align the wording with the
existing subagent-delegation skill documentation.
In `@src/local-agent-control.test.ts`:
- Around line 80-116: Update the responseFor test helper’s fake Request to
include a socket stub with remoteAddress set to "127.0.0.1", alongside its
existing method, body, and header properties, so handler peer-address checks can
execute without throwing.
- Around line 39-52: Add test cases alongside the existing authorization tests
for the handler’s validation branches: verify a non-POST request returns 405, a
request missing x-devspace-control-token returns 401, and a body missing target,
prompt, or workspaceRoot returns 400 with the relevant field name in the
response. Keep the existing 401, 202, and 409 assertions unchanged.
In `@src/local-agent-control.ts`:
- Around line 41-50: The LocalAgentControl constructor currently overwrites the
shared token file, while close() can remove another instance’s token. Update the
constructor and related cleanup using the existing token-path handling to reject
construction when a live token file already exists, preserving the current token
creation behavior only when no active token is present.
In `@src/local-agent-manager.test.ts`:
- Around line 143-145: Add an assertion in the test around the repeated
manager.close() calls to verify each tracked FakeRuntime has closed set, using
the existing runtime collection and tracking rather than adding new state.
Ensure the assertion runs after closing the manager and before removing the
temporary directory.
In `@src/local-agent-manager.ts`:
- Around line 252-256: Update assertDriver to validate and narrow the string
provider at runtime before looking it up in this.drivers, rather than casting
provider as LocalAgentProvider. Preserve the existing error for unsupported
providers and keep the map lookup typed against the validated union.
- Around line 146-166: Update begin so activeTurns is registered before invoking
runTurn, using a placeholder promise or deferred start that preserves the
existing turn tracking and cleanup behavior. Ensure runTurn’s completion cannot
remove an entry that is added afterward, while retaining the current
duplicate-running check and error swallowing.
In `@src/local-agent-runtime.test.ts`:
- Around line 55-88: Extend the tests around LocalAgentRuntimePool to cover
createRuntime rejection, asserting the error propagates and the failed entry is
removed; an existing pooled runtime whose isAlive() returns false, asserting run
recreates it; and finite idleTimeoutMs eviction, asserting an idle runtime is
closed and removed while active runtimes remain protected. Keep the existing
FakeRuntime limitation explicit by distinguishing proxy coverage from real
provider-runtime behavior.
In `@src/local-agent-runtime.ts`:
- Around line 37-43: Remove releaseSession from LocalAgentRuntime until a caller
exists, eliminating the unused implementation requirement. Narrow
LocalAgentRunResult.provider from string to LocalAgentProvider so it matches
LocalAgentRuntime.provider and LocalAgentRuntimeContext.provider across the
runtime contract.
- Around line 21-30: Rename LocalAgentRuntimeContext.workspace to workspaceRoot
and update all construction and consumption sites, including the assignment from
record.workspaceRoot in the local agent manager. Apply the same rename to
LocalAgentRunInput.workspace and its references so filesystem-root semantics are
explicit while preserving the existing workspaceId usage.
In `@src/local-agent-store.ts`:
- Around line 199-209: Update reconcileActiveRuns to accept an optional
workspace filter and apply it to the SQL WHERE clause when provided, while
preserving reconciliation of all active rows when omitted. Update the
unconditional caller in local-agent-manager.ts to pass the appropriate workspace
scope.
🪄 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: b5ece133-e7b7-421e-941a-9f30e720ac45
📒 Files selected for processing (11)
src/cli.test.tssrc/cli.tssrc/local-agent-control.test.tssrc/local-agent-control.tssrc/local-agent-manager.test.tssrc/local-agent-manager.tssrc/local-agent-runtime-pool.tssrc/local-agent-runtime.test.tssrc/local-agent-runtime.tssrc/local-agent-store.tssrc/server.ts
| const response = await fetch(localAgentControlUrl(config), { | ||
| method: "POST", | ||
| headers: { | ||
| "content-type": "application/json", | ||
| "x-devspace-control-token": token, | ||
| }, | ||
| body: JSON.stringify({ | ||
| target: parsed.target, | ||
| prompt: parsed.prompt, | ||
| workspaceRoot, | ||
| workspaceId: process.env.DEVSPACE_WORKSPACE_ID, | ||
| model: parsed.model, | ||
| thinking: parsed.thinking, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The control request has no timeout.
fetch runs without an AbortSignal. If the DevSpace server accepts the connection and then stalls, devspace agents run blocks with no output and no way to distinguish a slow start from a hang. Attach a timeout signal and report a clear error.
🔒 Proposed fix
const response = await fetch(localAgentControlUrl(config), {
method: "POST",
+ signal: AbortSignal.timeout(30_000),
headers: {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await fetch(localAgentControlUrl(config), { | |
| method: "POST", | |
| headers: { | |
| "content-type": "application/json", | |
| "x-devspace-control-token": token, | |
| }, | |
| body: JSON.stringify({ | |
| target: parsed.target, | |
| prompt: parsed.prompt, | |
| workspaceRoot, | |
| workspaceId: process.env.DEVSPACE_WORKSPACE_ID, | |
| model: parsed.model, | |
| thinking: parsed.thinking, | |
| }), | |
| }); | |
| const response = await fetch(localAgentControlUrl(config), { | |
| method: "POST", | |
| signal: AbortSignal.timeout(30_000), | |
| headers: { | |
| "content-type": "application/json", | |
| "x-devspace-control-token": token, | |
| }, | |
| body: JSON.stringify({ | |
| target: parsed.target, | |
| prompt: parsed.prompt, | |
| workspaceRoot, | |
| workspaceId: process.env.DEVSPACE_WORKSPACE_ID, | |
| model: parsed.model, | |
| thinking: parsed.thinking, | |
| }), | |
| }); |
🤖 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/cli.ts` around lines 354 - 368, Update the control request in the agents
run flow around fetch(localAgentControlUrl(config)) to attach an
AbortSignal-based timeout, ensuring stalled requests are aborted instead of
blocking indefinitely. Catch timeout failures and report a clear error that
distinguishes the request timeout from other fetch errors.
| const payload = await readJsonResponse(response); | ||
| if (!response.ok) { | ||
| throw new Error(payload && typeof payload.error === "string" ? payload.error : `Agent request failed (${response.status}).`); | ||
| } | ||
| const record = payload as unknown as LocalAgentRecord; | ||
| console.log(formatAgentLine(record)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A successful response with a non-JSON body crashes the CLI.
readJsonResponse returns undefined when the body is not JSON or is not an object. Line 370 only guards the !response.ok branch. If the server returns 202 with an empty or non-JSON body, payload is undefined, line 373 casts it, and formatAgentLine at line 374 dereferences agent.id on undefined. The CLI then fails with a TypeError instead of a clear message. Validate the payload before you format it.
🐛 Proposed fix
const payload = await readJsonResponse(response);
if (!response.ok) {
throw new Error(payload && typeof payload.error === "string" ? payload.error : `Agent request failed (${response.status}).`);
}
- const record = payload as unknown as LocalAgentRecord;
+ if (!payload || typeof payload.id !== "string") {
+ throw new Error(`Agent request returned an unexpected response (${response.status}).`);
+ }
+ const record = payload as unknown as LocalAgentRecord;
console.log(formatAgentLine(record));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const payload = await readJsonResponse(response); | |
| if (!response.ok) { | |
| throw new Error(payload && typeof payload.error === "string" ? payload.error : `Agent request failed (${response.status}).`); | |
| } | |
| const record = payload as unknown as LocalAgentRecord; | |
| console.log(formatAgentLine(record)); | |
| const payload = await readJsonResponse(response); | |
| if (!response.ok) { | |
| throw new Error(payload && typeof payload.error === "string" ? payload.error : `Agent request failed (${response.status}).`); | |
| } | |
| if (!payload || typeof payload.id !== "string") { | |
| throw new Error(`Agent request returned an unexpected response (${response.status}).`); | |
| } | |
| const record = payload as unknown as LocalAgentRecord; | |
| console.log(formatAgentLine(record)); |
🤖 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/cli.ts` around lines 369 - 374, Validate the successful `payload`
returned by `readJsonResponse` before casting it to `LocalAgentRecord` or
passing it to `formatAgentLine`; when it is missing or not an object, throw a
clear error instead of allowing `formatAgentLine` to dereference undefined.
Preserve the existing non-OK response error handling.
| const busyManager = { | ||
| get: () => ({ id: "agt_control" }), | ||
| start: fakeManager.start, | ||
| continue: fakeManager.continue, | ||
| } as unknown as LocalAgentManager; | ||
| const busyControl = new LocalAgentControl(stateDir, busyManager); | ||
| const busy = responseFor({ | ||
| "x-devspace-control-token": readLocalAgentControlToken(stateDir)!, | ||
| }, validBody()); | ||
| await busyControl.handle(busy.request, busy.response); | ||
| assert.equal(busy.status, 409); | ||
| assert.equal(continued, 1); | ||
|
|
||
| busyControl.close(); | ||
| control.close(); | ||
| assert.equal(readLocalAgentControlToken(stateDir), undefined); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The two controls share one stateDir, so the token assertions are ambiguous.
Line 59 constructs busyControl against the same stateDir as control. The constructor overwrites agent-control.token, so line 61 reads the busyControl token and the control token is no longer valid. Line 67 then removes the file, and the control.close() at line 68 removes nothing because rmSync uses force: true. The assertion at line 69 therefore passes even if close() did not delete a file.
Use a separate temporary stateDir for busyControl so each token lifecycle is asserted independently.
🤖 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.test.ts` around lines 54 - 69, Use a distinct
temporary state directory when constructing busyControl, while keeping control
on the original stateDir. Read the busy control token from its own directory for
the request, then close busyControl and assert its token is removed separately;
finally close control and assert the original control token is removed.
| async handle(req: Request, res: Response): Promise<void> { | ||
| if (req.method !== "POST") { | ||
| res.status(405).json({ error: "Method not allowed" }); | ||
| return; | ||
| } | ||
| if (!hasMatchingToken(req.header(CONTROL_TOKEN_HEADER), this.token)) { | ||
| res.status(401).json({ error: "Invalid local control token" }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
The "local" control endpoint does not verify that the caller is local. handle validates the method and the token only, so any caller that reaches the server address and holds the token starts agent turns with the local user's authority. The test fake mirrors that gap.
src/local-agent-control.ts#L52-L60: reject callers whosereq.socket.remoteAddressis not a loopback address, before the token check.src/local-agent-control.test.ts#L80-L116: add asocket: { remoteAddress: "127.0.0.1" }stub to the fakeRequest, and add a case that asserts a non-loopback address is rejected.
📍 Affects 2 files
src/local-agent-control.ts#L52-L60(this comment)src/local-agent-control.test.ts#L80-L116
🤖 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 52 - 60, Restrict
LocalAgentControl.handle to loopback callers by validating
req.socket.remoteAddress before the token check, rejecting non-loopback
addresses. In src/local-agent-control.ts#L52-L60, preserve the existing method
and token responses for valid local requests; in
src/local-agent-control.test.ts#L80-L116, add the loopback socket stub to the
fake Request and a test asserting non-loopback requests are rejected.
Source: Coding guidelines
| private buildRunInput( | ||
| record: LocalAgentRecord, | ||
| profile: LocalAgentProfile | undefined, | ||
| prompt: string, | ||
| overrides: RunOverrides, | ||
| ): LocalAgentRunInput { | ||
| const isRawProvider = record.profileName === record.provider; | ||
| if (!profile && !isRawProvider) { | ||
| throw new Error(`Subagent profile not found: ${record.profileName}`); | ||
| } | ||
| const body = profile?.body.trim(); | ||
| const fullPrompt = body ? `${body}\n\nTask:\n${prompt}` : prompt; | ||
| return { | ||
| prompt: fullPrompt, | ||
| workspace: record.workspaceRoot, | ||
| providerSessionId: record.providerSessionId, | ||
| writeMode: overrides.writeMode ?? "allowed", | ||
| model: record.model ?? profile?.model, | ||
| thinking: record.thinking ?? profile?.thinking, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect LocalAgentProfile fields and LocalAgentWriteMode values.
rg -nP -C 3 'interface LocalAgentProfile|type LocalAgentWriteMode' --type=ts
rg -nP -C 2 'writeMode' --type=tsRepository: Waishnav/devspace
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- candidate file outline ---'
ast-grep outline src/local-agent-manager.ts 2>/dev/null || true
printf '%s\n' '--- profile and write-mode declarations/usages ---'
rg -n -C 4 'LocalAgentProfile|LocalAgentWriteMode|writeMode' src --glob '*.ts' --glob '*.tsx' || true
printf '%s\n' '--- related control flow ---'
sed -n '150,270p' src/local-agent-manager.ts
sed -n '1,240p' src/local-agent-control.tsRepository: Waishnav/devspace
Length of output: 32164
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- profile definition and parser ---'
sed -n '1,210p' src/local-agent-profiles.ts
printf '%s\n' '--- manager entry points and target resolution ---'
sed -n '20,125p' src/local-agent-manager.ts
sed -n '1,135p' src/local-agent-targets.ts
printf '%s\n' '--- write-mode propagation and adapter behavior ---'
rg -n -C 5 'writeMode|LocalAgentRunInput|LocalAgentRuntimeContext' src --glob '*.ts'
printf '%s\n' '--- relevant tests ---'
rg -n -C 5 'writeMode|start\\(|continue\\(' src/local-agent-manager.test.ts src/local-agent-control.test.ts src --glob '*test.ts' || trueRepository: Waishnav/devspace
Length of output: 34639
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- provider translation of writeMode ---'
sed -n '45,145p' src/local-agent-adapters.ts
sed -n '165,330p' src/local-agent-adapters.ts
sed -n '333,430p' src/local-agent-adapters.ts
printf '%s\n' '--- CLI and control request construction ---'
rg -n -C 8 'parseLocalAgentRunArgs|LOCAL_AGENT_CONTROL_PATH|LocalAgentControlRequest|writeMode' src --glob '*.ts' --glob '*.tsx' --glob '!local-agent-manager.ts' --glob '!local-agent-adapters.ts'
printf '%s\n' '--- manager tests around input construction ---'
sed -n '80,260p' src/local-agent-manager.test.tsRepository: Waishnav/devspace
Length of output: 29819
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- runtime implementation and SDK option mapping ---'
cat -n src/local-agent-runtime.ts
printf '%s\n' '--- all exact write-mode references, including non-TypeScript files ---'
rg -n -C 6 'writeMode|permissionMode|allowDangerouslySkipPermissions|sandbox|approval' . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' || true
printf '%s\n' '--- CLI request body ---'
sed -n '347,425p' src/cli.ts
printf '%s\n' '--- source-level verifier ---'
python3 - <<'PY'
from pathlib import Path
manager = Path("src/local-agent-manager.ts").read_text()
control = Path("src/local-agent-control.ts").read_text()
targets = Path("src/local-agent-targets.ts").read_text()
profiles = Path("src/local-agent-profiles.ts").read_text()
runtime = Path("src/local-agent-runtime.ts").read_text()
adapters = Path("src/local-agent-adapters.ts").read_text()
def has_field(interface_text, interface_name, field):
start = interface_text.find(f"interface {interface_name}")
if start < 0:
return False
end = interface_text.find("}", start)
return field in interface_text[start:end]
print("profile_writeMode_field =", has_field(profiles, "LocalAgentProfile", "writeMode"))
print("control_request_writeMode_field =", has_field(control, "LocalAgentControlRequest", "writeMode"))
print("cli_parser_writeMode =", "writeMode" in targets)
print("manager_default_allowed =", 'overrides.writeMode ?? "allowed"' in manager)
print("manager_default_read_only =", 'overrides.writeMode ?? "read_only"' in manager)
print("runtime_declares_writeMode =", "writeMode" in runtime)
print("adapter_reads_writeMode =", "input.writeMode" in adapters)
PYRepository: Waishnav/devspace
Length of output: 17378
Enforce restrictive write mode across all provider adapters.
LocalAgentProfile has no writeMode field. Remove that concern. However, the CLI and LocalAgentControlRequest omit writeMode, so buildRunInput defaults to "allowed". Provider adapters also ignore input.writeMode; Claude hard-codes bypassPermissions, and ACP selects an allow option. Use "read_only" as the default and translate the mode explicitly in each adapter.
🤖 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-manager.ts` around lines 230 - 250, Update buildRunInput to
default writeMode to "read_only" without reading it from LocalAgentProfile.
Ensure every provider adapter consumes input.writeMode and explicitly maps
"read_only" to restrictive permissions, including Claude’s permission mode and
ACP’s allow option, while preserving the existing behavior for other supported
modes.
ce23beb to
42c5bd1
Compare
Waishnav
changed the title
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli.ts (1)
337-350: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winUse
LocalAgentStorefor read-only agent commands.SQLite uses WAL mode, so direct
listandgetreads can run while the daemon writes. The daemon startup also callsreconcileActiveRuns(), which changesstartingandrunningrecords toerror. Keep the daemon client forrunandcontinue, but avoid spawning it fromagents lsandagents show.🤖 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/cli.ts` around lines 337 - 350, Update the read-only agents ls/show command handlers, including runAgentsList, to use LocalAgentStore directly for list/get operations instead of createLocalAgentClient, while preserving workspace scoping and output behavior. Keep the daemon-backed client unchanged for agents run and continue commands.
🧹 Nitpick comments (17)
src/cli.test.ts (2)
86-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the protocol version instead of hardcoding
1.The mock daemon hardcodes
protocolVersion: 1in both thehelloresult and the response envelope.LocalAgentClientsendsLOCAL_AGENT_DAEMON_PROTOCOL_VERSIONand the daemon rejects a mismatch. When that constant is bumped, this test fails with an opaque protocol error rather than a clear signal.The file already imports from
./local-agent-daemon-lifecycle.js. Add the constant to that import.♻️ Proposed change
-import { localAgentDaemonPaths } from "./local-agent-daemon-lifecycle.js"; +import { + LOCAL_AGENT_DAEMON_PROTOCOL_VERSION, + localAgentDaemonPaths, +} from "./local-agent-daemon-lifecycle.js";- protocolVersion: 1, + protocolVersion: LOCAL_AGENT_DAEMON_PROTOCOL_VERSION,- protocolVersion: 1, + protocolVersion: LOCAL_AGENT_DAEMON_PROTOCOL_VERSION,🤖 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/cli.test.ts` around lines 86 - 102, Update the existing import from local-agent-daemon-lifecycle to include LOCAL_AGENT_DAEMON_PROTOCOL_VERSION, then replace both hardcoded protocolVersion: 1 values in the mock daemon’s hello result and response envelope with that constant.
110-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a timeout to the CLI invocation and use substring assertions.
execFileAsyncruns without atimeoutoption. If the CLI blocks, for example because the mock daemon does not answer a method, the test hangs until the test runner kills the whole process. Add a bounded timeout so the failure is attributable.The assertions build regular expressions from
current.idandother.id. Those ids are generated values. If the id format ever includes a regex metacharacter, the match silently changes meaning. Substring checks express the intent directly and remove the static analysis warning about a non-literal regular expression.💚 Proposed change
const { stdout: output } = await execFileAsync("node", ["--import", "tsx", "src/cli.ts", "agents", "ls"], { cwd: process.cwd(), encoding: "utf8", + timeout: 30_000, env: {- assert.match(output, new RegExp(`${current.id} idle reviewer codex gpt-5\\.4 thinking=high`)); + assert.ok(output.includes(`${current.id} idle reviewer codex gpt-5.4 thinking=high`)); assert.doesNotMatch(output, /profile reviewer/); - assert.doesNotMatch(output, new RegExp(other.id)); + assert.ok(!output.includes(other.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/cli.test.ts` around lines 110 - 128, Update the execFileAsync invocation in the CLI test to include a bounded timeout, ensuring blocked commands fail within the test rather than hanging. Replace the dynamic RegExp assertions involving current.id and other.id with substring-based assertions while preserving the existing positive and negative output checks.Source: Linters/SAST tools
docs/gotchas.md (1)
228-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the daemon troubleshooting commands to this page.
The statement is accurate. This page documents recovery steps, and the new daemon adds a failure mode that it does not cover.
spawnLocalAgentDaemoninsrc/local-agent-client.tsusesstdio: "ignore", so a failed daemon start produces no console output. The user sees only aDAEMON_START_FAILEDmessage after the startup timeout.Point the reader at the inspection commands that this PR adds.
📝 Proposed addition
Those commands automatically manage the internal local agent daemon; `devspace serve` is not a prerequisite. +If an agent command reports that the daemon failed to start, inspect it with +`devspace agents daemon status` and `devspace agents daemon logs`, then restart +it with `devspace agents daemon stop`.🤖 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 `@docs/gotchas.md` around lines 228 - 229, Add daemon troubleshooting guidance to the recovery steps in gotchas.md, referencing the inspection commands introduced by this PR so users can diagnose silent spawnLocalAgentDaemon startup failures and DAEMON_START_FAILED timeouts. Keep the existing statement about devspace serve unchanged.src/local-agent-client.ts (2)
216-232: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the data handler with
settledand cap the buffer.Two small gaps:
- The handler does not check
settled. After the success path resolves at Line 227, a furtherdataevent re-enters the parse and callsresolveagain. The extra call is a no-op, but the parse work and the possible throw path run needlessly.bufferhas no size limit. The daemon caps requests atMAX_REQUEST_BYTES, but the client accepts an unbounded response with no newline. Only the timeout bounds it, and it bounds time rather than memory.♻️ Proposed refactor
socket.on("data", (chunk: string | Buffer) => { + if (settled) return; buffer += chunk.toString(); + if (Buffer.byteLength(buffer, "utf8") > MAX_RESPONSE_BYTES) { + finish(new LocalAgentDaemonClientError("RESPONSE_TOO_LARGE", "Daemon response is too large."), true); + return; + } const newline = buffer.indexOf("\n");🤖 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-client.ts` around lines 216 - 232, Update the socket data handler around the data callback to return immediately when settled is already true, preventing post-completion parsing. Also enforce a response buffer size limit consistent with MAX_REQUEST_BYTES, rejecting via finish when accumulated data exceeds that limit before newline parsing.
101-107: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReadiness is never cached, so every request costs an extra
helloround trip.ensureReadymemoizesstartupPromiseonly for the duration of the call and clears it in thefinallyblock. Every laterrequesttherefore re-entersensureReadyInternal, which always performs atryHelloagainst the daemon before the real request runs.
src/local-agent-client.ts#L101-L107: cache the resolvedLocalAgentDaemonStatusafter a successful readiness check, and clear the cached value only when a request fails withDAEMON_UNAVAILABLEor a connection error. Keep clearingstartupPromiseon rejection so a failed start can be retried.src/cli.ts#L374-L381: after the client caches readiness, this 500 ms poll loop issues one request per iteration instead of two, which halves the socket connections over the 15-second window. No change is needed here once the client caches.🤖 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-client.ts` around lines 101 - 107, Update src/local-agent-client.ts lines 101-107 in ensureReady to cache the resolved LocalAgentDaemonStatus after successful readiness checks, while retaining startupPromise cleanup on rejection for retryability; invalidate the cached status only when requests fail with DAEMON_UNAVAILABLE or a connection error. No direct change is needed in src/cli.ts lines 374-381 because the client-side cache makes the existing poll loop issue only one request per iteration.src/local-agent-daemon.ts (2)
291-296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace message substring matching with typed errors.
errorCodederives the wire error code from error message text. The message forCONFLICTis owned bysrc/local-agent-manager.ts, and the message forDAEMON_STOPPINGis owned by Line 205 of this file. A wording change in either place silently downgrades the code toAGENT_ERROR. Clients then cannot distinguish a per-agent concurrency conflict from a provider failure.Export a typed error from the manager, for example
LocalAgentTurnConflictError, and aLocalAgentDaemonStoppingErrorhere. Match oninstanceof. This makes the failure contract explicit in types rather than in prose.As per coding guidelines: "Represent important behavior through schemas, types, checks, or explicit tool results rather than hidden prompt conventions."
🤖 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-daemon.ts` around lines 291 - 296, Update errorCode to use typed errors instead of errorMessage substring matching: export and throw LocalAgentTurnConflictError from the local-agent-manager flow, define and throw LocalAgentDaemonStoppingError for the daemon-stopping path in this file, and map both via instanceof to CONFLICT and DAEMON_STOPPING while preserving LocalAgentDaemonProtocolError handling and the AGENT_ERROR fallback.Source: Coding guidelines
298-320: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the daemon log size.
writeLocalAgentDaemonLogappends without any size cap or rotation. The daemon is long-lived and logs every start, stop, and idle-check failure. The file grows without limit in the state directory.readLocalAgentDaemonLogsthen reads the whole file into memory on eachdaemon.logsrequest to return the last N lines.Add a size check before the append and rotate to a single
.1file when the limit is exceeded.Note also that Line 306 spreads
fieldsafterlevelandevent. A caller field namedleveloreventoverwrites the structured value. Spreadfieldsfirst if you want the fixed keys to win.🤖 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-daemon.ts` around lines 298 - 320, Update writeLocalAgentDaemonLog to cap the daemon log size before appending: when paths.logPath reaches the configured limit, rotate it to a single .1 file and then continue writing the new entry, preserving the existing failure-safe behavior. Build the structured log object with fields spread before the fixed at, level, and event keys so caller fields cannot overwrite them; keep readLocalAgentDaemonLogs focused on reading the bounded active log.src/local-agent-daemon.test.ts (1)
66-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the error and log paths.
The test covers the happy path only. The daemon has several branches that no test exercises: the
PROTOCOL_MISMATCHrejection indispatch, theREQUEST_TOO_LARGEguard inhandleConnection, theerrorCodemapping toCONFLICT, anddaemon.logs. Those branches carry the failure contract thatLocalAgentClientdepends on.Add cases that send a request with a wrong
protocolVersion, send a payload overMAX_REQUEST_BYTES, and callclient.logs()after a start.🤖 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-daemon.test.ts` around lines 66 - 82, Extend the tests around the existing LocalAgentClient/daemon happy path to cover protocolVersion mismatch rejection in dispatch, oversized payload rejection against MAX_REQUEST_BYTES in handleConnection, and errorCode mapping to CONFLICT. Also call client.logs() after a successful start and assert the expected daemon logs, preserving the existing cleanup and failure-contract assertions.package.json (1)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm the runtime pool has a dedicated test suite.
The script adds
local-agent-daemon-lifecycle,local-agent-daemon-protocol,local-agent-daemon,local-agent-adapters, andlocal-agent-manager. Nolocal-agent-runtime-pool.test.tsentry appears.LocalAgentRuntimePoolowns idle eviction, crash recovery, and concurrent-run accounting, so it needs direct coverage.Separately, this
&&chain now holds 28 entries. A glob-driven runner keeps new suites from being forgotten.#!/bin/bash # List all test files and report which ones the test script omits. fd -e ts -g '*.test.ts' src | sort > /tmp/all.txt jq -r '.scripts.test' package.json | rg -o 'src/[A-Za-z0-9._/-]+\.test\.ts' | sort -u > /tmp/listed.txt echo '--- test files not referenced by the test script ---' comm -23 /tmp/all.txt /tmp/listed.txt🤖 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 `@package.json` at line 32, Update the package test script to include src/local-agent-runtime-pool.test.ts so LocalAgentRuntimePool receives direct coverage. Replace the long explicit &&-chained test list with a glob-driven runner that automatically discovers all src/*.test.ts files, preventing future suites from being omitted.src/local-agent-manager.ts (1)
120-137: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDrain active turns before you close the runtime pool.
closeawaitsthis.pool.close()and the active turns in the samePromise.allSettled. The pool therefore tears down runtimes while turns still run. Those turns fail and persiststatus: "error"even when they were about to complete. The daemon already applies a bounded force-exit timer insrc/local-agent-daemon-main.ts, so a sequential drain does not risk an unbounded wait.♻️ Proposed sequencing
this.closePromise = (async () => { - const results = await Promise.allSettled([ - this.pool.close(), - ...turns, - ]); + const results = await Promise.allSettled(turns); + results.push(...(await Promise.allSettled([this.pool.close()]))); for (const result of results) {🤖 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-manager.ts` around lines 120 - 137, Update LocalAgentManager.close so it first awaits all active turns from activeTurns, then closes this.pool only after those turns have drained. Preserve the existing Promise.allSettled rejection logging and closePromise idempotency, while ensuring pool shutdown cannot interrupt in-flight turns.src/local-agent-daemon-lifecycle.ts (1)
28-44: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider the Unix socket path length limit.
macOS limits
sun_pathto 104 bytes and Linux to 108 bytes. IfstateDiris deep,listenonsocketPathfails with an opaqueEINVALorENAMETOOLONG. A length check here produces a clear error, or the endpoint can fall back to a short path underos.tmpdir().🤖 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-daemon-lifecycle.ts` around lines 28 - 44, Update localAgentDaemonPaths to handle Unix socket path length limits before returning socketPath: validate the resolved socket path against the platform limit and produce a clear error or select a deterministic short fallback under os.tmpdir(). Preserve the existing Windows named-pipe endpoint and path fields, and ensure endpoint uses the validated Unix socket path.src/local-agent-daemon-main.ts (1)
38-52: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReport a failed shutdown instead of exiting with code 0.
.finally(() => process.exit(0))runs for both a resolved and a rejecteddaemon.close(). A close failure, for example a socket or lock cleanup error, is discarded and the daemon reports success. Log the error and exit with a non-zero code.♻️ Proposed change
forceTimer.unref(); - void daemon.close().finally(() => process.exit(0)); + void daemon.close().then( + () => process.exit(0), + (error) => { + log("error", "daemon_close_failed", { + error: error instanceof Error ? error.message : String(error), + }); + process.exit(1); + }, + ); };🤖 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-daemon-main.ts` around lines 38 - 52, Update the shutdown flow in shutdown so daemon.close() failures are caught, logged with the error details using the existing log helper, and followed by process.exit(1); retain process.exit(0) only for successful closure and preserve the forced-shutdown timer behavior.src/local-agent-daemon-lifecycle.test.ts (1)
28-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe stale-PID recovery branch is not exercised.
Line 26 calls
lock.release(), which removes bothpidPathandlockPath. Line 28 then writes onlypidPath.recovered.acquire()therefore succeeds through the normalopenSync(lockPath, "wx")path. TheEEXISTrecovery branch inLocalAgentDaemonLock.acquirenever runs.Write a stale
lockPathas well so the recovery path is covered. Add a negative case forisProcessAlivewith a dead PID.💚 Proposed test change
+ await writeFile(paths.lockPath, "999999\n", { mode: 0o600 }); await writeFile(paths.pidPath, "999999\n", { mode: 0o600 }); + assert.equal(isProcessAlive(999999), false); const recovered = new LocalAgentDaemonLock(paths); recovered.acquire();This case also demonstrates the PID-source problem raised on
src/local-agent-daemon-lifecycle.tslines 65-85. If you write onlylockPathand omitpidPath, the current implementation deletes a live daemon's lock.🤖 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-daemon-lifecycle.test.ts` around lines 28 - 35, Update the stale-PID recovery test around LocalAgentDaemonLock.acquire to create both the stale pidPath and lockPath after the initial lock is released, ensuring the EEXIST recovery branch executes. Add an assertion that isProcessAlive returns false for the dead PID, while preserving the existing live-PID assertions and cleanup.src/local-agent-daemon-protocol.test.ts (1)
25-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the remaining protocol validation branches.
The current tests cover a valid
agent.start, oneINVALID_PARAMScase, a record decode, and a success response. These branches remain untested:
UNKNOWN_METHODfor an unrecognizedmethod.decodeWriteModerejection for an unsupported mode.decodeLogsParamsbounds, for examplelines: 0andlines: 10001.decodeEmptyParamsrejection whenhellocarries parameters.- The
ok: falseresponse path andINVALID_RESPONSE.🤖 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-daemon-protocol.test.ts` around lines 25 - 52, Extend the protocol tests around decodeLocalAgentDaemonRequest and related decoders to cover UNKNOWN_METHOD, unsupported decodeWriteMode values, decodeLogsParams boundaries at lines 0 and 10001, and decodeEmptyParams rejecting parameters on hello. Also add assertions for the ok: false response path and INVALID_RESPONSE, verifying each throws or decodes with the expected LocalAgentDaemonProtocolError code.src/local-agent-manager.test.ts (1)
100-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the assertions in
try/finallyso the temporary directory is always removed.If any assertion fails, line 140 never runs and the temporary directory stays in
tmpdir().src/local-agent-daemon-lifecycle.test.tsalready uses this pattern.🤖 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-manager.test.ts` around lines 100 - 140, Wrap the test body in a try/finally block so cleanup always runs, moving the existing rm(root, { recursive: true, force: true }) call into finally. Keep the manager.close() calls and all assertions in the try block, using the root temporary-directory variable created by the test.src/local-agent-adapters.ts (2)
84-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound
thread.runwith anAbortSignal. Use anAbortControllertimeout so a hung provider run is cancelled and removed fromactiveTurnsbefore daemon shutdown.🤖 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-adapters.ts` around lines 84 - 97, Update the provider run flow around thread.run in the local agent adapter to create an AbortController with a timeout, pass its signal to thread.run, and clear the timeout when the run completes or fails. Ensure the abort path cancels the hung run and removes the corresponding turn from activeTurns before daemon shutdown, while preserving the existing response mapping.
109-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
thinkingbefore casting it toModelReasoningEffort. Mapminimal,low,medium,high, andxhighexplicitly, then reject or omit unknown values before forwarding them to Codex.🤖 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-adapters.ts` around lines 109 - 117, Update threadOptionsFor and add validation for input.thinking before assigning modelReasoningEffort: explicitly accept only minimal, low, medium, high, and xhigh, and reject or omit unknown values instead of forwarding an unchecked cast to Codex.
🤖 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/local-agent-daemon.md`:
- Around line 30-33: Update LocalAgentDaemonLock.acquire in
src/local-agent-daemon-lifecycle.ts to read the recorded PID from the same lock
file it writes, ensuring a missing pidPath cannot cause removal of a live lock;
preserve stale-file recovery only when that PID is no longer alive, and keep the
documentation aligned with the enforced single-owner guarantee.
In `@src/cli.ts`:
- Around line 374-381: Update LocalAgentClient.ensureReady so a successful
readiness check is cached and reused on subsequent client.get calls, rather than
issuing another hello request each time. Preserve retry/error behavior for
unsuccessful readiness checks, and keep the polling behavior in the CLI loop
unchanged.
- Around line 405-411: Update the “stop” case in the CLI command handler to
report that shutdown was requested or is in progress rather than claiming the
daemon has stopped. Update the “logs” case to provide explicit user feedback
when client.logs() returns an empty string, while preserving the existing output
for non-empty logs.
In `@src/local-agent-client.ts`:
- Around line 93-99: Update stop and logs to use the request path that skips
ensureReady, preventing either operation from spawning the daemon. When no
daemon is reachable, return the established clear “daemon is not running” result
while preserving normal decoding and behavior when the daemon is already
running.
- Around line 177-192: Update spawnLocalAgentDaemon to filter inspector-related
flags from process.execArgv before constructing the detached child arguments,
excluding --inspect, --inspect-brk, and forms with =... while preserving all
other runtime arguments. Keep resolveDaemonEntrypoint unchanged.
In `@src/local-agent-daemon-lifecycle.ts`:
- Around line 65-85: Update LocalAgentDaemonLock.acquire in
src/local-agent-daemon-lifecycle.ts (lines 65-85) to read the owning PID from
lockPath first, falling back to pidPath, in both the EEXIST retry branch and
final AlreadyRunning error; retain the single-owner statement in
docs/local-agent-daemon.md (lines 30-33) after the fix; update
src/local-agent-daemon-lifecycle.test.ts (lines 28-35) to create a stale
lockPath alongside pidPath and add coverage for lockPath existing while pidPath
is absent.
In `@src/local-agent-daemon-main.ts`:
- Around line 56-67: Constructing LocalAgentManager triggers
store.reconcileActiveRuns before daemon.start acquires the stateDir lock,
allowing a competing process to mutate the live daemon’s runs. Acquire the
daemon lock before constructing LocalAgentManager, its store, and the daemon;
alternatively, move reconcileActiveRuns out of the constructor into an explicit
post-start lifecycle call while preserving reconciliation only after successful
ownership acquisition.
In `@src/local-agent-daemon.test.ts`:
- Around line 59-64: Update the fire-and-forget callbacks creating the
LocalAgentClient instances to attach a rejection handler to daemon.start(),
including both daemon and idleDaemon cases. Preserve the asynchronous startup
behavior while routing rejected starts through the client’s expected
DAEMON_START_FAILED handling instead of leaving unhandled rejections.
In `@src/local-agent-daemon.ts`:
- Around line 80-84: Update the Windows endpoint setup in the daemon startup
flow around createServer, listen, and localAgentDaemonPaths so the named pipe is
access-restricted to the intended local user before accepting connections. Use
the platform’s supported named-pipe security mechanism or add request
authentication backed by a secret stored in the protected state directory; do
not leave the default pipe security descriptor in place.
- Around line 131-146: In the shutdown flow around closeServer and this.sockets,
destroy all tracked client sockets and clear this.sockets before awaiting
Promise.allSettled. Keep the existing server and manager close calls and
rejection logging unchanged, ensuring closeServer can resolve even when clients
have not sent a newline.
- Around line 155-176: Update handleConnection to apply a read timeout to each
accepted socket using a dedicated timeout constant defined alongside the
existing limits, and destroy the socket when the timeout fires. Preserve the
current request parsing and cleanup behavior while ensuring idle connections
cannot keep this.sockets populated indefinitely.
- Around line 78-104: Update the startup flow in the daemon start method around
this.lock.acquire() to track whether this instance owns the lock, and only
release it and call removeLocalAgentDaemonFiles(this.paths) when ownership was
obtained. Ensure the created server is explicitly closed if listen fails,
including when server.listening is false, using the available closeServer
behavior or an equivalent direct close. Preserve propagation of startup errors
without the redundant LocalAgentDaemonAlreadyRunningError conditional rethrow.
---
Outside diff comments:
In `@src/cli.ts`:
- Around line 337-350: Update the read-only agents ls/show command handlers,
including runAgentsList, to use LocalAgentStore directly for list/get operations
instead of createLocalAgentClient, while preserving workspace scoping and output
behavior. Keep the daemon-backed client unchanged for agents run and continue
commands.
---
Nitpick comments:
In `@docs/gotchas.md`:
- Around line 228-229: Add daemon troubleshooting guidance to the recovery steps
in gotchas.md, referencing the inspection commands introduced by this PR so
users can diagnose silent spawnLocalAgentDaemon startup failures and
DAEMON_START_FAILED timeouts. Keep the existing statement about devspace serve
unchanged.
In `@package.json`:
- Line 32: Update the package test script to include
src/local-agent-runtime-pool.test.ts so LocalAgentRuntimePool receives direct
coverage. Replace the long explicit &&-chained test list with a glob-driven
runner that automatically discovers all src/*.test.ts files, preventing future
suites from being omitted.
In `@src/cli.test.ts`:
- Around line 86-102: Update the existing import from
local-agent-daemon-lifecycle to include LOCAL_AGENT_DAEMON_PROTOCOL_VERSION,
then replace both hardcoded protocolVersion: 1 values in the mock daemon’s hello
result and response envelope with that constant.
- Around line 110-128: Update the execFileAsync invocation in the CLI test to
include a bounded timeout, ensuring blocked commands fail within the test rather
than hanging. Replace the dynamic RegExp assertions involving current.id and
other.id with substring-based assertions while preserving the existing positive
and negative output checks.
In `@src/local-agent-adapters.ts`:
- Around line 84-97: Update the provider run flow around thread.run in the local
agent adapter to create an AbortController with a timeout, pass its signal to
thread.run, and clear the timeout when the run completes or fails. Ensure the
abort path cancels the hung run and removes the corresponding turn from
activeTurns before daemon shutdown, while preserving the existing response
mapping.
- Around line 109-117: Update threadOptionsFor and add validation for
input.thinking before assigning modelReasoningEffort: explicitly accept only
minimal, low, medium, high, and xhigh, and reject or omit unknown values instead
of forwarding an unchecked cast to Codex.
In `@src/local-agent-client.ts`:
- Around line 216-232: Update the socket data handler around the data callback
to return immediately when settled is already true, preventing post-completion
parsing. Also enforce a response buffer size limit consistent with
MAX_REQUEST_BYTES, rejecting via finish when accumulated data exceeds that limit
before newline parsing.
- Around line 101-107: Update src/local-agent-client.ts lines 101-107 in
ensureReady to cache the resolved LocalAgentDaemonStatus after successful
readiness checks, while retaining startupPromise cleanup on rejection for
retryability; invalidate the cached status only when requests fail with
DAEMON_UNAVAILABLE or a connection error. No direct change is needed in
src/cli.ts lines 374-381 because the client-side cache makes the existing poll
loop issue only one request per iteration.
In `@src/local-agent-daemon-lifecycle.test.ts`:
- Around line 28-35: Update the stale-PID recovery test around
LocalAgentDaemonLock.acquire to create both the stale pidPath and lockPath after
the initial lock is released, ensuring the EEXIST recovery branch executes. Add
an assertion that isProcessAlive returns false for the dead PID, while
preserving the existing live-PID assertions and cleanup.
In `@src/local-agent-daemon-lifecycle.ts`:
- Around line 28-44: Update localAgentDaemonPaths to handle Unix socket path
length limits before returning socketPath: validate the resolved socket path
against the platform limit and produce a clear error or select a deterministic
short fallback under os.tmpdir(). Preserve the existing Windows named-pipe
endpoint and path fields, and ensure endpoint uses the validated Unix socket
path.
In `@src/local-agent-daemon-main.ts`:
- Around line 38-52: Update the shutdown flow in shutdown so daemon.close()
failures are caught, logged with the error details using the existing log
helper, and followed by process.exit(1); retain process.exit(0) only for
successful closure and preserve the forced-shutdown timer behavior.
In `@src/local-agent-daemon-protocol.test.ts`:
- Around line 25-52: Extend the protocol tests around
decodeLocalAgentDaemonRequest and related decoders to cover UNKNOWN_METHOD,
unsupported decodeWriteMode values, decodeLogsParams boundaries at lines 0 and
10001, and decodeEmptyParams rejecting parameters on hello. Also add assertions
for the ok: false response path and INVALID_RESPONSE, verifying each throws or
decodes with the expected LocalAgentDaemonProtocolError code.
In `@src/local-agent-daemon.test.ts`:
- Around line 66-82: Extend the tests around the existing
LocalAgentClient/daemon happy path to cover protocolVersion mismatch rejection
in dispatch, oversized payload rejection against MAX_REQUEST_BYTES in
handleConnection, and errorCode mapping to CONFLICT. Also call client.logs()
after a successful start and assert the expected daemon logs, preserving the
existing cleanup and failure-contract assertions.
In `@src/local-agent-daemon.ts`:
- Around line 291-296: Update errorCode to use typed errors instead of
errorMessage substring matching: export and throw LocalAgentTurnConflictError
from the local-agent-manager flow, define and throw
LocalAgentDaemonStoppingError for the daemon-stopping path in this file, and map
both via instanceof to CONFLICT and DAEMON_STOPPING while preserving
LocalAgentDaemonProtocolError handling and the AGENT_ERROR fallback.
- Around line 298-320: Update writeLocalAgentDaemonLog to cap the daemon log
size before appending: when paths.logPath reaches the configured limit, rotate
it to a single .1 file and then continue writing the new entry, preserving the
existing failure-safe behavior. Build the structured log object with fields
spread before the fixed at, level, and event keys so caller fields cannot
overwrite them; keep readLocalAgentDaemonLogs focused on reading the bounded
active log.
In `@src/local-agent-manager.test.ts`:
- Around line 100-140: Wrap the test body in a try/finally block so cleanup
always runs, moving the existing rm(root, { recursive: true, force: true }) call
into finally. Keep the manager.close() calls and all assertions in the try
block, using the root temporary-directory variable created by the test.
In `@src/local-agent-manager.ts`:
- Around line 120-137: Update LocalAgentManager.close so it first awaits all
active turns from activeTurns, then closes this.pool only after those turns have
drained. Preserve the existing Promise.allSettled rejection logging and
closePromise idempotency, while ensuring pool shutdown cannot interrupt
in-flight turns.
🪄 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: b9787bc2-a757-4531-bdec-c8f5ea2d737b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
docs/agent-profile-schema.mddocs/gotchas.mddocs/local-agent-daemon.mdpackage.jsonskills/subagent-delegation/SKILL.mdsrc/cli.test.tssrc/cli.tssrc/local-agent-adapters.tssrc/local-agent-client.tssrc/local-agent-daemon-lifecycle.test.tssrc/local-agent-daemon-lifecycle.tssrc/local-agent-daemon-main.tssrc/local-agent-daemon-protocol.test.tssrc/local-agent-daemon-protocol.tssrc/local-agent-daemon.test.tssrc/local-agent-daemon.tssrc/local-agent-manager.test.tssrc/local-agent-manager.tssrc/local-agent-store.ts
| let record = await client.get(id); | ||
| if (!record) throw new Error(`Unknown subagent id: ${id}`); | ||
|
|
||
| const deadline = Date.now() + 15_000; | ||
| while ((record.status === "starting" || record.status === "running") && Date.now() < deadline) { | ||
| await sleep(500); | ||
| record = store.get(id) ?? record; | ||
| record = await client.get(id) ?? record; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
The poll loop pays a duplicate round trip on every iteration.
Each client.get call runs ensureReady first, and ensureReady never caches a successful result. The loop therefore performs a hello request and a get request per iteration, that is up to 60 socket connections over the 15-second window. The root cause is the readiness caching in src/local-agent-client.ts at Lines 101-107. See the consolidated comment.
🤖 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/cli.ts` around lines 374 - 381, Update LocalAgentClient.ensureReady so a
successful readiness check is cached and reused on subsequent client.get calls,
rather than issuing another hello request each time. Preserve retry/error
behavior for unsuccessful readiness checks, and keep the polling behavior in the
CLI loop unchanged.
| case "stop": | ||
| await client.stop(); | ||
| console.log("Local agent daemon stopped."); | ||
| return; | ||
| case "logs": | ||
| console.log(await client.logs()); | ||
| return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The stop message overstates the result, and empty logs give no feedback.
client.stop returns after the daemon acknowledges the request. The daemon sets stopping and schedules close() with setImmediate in src/local-agent-daemon.ts at Line 191. The shutdown is therefore still in progress when the CLI prints "Local agent daemon stopped."
client.logs() returns an empty string when the log file is missing or unreadable, because readLocalAgentDaemonLogs swallows the error. The user then sees a blank line.
♻️ Proposed change
case "stop":
await client.stop();
- console.log("Local agent daemon stopped.");
+ console.log("Local agent daemon stop requested.");
return;
case "logs":
- console.log(await client.logs());
+ const output = await client.logs();
+ console.log(output === "" ? "No local agent daemon logs found." : output);
return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "stop": | |
| await client.stop(); | |
| console.log("Local agent daemon stopped."); | |
| return; | |
| case "logs": | |
| console.log(await client.logs()); | |
| return; | |
| case "stop": | |
| await client.stop(); | |
| console.log("Local agent daemon stop requested."); | |
| return; | |
| case "logs": | |
| const output = await client.logs(); | |
| console.log(output === "" ? "No local agent daemon logs found." : output); | |
| return; |
🤖 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/cli.ts` around lines 405 - 411, Update the “stop” case in the CLI command
handler to report that shutdown was requested or is in progress rather than
claiming the daemon has stopped. Update the “logs” case to provide explicit user
feedback when client.logs() returns an empty string, while preserving the
existing output for non-empty logs.
| async stop(): Promise<LocalAgentDaemonStatus> { | ||
| return decodeDaemonStatus(await this.request("daemon.stop", {})); | ||
| } | ||
|
|
||
| async logs(lines = 200): Promise<string> { | ||
| return decodeDaemonLogs(await this.request("daemon.logs", { lines })); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
stop and logs start a daemon that is not running.
Both methods call this.request, and request calls ensureReady at Line 159. ensureReady spawns the daemon when none is reachable. If no daemon runs, devspace agents daemon stop starts one, waits for it to become ready, and then stops it. devspace agents daemon logs starts one for a read that only touches a log file.
Do not require readiness for these two operations. Report a clear "daemon is not running" result instead.
🐛 Proposed fix
async stop(): Promise<LocalAgentDaemonStatus> {
- return decodeDaemonStatus(await this.request("daemon.stop", {}));
+ return decodeDaemonStatus(await this.requestWithoutStart("daemon.stop", {}));
}
async logs(lines = 200): Promise<string> {
- return decodeDaemonLogs(await this.request("daemon.logs", { lines }));
+ return decodeDaemonLogs(await this.requestWithoutStart("daemon.logs", { lines }));
}Add the variant that skips ensureReady:
+ private async requestWithoutStart<M extends LocalAgentDaemonRequest["method"]>(
+ method: M,
+ params: Extract<LocalAgentDaemonRequest, { method: M }>['params'],
+ ): Promise<unknown> {
+ const response = await sendRequest(this.endpoint, {
+ requestId: randomUUID(),
+ protocolVersion: LOCAL_AGENT_DAEMON_PROTOCOL_VERSION,
+ method,
+ params,
+ } as LocalAgentDaemonRequest, this.requestTimeoutMs);
+ if (!response.ok) throw new LocalAgentDaemonClientError(response.error.code, response.error.message);
+ return response.result;
+ }🤖 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-client.ts` around lines 93 - 99, Update stop and logs to use
the request path that skips ensureReady, preventing either operation from
spawning the daemon. When no daemon is reachable, return the established clear
“daemon is not running” result while preserving normal decoding and behavior
when the daemon is already running.
| export function spawnLocalAgentDaemon(stateDir: string, env: NodeJS.ProcessEnv = process.env): void { | ||
| const entrypoint = resolveDaemonEntrypoint(); | ||
| const child = spawn(process.execPath, [...process.execArgv, entrypoint], { | ||
| detached: true, | ||
| stdio: "ignore", | ||
| windowsHide: true, | ||
| env: { ...env, DEVSPACE_STATE_DIR: stateDir }, | ||
| }); | ||
| child.unref(); | ||
| } | ||
|
|
||
| export function resolveDaemonEntrypoint(): string { | ||
| const compiled = fileURLToPath(new URL("./local-agent-daemon-main.js", import.meta.url)); | ||
| if (existsSync(compiled)) return compiled; | ||
| return fileURLToPath(new URL("./local-agent-daemon-main.ts", import.meta.url)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check published files, build output location, and daemon bin wiring.
fd '^package.json$' --max-depth 2 --exec jq '{files, bin, main, exports, scripts}' {}
fd '^tsconfig(\..*)?\.json$' --max-depth 2 --exec jq '.compilerOptions.outDir, .compilerOptions.rootDir' {}
rg -n 'local-agent-daemon-main' --type ts --type jsonRepository: Waishnav/devspace
Length of output: 1928
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package/build configuration ---'
cat -n package.json | sed -n '1,180p'
for f in tsconfig.build.json tsconfig.json; do
if [ -f "$f" ]; then
printf '%s\n' "--- $f ---"
cat -n "$f"
fi
done
printf '%s\n' '--- daemon entrypoint references ---'
rg -n -C 4 'local-agent-daemon-main|resolveDaemonEntrypoint|spawnLocalAgentDaemon|process\.execArgv|DAEMON_START_FAILED' src package.json tsconfig*.json
printf '%s\n' '--- tracked source and dist candidates ---'
git ls-files | rg '(^|/)(local-agent-daemon-main|local-agent-client)(\.[^.]+)?$|(^|/)dist/' || trueRepository: Waishnav/devspace
Length of output: 7849
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- client startup and daemon command path ---'
cat -n src/local-agent-client.ts | sed -n '1,215p'
printf '%s\n' '--- CLI daemon path ---'
rg -n -C 8 'agents daemon|daemon|createLocalAgentClient|spawnLocalAgentDaemon' src/cli.ts src/*.test.ts
printf '%s\n' '--- standalone Node execArgv probe ---'
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat > "$tmpdir/child.mjs" <<'JS'
console.log(JSON.stringify(process.argv.slice(1)));
JS
for flag in --inspect --inspect-brk; do
timeout 2s node "$flag=127.0.0.1:0" "$tmpdir/child.mjs" >"$tmpdir/$flag.out" 2>"$tmpdir/$flag.err" || true
printf '%s stdout: ' "$flag"; cat "$tmpdir/$flag.out"
printf '%s stderr: ' "$flag"; sed 's/^/ /' "$tmpdir/$flag.err"
doneRepository: Waishnav/devspace
Length of output: 27235
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
from subprocess import Popen, PIPE, TimeoutExpired
import tempfile
with tempfile.TemporaryDirectory() as d:
root = Path(d)
(root / "child.mjs").write_text("console.log('DAEMON_ENTRYPOINT_REACHED');\n")
(root / "parent.mjs").write_text(
"import { spawn } from 'node:child_process';\n"
"const child = spawn(process.execPath, [...process.execArgv, new URL('./child.mjs', import.meta.url)], "
"{ stdio: ['ignore', 'pipe', 'pipe'] });\n"
"child.stdout.pipe(process.stdout); child.stderr.pipe(process.stderr);\n"
"child.on('exit', (code, signal) => { console.log(JSON.stringify({code, signal})); process.exit(0); });\n"
)
for flag in ("--inspect=127.0.0.1:19321", "--inspect-brk=127.0.0.1:19322"):
proc = Popen(["node", flag, str(root / "parent.mjs")], stdout=PIPE, stderr=PIPE, text=True)
try:
stdout, stderr = proc.communicate(timeout=2)
print(f"{flag} exit={proc.returncode}")
print("stdout:", stdout.strip())
print("stderr:", stderr.strip())
except TimeoutExpired:
proc.kill()
stdout, stderr = proc.communicate()
print(f"{flag} timed_out")
print("stdout:", stdout.strip())
print("stderr:", stderr.strip())
PYRepository: Waishnav/devspace
Length of output: 1385
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
from subprocess import Popen, PIPE
import tempfile
with tempfile.TemporaryDirectory() as d:
root = Path(d)
child = root / "child.mjs"
parent = root / "parent.mjs"
child.write_text("console.log('DAEMON_ENTRYPOINT_REACHED');\n")
parent.write_text(
"import { spawn } from 'node:child_process';\n"
f"const child = spawn(process.execPath, [...process.execArgv, {str(child)!r}], "
"{ stdio: ['ignore', 'pipe', 'pipe'] });\n"
"child.stdout.pipe(process.stdout); child.stderr.pipe(process.stderr);\n"
"child.on('exit', (code, signal) => { console.log(JSON.stringify({code, signal})); process.exit(0); });\n"
)
proc = Popen(["node", "--inspect=127.0.0.1:19323", str(parent)],
stdout=PIPE, stderr=PIPE, text=True)
stdout, stderr = proc.communicate(timeout=2)
print(f"exit={proc.returncode}")
print("stdout:", stdout.strip())
print("stderr:", stderr.strip())
PYRepository: Waishnav/devspace
Length of output: 430
Do not forward inspector flags to the detached daemon.
When the CLI runs with --inspect-brk, the daemon also pauses before executing local-agent-daemon-main.js. ensureReady() then returns DAEMON_START_FAILED. Filter inspector flags, including --inspect, --inspect-brk, and their =... forms, before spawning the daemon.
🤖 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-client.ts` around lines 177 - 192, Update
spawnLocalAgentDaemon to filter inspector-related flags from process.execArgv
before constructing the detached child arguments, excluding --inspect,
--inspect-brk, and forms with =... while preserving all other runtime arguments.
Keep resolveDaemonEntrypoint unchanged.
Source: Path instructions
| private handleConnection(socket: Socket): void { | ||
| this.sockets.add(socket); | ||
| socket.setEncoding("utf8"); | ||
| let buffer = ""; | ||
| let handled = false; | ||
| socket.on("data", (chunk: string | Buffer) => { | ||
| if (handled) return; | ||
| buffer += chunk.toString(); | ||
| if (Buffer.byteLength(buffer, "utf8") > MAX_REQUEST_BYTES) { | ||
| handled = true; | ||
| this.writeError(socket, "", "REQUEST_TOO_LARGE", "Daemon request is too large."); | ||
| return; | ||
| } | ||
| const newline = buffer.indexOf("\n"); | ||
| if (newline === -1) return; | ||
| handled = true; | ||
| const line = buffer.slice(0, newline); | ||
| void this.handleLine(socket, line); | ||
| }); | ||
| socket.on("error", () => undefined); | ||
| socket.on("close", () => this.sockets.delete(socket)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Set a read timeout on accepted sockets.
The daemon adds every accepted socket to this.sockets and never applies a timeout. A local process can open connections and send no newline. Those sockets stay open indefinitely. Two effects follow: maintainIdle at Line 251 never reaches the idle branch because this.sockets.size > 0, so the daemon never shuts down automatically; and close() blocks on closeServer.
Apply socket.setTimeout and destroy the socket when the deadline passes.
🔒 Proposed fix
private handleConnection(socket: Socket): void {
this.sockets.add(socket);
socket.setEncoding("utf8");
+ socket.setTimeout(SOCKET_IDLE_TIMEOUT_MS, () => socket.destroy());
let buffer = "";Add the constant near the other limits:
const MAX_REQUEST_BYTES = 512 * 1024;
+const SOCKET_IDLE_TIMEOUT_MS = 60_000;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private handleConnection(socket: Socket): void { | |
| this.sockets.add(socket); | |
| socket.setEncoding("utf8"); | |
| let buffer = ""; | |
| let handled = false; | |
| socket.on("data", (chunk: string | Buffer) => { | |
| if (handled) return; | |
| buffer += chunk.toString(); | |
| if (Buffer.byteLength(buffer, "utf8") > MAX_REQUEST_BYTES) { | |
| handled = true; | |
| this.writeError(socket, "", "REQUEST_TOO_LARGE", "Daemon request is too large."); | |
| return; | |
| } | |
| const newline = buffer.indexOf("\n"); | |
| if (newline === -1) return; | |
| handled = true; | |
| const line = buffer.slice(0, newline); | |
| void this.handleLine(socket, line); | |
| }); | |
| socket.on("error", () => undefined); | |
| socket.on("close", () => this.sockets.delete(socket)); | |
| } | |
| private handleConnection(socket: Socket): void { | |
| this.sockets.add(socket); | |
| socket.setEncoding("utf8"); | |
| socket.setTimeout(SOCKET_IDLE_TIMEOUT_MS, () => socket.destroy()); | |
| let buffer = ""; | |
| let handled = false; | |
| socket.on("data", (chunk: string | Buffer) => { | |
| if (handled) return; | |
| buffer += chunk.toString(); | |
| if (Buffer.byteLength(buffer, "utf8") > MAX_REQUEST_BYTES) { | |
| handled = true; | |
| this.writeError(socket, "", "REQUEST_TOO_LARGE", "Daemon request is too large."); | |
| return; | |
| } | |
| const newline = buffer.indexOf("\n"); | |
| if (newline === -1) return; | |
| handled = true; | |
| const line = buffer.slice(0, newline); | |
| void this.handleLine(socket, line); | |
| }); | |
| socket.on("error", () => undefined); | |
| socket.on("close", () => this.sockets.delete(socket)); | |
| } |
🤖 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-daemon.ts` around lines 155 - 176, Update handleConnection to
apply a read timeout to each accepted socket using a dedicated timeout constant
defined alongside the existing limits, and destroy the socket when the timeout
fires. Preserve the current request parsing and cleanup behavior while ensuring
idle connections cannot keep this.sockets populated indefinitely.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/local-agent-runtime-pool.ts (1)
171-190: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not return an entry that is closing.
After Line 187 yields,
evictIdle()orclose()can setexisting.closing, remove the entry, and startruntime.close(). Line 188 checks onlyisAlive().run()can then invoke a runtime while closure is in progress.After
await existing.createPromise, verify!existing.closingandthis.entries.get(key) === existingbefore returning the entry. Otherwise, wait for its close and acquire a new entry or reject when the pool is closing. Add a regression test that interleaves reuse with idle eviction.🤖 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-runtime-pool.ts` around lines 171 - 190, Update acquire() after await existing.createPromise to return the entry only when it is still open and remains mapped by this.entries.get(key) === existing, in addition to the existing runtime liveness check. If closure or removal occurred during the await, wait for the close and continue acquiring a replacement or reject when the pool is closing; add a regression test covering reuse interleaved with idle eviction.src/local-agent-manager.ts (1)
93-129: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBind agent operations to
workspaceId.Line 104 stores an optional
workspaceId. Lines 124-128 load an agent by a global ID and validate onlyworkspaceRoot. An allowed root is not a workspace identity.Require the opaque
workspaceIdreturned byopen_workspacewhen an agent starts. Include it in continue, get, and list requests. Validate that it matches the stored record before each operation. Trace the daemon protocol, client, CLI, persistence, and MCP schemas together.As per coding guidelines: “Treat every operation as workspace-scoped and use
workspaceIdas the opaque handle returned byopen_workspace. Do not conflate workspaces, allowed roots, checkouts, or worktrees.”🤖 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-manager.ts` around lines 93 - 129, Bind all local-agent operations to the opaque workspaceId rather than relying on workspaceRoot or globally resolving agent IDs. Update start, continue, get, and list request flows across the daemon protocol, client, CLI, persistence, and MCP schemas to require and propagate workspaceId; validate it against each stored LocalAgentRecord before operating, including in continue and the corresponding retrieval/list methods. Preserve workspaceRoot authorization as a separate check, without using it as workspace identity.Source: Coding guidelines
🧹 Nitpick comments (2)
docs/agent-profile-schema.md (1)
5-6: 🩺 Stability & Availability | 🔵 TrivialVerify the packaged and real-MCP client path.
The supplied context verifies the source-level
src/cli.tsdelegation toLocalAgentClient. It does not verify that the packagednpm/npxentrypoint includes daemon startup or that a real MCP host uses the same client andstateDir. Exercise those paths, including daemon restart on supported platforms, before treating this ownership statement as validated. State the narrower verified scope if only the source-level path was checked.As per coding guidelines, verify the actual user-consumption path, including packaged npm/npx usage, real MCP hosts, restart requirements, and supported platforms.
🤖 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 `@docs/agent-profile-schema.md` around lines 5 - 6, Update the ownership statement in the documentation to reflect only the scope actually verified: source-level CLI delegation to LocalAgentClient unless packaged npm/npx entrypoints, real MCP host usage with the same stateDir, daemon startup and restart behavior, and supported platforms have been exercised. If those paths are verified, document the results; otherwise state the narrower source-level scope rather than claiming full client-path validation.Source: Coding guidelines
docs/local-agent-daemon.md (1)
46-54: 🩺 Stability & Availability | 🔵 TrivialVerify the documented daemon lifecycle through the packaged path.
The supplied context verifies only source-level CLI delegation. It does not verify that the packaged
npm/npxentrypoint starts the daemon, that a real MCP host uses the sameLocalAgentClientandstateDir, or that bounded shutdown and reconciliation work on supported transports. Run the packaged smoke path, including restart recovery, and state the narrower verified scope if only the source-level proxy was tested.As per coding guidelines, verify the actual user-consumption path, including packaged npm/npx usage, real MCP hosts, restart requirements, and supported platforms.
🤖 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 `@docs/local-agent-daemon.md` around lines 46 - 54, Expand the daemon lifecycle verification around the documented client-boundary and shutdown behavior to exercise the packaged npm/npx entrypoint, a real MCP host using the same LocalAgentClient and stateDir, supported transports, bounded shutdown, and restart reconciliation. Verify stale starting/running records become error while preserving providerSessionId and latestResponse; if only source-level delegation can be tested, narrow the documentation to explicitly state that limitation.Source: Coding guidelines
🤖 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/cli.ts`:
- Around line 300-301: Update the top-level command descriptions in printHelp
for agents run and agents continue to include the supported [--thinking <level>]
option, matching printAgentsHelp and the parsed.thinking forwarding in
runAgentsRun and runAgentsContinue.
- Around line 373-380: Update the agents-continue flow rooted at
runAgentsContinue to require and propagate the current workspaceId through the
CLI arguments, local client, daemon protocol, and manager continuation APIs.
Validate that workspaceId is present and matches the agent’s stored workspace
ownership before starting the turn, rejecting missing or mismatched ownership
while preserving normal continuation for valid requests.
In `@src/local-agent-runtime-pool.ts`:
- Around line 290-297: Update releaseIdleSessions and the session-reuse path in
run so each providerSessionId tracks an in-flight release promise; when run
selects a session with a pending release, await that promise before invoking
runtime.run, and ensure release bookkeeping is cleared afterward. Route shutdown
cleanup through the same coordination mechanism so releases cannot overlap newly
started provider work.
In `@src/local-agent-runtime.ts`:
- Around line 21-28: Replace the provider-specific value in
LocalAgentRunCallbacks.onSessionId with a DevSpace-owned opaque continuation
handle, and update src/local-agent-runtime.ts lines 21-28 accordingly. In
src/local-agent-manager.ts lines 220-227, persist that core continuation handle
while keeping any provider-specific continuation data in adapter-owned state;
update the related manager symbols and call sites consistently.
---
Outside diff comments:
In `@src/local-agent-manager.ts`:
- Around line 93-129: Bind all local-agent operations to the opaque workspaceId
rather than relying on workspaceRoot or globally resolving agent IDs. Update
start, continue, get, and list request flows across the daemon protocol, client,
CLI, persistence, and MCP schemas to require and propagate workspaceId; validate
it against each stored LocalAgentRecord before operating, including in continue
and the corresponding retrieval/list methods. Preserve workspaceRoot
authorization as a separate check, without using it as workspace identity.
In `@src/local-agent-runtime-pool.ts`:
- Around line 171-190: Update acquire() after await existing.createPromise to
return the entry only when it is still open and remains mapped by
this.entries.get(key) === existing, in addition to the existing runtime liveness
check. If closure or removal occurred during the await, wait for the close and
continue acquiring a replacement or reject when the pool is closing; add a
regression test covering reuse interleaved with idle eviction.
---
Nitpick comments:
In `@docs/agent-profile-schema.md`:
- Around line 5-6: Update the ownership statement in the documentation to
reflect only the scope actually verified: source-level CLI delegation to
LocalAgentClient unless packaged npm/npx entrypoints, real MCP host usage with
the same stateDir, daemon startup and restart behavior, and supported platforms
have been exercised. If those paths are verified, document the results;
otherwise state the narrower source-level scope rather than claiming full
client-path validation.
In `@docs/local-agent-daemon.md`:
- Around line 46-54: Expand the daemon lifecycle verification around the
documented client-boundary and shutdown behavior to exercise the packaged
npm/npx entrypoint, a real MCP host using the same LocalAgentClient and
stateDir, supported transports, bounded shutdown, and restart reconciliation.
Verify stale starting/running records become error while preserving
providerSessionId and latestResponse; if only source-level delegation can be
tested, narrow the documentation to explicitly state that limitation.
🪄 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: 189eab0c-799b-4264-acf4-55fd6ef42dac
📒 Files selected for processing (19)
docs/agent-profile-schema.mddocs/chatgpt-coding-workflow.mddocs/configuration.mddocs/gotchas.mddocs/local-agent-daemon.mdskills/subagent-delegation/SKILL.mdsrc/cli.tssrc/local-agent-client.tssrc/local-agent-daemon-main.tssrc/local-agent-daemon-protocol.tssrc/local-agent-daemon.tssrc/local-agent-manager.test.tssrc/local-agent-manager.tssrc/local-agent-runtime-pool.tssrc/local-agent-runtime.test.tssrc/local-agent-runtime.tssrc/local-agent-store.test.tssrc/local-agent-store.tssrc/local-agent-targets.ts
💤 Files with no reviewable changes (1)
- src/local-agent-daemon-protocol.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/gotchas.md
- src/local-agent-client.ts
- src/local-agent-daemon.ts
- skills/subagent-delegation/SKILL.md
| " devspace agents run <profile-or-provider> [--model <model>] <prompt>", | ||
| " devspace agents continue <id> [--model <model>] <prompt>", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Expose --thinking in top-level help.
runAgentsRun and runAgentsContinue forward parsed.thinking. printAgentsHelp documents [--thinking <level>], but printHelp omits it for both commands. Users who run devspace --help cannot discover a supported option.
Proposed fix
- " devspace agents run <profile-or-provider> [--model <model>] <prompt>",
- " devspace agents continue <id> [--model <model>] <prompt>",
+ " devspace agents run <profile-or-provider> [--model <model>] [--thinking <level>] <prompt>",
+ " devspace agents continue <id> [--model <model>] [--thinking <level>] <prompt>",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| " devspace agents run <profile-or-provider> [--model <model>] <prompt>", | |
| " devspace agents continue <id> [--model <model>] <prompt>", | |
| " devspace agents run <profile-or-provider> [--model <model>] [--thinking <level>] <prompt>", | |
| " devspace agents continue <id> [--model <model>] [--thinking <level>] <prompt>", |
🤖 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/cli.ts` around lines 300 - 301, Update the top-level command descriptions
in printHelp for agents run and agents continue to include the supported
[--thinking <level>] option, matching printAgentsHelp and the parsed.thinking
forwarding in runAgentsRun and runAgentsContinue.
| export interface LocalAgentRunCallbacks { | ||
| /** | ||
| * Called as soon as a provider creates or resolves a durable continuation | ||
| * identity. The callback is awaited before the provider starts work that | ||
| * could otherwise fail and lose that identity. | ||
| */ | ||
| onSessionId?: (providerSessionId: string) => void | Promise<void>; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep provider continuation state in adapters.
The core runtime callback exposes providerSessionId, and the manager persists it. This makes provider continuation formats part of the DevSpace core domain.
src/local-agent-runtime.ts#L21-L28: replace the provider-specific callback value with a DevSpace-owned opaque continuation handle.src/local-agent-manager.ts#L220-L227: persist the core continuation handle. Keep provider-specific continuation data in adapter-owned state when required.
As per coding guidelines: “Keep DevSpace policy in the core domain and provider-specific translation in adapters; do not let Pi, MCP host, or model-provider terminology become the core domain model.”
📍 Affects 2 files
src/local-agent-runtime.ts#L21-L28(this comment)src/local-agent-manager.ts#L220-L227
🤖 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-runtime.ts` around lines 21 - 28, Replace the
provider-specific value in LocalAgentRunCallbacks.onSessionId with a
DevSpace-owned opaque continuation handle, and update src/local-agent-runtime.ts
lines 21-28 accordingly. In src/local-agent-manager.ts lines 220-227, persist
that core continuation handle while keeping any provider-specific continuation
data in adapter-owned state; update the related manager symbols and call sites
consistently.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/local-agent-daemon-lifecycle.ts (1)
73-88: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftPrevent stale recovery from deleting a live lock.
If the owner pauses after
openSync(this.paths.lockPath, "wx", 0o600)and before writing its PID, another process can delete the empty lock after one second and acquire a replacement. The original owner can then continue writing through its already-open descriptor and return successfully. Two daemons can then use the same state directory.Do not delete an undecodable lock based only on its age. Write the owner record before publishing the lock, or verify that
lockPathstill identifiesfileDescriptorbefore writingpidPathand returning. Add a regression test for this interleaving and assert that only one acquisition succeeds.🤖 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-daemon-lifecycle.ts` around lines 73 - 88, Update the lock acquisition flow in the lifecycle method containing openSync, writeSync, and pidPath publication so an undecodable lock is never removed solely due to age. Publish the owner record before exposing the lock, or verify that lockPath still refers to this.fileDescriptor immediately before writing pidPath and returning; preserve exclusive acquisition and ensure a replaced lock causes the original attempt to fail. Add a regression test for the pause-after-open interleaving and assert that only one acquisition succeeds.
🤖 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-daemon-lifecycle.ts`:
- Around line 110-111: Update both secret read paths in the local-agent
lifecycle code to accept only exactly 64 hexadecimal characters, replacing the
length-only check before returning the secret for IPC authentication. Preserve
the existing file reading and trimming behavior, and reject all other serialized
formats.
---
Outside diff comments:
In `@src/local-agent-daemon-lifecycle.ts`:
- Around line 73-88: Update the lock acquisition flow in the lifecycle method
containing openSync, writeSync, and pidPath publication so an undecodable lock
is never removed solely due to age. Publish the owner record before exposing the
lock, or verify that lockPath still refers to this.fileDescriptor immediately
before writing pidPath and returning; preserve exclusive acquisition and ensure
a replaced lock causes the original attempt to fail. Add a regression test for
the pause-after-open interleaving and assert that only one acquisition succeeds.
🪄 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: 556e284f-49f2-4b04-8037-f7fd3f9535d4
📒 Files selected for processing (8)
src/local-agent-client.tssrc/local-agent-daemon-lifecycle.test.tssrc/local-agent-daemon-lifecycle.tssrc/local-agent-daemon-protocol.test.tssrc/local-agent-daemon-protocol.tssrc/local-agent-daemon.tssrc/local-agent-runtime-pool.tssrc/local-agent-runtime.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/local-agent-daemon-protocol.test.ts
- src/local-agent-daemon-lifecycle.test.ts
- src/local-agent-client.ts
- src/local-agent-daemon-protocol.ts
- src/local-agent-runtime.test.ts
- src/local-agent-runtime-pool.ts
- src/local-agent-daemon.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-daemon.test.ts`:
- Around line 154-158: Update the shutdown test around shutdownSocket so it
registers a promise for the socket’s "close" event before calling
socketDaemon.close(), then awaits that promise after shutdown completes. Retain
the existing timing assertion while explicitly verifying the connected client
socket closes.
- Around line 123-131: Update the ownership test around
ownerDaemon.paths.endpoint to verify availability by connecting through the
endpoint rather than checking existsSync(ownerDaemon.paths.socketPath). Use the
project’s existing IPC client or connection helper so the assertion works for
both filesystem sockets and Windows named pipes, while preserving the ownership
and lock/PID checks.
In `@src/local-agent-store.ts`:
- Around line 32-36: Make workspace authorization require an opaque workspaceId
throughout LocalAgentWorkspaceScope, local-agent-store
persistence/deserialization, and local-agent-manager agent operations. Reject or
migrate persisted records whose workspaceId is NULL instead of allowing
root-only authorization, and ensure continuation rejects mismatched workspace
IDs. Canonicalize persisted workspace roots and validate them against
allowedRoots while using the workspaceId returned by open_workspace as the
required workspace handle.
🪄 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: 92fdcd04-2409-4b8d-b834-787c692b8e71
📒 Files selected for processing (9)
src/cli.tssrc/local-agent-client.tssrc/local-agent-daemon-protocol.tssrc/local-agent-daemon.test.tssrc/local-agent-daemon.tssrc/local-agent-manager.test.tssrc/local-agent-manager.tssrc/local-agent-runtime-pool.tssrc/local-agent-store.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/cli.ts
- src/local-agent-daemon-protocol.ts
- src/local-agent-runtime-pool.ts
- src/local-agent-client.ts
- src/local-agent-manager.test.ts
- src/local-agent-daemon.ts
- src/local-agent-manager.ts
| try { | ||
| await ownerDaemon.start(); | ||
| const lockBefore = readFileSync(ownerDaemon.paths.lockPath, "utf8"); | ||
| const pidBefore = readFileSync(ownerDaemon.paths.pidPath, "utf8"); | ||
| assert.notEqual(ownerDaemon.paths.endpoint, ""); | ||
| await assert.rejects(competingDaemon.start(), /already running/); | ||
| assert.equal(readFileSync(ownerDaemon.paths.lockPath, "utf8"), lockBefore); | ||
| assert.equal(readFileSync(ownerDaemon.paths.pidPath, "utf8"), pidBefore); | ||
| assert.equal(existsSync(ownerDaemon.paths.socketPath), true); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the ownership test portable.
Line 131 treats socketPath as a filesystem entry. This fails for the named-pipe endpoint on Windows, although the owner daemon is available. Connect to ownerDaemon.paths.endpoint to verify the actual listener on both supported IPC transports.
Proposed fix
-import { existsSync, readFileSync } from "node:fs";
+import { readFileSync } from "node:fs";
...
- assert.equal(existsSync(ownerDaemon.paths.socketPath), true);
+ const ownerSocket = createConnection(ownerDaemon.paths.endpoint);
+ await onceSocket(ownerSocket, "connect");
+ ownerSocket.destroy();As per coding guidelines: “Verify the actual user-consumption path, including ... supported platforms.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await ownerDaemon.start(); | |
| const lockBefore = readFileSync(ownerDaemon.paths.lockPath, "utf8"); | |
| const pidBefore = readFileSync(ownerDaemon.paths.pidPath, "utf8"); | |
| assert.notEqual(ownerDaemon.paths.endpoint, ""); | |
| await assert.rejects(competingDaemon.start(), /already running/); | |
| assert.equal(readFileSync(ownerDaemon.paths.lockPath, "utf8"), lockBefore); | |
| assert.equal(readFileSync(ownerDaemon.paths.pidPath, "utf8"), pidBefore); | |
| assert.equal(existsSync(ownerDaemon.paths.socketPath), true); | |
| try { | |
| await ownerDaemon.start(); | |
| const lockBefore = readFileSync(ownerDaemon.paths.lockPath, "utf8"); | |
| const pidBefore = readFileSync(ownerDaemon.paths.pidPath, "utf8"); | |
| assert.notEqual(ownerDaemon.paths.endpoint, ""); | |
| await assert.rejects(competingDaemon.start(), /already running/); | |
| assert.equal(readFileSync(ownerDaemon.paths.lockPath, "utf8"), lockBefore); | |
| assert.equal(readFileSync(ownerDaemon.paths.pidPath, "utf8"), pidBefore); | |
| const ownerSocket = createConnection(ownerDaemon.paths.endpoint); | |
| await onceSocket(ownerSocket, "connect"); | |
| ownerSocket.destroy(); |
🤖 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-daemon.test.ts` around lines 123 - 131, Update the ownership
test around ownerDaemon.paths.endpoint to verify availability by connecting
through the endpoint rather than checking
existsSync(ownerDaemon.paths.socketPath). Use the project’s existing IPC client
or connection helper so the assertion works for both filesystem sockets and
Windows named pipes, while preserving the ownership and lock/PID checks.
Source: Coding guidelines
| export interface LocalAgentWorkspaceScope { | ||
| workspaceId?: string; | ||
| workspaceRoot: string; | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Require an opaque workspaceId for workspace authorization.
workspaceId is optional, so a continuation can use only workspaceRoot. src/local-agent-manager.ts:304-312 compares IDs only when the persisted record has one. src/local-agent-store.ts:223-227 converts a database NULL into undefined.
This permits root-only authorization for legacy or incomplete records. A root can represent a checkout or worktree without uniquely identifying the workspace. Require workspaceId, or migrate and reject records without one. Make the manager scope required at every agent operation.
Proposed contract hardening
export interface LocalAgentWorkspaceScope {
- workspaceId?: string;
+ workspaceId: string;
workspaceRoot: string;
}As per coding guidelines, treat every operation as workspace-scoped and use workspaceId as the opaque handle returned by open_workspace.
Based on learnings, persisted workspace roots must be canonicalized and validated against allowedRoots, and continuation must reject a different workspace.
🤖 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.ts` around lines 32 - 36, Make workspace authorization
require an opaque workspaceId throughout LocalAgentWorkspaceScope,
local-agent-store persistence/deserialization, and local-agent-manager agent
operations. Reject or migrate persisted records whose workspaceId is NULL
instead of allowing root-only authorization, and ensure continuation rejects
mismatched workspace IDs. Canonicalize persisted workspace roots and validate
them against allowedRoots while using the workspaceId returned by open_workspace
as the required workspace handle.
Sources: Coding guidelines, Learnings
1 participant
Local-agent execution needs a lifecycle owner independent of the MCP HTTP server. This foundation adds an on-demand devspace-agentd scoped to one DevSpace state directory, private Unix-socket or named-pipe IPC, an atomic startup lock, readiness handshake, idle lifecycle, durable-store reconciliation, and bounded shutdown recovery.
The MCP server no longer constructs the local-agent manager, store, runtime pool, or provider drivers. CLI agent commands use LocalAgentClient and automatically start or reuse the daemon, so devspace serve is optional and MCP restarts do not stop agent work. Provider-specific runtime implementations remain in the dependent stacked PRs.
Verified with npm test, npm run typecheck, npm run build, and git diff --check.
Summary by CodeRabbit
New Features
agents continueandagents daemon status,stop, andlogscommands.Bug Fixes
Documentation
devspace serveis not required.