feat: upgrade to @opencode-ai/sdk and add parallel projects tool#11
feat: upgrade to @opencode-ai/sdk and add parallel projects tool#11AlaeddineMessadi wants to merge 4 commits into
Conversation
This commit completely revamps the MCP server management by moving from custom manual subprocess shell executions to the official `@opencode-ai/sdk` (`createOpencodeServer`). This eliminates the 'zombie process' bugs by leaning on the SDK's native process lifecycle handling. Additionally, it adds the new `opencode_project_init` tool which allows the AI client to dynamically create new absolute directories on the host system or explicitly target preexisting directories for parallel concurrent workloads.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request migrates the OpenCode MCP server from using native ChangesSDK Migration: HTTP Client and Server Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: b31f333b81
ℹ️ 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".
| const response = await fetch(`${baseUrl.replace(/\/$/, "")}/global/health`, { | ||
| method: "GET", | ||
| signal: AbortSignal.timeout(3000), | ||
| }); |
There was a problem hiding this comment.
When OPENCODE_SERVER_PASSWORD is configured, /global/health is expected to require the same Basic auth as other endpoints, but this probe now sends no Authorization header. In that setup, isServerRunning() reports a healthy server as down (401), so ensureServer() attempts a redundant auto-start and the reconnect path can loop into failed restart attempts instead of recognizing the existing server.
Useful? React with 👍 / 👎.
| if (normalized) { | ||
| headers["x-opencode-directory"] = encodeURIComponent(normalized); | ||
| } |
There was a problem hiding this comment.
Directory-scoped requests now URI-encode the normalized filesystem path before placing it in x-opencode-directory. That transforms absolute paths like /tmp/proj into %2Ftmp%2Fproj, which is no longer a valid absolute path unless the server explicitly decodes it first; this can break project scoping for nearly all tools that accept directory and cause false "invalid directory" behavior.
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client.ts (1)
88-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass timeout to API client methods via AbortSignal.
request()acceptsopts.timeoutandpost()forwards it (line 188), but the HTTP dispatch calls on lines 117–129 never pass it through. Long-running tools relying on per-call timeouts will now hang until the underlying socket gives up. This is a regression from the priorfetch-based implementation.The SDK's HTTP methods support standard
AbortSignalviaRequestInit, so the fix is to create an abort signal when timeout is provided and pass it through all dispatch calls:Suggested fix
const apiClient = (this.api as any)._client; + const signal = opts?.timeout ? AbortSignal.timeout(opts.timeout) : undefined; let res; switch (method.toUpperCase()) { case 'GET': - res = await apiClient.get({ url: path, query: opts?.query, headers }); + res = await apiClient.get({ url: path, query: opts?.query, headers, signal }); break; case 'POST': - res = await apiClient.post({ url: path, query: opts?.query, body: opts?.body as any, headers }); + res = await apiClient.post({ url: path, query: opts?.query, body: opts?.body as any, headers, signal }); break; case 'PATCH': - res = await apiClient.patch({ url: path, query: opts?.query, body: opts?.body as any, headers }); + res = await apiClient.patch({ url: path, query: opts?.query, body: opts?.body as any, headers, signal }); break; case 'PUT': - res = await apiClient.put({ url: path, query: opts?.query, body: opts?.body as any, headers }); + res = await apiClient.put({ url: path, query: opts?.query, body: opts?.body as any, headers, signal }); break; case 'DELETE': - res = await apiClient.delete({ url: path, query: opts?.query, headers }); + res = await apiClient.delete({ url: path, query: opts?.query, headers, signal }); break;Also applies to: 187-189
🤖 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/client.ts` around lines 88 - 152, The request() method does not pass opts.timeout to the underlying api client calls, so create an AbortController when opts.timeout is provided (use setTimeout to call controller.abort() after the timeout) and add the resulting signal to every apiClient call options in the switch (get/post/patch/put/delete); ensure you clear the timeout timer after the request completes and also update post() (and any other helper that forwards timeout) to accept/forward opts.timeout consistently so per-call timeouts are honored.
🧹 Nitpick comments (7)
src/server-manager.ts (3)
17-17: 💤 Low valueAvoid
anyformanagedServer.The SDK ships TypeScript types; a structural type (
{ url: string; close(): unknown | Promise<unknown> }) or the SDK’s exported server type would catch shape drift at compile time. See diff in the shutdown comment above.🤖 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/server-manager.ts` at line 17, Replace the loose any on the managedServer variable with a concrete type that reflects the SDK server shape (for example a structural type like { url: string; close(): unknown | Promise<unknown> } or the SDK’s exported server type if available) so the compiler enforces the expected properties and methods; update the declaration of managedServer accordingly and import/use the SDK server type where applicable to avoid runtime shape drift.
72-91: ⚡ Quick win
version: "unknown"is a regression — the health endpoint returns it.
isServerRunning()already extracts aversionfrom/global/health(line 63). AftercreateOpencodeServerresolves, you can call it once to populate a real version and surface it throughensureServer(currently downstream consumers always see"unknown"for managed servers, while detected pre-existing servers report the real version — the inconsistency will leak into logs and any UI that displays it).♻️ Suggested fix
- managedServer = await createOpencodeServer({ + managedServer = await createOpencodeServer({ hostname, port, timeout: timeoutMs, }); registerShutdownHandlers(); - return { url: managedServer.url, version: "unknown" }; + const status = await isServerRunning(managedServer.url); + return { url: managedServer.url, version: status.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/server-manager.ts` around lines 72 - 91, startServer currently returns version: "unknown"; after createOpencodeServer resolves, call the health-check logic (reuse isServerRunning or its /global/health fetch) against the newly created managedServer to extract the real version and return it instead of "unknown". Specifically, after managedServer = await createOpencodeServer({...}) invoke the same health/version extraction used in isServerRunning (or call isServerRunning(managedServer.url) if it accepts a target) to populate the version field returned from startServer so downstream callers see the real managed server version.
78-78: 💤 Low valueMisleading startup log.
The log claims
Starting: opencode serve --hostname … --port …, but no subprocess or CLI is being invoked anymore — the SDK manages the lifecycle in-process. Operators grepping logs foropencode servewill be misled.📝 Suggested wording
- console.error(`Starting: opencode serve --hostname ${hostname} --port ${port}`); + console.error(`Starting OpenCode SDK server on ${hostname}:${port}`);🤖 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/server-manager.ts` at line 78, The startup log line using console.error that prints `Starting: opencode serve --hostname ${hostname} --port ${port}` is misleading because we no longer invoke a subprocess; update the message text (the console.error call) to accurately state the server is starting in-process via the SDK (e.g., "Starting OpenCode SDK server on ${hostname}:${port}") and consider switching to a less severe logging level like console.info or the module's logger; change the literal "opencode serve" to an in-process description and keep hostname and port variables intact.src/client.ts (2)
165-173: 💤 Low value
username/passwordpassed here are ignored by the newensureServer.Per the updated
src/server-manager.ts,ensureServerno longer uses the credential fields (the SDK manages auth via headers configured at client-construction time). Forwarding them here is dead data and is misleading on read. Either drop them or remove the fields fromServerManagerOptions.♻️ Suggested cleanup
if (!status.healthy) { - await ensureServer({ - baseUrl: this.baseUrl, - autoServe: true, - username: this.username, - password: this.password - }); + await ensureServer({ baseUrl: this.baseUrl, autoServe: 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/client.ts` around lines 165 - 173, The call in client code passing this.username and this.password to ensureServer is forwarding dead data because the updated ensureServer/ServerManagerOptions no longer use credentials; remove the username and password properties from the ensureServer call in the block that checks isServerRunning (i.e., where ensureServer is invoked), or alternatively remove credential fields from ServerManagerOptions if you prefer API cleanup—pick one approach and make callers (e.g., the method containing isServerRunning and ensureServer) only pass the used options (baseUrl, autoServe) and remove references to this.username / this.password.
72-82: 💤 Low valueDuplicated Basic-auth header construction.
The
Authorization: Basic …header is built inline in two places (constructor andsubscribeSSE). Extract a small helper to keep them in sync (e.g., when adding aWWW-Authenticaterealm or switching encodings).♻️ Suggested helper
+function buildBasicAuthHeader(username: string | undefined, password: string): string { + const user = username ?? "opencode"; + return "Basic " + Buffer.from(`${user}:${password}`).toString("base64"); +}Then use it in both the constructor and
subscribeSSE.Also applies to: 205-212
🤖 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/client.ts` around lines 72 - 82, The code duplicates Basic auth header construction in the constructor and subscribeSSE; extract a single helper (e.g., createBasicAuthHeader(username?: string, password?: string): string | undefined) and use it from the class that builds headers for this.api and from subscribeSSE to produce the "Authorization" value; update the constructor code (where headers: Record<string,string> is built) to call createBasicAuthHeader(options.username, options.password) and set headers["Authorization"] only if the helper returns a value, and update subscribeSSE to call the same helper instead of reconstructing the header inline so both places stay in sync (adjust names Client, subscribeSSE, and constructor usages accordingly).src/tools/project.ts (2)
67-67: 💤 Low valueStatic import is preferred over dynamic
import("fs/promises")here.This file is already ESM (
type: "module"inpackage.json) and there's no circularity or lazy-load reason to dynamically import on every invocation. Hoisting to a top-levelimport { mkdir } from "node:fs/promises"(also note thenode:prefix is idiomatic) avoids per-call resolution and matches the diff in the comment above.🤖 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/tools/project.ts` at line 67, Replace the dynamic per-call import of fs/promises with a top-level static ESM import: add a top-level import like import { mkdir } from "node:fs/promises" and remove the runtime await import("fs/promises"); then update usages that referenced the imported module (e.g., any calls currently using fs.mkdir or fs.* in this file, such as in project.ts functions) to call the statically imported symbol(s) (mkdir) instead so the module is resolved once at load time and uses the node: prefix idiom.
71-75: 💤 Low valueConsider logging the swallowed ping error at debug level.
Silently catching the ping failure is reasonable (the directory is what matters), but completely losing the error makes "I created the dir but the server never registered it" hard to diagnose. A
console.error(or the project’s structured logger) at non-fatal level would help operators without changing the tool result.📝 Suggested change
- } catch (e) { - // Ignore errors if the ping fails, the directory is still created - } + } catch (e) { + // Best-effort: the directory is already created. + console.error( + `opencode_project_init: project ping failed for ${path}: ${e instanceof Error ? e.message : String(e)}`, + ); + }🤖 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/tools/project.ts` around lines 71 - 75, The catch block that swallows errors from the call to client.get("/project/current", undefined, path) should log the caught exception at a non-fatal debug level so operators can diagnose "dir created but server not registered" cases; update the catch to call the project's logger (or console.debug if no logger is available) with a clear message like "ping /project/current failed for path=<path>" and include the error object (e) for context while leaving the directory creation behavior unchanged.
🤖 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/tools/project.ts`:
- Around line 60-83: The current project-init handler accepts any string and
calls fs.mkdir directly; validate and harden the input before creating
directories: update the Zod input/schema or pre-check in the async handler that
the provided path is an absolute path (use path.isAbsolute), normalize and
canonicalize it (use path.resolve and optionally fs.realpath to remove
symlinks), reject inputs with NUL bytes or path traversal that escape an allowed
root, enforce an allow-list/OPENCODE_PROJECTS_ROOT (or refuse system roots like
"/" and common system dirs such as "/etc", "/usr"), verify the target does not
already exist as a file (use fs.stat) and is within the allowed root, then call
fs.mkdir; also stop silently swallowing the client.get errors — surface or log
the ping error (include the resolved path) so unexpected resolutions are
visible; refer to the async handler that performs fs.mkdir and the
client.get("/project/current") ping to locate where to apply these checks.
---
Outside diff comments:
In `@src/client.ts`:
- Around line 88-152: The request() method does not pass opts.timeout to the
underlying api client calls, so create an AbortController when opts.timeout is
provided (use setTimeout to call controller.abort() after the timeout) and add
the resulting signal to every apiClient call options in the switch
(get/post/patch/put/delete); ensure you clear the timeout timer after the
request completes and also update post() (and any other helper that forwards
timeout) to accept/forward opts.timeout consistently so per-call timeouts are
honored.
---
Nitpick comments:
In `@src/client.ts`:
- Around line 165-173: The call in client code passing this.username and
this.password to ensureServer is forwarding dead data because the updated
ensureServer/ServerManagerOptions no longer use credentials; remove the username
and password properties from the ensureServer call in the block that checks
isServerRunning (i.e., where ensureServer is invoked), or alternatively remove
credential fields from ServerManagerOptions if you prefer API cleanup—pick one
approach and make callers (e.g., the method containing isServerRunning and
ensureServer) only pass the used options (baseUrl, autoServe) and remove
references to this.username / this.password.
- Around line 72-82: The code duplicates Basic auth header construction in the
constructor and subscribeSSE; extract a single helper (e.g.,
createBasicAuthHeader(username?: string, password?: string): string | undefined)
and use it from the class that builds headers for this.api and from subscribeSSE
to produce the "Authorization" value; update the constructor code (where
headers: Record<string,string> is built) to call
createBasicAuthHeader(options.username, options.password) and set
headers["Authorization"] only if the helper returns a value, and update
subscribeSSE to call the same helper instead of reconstructing the header inline
so both places stay in sync (adjust names Client, subscribeSSE, and constructor
usages accordingly).
In `@src/server-manager.ts`:
- Line 17: Replace the loose any on the managedServer variable with a concrete
type that reflects the SDK server shape (for example a structural type like {
url: string; close(): unknown | Promise<unknown> } or the SDK’s exported server
type if available) so the compiler enforces the expected properties and methods;
update the declaration of managedServer accordingly and import/use the SDK
server type where applicable to avoid runtime shape drift.
- Around line 72-91: startServer currently returns version: "unknown"; after
createOpencodeServer resolves, call the health-check logic (reuse
isServerRunning or its /global/health fetch) against the newly created
managedServer to extract the real version and return it instead of "unknown".
Specifically, after managedServer = await createOpencodeServer({...}) invoke the
same health/version extraction used in isServerRunning (or call
isServerRunning(managedServer.url) if it accepts a target) to populate the
version field returned from startServer so downstream callers see the real
managed server version.
- Line 78: The startup log line using console.error that prints `Starting:
opencode serve --hostname ${hostname} --port ${port}` is misleading because we
no longer invoke a subprocess; update the message text (the console.error call)
to accurately state the server is starting in-process via the SDK (e.g.,
"Starting OpenCode SDK server on ${hostname}:${port}") and consider switching to
a less severe logging level like console.info or the module's logger; change the
literal "opencode serve" to an in-process description and keep hostname and port
variables intact.
In `@src/tools/project.ts`:
- Line 67: Replace the dynamic per-call import of fs/promises with a top-level
static ESM import: add a top-level import like import { mkdir } from
"node:fs/promises" and remove the runtime await import("fs/promises"); then
update usages that referenced the imported module (e.g., any calls currently
using fs.mkdir or fs.* in this file, such as in project.ts functions) to call
the statically imported symbol(s) (mkdir) instead so the module is resolved once
at load time and uses the node: prefix idiom.
- Around line 71-75: The catch block that swallows errors from the call to
client.get("/project/current", undefined, path) should log the caught exception
at a non-fatal debug level so operators can diagnose "dir created but server not
registered" cases; update the catch to call the project's logger (or
console.debug if no logger is available) with a clear message like "ping
/project/current failed for path=<path>" and include the error object (e) for
context while leaving the directory creation behavior unchanged.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75846958-b272-4ba9-9b8d-a5c0c2821554
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/client.tssrc/server-manager.tssrc/tools/project.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/tools/project.ts (1)
72-72: ⚡ Quick winConsider resolving symlinks for additional hardening.
pathUtil.resolve()canonicalizes..and.but does not resolve symlinks. If the target path or any parent is a symlink controlled by an attacker, the resolved path could still point to an unintended location.For defense-in-depth, consider using
fs.realpath()afterstat()succeeds (for existing directories) to ensure the final path is the true filesystem location.🔗 Suggested symlink resolution
try { const stats = await stat(resolvedPath); if (!stats.isDirectory()) { throw new Error(`Target exists but is not a directory: ${resolvedPath}`); } + // Resolve symlinks for existing directories + const realPath = await import("node:fs/promises").then(fs => fs.realpath(resolvedPath)); + // Use realPath for subsequent operations or logging } catch (err: any) {🤖 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/tools/project.ts` at line 72, The code uses pathUtil.resolve() to build resolvedPath but doesn't handle filesystem symlinks; after verifying the path exists (stat succeeds) update the logic in the function that computes resolvedPath to call fs.realpath or fs.promises.realpath on the resolvedPath and use that returned canonical path (handle errors), so the final path variable (resolvedPath) is the true filesystem target rather than just a normalized string from pathUtil.resolve().
🤖 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/tools/project.ts`:
- Around line 63-71: The path handling needs NUL-byte rejection and
system-directory protection: add a Zod refinement on the existing path schema
(the `path: z.string()` value) to reject any value containing "\0" (e.g.,
`refine(p => !p.includes("\0"))`) and also enforce the same check at runtime
before using pathUtil.isAbsolute in the async handler; additionally introduce an
environment-based allow-list or a deny-list check (use
`process.env.OPENCODE_PROJECTS_ROOT` as the allow-root or a deny set containing
"/", "/etc", "/usr", "/tmp", the user home root, etc.) and validate the incoming
`path` against that list in the async function (where
`pathUtil.isAbsolute(path)` is checked), throwing a clear error if it fails so
directory creation is prevented for disallowed system or out-of-root locations.
---
Nitpick comments:
In `@src/tools/project.ts`:
- Line 72: The code uses pathUtil.resolve() to build resolvedPath but doesn't
handle filesystem symlinks; after verifying the path exists (stat succeeds)
update the logic in the function that computes resolvedPath to call fs.realpath
or fs.promises.realpath on the resolvedPath and use that returned canonical path
(handle errors), so the final path variable (resolvedPath) is the true
filesystem target rather than just a normalized string from pathUtil.resolve().
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d79bc2e3-22c3-4ca9-bfd3-410bf795055d
📒 Files selected for processing (3)
src/client.tssrc/server-manager.tssrc/tools/project.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client.ts
- src/server-manager.ts
1 participant
Overview
This PR completely overhauls the internal architecture of the OpenCode MCP Server by migrating to the official
@opencode-ai/sdk(v1.14.46). This upgrade eliminates buggy manual subprocess management and standardizes all tool requests. Additionally, it introduces full support for independent, parallel workspace initialization via the newopencode_project_inittool.Changelog
spawn("opencode serve")bash commands with the SDK's nativecreateOpencodeServer. This allows the SDK to safely handle port binding, timeouts, and process lifecycle management—eliminating zombie process bugs.src/client.tsto wrap the nativeOpencodeClient. This routes all 79 of our existing MCP tools through the official SDK networking pipeline without requiring thousands of lines of manual refactoring.opencode_project_initTool: Added a new explicit tool that allows the AI client to dynamically scaffold new directories on the host machine, or attach to preexisting directories. This enables parallel, isolated agent workloads.fetchlogic with exponential backoff and timeouts to ensure the MCP auto-recovers if the headless engine restarts.Testing
scratch_test_mcp.js.variant) support.Summary by CodeRabbit
New Features
Refactor