feat(providers): add OpenCode community provider with agents support#1384
Open
cropse wants to merge 15 commits intocoleam00:devfrom
Open
feat(providers): add OpenCode community provider with agents support#1384cropse wants to merge 15 commits intocoleam00:devfrom
cropse wants to merge 15 commits intocoleam00:devfrom
Conversation
…ities - Add OpenCode provider using @opencode-ai/sdk - Support both embedded server and external server modes - Implement session resume, MCP, structured output, env injection - Correctly declare capabilities: hooks, skills, agents, toolRestrictions, effortControl, thinkingControl all supported - Add model/agent validation (one required) - Include E2E smoke workflow and registry tests - Update docs with auth guidance and feature table
… impl Archon has its own agent implementation and should not delegate to OpenCode's agent profiles. Removed the agent field from: - OpencodeProviderDefaults interface - parseOpencodeConfig parsing - streamOpencodeSession function - Updated capabilities to agents: false Model is now required (no agent fallback).
- Flip agents capability from false to true - Add agent adaptation layer that maps nodeConfig.agents to OpenCode API: - Agent selection by sorted key order - Model override from agent config - Tools permissions map (deny wins) - Add 4 tests for agent adaptation behavior - Update smoke test to verify agent field works
|
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:
📝 WalkthroughWalkthroughAdds a new OpenCode (opencode) community provider: implementation, types, registration, tests, docs, package exports/dependencies, core config allowlist update, and two E2E smoke workflows exercising provider and DAG features. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OpencodeProvider
participant Runtime as EmbeddedRuntime/ExternalClient
participant DAG as DAGExecutor
Client->>OpencodeProvider: sendQuery(prompt, cwd, resumeSessionId?)
OpencodeProvider->>Runtime: resolve client or start embedded runtime / probe localhost
alt session resume succeeds
Runtime-->>OpencodeProvider: resumed session events (stream)
else resume fails or new session
Runtime-->>OpencodeProvider: new session events (stream)
end
OpencodeProvider->>OpencodeProvider: normalize events -> MessageChunk(s)
OpencodeProvider->>DAG: yield MessageChunk(s) (assistant/tool/result)
DAG->>Client: consume chunks and continue workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
packages/core/src/config/config-loader.ts (1)
101-101: Allowlist exposesagentbutOpencodeProviderDefaultsdoesn't declare it.The type in
packages/providers/src/types.ts(lines 89-95) only declaresmodelandbaseUrl. The index signature makes this type-safe, but the docs section for OpenCode does referenceagent: buildin the config example. For discoverability and to keep the allowlist honest with the declared shape, add theagentfield (and any other intended defaults) explicitly toOpencodeProviderDefaults:♻️ Proposed type addition (in packages/providers/src/types.ts)
export interface OpencodeProviderDefaults { [key: string]: unknown; /** Default model ref in '<provider>/<model>' format, e.g. 'anthropic/claude-3-5-sonnet' */ model?: string; + /** Default OpenCode agent profile name (e.g. 'build', 'general'). */ + agent?: string; /** Base URL of an existing OpenCode server to connect to. */ baseUrl?: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/config-loader.ts` at line 101, The allowlist in config-loader includes "opencode: ['model', 'agent']" but the OpencodeProviderDefaults type (OpencodeProviderDefaults in packages/providers/src/types.ts) only declares model and baseUrl, causing a mismatch; update the OpencodeProviderDefaults type to explicitly include an agent field (and any other intended default keys referenced in docs/config examples) with the appropriate type (e.g., string or a union that matches expected values like 'build'), so the declared shape matches the allowlist and documentation.packages/providers/package.json (1)
22-22: Test script is a long&&chain — consider switching to a runner or glob.Every new provider test file appends another
&&segment to the test script, and a single failure short-circuits the rest.bun testaccepts multiple paths / supports glob patterns natively, so you can run them in one invocation and get a full report. This is optional and not blocking, but will scale better as more community providers arrive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/package.json` at line 22, The package.json "test" script currently chains many bun test invocations with &&; replace it with a single bun test invocation that accepts multiple paths or a glob (e.g., bun test "src/**/*.test.ts" or bun test src/**/{provider,*.test}.ts) so tests run in one process and report all results; update the "test" script entry in package.json (the "test" field) to use the chosen glob or multi-path form and remove the long && chain.packages/providers/src/community/opencode/registration.ts (1)
6-8: Compatibility predicate is looser than runtime parser.
isOpencodeModelCompatibleaccepts any string containing/(including/foo,foo/,/), butparseModelRefinprovider.tsenforces non-empty segments on both sides. Workflow validation will green-light model refs that later throw at send-time. Consider tightening to match, e.g., requiring a non-empty provider and model segment:♻️ Proposed tighter predicate
export function isOpencodeModelCompatible(model: string): boolean { - return model.includes('/'); + const i = model.indexOf('/'); + return i > 0 && i < model.length - 1; }Additionally, note that Pi's model ref format is also
<provider>/<model>, so both predicates will claim compatibility for the same model string — if provider inference from model name is ever used in ambiguous contexts, the order of registration becomes load-bearing. Worth a brief comment if the design relies on explicitprovider:being set whenever these overlap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/opencode/registration.ts` around lines 6 - 8, The compatibility check in isOpencodeModelCompatible currently accepts any string containing '/', which allows invalid refs like '/foo' or 'foo/' that parseModelRef (in provider.ts) rejects; update isOpencodeModelCompatible to require a non-empty provider and non-empty model segment (i.e., both sides of the slash present) to match parseModelRef's behavior, and add a brief comment noting potential ambiguity with other providers that also use "<provider>/<model>" so registration order or explicit provider: should be used to disambiguate.packages/providers/src/community/opencode/provider.test.ts (1)
496-523:test.skipon abort propagation — consider a follow-up.Abort handling is non-trivial (
abortableStream,abortHandlerinstreamOpencodeSession, and the outer pre-flight check). Leaving the only abort test skipped means regressions in cancellation will go uncaught. Please file a follow-up to un-skip, or replace with a deterministic variant (e.g., abort before the firstawaitso you don't needPromise.resolve()microtask ordering).Want me to open a tracking issue for re-enabling this test?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/opencode/provider.test.ts` around lines 496 - 523, The skipped abort test should be made deterministic instead of left skipped: remove test.skip and update the test that uses OpencodeProvider().sendQuery / consume so the AbortController is aborted synchronously (e.g., call abortController.abort() before the first await or create the abortSignal from an already-aborted AbortController) so you don't rely on microtask ordering (Promise.resolve()). Verify that consume still yields no chunks and that runtime.client.session.abort was called with the expected args; if you prefer, file a follow-up tracking issue to re-enable the original async-abort variant, but ensure the test referencing sendQuery, consume, abortSignal, and the session.abort expectation is un-skipped and deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.archon/workflows/e2e-opencode-smoke.yaml:
- Around line 16-19: The assert node's bash command (node id "assert" reading
"$simple.output") currently uses a shell pipeline that always exits 0 because
the final echo succeeds; change the bash to make the step fail with a non‑zero
exit when "OPENCODE_OK" is missing by replacing the current short-circuit `&&
... || ...` with a conditional that prints PASS on success and prints FAIL and
then exits 1 on failure (or wrap the failure branch in a group that runs exit
1), so the workflow actually fails when the pattern isn't found.
In `@packages/docs-web/src/content/docs/getting-started/ai-assistants.md`:
- Around line 259-260: The heading "# Optional: select a specific agent profile"
is placed after the baseUrl example but describes the agent setting; move that
heading so it appears directly under the agent: field (e.g., immediately below
the "agent: build" line) in the markdown example so readers clearly associate
the comment with the agent configuration rather than the baseUrl entry.
- Line 285: Update the "Inline sub-agents (`agents:`)" capability row to reflect
partial support rather than full parity with Claude: change the cell text to
something like "Partial — supports named/config-file subagents, `@` mentions,
child sessions, task_budget; does NOT support programmatic inline agent arrays
(Claude-style `options.agents`)". Reference the existing note in
packages/providers/src/community/opencode/capabilities.ts and the table row
"Inline sub-agents (`agents:`)" in
packages/docs-web/src/content/docs/getting-started/ai-assistants.md when making
this wording change so users aren't misled about nodeConfig.agents-style inline
definitions.
In `@packages/providers/src/community/opencode/capabilities.ts`:
- Around line 28-29: The package claims effortControl and thinkingControl but
streamOpencodeSession in provider.ts never forwards nodeConfig.effort or
nodeConfig.thinking into the OpenCode request (promptBody), so either wire those
fields through to the request body (add nodeConfig.effort and
nodeConfig.thinking to the promptBody construction in streamOpencodeSession and
any necessary SDK mapping) or change capabilities.ts to set effortControl and
thinkingControl to false (or keep true only with an inline comment like "//
Fallback: honored via opencode.json agent config") so we don't silently swallow
unsupported states; update the code in capabilities.ts and provider.ts
accordingly and ensure any mapping uses the exact symbols nodeConfig.effort,
nodeConfig.thinking, streamOpencodeSession, and capabilities.effortControl/
thinkingControl to locate the spots to change.
In `@packages/providers/src/community/opencode/provider.ts`:
- Around line 23-29: CRASH_PATTERNS currently contains the overly broad token
'terminated' which causes any error message containing that substring to be
treated as a transient crash; update the CRASH_PATTERNS array to replace the
generic 'terminated' with more specific anchored phrases (for example
'connection terminated' and/or 'process terminated') or remove it entirely so
only explicit connection/socket process failure messages are matched; ensure you
modify the CRASH_PATTERNS constant in provider.ts and run/update any tests that
assert crash-detection behavior accordingly.
- Around line 327-336: The releaseEmbeddedRuntime routine currently
unconditionally clears the global embeddedRuntimePromise and decrements
runtime.refCount, causing races with overlapping sendQuery calls; update
releaseEmbeddedRuntime(runtimePromise, runtime) to skip decrement when
runtime.refCount is already 0 and only clear embeddedRuntimePromise if
embeddedRuntimePromise === the promise associated with this runtime (i.e., guard
the finally block so it doesn't wipe out a newer promise), and in sendQuery
capture the promise returned when acquiring the runtime (store it in a local
like acquiredPromise) and pass that promise (or otherwise bind it) to
releaseEmbeddedRuntime so the clear check can compare against the exact promise
that was used to acquire the runtime.
- Around line 250-276: Replace the liveness probe in tryExistingServer so it
calls the lightweight health endpoint instead of creating sessions: import/use
client.global.health() and check the returned { healthy } flag (and version if
desired) rather than calling client.session.create (which creates orphan
sessions); keep the existing OPENCODE_DEFAULT_PORT and logging behavior
(getLog().info / debug) but remove the directory: process.cwd() parameter from
the health check (or change tryExistingServer to accept the caller's cwd if you
must pass a specific cwd from sendQuery), and only return the client when the
health response indicates healthy === true, otherwise treat connection-refused
the same way and return null or propagate other errors.
- Around line 596-623: The abortableStream generator currently drops the
in-flight nextPromise when signal aborts, leaking the underlying iterator;
update abort handling in abortableStream to call iterator.return?.() (and await
it) when the abort branch wins the Promise.race so the upstream iterator can
clean up resources, ensure the abort event listener is removed, and only then
return from the generator; reference the existing iterator variable and the
abort signal logic inside abortableStream to implement this change.
---
Nitpick comments:
In `@packages/core/src/config/config-loader.ts`:
- Line 101: The allowlist in config-loader includes "opencode: ['model',
'agent']" but the OpencodeProviderDefaults type (OpencodeProviderDefaults in
packages/providers/src/types.ts) only declares model and baseUrl, causing a
mismatch; update the OpencodeProviderDefaults type to explicitly include an
agent field (and any other intended default keys referenced in docs/config
examples) with the appropriate type (e.g., string or a union that matches
expected values like 'build'), so the declared shape matches the allowlist and
documentation.
In `@packages/providers/package.json`:
- Line 22: The package.json "test" script currently chains many bun test
invocations with &&; replace it with a single bun test invocation that accepts
multiple paths or a glob (e.g., bun test "src/**/*.test.ts" or bun test
src/**/{provider,*.test}.ts) so tests run in one process and report all results;
update the "test" script entry in package.json (the "test" field) to use the
chosen glob or multi-path form and remove the long && chain.
In `@packages/providers/src/community/opencode/provider.test.ts`:
- Around line 496-523: The skipped abort test should be made deterministic
instead of left skipped: remove test.skip and update the test that uses
OpencodeProvider().sendQuery / consume so the AbortController is aborted
synchronously (e.g., call abortController.abort() before the first await or
create the abortSignal from an already-aborted AbortController) so you don't
rely on microtask ordering (Promise.resolve()). Verify that consume still yields
no chunks and that runtime.client.session.abort was called with the expected
args; if you prefer, file a follow-up tracking issue to re-enable the original
async-abort variant, but ensure the test referencing sendQuery, consume,
abortSignal, and the session.abort expectation is un-skipped and deterministic.
In `@packages/providers/src/community/opencode/registration.ts`:
- Around line 6-8: The compatibility check in isOpencodeModelCompatible
currently accepts any string containing '/', which allows invalid refs like
'/foo' or 'foo/' that parseModelRef (in provider.ts) rejects; update
isOpencodeModelCompatible to require a non-empty provider and non-empty model
segment (i.e., both sides of the slash present) to match parseModelRef's
behavior, and add a brief comment noting potential ambiguity with other
providers that also use "<provider>/<model>" so registration order or explicit
provider: should be used to disambiguate.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6551a8b7-b41c-4813-9105-2ded36faf942
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.archon/workflows/e2e-opencode-all-nodes-smoke.yaml.archon/workflows/e2e-opencode-smoke.yamlpackage.jsonpackages/core/src/config/config-loader.tspackages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/providers/package.jsonpackages/providers/src/community/opencode/capabilities.tspackages/providers/src/community/opencode/config.tspackages/providers/src/community/opencode/index.tspackages/providers/src/community/opencode/provider.test.tspackages/providers/src/community/opencode/provider.tspackages/providers/src/community/opencode/registration.tspackages/providers/src/index.tspackages/providers/src/registry.test.tspackages/providers/src/registry.tspackages/providers/src/types.ts
- Fix assert node to fail with exit 1 when pattern not found - Set effortControl/thinkingControl to false (not wired to SDK) - Replace generic 'terminated' with specific crash patterns - Add TODO for health endpoint (SDK limitation) - Fix race condition in releaseEmbeddedRuntime - Call iterator.return on abort in abortableStream - Tighten isOpencodeModelCompatible validation - Add agent field to OpencodeProviderDefaults type
Author
|
Addressed all 8 actionable comments from CodeRabbit review:
Nitpick items (documentation) will be addressed in a follow-up if needed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/providers/src/community/opencode/provider.ts (1)
160-174: Minor:selectPrimaryAgentpicks alphabetically first agent — consider documenting or using insertion order.When
nodeConfig.agentscontains multiple entries,Object.keys(agents).sort()[0]picks the lexicographically smallest name, which may surprise users who expect declaration order (e.g.,{ senior, junior }selectsjunior). Either document this semantic explicitly on the function/capability, or drop the.sort()so insertion order is honored (ES2015+ preserves it for string keys). Not blocking — single-agent usage is the common case.♻️ Optional change
function selectPrimaryAgent(agents: Record<string, AgentConfig>): string | undefined { - const agentNames = Object.keys(agents).sort(); - return agentNames[0]; + // Honor insertion order — first declared agent wins. + return Object.keys(agents)[0]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/opencode/provider.ts` around lines 160 - 174, selectPrimaryAgent currently sorts agent names lexicographically which changes selection from insertion (declaration) order; update selectPrimaryAgent to respect insertion order by removing the .sort() call on Object.keys(agents) (i.e., use Object.keys(agents) and return its first element), and add a short comment on selectPrimaryAgent (or related nodeConfig.agents docs) stating that selection follows insertion order to make the behavior explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/providers/src/community/opencode/provider.ts`:
- Around line 160-174: selectPrimaryAgent currently sorts agent names
lexicographically which changes selection from insertion (declaration) order;
update selectPrimaryAgent to respect insertion order by removing the .sort()
call on Object.keys(agents) (i.e., use Object.keys(agents) and return its first
element), and add a short comment on selectPrimaryAgent (or related
nodeConfig.agents docs) stating that selection follows insertion order to make
the behavior explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e5bbef6-aa7d-49db-b29d-06d4f4b159b1
📒 Files selected for processing (5)
.archon/workflows/e2e-opencode-smoke.yamlpackages/providers/src/community/opencode/capabilities.tspackages/providers/src/community/opencode/provider.tspackages/providers/src/community/opencode/registration.tspackages/providers/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .archon/workflows/e2e-opencode-smoke.yaml
- packages/providers/src/community/opencode/registration.ts
- Fix race condition: capture runtime instance at acquire time - Add agent field parsing in parseOpencodeConfig - Tighten isOpencodeModelCompatible to trim whitespace - Update registry test for effortControl/thinkingControl
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/providers/src/community/opencode/provider.ts (3)
713-721: RedundantnodeConfigargument.
requestOptionsandrequestOptions?.nodeConfigare both passed, andstreamOpencodeSessionat line 448-450 re-resolvesnodeConfig ?? requestOptions?.nodeConfig. Since they are always the same value here, the explicit 7th argument is noise; drop one of the two fallbacks for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/opencode/provider.ts` around lines 713 - 721, Call to streamOpencodeSession passes both requestOptions and requestOptions?.nodeConfig redundantly; remove the explicit seventh argument (requestOptions?.nodeConfig) so the call becomes streamOpencodeSession(runtime.client, cwd, sessionId, prompt, parsedModel, requestOptions) and let streamOpencodeSession re-resolve nodeConfig internally (it already checks nodeConfig ?? requestOptions?.nodeConfig).
554-561:session.errorloses structure by stringifying to JSON.
throw new Error(JSON.stringify(rawError))flattens structured error details (status codes, nesteddata, etc.) into a single message, making downstream classification (classifyOpencodeError) rely on substring matches over JSON text and breakingerr.causechains. Consider attachingcauseor a richer shape so status/type fields remain programmatically accessible.♻️ Suggested change
- const rawError = isRecord(properties.error) ? properties.error : properties; - throw new Error(JSON.stringify(rawError)); + const rawError = isRecord(properties.error) ? properties.error : properties; + const msg = + (isRecord(rawError) && typeof rawError.message === 'string' && rawError.message) || + JSON.stringify(rawError); + const err = new Error(msg); + (err as Error & { data?: unknown }).data = rawError; + throw err;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/opencode/provider.ts` around lines 554 - 561, The current session.error handler flattens structured error details by doing throw new Error(JSON.stringify(rawError)); change this to throw a richer error that preserves the original structure (e.g., attach rawError as the Error.cause or construct a small OpencodeSessionError that includes fields like status, type and nested data) so downstream code such as classifyOpencodeError can access status/type/cause programmatically; update the session.error branch (the event.type === 'session.error' block that defines rawError) to create and throw an Error (or Error subclass) that retains rawError instead of stringifying it.
160-169: Duplicated model-ref validation withregistration.ts.
parseModelRefhere andisOpencodeModelCompatibleinregistration.tsimplement the same<provider>/<model>validation rules (non-empty trimmed segments on both sides of/). They can drift. Consider extracting a shared helper (e.g., inconfig.ts) and havingisOpencodeModelCompatibledelegate toparseModelRef(...) !== null.♻️ Suggested consolidation
# in registration.ts -export function isOpencodeModelCompatible(model: string): boolean { - const i = model.indexOf('/'); - if (i <= 0 || i >= model.length - 1) return false; - const provider = model.slice(0, i).trim(); - const modelName = model.slice(i + 1).trim(); - return provider.length > 0 && modelName.length > 0; -} +import { parseModelRef } from './provider'; +export function isOpencodeModelCompatible(model: string): boolean { + return parseModelRef(model) !== null; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/opencode/provider.ts` around lines 160 - 169, The validation logic for "<provider>/<model>" is duplicated between parseModelRef and isOpencodeModelCompatible; extract the shared check into a single helper (e.g., export a function like validateModelRef or parseModelRef) in a shared module (suggest config.ts) and update both parseModelRef (or replace it) and isOpencodeModelCompatible to call that helper (e.g., isOpencodeModelCompatible => return validateModelRef(modelRef) !== null) so the rule is implemented once and reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/providers/src/community/opencode/provider.ts`:
- Around line 602-611: The generator can finish without emitting a terminal
result when the upstream stream closes before emitting session.idle; update the
code in provider.ts (inside the async generator that uses aborted, sessionId,
sessionIdleEmitted, streamController, abortHandler) so that when the for-await
loop exits cleanly and aborted is false and sessionIdleEmitted[sessionId] is not
true, you emit a single terminal chunk like { type: 'result', sessionId,
isError: true, errorSubtype: 'stream_ended_without_idle' } (or alternatively
throw a specific Error to trigger retry/classification) before removing the
abort listener and calling streamController.abort(); ensure this check runs
before the finally block releases resources so downstream executors always
receive exactly one terminal result.
- Around line 259-285: In tryExistingServer(), replace the transient session
health probe client.session.create({ query: { directory: process.cwd() } }) with
the stateless client.global.health() call so the check doesn't create orphan
sessions or rely on cwd; keep the existing connection-refused detection logic
(checking error.message for 'Unable to
connect'|'ConnectionRefused'|'ECONNREFUSED') and return null on
connection-refused, otherwise rethrow, and leave resolveSessionId() to create
sessions with the correct directory context; use OPENCODE_DEFAULT_PORT and
getLog() as before to log found/no-server outcomes.
---
Nitpick comments:
In `@packages/providers/src/community/opencode/provider.ts`:
- Around line 713-721: Call to streamOpencodeSession passes both requestOptions
and requestOptions?.nodeConfig redundantly; remove the explicit seventh argument
(requestOptions?.nodeConfig) so the call becomes
streamOpencodeSession(runtime.client, cwd, sessionId, prompt, parsedModel,
requestOptions) and let streamOpencodeSession re-resolve nodeConfig internally
(it already checks nodeConfig ?? requestOptions?.nodeConfig).
- Around line 554-561: The current session.error handler flattens structured
error details by doing throw new Error(JSON.stringify(rawError)); change this to
throw a richer error that preserves the original structure (e.g., attach
rawError as the Error.cause or construct a small OpencodeSessionError that
includes fields like status, type and nested data) so downstream code such as
classifyOpencodeError can access status/type/cause programmatically; update the
session.error branch (the event.type === 'session.error' block that defines
rawError) to create and throw an Error (or Error subclass) that retains rawError
instead of stringifying it.
- Around line 160-169: The validation logic for "<provider>/<model>" is
duplicated between parseModelRef and isOpencodeModelCompatible; extract the
shared check into a single helper (e.g., export a function like validateModelRef
or parseModelRef) in a shared module (suggest config.ts) and update both
parseModelRef (or replace it) and isOpencodeModelCompatible to call that helper
(e.g., isOpencodeModelCompatible => return validateModelRef(modelRef) !== null)
so the rule is implemented once and reused.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 370386c9-56cf-4276-9f5f-ab0b3172f250
📒 Files selected for processing (4)
packages/providers/src/community/opencode/config.tspackages/providers/src/community/opencode/provider.tspackages/providers/src/community/opencode/registration.tspackages/providers/src/registry.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/providers/src/community/opencode/config.ts
- Replace session.create() health check with global.health() (stateless) - Yield terminal result chunk when stream ends before session.idle - Move comment under agent: field in ai-assistants.md - Change 'Inline sub-agents' support to⚠️ Partial - Preserve insertion order in selectPrimaryAgent (remove .sort()) - Remove redundant nodeConfig argument from streamOpencodeSession - Preserve error structure in session.error handler (err.cause) - Consolidate model-ref validation (parseModelRef in registration.ts) - Update test mocks to include global.health()
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/providers/src/community/opencode/registration.ts (1)
3-6: Consolidate the two imports from./provider.
OpencodeProvider(line 4) andparseModelRef(line 6) are imported from the same module in separate statements.♻️ Proposed change
import { OPENCODE_CAPABILITIES } from './capabilities'; -import { OpencodeProvider } from './provider'; - -import { parseModelRef } from './provider'; +import { OpencodeProvider, parseModelRef } from './provider';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/opencode/registration.ts` around lines 3 - 6, Consolidate the duplicate imports from the same module by combining OpencodeProvider and parseModelRef into a single import from './provider' (replace the separate imports of OpencodeProvider and parseModelRef with one statement importing both symbols).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/providers/src/community/opencode/provider.test.ts`:
- Around line 505-532: Unskip the skipped test 'abort propagates to the OpenCode
session and surfaces aborted error' (or if there is a known blocker, create a
follow-up issue and add a TODO referencing that issue) so the abort path is
covered by CI; ensure the test runs against OpencodeProvider.sendQuery and the
abort flow (abortHandler → client.session.abort → streamController.abort →
abortableStream iterator.return) remains covered and deterministic by keeping
the existing pending stream pattern and microtask synchronization
(queueMicrotask/await Promise.resolve()) used in the test, and if filing an
issue include the test name and describe the race/iterator-leak scenario it
guards.
In `@packages/providers/src/community/opencode/provider.ts`:
- Around line 174-177: selectPrimaryAgent silently picks the first
insertion-ordered agent and drops others; change it to detect when
Object.keys(agents).length > 1 and emit a one-time warning that includes the
selected agent name and the full list/count of configured agents (use a
module-level boolean like warnedMultipleAgents to avoid spamming), then return
the same agent; apply the same warning behavior to the other OpenCode per-call
agent selection/mapping code that currently discards extras so users see the
lossy mapping.
- Around line 256-282: The health probe in tryExistingServer currently awaits
client.global.health() with no timeout; modify tryExistingServer to race
client.global.health() against a short timeout Promise (e.g., 2s) so the call
rejects/returns null on timeout, treat that timeout the same as the existing
connection-refused branch (log opencode.no_existing_server and return null), and
ensure you reference OPENCODE_DEFAULT_PORT and client.global.health() in the
implementation; also thread any available abort signal from
acquireEmbeddedRuntime into the timeout if present so the probe can be
cancelled.
---
Nitpick comments:
In `@packages/providers/src/community/opencode/registration.ts`:
- Around line 3-6: Consolidate the duplicate imports from the same module by
combining OpencodeProvider and parseModelRef into a single import from
'./provider' (replace the separate imports of OpencodeProvider and parseModelRef
with one statement importing both symbols).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12cd784a-95f4-4521-85b7-cda3a8646d31
📒 Files selected for processing (4)
packages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/providers/src/community/opencode/provider.test.tspackages/providers/src/community/opencode/provider.tspackages/providers/src/community/opencode/registration.ts
✅ Files skipped from review due to trivial changes (1)
- packages/docs-web/src/content/docs/getting-started/ai-assistants.md
Comment on lines
+505
to
+532
| test.skip('abort propagates to the OpenCode session and surfaces aborted error', async () => { | ||
| const runtime = makeRuntime({ | ||
| subscribe: mock(async () => ({ | ||
| stream: createPendingStream(), | ||
| })), | ||
| }); | ||
| runtimeQueue.push(runtime); | ||
| const abortController = new AbortController(); | ||
|
|
||
| const consumption = consume( | ||
| new OpencodeProvider().sendQuery('hi', '/tmp', undefined, { | ||
| assistantConfig: TEST_MODEL, | ||
| abortSignal: abortController.signal, | ||
| }) | ||
| ); | ||
|
|
||
| await Promise.resolve(); | ||
| abortController.abort(); | ||
|
|
||
| const { chunks, error } = await consumption; | ||
|
|
||
| expect(chunks).toEqual([]); | ||
| expect(error?.message).toBe('OpenCode query aborted'); | ||
| expect(runtime.client.session.abort).toHaveBeenCalledWith({ | ||
| path: { id: 'session-1' }, | ||
| query: { directory: '/tmp' }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Skipped abort test leaves the most failure-prone path uncovered.
The abort codepath (abortHandler → client.session.abort → streamController.abort() → abortableStream's iterator.return?.()) was specifically hardened in this PR to address iterator leaks and race conditions, yet the only behavioral test for it is test.skip'd. Any future regression in abort handling (e.g., forgetting to remove the abort listener, or reintroducing the iterator leak) will not be caught by CI.
Please either unskip this test (the pending-stream pattern at line 51-59 combined with queueMicrotask/await Promise.resolve() should make it deterministic) or file a follow-up issue and leave a TODO noting the reason it's skipped.
🧪 One way to stabilize the abort test
- test.skip('abort propagates to the OpenCode session and surfaces aborted error', async () => {
+ test('abort propagates to the OpenCode session and surfaces aborted error', async () => {
const runtime = makeRuntime({
subscribe: mock(async () => ({
stream: createPendingStream(),
})),
});
runtimeQueue.push(runtime);
const abortController = new AbortController();
- const consumption = consume(
+ const gen = new OpencodeProvider().sendQuery('hi', '/tmp', undefined, {
+ assistantConfig: TEST_MODEL,
+ abortSignal: abortController.signal,
+ });
+ const consumption = consume(gen);
- await Promise.resolve();
+ // Let sendQuery reach the `for await` on the pending stream before aborting.
+ await new Promise(r => setTimeout(r, 10));
abortController.abort();
const { chunks, error } = await consumption;Do you want me to open a follow-up issue tracking the skipped abort test?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/community/opencode/provider.test.ts` around lines 505
- 532, Unskip the skipped test 'abort propagates to the OpenCode session and
surfaces aborted error' (or if there is a known blocker, create a follow-up
issue and add a TODO referencing that issue) so the abort path is covered by CI;
ensure the test runs against OpencodeProvider.sendQuery and the abort flow
(abortHandler → client.session.abort → streamController.abort → abortableStream
iterator.return) remains covered and deterministic by keeping the existing
pending stream pattern and microtask synchronization (queueMicrotask/await
Promise.resolve()) used in the test, and if filing an issue include the test
name and describe the race/iterator-leak scenario it guards.
- Add warning when multiple agents configured (first wins) - Add 2s timeout to global.health() probe - Add TODO for skipped abort test - Consolidate imports in registration.ts - Fix TypeScript error: use deferred pattern for creationPromise
Author
|
Addressed all 4 actionable comments + 1 nitpick from latest review:
Also fixed a TypeScript error in \�cquireEmbeddedRuntime\ where \promise\ was used before being assigned - now uses a deferred pattern. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/providers/src/community/opencode/provider.test.ts (1)
446-446: Prefer asserting the specific debug event over a raw call count.
toHaveBeenCalledTimes(1)will break if any new unrelated debug log is added to the happy path (e.g., someone adds aopencode.session_resume_faileddebug log). Asserting against the specific event name makes the test robust to unrelated additions.♻️ Suggested change
- expect(mockLogger.debug).toHaveBeenCalledTimes(1); + expect(mockLogger.debug).toHaveBeenCalledWith( + expect.objectContaining({ err: expect.any(Error), sessionId: 'session-1' }), + 'opencode.structured_output_lookup_failed' + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/opencode/provider.test.ts` at line 446, Replace the brittle call-count assertion on mockLogger.debug with an assertion that the specific debug event for the happy path was logged: locate the assertion using mockLogger.debug in provider.test.ts and change it to assert that mockLogger.debug was called with the expected event name (e.g., the opencode session-resume success event string used elsewhere in the test), so the test verifies the exact debug event rather than relying on toHaveBeenCalledTimes(1).packages/providers/src/community/opencode/provider.ts (1)
390-403: Consider logging the swallowed resume error for debuggability.The
catch {}silently discards the underlying reason for the resume failure (expired session, network error, permission issue, etc.) — the user only sees a generic "Could not resume" warning downstream and has no way to diagnose. A one-line debug log preserves the info without changing behavior.♻️ Suggested fix
- } catch { - // Fall through to fresh session creation and surface a warning upstream. + } catch (error) { + getLog().debug( + { err: error, resumeSessionId, cwd }, + 'opencode.session_resume_failed' + ); + // Fall through to fresh session creation and surface a warning upstream. }As per coding guidelines: "never silently swallow errors" — document fallback behavior with a comment when intentional; pair with a debug log so operators can diagnose production resume failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/opencode/provider.ts` around lines 390 - 403, The catch block that swallows errors when attempting to resume a session (around resumeSessionId and client.session.get with query { directory: cwd }) should log the caught error for debuggability; update the catch to capture the error (catch (err)) and emit a debug-level log using the local logger used in this module (e.g., processLogger.debug or similar) or console.debug if no logger is available, including context like resumeSessionId and cwd, then fall through as before so behavior doesn't change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/providers/src/community/opencode/provider.ts`:
- Around line 364-378: In releaseEmbeddedRuntime, when runtime.refCount reaches
0 always call runtime.server.close() for that specific runtime to avoid leaking
servers even if embeddedRuntimePromise no longer equals runtime.creationPromise;
only the cache-clearing assignment to embeddedRuntimePromise should remain gated
by the condition (embeddedRuntimePromise === runtime.creationPromise). Update
releaseEmbeddedRuntime to first close the runtime.server (with appropriate
try/catch/finally handling per current style), then inside the existing if block
clear embeddedRuntimePromise = undefined; reference the function name
releaseEmbeddedRuntime and the symbols runtime.server.close,
embeddedRuntimePromise, and runtime.creationPromise when locating the change
(tests that call resetEmbeddedRuntime should no longer cause leaks).
- Around line 300-362: The deferred Promise created with only resolve (in
acquireEmbeddedRuntime) can remain forever pending if the async IIFE throws
before calling resolveRuntime, leaving concurrent awaiters hung; fix by creating
and assigning a real Promise (e.g., name it `created`) that is immediately set
to the async IIFE's returned promise, assign `embeddedRuntimePromise = created`
before awaiting it, and pass that same `created` promise into the runtime
object's `creationPromise` field instead of using the manual resolve pattern —
this way any rejection from tryExistingServer / import / findAvailablePort /
createOpencode will reject `created` and propagate to all awaiting callers
rather than leaving them suspended.
---
Nitpick comments:
In `@packages/providers/src/community/opencode/provider.test.ts`:
- Line 446: Replace the brittle call-count assertion on mockLogger.debug with an
assertion that the specific debug event for the happy path was logged: locate
the assertion using mockLogger.debug in provider.test.ts and change it to assert
that mockLogger.debug was called with the expected event name (e.g., the
opencode session-resume success event string used elsewhere in the test), so the
test verifies the exact debug event rather than relying on
toHaveBeenCalledTimes(1).
In `@packages/providers/src/community/opencode/provider.ts`:
- Around line 390-403: The catch block that swallows errors when attempting to
resume a session (around resumeSessionId and client.session.get with query {
directory: cwd }) should log the caught error for debuggability; update the
catch to capture the error (catch (err)) and emit a debug-level log using the
local logger used in this module (e.g., processLogger.debug or similar) or
console.debug if no logger is available, including context like resumeSessionId
and cwd, then fall through as before so behavior doesn't change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11be4d85-c511-4a32-b138-8d924edb768f
📒 Files selected for processing (3)
packages/providers/src/community/opencode/provider.test.tspackages/providers/src/community/opencode/provider.tspackages/providers/src/community/opencode/registration.ts
3 tasks
- Fix deferred pattern hang: wire both resolve and reject in deferred promise so startup errors propagate to callers (3137799074) - Fix server close leak: decouple server.close() from cache identity check in releaseEmbeddedRuntime (3137799084) - Update TODO reference to follow-up issue coleam00#1400 for abort test (3136883117)
Author
Review Feedback Addressed✅ Fixed3137799074 (Major) — Deferred pattern hang
3137799084 (Minor) — Server close leak
ℹ️ Already Addressed3134660170 (Minor) — Missing terminal result chunk
📋 Follow-Up3136883117 (Minor) — Skipped abort test
Commit: 878235a |
The SDK's global.health() method only exists in v2, but we import from the root SDK which uses the old client. Switch to direct HTTP fetch to /global/health endpoint for checking existing servers. - Remove global.health from OpencodeClientLike interface - Use fetch() directly with 2s timeout for health check - Update tests to mock fetch for health check scenarios
3 tasks
Extract runtime, session, multi-agent, agent-config, agent-fs, and error handling into separate files to reduce provider.ts complexity. Add inline multi-agent e2e workflow and expand test coverage.
Add hook-node to e2e smoke workflow covering PreToolUse/PostToolUse hooks (10 node types total). Switch smoke model to cpamc/minimax. Remove deprecated baseUrl option and refresh feature support table in docs.
Collaborator
Collaborator
|
Thanks for the implementation, @cropse! Heads up: there's another OpenCode community provider PR open at #1372 (@choufeng). Could you two coordinate on which one to land? Only one OpenCode community provider should merge. Since this is a community provider, I won't review or run it deeply myself. For evidence-to-merge, please share:
A smoke test workflow in Once you and @choufeng align on a single PR and the evidence is in, happy to merge. |
Author
Copy that!! I can compare that PR and discuss the implementation diff from #1372 and send PR into this branch? If you don't mind I can do it then let you review or any design suggestion? @choufeng. The test evidence will be provided later. |
Author
Models tested:minimax, kimi, big pickle(default general agent, literally GLM4.7, it's build-in models so will all based on this one) Configs used:> cat config.yaml
assistant: opencode
assistants:
opencode:
model: cpamc/minimaxResults: Smoke test:commands:
All node smoke test:commands:
Inline agent Smoke test:commands:
|
Switch from cpamc/minimax to opencode/big-pickle (provider default) for general e2e testing of OpenCode provider.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nodeConfig.agents. Wiring them together unlocks heterogeneous-agent workflows (e.g. a planner agent on GPT-4o and an implementer on Claude Sonnet in the same workflow node).How OpenCode Agents Work
TLDR: I literally copy&paste inline agents into .opencode workspace config folder to create a custom agent, since opencode agent are config based.
OpenCode's agent system uses markdown files under
.opencode/agents/to define named subagents. Each file carries YAML front-matter (mode,model,steps,skills,tools) and an optional body that becomes the agent's system prompt.Archon maps
nodeConfig.agentsto this system in two steps:Step 1 — Materialize agent files (
agent-fs.ts)Before each run,
materializeAgents()writes one.opencode/agents/archon-<name>.mdfile per configured agent. Agent names are kebab-cased and prefixedarchon-to avoid collisions with user-owned files. Stalearchon-*files from previous runs are removed.Step 2 — Select agent at session creation (
agent-config.ts,session.ts)adaptNamedAgentForOpencode()converts anAgentConfigto OpenCode session body fields:agentarchon-plannermodelmodel.providerID+model.modelIDprovider/modelreftools/disallowedToolstoolsmappromptmaxTurnsstepsin agent fileSingle-Agent vs Multi-Agent
Single agent (
provider.ts→session.ts): one OpenCode session is created, the configured agent is passed in the prompt body, and chunks stream back directly.Multi-agent (
provider.ts→multi-agent.ts): whennodeConfig.agentshas more than one entry, a parallel fan-out runs:Aggregated output is formatted as:
Architecture Diagram
Before
After
UX Journey
Before
After — single agent
After — multi-agent
Label Snapshot
risk: lowsize: Lprovidersproviders:opencodeChange Metadata
featureprovidersLinked Issue
Validation Evidence
Results:
Security Impact
.opencode/agents/archon-*.mdinside the workflow cwdCompatibility / Migration
Human Verification
agent: generalpasses end-to-endSide Effects / Blast Radius
archon-*agent files are written into the workflow cwd and cleaned up on each run; user-owned files with other names are untouchedRollback Plan
git revert <commit-sha>Risks and Mitigations
OpencodeClientLikeis a structural interface typed only to the methods Archon uses, making it easier to adapt.opencode/agents/filesarchon-; stale ones are removed on each run; other files are untouchedabortAll()sends abort requests to every active session on abort signal or stream error