refactor(instance): add Effect foundation and effectCmd proof#507
Conversation
Astro-Han
added
P1
|
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR refactors the instance provisioning system from callback-based ChangesInstance Lifecycle and Bootstrap Service Refactor
CLI Effect-Based Command Integration
Bus Event Fallback for Missing Instance
Configuration Test Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors instance management by introducing an InstanceStore to handle the lifecycle, caching, and disposal of project instances. It migrates InstanceBootstrap to an Effect-based service and layer, enabling automatic instance loading and removing explicit bootstrap calls across the codebase. The Instance utility now delegates to the new store and runtime. Feedback highlights a resource leak in the InstanceBootstrap layer due to ignored subscription cleanups and notes potential issues with premature instance disposal in the effectCmd utility during concurrent operations.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/opencode/test/config/config.test.ts (1)
108-112: ⚡ Quick winUse fixture instance helpers instead of introducing
withRawInstance
withRawInstancere-implements instance binding (Project.fromDirectory+Instance.restore) in test code. Please route these tests throughprovideInstance(...)/provideTmpdirInstance(...)fromfixture/fixture.tsto avoid drift as instance lifecycle wiring evolves.As per coding guidelines, "Prefer Effect-aware helpers from
fixture/fixture.ts... useprovideInstance(dir)(effect)... orprovideTmpdirInstance(...)" and "When a test needs instance-local state, preferprovideTmpdirInstance(...)orprovideInstance(...)..."🤖 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 `@packages/opencode/test/config/config.test.ts` around lines 108 - 112, The test introduces a local helper withRawInstance that re-implements Project.fromDirectory + Instance.restore; replace uses of withRawInstance by wiring tests through the existing fixture helpers provideInstance(...) or provideTmpdirInstance(...) from fixture/fixture.ts instead: call provideInstance(directory)(effect) or provideTmpdirInstance(...) to obtain the bound Instance instead of manually calling Project.fromDirectory and Instance.restore, and remove the withRawInstance helper entirely so instance lifecycle follows the centralized fixture implementation.packages/opencode/src/project/instance-layer.ts (1)
4-8: 💤 Low valueDynamic import via
Effect.promisewill surface load failures as defects.
Effect.promise(() => import("./bootstrap"))treats any rejection as a defect (fiber dies) rather than a typed error. For an internal module import this is usually acceptable, but if you want a clean error path considerEffect.tryPromisewith a typed error or aLayer.suspend(() => ...)wrapper. Otherwise the lazy-load shape looks correct and is a reasonable way to break the eager dependency chain intobootstrap.ts.🤖 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 `@packages/opencode/src/project/instance-layer.ts` around lines 4 - 8, The dynamic import uses Effect.promise which surfaces import failures as defects; change the bootstrapLayer construction to use a safe effect (e.g., Effect.tryPromise with a typed error or wrap the import in Layer.suspend) so import rejections become handled errors instead of defects. Locate bootstrapLayer (currently built from Effect.promise(() => import("./bootstrap").then(...)) and replace the Effect.promise call with Effect.tryPromise or Layer.suspend around the import, preserving the extraction of InstanceBootstrap.defaultLayer and the final composition with InstanceStore.defaultLayer into layer via Layer.provideMerge.packages/opencode/src/project/with-instance.ts (1)
4-7: 💤 Low valueConsider clarifying lifecycle ownership of the loaded
InstanceRuntime.
provideloads an instance viaInstanceRuntime.load(...)but never disposes it on its own — it relies on the centralizedInstanceStorelifecycle (e.g.,disposeAllInstances()). That's fine for tests and short-lived calls, but a brief comment here would prevent callers from assumingprovideis fully scoped/cleanup-safe and reaching for it as a long-running boundary.Also the typing
fn: () => Raccepts both sync and async callbacks, which is correct givenAsyncLocalStoragepropagates across awaits, but you may want to constrain or document the expectation thatfnshould not outlive the returned promise (e.g., no unawaited fire-and-forget work) since context restoration depends on the await chain.🤖 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 `@packages/opencode/src/project/with-instance.ts` around lines 4 - 7, Add a short doc comment above provide explaining that the InstanceRuntime returned by InstanceRuntime.load is owned/managed by the global InstanceStore (e.g., cleaned up via InstanceStore.disposeAllInstances) so provide does not dispose the instance itself; also note that context.provide relies on AsyncLocalStorage propagation so the callback fn passed to provide (the provide function signature) may be sync or async but must not spawn unawaited background work that outlives the returned promise. While here you can keep the current signature of provide(fn: () => R), consider making the signature explicitly allow Promise returns (fn: () => R | Promise<R>) or add a typed comment indicating callers must await the promise to retain context.packages/opencode/test/cli/effect-cmd-instance-als.test.ts (1)
12-35: 💤 Low valueConsider catching/awaiting handler failures explicitly for clearer test diagnostics.
The
expect(...)runs inside theEffect.promisecallback. If the assertion fails, the rejection propagates throughEffect.promise → AppRuntime.runPromise → handler, but Effect's failure wrapping can obscure the original Jest-style error frame, making diagnosis harder. Not a correctness issue, just an observability nit. Otherwise the test cleanly exercises the AsyncLocalStorage bridging path.🤖 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 `@packages/opencode/test/cli/effect-cmd-instance-als.test.ts` around lines 12 - 35, The test places the Jest assertion inside Effect.promise which can obscure assertion failures; update the test so the effect handler's returned promise is awaited and any rejection is rethrown to surface original Jest errors: call and await the effectCmd handler (the handler created in effectCmd in the "effectCmd preserves Instance.current..." test), capture errors from Effect.promise/AppRuntime.runPromise and rethrow them (or fail the test explicitly) so assertion stack traces from inside Effect.promise are preserved for Jest diagnostics.packages/opencode/src/cli/cmd/models.ts (1)
30-33: 💤 Low valueConsider surfacing refresh failures as
CliErrorinstead of defects.
Effect.promise(...)turns anyModelsDev.refreshrejection into a Die, which bypasses the newFormatError/CliErrorformatter and prints a raw stack to the user. Wrapping withEffect.tryPromise+Effect.mapErrortofail(...)keeps user-facing CLI errors uniform.♻️ Proposed refactor
if (args.refresh) { - yield* Effect.promise(() => ModelsDev.refresh(true)) + yield* Effect.tryPromise({ + try: () => ModelsDev.refresh(true), + catch: (e) => new CliError({ message: `Failed to refresh models cache: ${String(e)}` }), + }) UI.println(UI.Style.TEXT_SUCCESS_BOLD + "Models cache refreshed" + UI.Style.TEXT_NORMAL) }🤖 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 `@packages/opencode/src/cli/cmd/models.ts` around lines 30 - 33, The current use of Effect.promise when calling ModelsDev.refresh (inside the args.refresh branch) converts rejections into defects; replace it with Effect.tryPromise calling ModelsDev.refresh and then use Effect.mapError to convert the rejection into a CLI-friendly failure (e.g., fail(new CliError(...) or FormatError-wrapping) so the error flows through the CLI formatter rather than printing a raw stack; update the call site where Effect.promise(() => ModelsDev.refresh(true)) appears to use Effect.tryPromise(() => ModelsDev.refresh(true)) and then Effect.mapError to produce fail(...) with a descriptive CliError.
🤖 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 `@packages/opencode/src/effect/run-service.ts`:
- Around line 27-40: WorkspaceContext.workspaceID is read eagerly and can throw
LocalContext.NotFound before the fiber fallback runs; change how workspace is
obtained to mirror the instance lookup: replace the top-level const workspace =
WorkspaceContext.workspaceID with a safe getter that catches
LocalContext.NotFound (like the instance IIFE), e.g., compute workspace using an
IIFE that returns WorkspaceContext.workspaceID inside a try/catch and rethrows
non-LocalContext.NotFound errors, then keep the existing attachWith(effect, {
instance: ..., workspace: ... }) fallback using Fiber.getCurrent and
Context.getReferenceUnsafe(…, WorkspaceRef).
In `@packages/opencode/src/file/watcher.ts`:
- Around line 98-106: The watcher callback cb currently swallows errors by
simply returning inside Instance.restore, which hides runtime failures; modify
the cb in file/watcher.ts so that when the native watcher passes an error (the
err parameter) you log it before returning (e.g., call log.error("watcher
callback error", { err })) and then return; keep the existing Instance.restore
wrapping and the existing event handling (Bus.publish with Event.Updated)
unchanged aside from adding the error log.
In `@packages/opencode/src/project/bootstrap-service.ts`:
- Around line 3-7: The Interface.run is declared as a never-fail Effect but the
implementation calls plugin.init() and config.get() directly (via yield*), which
can throw; update the bootstrap implementation so both calls are wrapped with
the existing boot helper (e.g., replace yield* plugin.init() with yield*
boot(plugin.init()) and yield* config.get() with yield* boot(config.get())) to
preserve the never-fail contract of Service/Interface.run; alternatively, if you
intend failures to be fatal, change the Interface.run type instead—prefer
wrapping with boot to match the other init calls.
In `@packages/opencode/src/project/bootstrap.ts`:
- Around line 49-72: The bootstrap sequence inconsistently calls plugin.init()
and config.get() directly instead of via the error-catching wrapper boot(...);
update the code to call boot(plugin.init()) and boot(config.get()) (or
explicitly document why these must hard-fail) so they get the same
logged-and-continue behavior as shareNext.init(), format.init(), lsp.init(),
file.init(), fileWatcher.init(), vcs.init(), and snapshot.init(); ensure you use
the existing boot function so any thrown Cause is logged and does not abort the
whole instance.
---
Nitpick comments:
In `@packages/opencode/src/cli/cmd/models.ts`:
- Around line 30-33: The current use of Effect.promise when calling
ModelsDev.refresh (inside the args.refresh branch) converts rejections into
defects; replace it with Effect.tryPromise calling ModelsDev.refresh and then
use Effect.mapError to convert the rejection into a CLI-friendly failure (e.g.,
fail(new CliError(...) or FormatError-wrapping) so the error flows through the
CLI formatter rather than printing a raw stack; update the call site where
Effect.promise(() => ModelsDev.refresh(true)) appears to use
Effect.tryPromise(() => ModelsDev.refresh(true)) and then Effect.mapError to
produce fail(...) with a descriptive CliError.
In `@packages/opencode/src/project/instance-layer.ts`:
- Around line 4-8: The dynamic import uses Effect.promise which surfaces import
failures as defects; change the bootstrapLayer construction to use a safe effect
(e.g., Effect.tryPromise with a typed error or wrap the import in Layer.suspend)
so import rejections become handled errors instead of defects. Locate
bootstrapLayer (currently built from Effect.promise(() =>
import("./bootstrap").then(...)) and replace the Effect.promise call with
Effect.tryPromise or Layer.suspend around the import, preserving the extraction
of InstanceBootstrap.defaultLayer and the final composition with
InstanceStore.defaultLayer into layer via Layer.provideMerge.
In `@packages/opencode/src/project/with-instance.ts`:
- Around line 4-7: Add a short doc comment above provide explaining that the
InstanceRuntime returned by InstanceRuntime.load is owned/managed by the global
InstanceStore (e.g., cleaned up via InstanceStore.disposeAllInstances) so
provide does not dispose the instance itself; also note that context.provide
relies on AsyncLocalStorage propagation so the callback fn passed to provide
(the provide function signature) may be sync or async but must not spawn
unawaited background work that outlives the returned promise. While here you can
keep the current signature of provide(fn: () => R), consider making the
signature explicitly allow Promise returns (fn: () => R | Promise<R>) or add a
typed comment indicating callers must await the promise to retain context.
In `@packages/opencode/test/cli/effect-cmd-instance-als.test.ts`:
- Around line 12-35: The test places the Jest assertion inside Effect.promise
which can obscure assertion failures; update the test so the effect handler's
returned promise is awaited and any rejection is rethrown to surface original
Jest errors: call and await the effectCmd handler (the handler created in
effectCmd in the "effectCmd preserves Instance.current..." test), capture errors
from Effect.promise/AppRuntime.runPromise and rethrow them (or fail the test
explicitly) so assertion stack traces from inside Effect.promise are preserved
for Jest diagnostics.
In `@packages/opencode/test/config/config.test.ts`:
- Around line 108-112: The test introduces a local helper withRawInstance that
re-implements Project.fromDirectory + Instance.restore; replace uses of
withRawInstance by wiring tests through the existing fixture helpers
provideInstance(...) or provideTmpdirInstance(...) from fixture/fixture.ts
instead: call provideInstance(directory)(effect) or provideTmpdirInstance(...)
to obtain the bound Instance instead of manually calling Project.fromDirectory
and Instance.restore, and remove the withRawInstance helper entirely so instance
lifecycle follows the centralized fixture implementation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f189f9f-e458-4fb6-a12c-c11511e41875
📒 Files selected for processing (30)
packages/opencode/script/seed-e2e.tspackages/opencode/src/bus/index.tspackages/opencode/src/cli/bootstrap.tspackages/opencode/src/cli/cmd/cmd.tspackages/opencode/src/cli/cmd/models.tspackages/opencode/src/cli/effect-cmd.tspackages/opencode/src/cli/error.tspackages/opencode/src/effect/app-runtime.tspackages/opencode/src/effect/run-service.tspackages/opencode/src/file/watcher.tspackages/opencode/src/project/bootstrap-service.tspackages/opencode/src/project/bootstrap.tspackages/opencode/src/project/instance-context.tspackages/opencode/src/project/instance-layer.tspackages/opencode/src/project/instance-runtime.tspackages/opencode/src/project/instance-store.tspackages/opencode/src/project/instance.tspackages/opencode/src/project/with-instance.tspackages/opencode/src/server/instance/middleware.tspackages/opencode/src/server/instance/project.tspackages/opencode/src/server/routes/instance/middleware.tspackages/opencode/src/tool/bash.tspackages/opencode/src/worktree/index.tspackages/opencode/test/cli/effect-cmd-instance-als.test.tspackages/opencode/test/cli/error.test.tspackages/opencode/test/config/config.test.tspackages/opencode/test/fixture/fixture.tspackages/opencode/test/plugin/workspace-adaptor.test.tspackages/opencode/test/project/instance-bootstrap-regression.test.tspackages/opencode/test/project/instance-store.test.ts
💤 Files with no reviewable changes (6)
- packages/opencode/src/server/routes/instance/middleware.ts
- packages/opencode/src/cli/bootstrap.ts
- packages/opencode/src/server/instance/middleware.ts
- packages/opencode/src/worktree/index.ts
- packages/opencode/script/seed-e2e.ts
- packages/opencode/src/server/instance/project.ts
1 participant
Summary
InstanceBootstrap/InstanceStorefoundation for instance boot, reload, dispose, and context propagationeffectCmdwrapper and migrate only themodelscommand as the proof chainWhy
This is the PR5 foundation slice for #477. The goal is to absorb the upstream Effect foundation and the smallest
effectCmdproof without switching the default server path.The important boundary is that this PR touches production Hono and CLI bootstrap paths, so it is reviewed as a production-path runtime change. It does not claim a HttpApi rollback surface and does not introduce the non-default HttpApi backend.
Related Issue
Refs #477
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
InstanceStoreload, reload, dispose, and failed-boot behaviorAppRuntime.runPromiseeffectCmdwrapper behavior and the minimalmodelscommand migrationRisk Notes
How To Verify
Screenshots or Recordings
Not required. No visible UI changes.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Bug Fixes
Refactor