Refactor OpenCode lifecycle and structured output handling#2218
Refactor OpenCode lifecycle and structured output handling#2218juliusmarminge merged 9 commits intomainfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Commented-out debug code with console.log statements
- Removed the 23-line commented-out block containing the old Effect.tryPromise implementation with console.log debug statements, as it was replaced by the active implementation above.
Or push these changes by commenting:
@cursor push b6b6a40711
Preview (b6b6a40711)
diff --git a/apps/server/src/provider/opencodeRuntime.ts b/apps/server/src/provider/opencodeRuntime.ts
--- a/apps/server/src/provider/opencodeRuntime.ts
+++ b/apps/server/src/provider/opencodeRuntime.ts
@@ -535,29 +535,6 @@
}),
),
);
- // Effect.tryPromise({
- // try: async () => {
- // const [providerListResult, agentsResult] = await Promise.all([
- // client.provider.list(),
- // client.app.agents(),
- // ]);
- // console.log(JSON.stringify(providerListResult, null, 4));
- // console.log(JSON.stringify(agentsResult, null, 4));
- // if (!providerListResult.data) {
- // throw new Error("OpenCode provider inventory was empty.");
- // }
- // return {
- // providerList: providerListResult.data,
- // agents: agentsResult.data ?? [],
- // } satisfies OpenCodeInventory;
- // },
- // catch: (cause) =>
- // new OpenCodeRuntimeError({
- // operation: "loadOpenCodeInventory",
- // detail: `Failed to load OpenCode inventory: ${openCodeRuntimeErrorDetail(cause)}`,
- // cause: cause,
- // }),
- // });
return {
startOpenCodeServerProcess,You can send follow-ups to the cloud agent here.
ApprovabilityVerdict: Needs human review Major refactor replacing Promise-based OpenCode server lifecycle management with Effect's Scope-based patterns. Changes fundamental process spawning, session management, and cleanup logic across multiple files. Three unresolved medium-severity review comments identify potential bugs including lost error details and removed variant fallbacks. Requires human review due to scope and complexity of infrastructure changes. You can customize Macroscope's approvability policy. Learn more. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
- Centralize OpenCode runtime wiring behind an Effect service - Clean up session shutdown and event fibers - Update tests and traits picker model handling
- Remove manual close handles from runtime server/session flows - Add scope-based cleanup for shared servers and adapter shutdown - Update tests for lifecycle finalizers and shutdown behavior
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Error detail lost using
cause.messageinstead of helper- Replaced
cause instanceof Error ? cause.message : ...withopenCodeRuntimeErrorDetail(cause)in the startSession mapError handler, matching the pattern used at the two other error-mapping sites in the same file.
- Replaced
- ✅ Fixed: Duplicate
OpenCodeInventoryinterface across two files- Removed the duplicate interface from OpenCodeProvider.ts and replaced it with a type re-export from the canonical definition in opencodeRuntime.ts.
Or push these changes by commenting:
@cursor push 4152284ed1
Preview (4152284ed1)
diff --git a/apps/server/src/provider/Layers/OpenCodeAdapter.ts b/apps/server/src/provider/Layers/OpenCodeAdapter.ts
--- a/apps/server/src/provider/Layers/OpenCodeAdapter.ts
+++ b/apps/server/src/provider/Layers/OpenCodeAdapter.ts
@@ -1015,8 +1015,7 @@
: new ProviderAdapterProcessError({
provider: PROVIDER,
threadId: input.threadId,
- detail:
- cause instanceof Error ? cause.message : "Failed to start OpenCode session.",
+ detail: openCodeRuntimeErrorDetail(cause),
cause,
}),
),
diff --git a/apps/server/src/provider/Layers/OpenCodeProvider.ts b/apps/server/src/provider/Layers/OpenCodeProvider.ts
--- a/apps/server/src/provider/Layers/OpenCodeProvider.ts
+++ b/apps/server/src/provider/Layers/OpenCodeProvider.ts
@@ -15,7 +15,11 @@
providerModelsFromSettings,
} from "../providerSnapshot.ts";
import { OpenCodeProvider } from "../Services/OpenCodeProvider.ts";
-import { OpenCodeRuntime, openCodeRuntimeErrorDetail } from "../opencodeRuntime.ts";
+import {
+ OpenCodeRuntime,
+ openCodeRuntimeErrorDetail,
+ type OpenCodeInventory,
+} from "../opencodeRuntime.ts";
import type { Agent, ProviderListResponse } from "@opencode-ai/sdk/v2";
const PROVIDER = "opencode" as const;
@@ -124,10 +128,7 @@
};
}
-export interface OpenCodeInventory {
- readonly providerList: ProviderListResponse;
- readonly agents: ReadonlyArray<Agent>;
-}
+export type { OpenCodeInventory };
function titleCaseSlug(value: string): string {
return valueYou can send follow-ups to the cloud agent here.
| provider: PROVIDER, | ||
| threadId: input.threadId, | ||
| detail: | ||
| cause instanceof Error ? cause.message : "Failed to start OpenCode session.", |
There was a problem hiding this comment.
Error detail lost using cause.message instead of helper
Medium Severity
When connectToOpenCodeServer fails with an OpenCodeRuntimeError (which extends Data.TaggedError), the mapError handler at line 1018 reads cause.message to build the ProviderAdapterProcessError detail. However, Data.TaggedError doesn't set a meaningful .message — the human-readable text lives in .detail. Other error-mapping sites in this same file (lines 889, 991) correctly use openCodeRuntimeErrorDetail(cause), which extracts .detail for OpenCodeRuntimeError instances. This inconsistency causes the error detail surfaced to users to be empty or unhelpful when server connection fails.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ac8ee91. Configure here.
juliusmarminge
force-pushed
the
t3code/effect-lifecycle-cleanup
branch
from
ac8ee91 to
73a76eb
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Redundant
OpenCodeRuntimeLiveprovision creates duplicate layer paths- Removed the redundant
Layer.provideMerge(OpenCodeRuntimeLive)fromRuntimeDependenciesLiveinserver.ts, leavingProviderRegistryLiveas the single canonical provision site forOpenCodeRuntime.
- Removed the redundant
Or push these changes by commenting:
@cursor push 2803bdad88
Preview (2803bdad88)
diff --git a/apps/server/src/server.ts b/apps/server/src/server.ts
--- a/apps/server/src/server.ts
+++ b/apps/server/src/server.ts
@@ -72,7 +72,6 @@
makePersistedServerRuntimeState,
persistServerRuntimeState,
} from "./serverRuntimeState.ts";
-import { OpenCodeRuntimeLive } from "./provider/opencodeRuntime.ts";
import {
orchestrationDispatchRouteLayer,
orchestrationSnapshotRouteLayer,
@@ -227,7 +226,6 @@
const RuntimeDependenciesLive = ReactorLayerLive.pipe(
// Core Services
- Layer.provideMerge(OpenCodeRuntimeLive),
Layer.provideMerge(CheckpointingLayerLive),
Layer.provideMerge(GitLayerLive),
Layer.provideMerge(ProviderRuntimeLayerLive),You can send follow-ups to the cloud agent here.
`OpenCodeSessionContext` kept three parallel lifecycle handles next to `sessionScope`: `eventsFiber`, `exitFiber`, and `eventsAbortController`. They existed because the event-pump and server-exit fibers were forked via `Effect.runForkWith(runtimeContext)`, which bypasses the scope tree, so cleanup had to interrupt each fiber and abort the controller explicitly. Switch both fibers to `Effect.forkIn(context.sessionScope)` and register `AbortController.abort()` as a `Scope.addFinalizer` on the same scope. Closing the session scope now atomically interrupts the fibers, aborts the in-flight `event.subscribe` fetch, and — for scope-owned servers — tears down the child process. The context shrinks to just `sessionScope` plus a `Ref<boolean>` for the race-safe stop flag. Along the way: replace hand-written `isProviderAdapter*Error` typeguards with `Schema.is(...)`, matching how the rest of the codebase derives tagged-error predicates. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Scope self-close interrupts fiber before error events emitted
- Moved the two emit calls (runtime.error and session.exited) before the stopOpenCodeContext call so they execute before the session scope is closed and the fiber is interrupted.
- ✅ Fixed: Unused
isProviderAdapterProcessErrorvariable is dead code- Removed the unused
isProviderAdapterProcessErrorvariable which was dead code left over from the Schema.is refactor.
- Removed the unused
Or push these changes by commenting:
@cursor push 377bdad386
Preview (377bdad386)
diff --git a/apps/server/src/provider/Layers/OpenCodeAdapter.ts b/apps/server/src/provider/Layers/OpenCodeAdapter.ts
--- a/apps/server/src/provider/Layers/OpenCodeAdapter.ts
+++ b/apps/server/src/provider/Layers/OpenCodeAdapter.ts
@@ -95,7 +95,6 @@
}
const isProviderAdapterRequestError = Schema.is(ProviderAdapterRequestError);
-const isProviderAdapterProcessError = Schema.is(ProviderAdapterProcessError);
function buildEventBase(input: {
readonly threadId: ThreadId;
@@ -473,7 +472,12 @@
}
const turnId = context.activeTurnId;
sessions.delete(context.session.threadId);
- yield* stopOpenCodeContext(context);
+ // Emit error and exit events *before* closing the session scope.
+ // `stopOpenCodeContext` closes `context.sessionScope`, which
+ // interrupts every fiber forked into it — including the event-pump
+ // fiber that may be running this function. Emitting after the close
+ // would silently drop these events because the fiber observes the
+ // pending interruption at the next yield point.
yield* emit({
...buildEventBase({ threadId: context.session.threadId, turnId }),
type: "runtime.error",
@@ -491,6 +495,7 @@
exitKind: "error",
},
}).pipe(Effect.ignore);
+ yield* stopOpenCodeContext(context);
});
/** Emit content.delta and item.completed events for an assistant text part. */You can send follow-ups to the cloud agent here.
- Swallow finalizer defects during session teardown so cleanup completes - Route SDK calls through shared runtime error wrappers - Simplify lifecycle failure handling in the adapter
- Guard server start and shutdown with one-shot scope cleanup - Emit runtime errors before closing provider scopes - Remove unused OpenCode runtime wiring from server layer
- Keep caller-provided native event loggers alive - Close only adapter-managed loggers after session teardown - Use effect duration strings for idle text-generation shutdown
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Variant fallback for models without explicit variants removed
- Restored the
inferVariantValuesfallback (with OPENAI_VARIANTS, ANTHROPIC_VARIANTS, GOOGLE_VARIANTS constants) so models without explicitvariantsin the inventory response still receive provider-inferred variant options.
- Restored the
- ✅ Fixed: Scope self-close from forked fiber risks incomplete cleanup
- Wrapped the
Scope.closecall inemitUnexpectedExitwithEffect.ignoreCauseto match the explicit error suppression pattern used at all otherstopOpenCodeContextcall sites, preventing silent defect drops from forked fibers.
- Wrapped the
Or push these changes by commenting:
@cursor push 4b14c03f57
Preview (4b14c03f57)
diff --git a/apps/server/src/provider/Layers/OpenCodeAdapter.ts b/apps/server/src/provider/Layers/OpenCodeAdapter.ts
--- a/apps/server/src/provider/Layers/OpenCodeAdapter.ts
+++ b/apps/server/src/provider/Layers/OpenCodeAdapter.ts
@@ -540,7 +540,7 @@
yield* runOpenCodeSdk("session.abort", () =>
context.client.session.abort({ sessionID: context.openCodeSessionId }),
).pipe(Effect.ignore({ log: true }));
- yield* Scope.close(context.sessionScope, Exit.void);
+ yield* Effect.ignoreCause(Scope.close(context.sessionScope, Exit.void));
});
/** Emit content.delta and item.completed events for an assistant text part. */
diff --git a/apps/server/src/provider/Layers/OpenCodeProvider.ts b/apps/server/src/provider/Layers/OpenCodeProvider.ts
--- a/apps/server/src/provider/Layers/OpenCodeProvider.ts
+++ b/apps/server/src/provider/Layers/OpenCodeProvider.ts
@@ -136,6 +136,23 @@
.join(" ");
}
+const OPENAI_VARIANTS = ["none", "minimal", "low", "medium", "high", "xhigh"];
+const ANTHROPIC_VARIANTS = ["high", "max"];
+const GOOGLE_VARIANTS = ["low", "high"];
+
+function inferVariantValues(providerID: string): ReadonlyArray<string> {
+ if (providerID === "anthropic") {
+ return ANTHROPIC_VARIANTS;
+ }
+ if (providerID === "openai" || providerID === "opencode") {
+ return OPENAI_VARIANTS;
+ }
+ if (providerID.startsWith("google")) {
+ return GOOGLE_VARIANTS;
+ }
+ return [];
+}
+
function inferDefaultVariant(
providerID: string,
variants: ReadonlyArray<string>,
@@ -169,7 +186,9 @@
readonly model: ProviderListResponse["all"][number]["models"][string];
readonly agents: ReadonlyArray<Agent>;
}): ModelCapabilities {
- const variantValues = Object.keys(input.model.variants ?? {});
+ const rawVariantValues = Object.keys(input.model.variants ?? {});
+ const variantValues =
+ rawVariantValues.length > 0 ? rawVariantValues : [...inferVariantValues(input.providerID)];
const defaultVariant = inferDefaultVariant(input.providerID, variantValues);
const variantOptions: ModelCapabilities["variantOptions"] = variantValues.map((value) =>
Object.assign(You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 2d4eab3. Configure here.
| yield* runOpenCodeSdk("session.abort", () => | ||
| context.client.session.abort({ sessionID: context.openCodeSessionId }), | ||
| ).pipe(Effect.ignore({ log: true })); | ||
| yield* Scope.close(context.sessionScope, Exit.void); |
There was a problem hiding this comment.
Scope self-close from forked fiber risks incomplete cleanup
Medium Severity
emitUnexpectedExit calls Scope.close(context.sessionScope, Exit.void) from within a fiber forked into that same sessionScope. If Scope.close fails (e.g., a finalizer defects), the failure propagates as a defect in the forked fiber, but nobody joins or observes that fiber. This means any Scope.close failure is silently discarded — and the caller has no way to know if cleanup actually completed. By contrast, stopOpenCodeContext is wrapped in Effect.ignoreCause at call sites, making the discard explicit.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2d4eab3. Configure here.
Upstream additions: - fix(web): restore manual sort drag and keep per-group expand state (pingdotgg#2221) - fix: Change right panel sheet to be below title bar / action bar (pingdotgg#2224) - Refactor OpenCode lifecycle and structured output handling (pingdotgg#2218) - effect-codex-app-server (pingdotgg#1942) - Redesign model picker with favorites and search (pingdotgg#2153) - fix(server): prevent probeClaudeCapabilities from wasting API requests (pingdotgg#2192) - fix(server): handle OpenCode text response format in commit message gen (pingdotgg#2202) - Devcontainer / IDE updates (pingdotgg#2208) - Expand leading ~ in Codex home paths before exporting CODEX_HOME (pingdotgg#2210) - fix(release): use v<semver> tag format for nightly releases (pingdotgg#2186) Fork adaptations: - Took upstream's redesigned model picker with favorites and search - Removed deleted codexAppServerManager (replaced by effect-codex-app-server) - Stubbed fetchCodexUsage (manager-based readout no longer available) - Extended PROVIDER_ICON_BY_PROVIDER for all 8 fork providers - Extended modelOptionsByProvider test fixtures for all 8 providers - Inline ClaudeSlashCommand type (not yet re-exported from SDK) - Updated SettingsPanels imports for new picker module structure - Preserved fork's CI customizations (ubuntu-24.04 not Blacksmith)
1 participant
Summary
Testing
bun fmt,bun lint,bun typecheck, andbun run test.Note
Medium Risk
Reworks OpenCode process/session lifecycle to be Scope-driven and moves error handling into a new runtime service, which can change cleanup timing and how failures surface (including defects during finalizers). Also changes provider model capability inference/default selection logic that impacts UI option defaults.
Overview
OpenCode runtime/lifecycle has been refactored to be Scope-owned and service-driven. Introduces an
OpenCodeRuntimeEffect service (OpenCodeRuntimeLive) that encapsulates server spawning/connecting, CLI execution, SDK client creation, and inventory loading, replacing direct function imports and imperativeclose()calls.Text generation and the provider adapter now rely on scopes/finalizers for cleanup.
OpenCodeTextGenerationLivereuses a shared server via a storedScope.Closeable(with idle TTL scheduling) and adds a layer finalizer to prevent leaked servers.OpenCodeAdapterLivesimilarly binds each session to asessionScope, rewrites the event pump and stop logic around scope closure, standardizes SDK error mapping viarunOpenCodeSdk, and makesstopAllbest-effort (no aggregate typed errors).Provider probing/model metadata was updated.
OpenCodeProviderLivenow usesOpenCodeRuntimefor health/version/inventory checks, improves probe error normalization, and derives modelcapabilitiesincludingvariantOptions/agentOptionswith inferred defaults.UI defaults/separators were tightened.
TraitsPickerand shared model normalization now fall back to the first option when no default is marked, and the menu no longer renders a leading divider when only the Agent section is present.Tests were updated to inject
OpenCodeRuntimetest doubles via Layers and to observe scope-finalizer-driven cleanup behavior.Reviewed by Cursor Bugbot for commit 2d4eab3. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Refactor OpenCode runtime to Effect-based service with scope-managed lifecycle
opencodeRuntime.tswith anOpenCodeRuntimeEffect service andOpenCodeRuntimeLivelayer; consumers now access server/process/SDK operations via dependency injection instead of direct importsScope, eliminating manualclose()calls; exit codes are observable viaEffect.Effect<number>OpenCodeAdapter.tsroutes all SDK calls throughrunOpenCodeSdkwith typedOpenCodeRuntimeErrorpropagation; session lifecycle usesScopeandRef-backed one-shot guards instead of ad-hoc booleans andAbortControllersOpenCodeProvider.tsaddsvariantOptionsandagentOptionswith inferred defaults to model capabilities, and fixes missing CLI detection to useenoent/notfoundsubstring checksTraitsPicker.tsxfixes a leading menu divider appearing when only the Agent section is present, andresolveNamedOptionnow falls back to the first option instead ofnullwhen no default is flaggedstopAllnow ignores per-session stop errors (best-effort cleanup); finalizer errors from session teardown surface as defects rather thanProviderAdapterProcessErrorMacroscope summarized 2d4eab3.