refactor(instance): remove legacy runtime fallback (#27757)

This commit is contained in:
Shoubhit Dash
2026-05-15 23:05:44 +05:30
committed by GitHub
parent 9975c1ed1c
commit 0c9cfe923f
47 changed files with 937 additions and 1254 deletions

View File

@@ -6,7 +6,6 @@ Current status on this branch:
- `src/` has 5 `makeRuntime(...)` call sites total.
- 2 are intentionally excluded from this checklist: `src/bus/index.ts` and `src/effect/cross-spawn-spawner.ts`.
- 1 is tracked primarily by the instance-context migration rather than facade removal: `src/project/instance.ts`.
- That leaves 2 live runtime-backed service facades still worth tracking here: `src/npm/index.ts` and `src/cli/cmd/tui/config/tui.ts`.
Recent progress:
@@ -18,7 +17,6 @@ Recent progress:
- `src/cli/cmd/tui/config/tui.ts` still exports `makeRuntime(...)` plus async facade helpers for `get()` and `waitForDependencies()`.
- `src/npm/index.ts` still exports `makeRuntime(...)` plus async facade helpers for `install()`, `add()`, `outdated()`, and `which()`.
- `src/project/instance.ts` still uses a dedicated runtime for project boot, but that file is really part of the broader legacy instance-context transition tracked in `instance-context.md`.
## Completed Batches
@@ -192,7 +190,6 @@ Most of the original facade-removal backlog is already done. The practical remai
1. remove the `Npm` runtime-backed facade from `src/npm/index.ts`
2. remove the `TuiConfig` runtime-backed facade from `src/cli/cmd/tui/config/tui.ts`
3. keep `src/project/instance.ts` in the separate instance-context migration, not this checklist
## Checklist

View File

@@ -197,13 +197,9 @@ For background loops, use `Effect.repeat` or `Effect.schedule` with
[`EffectBridge`](../../src/effect/bridge.ts) is the sanctioned helper for
Promise/callback interop that needs to preserve instance/workspace context.
Keep it, but reduce its dependency on legacy `Instance.current` /
`Instance.restore` over time.
`Instance.bind` / `Instance.restore` are transitional legacy tools. Use
them only for native callbacks that still require legacy ALS context. Do
not use them for `setTimeout`, `Promise.then`, `EventEmitter.on`, or
Effect fibers.
It preserves explicit `InstanceRef` / `WorkspaceRef` context for effects run
through the bridge. Plain JS callbacks that need instance data should receive
that data explicitly.
## Testing

View File

@@ -1,309 +1,13 @@
# Instance context migration
# Instance Context
Practical plan for retiring the promise-backed / ALS-backed `Instance` helper in `src/project/instance.ts` and moving instance selection fully into Effect-provided scope.
Instance selection is now Effect-provided context.
## Goal
Use these APIs:
End state:
- `InstanceRef` for the current project context.
- `WorkspaceRef` for the current workspace id.
- `InstanceState.context` / `InstanceState.directory` inside Effect services that require an instance.
- `InstanceStore` at entry boundaries that need to load, reload, or dispose project contexts.
- `EffectBridge` for native, plugin, or plain JavaScript callback boundaries that need to re-enter Effect with captured refs.
- request, CLI, TUI, and tool entrypoints shift into an instance through Effect, not `Instance.provide(...)`
- Effect code reads the current instance from `InstanceRef` or its eventual replacement, not from ALS-backed sync getters
- per-directory boot, caching, and disposal are scoped Effect resources, not a module-level `Map<string, Promise<InstanceContext>>`
- ALS remains only as a temporary bridge for native callback APIs that fire outside the Effect fiber tree
## Current split
Today `src/project/instance.ts` still owns two separate concerns:
- ambient current-instance context through `LocalContext` / `AsyncLocalStorage`
- per-directory boot and deduplication through `cache: Map<string, Promise<InstanceContext>>`
At the same time, the Effect side already exists:
- `src/effect/instance-ref.ts` provides `InstanceRef` and `WorkspaceRef`
- `src/effect/run-service.ts` already attaches those refs when a runtime starts inside an active instance ALS context
- `src/effect/instance-state.ts` already prefers `InstanceRef` and only falls back to ALS when needed
That means the migration is not "invent instance context in Effect". The migration is "stop relying on the legacy helper as the primary source of truth".
## End state shape
Near-term target shape:
```ts
InstanceScope.with({ directory, workspaceID }, effect)
```
Responsibilities of `InstanceScope.with(...)`:
- resolve `directory`, `project`, and `worktree`
- acquire or reuse the scoped per-directory instance environment
- provide `InstanceRef` and `WorkspaceRef`
- run the caller's Effect inside that environment
Code inside the boundary should then do one of these:
```ts
const ctx = yield * InstanceState.context
const dir = yield * InstanceState.directory
```
Long-term, once `InstanceState` itself is replaced by keyed layers / `LayerMap`, those reads can move to an `InstanceContext` service without changing the outer migration order.
## Migration phases
### Phase 1: stop expanding the legacy surface
Rules for all new code:
- do not add new `Instance.directory`, `Instance.worktree`, `Instance.project`, or `Instance.current` reads inside Effect code
- do not add new `Instance.provide(...)` boundaries unless there is no Effect-native seam yet
- use `InstanceState.context`, `InstanceState.directory`, or an explicit `ctx` parameter inside Effect code
Success condition:
- the file inventory below only shrinks from here
### Phase 2: remove direct sync getter reads from Effect services
Convert Effect services first, before replacing the top-level boundary. These modules already run inside Effect and mostly need `yield* InstanceState.context` or a yielded `ctx` instead of ambient sync access.
Primary batch, highest payoff:
- `src/file/index.ts`
- `src/lsp/server.ts`
- `src/worktree/index.ts`
- `src/file/watcher.ts`
- `src/format/formatter.ts`
- `src/session/index.ts`
- `src/project/vcs.ts`
Mechanical replacement rule:
- `Instance.directory` -> `ctx.directory` or `yield* InstanceState.directory`
- `Instance.worktree` -> `ctx.worktree`
- `Instance.project` -> `ctx.project`
Do not thread strings manually through every public method if the service already has access to Effect context.
### Phase 3: convert entry boundaries to provide instance refs directly
After the service bodies stop assuming ALS, move the top-level boundaries to shift into Effect explicitly.
Main boundaries:
- HTTP server middleware and experimental `HttpApi` entrypoints
- CLI commands
- TUI worker / attach / thread entrypoints
- tool execution entrypoints
These boundaries should become Effect-native wrappers that:
- decode directory / workspace inputs
- resolve the instance context once
- provide `InstanceRef` and `WorkspaceRef`
- run the requested Effect
At that point `Instance.provide(...)` becomes a legacy adapter instead of the normal code path.
### Phase 4: replace promise boot cache with scoped instance runtime
Once boundaries and services both rely on Effect context, replace the module-level promise cache in `src/project/instance.ts`.
Target replacement:
- keyed scoped runtime or keyed layer acquisition for each directory
- reuse via `ScopedCache`, `LayerMap`, or another keyed Effect resource manager
- cleanup performed by scope finalizers instead of `disposeAll()` iterating a Promise map
This phase should absorb the current responsibilities of:
- `cache` in `src/project/instance.ts`
- `boot(...)`
- most of `disposeInstance(...)`
- manual `reload(...)` / `disposeAll()` fan-out logic
### Phase 5: shrink ALS to callback bridges only
Keep ALS only where a library invokes callbacks outside the Effect fiber tree and we still need to call code that reads instance context synchronously.
Known bridge cases today:
- `src/file/watcher.ts`
- `src/session/llm.ts`
- some LSP and plugin callback paths
If those libraries become fully wrapped in Effect services, the remaining `Instance.bind(...)` uses can disappear too.
### Phase 6: delete the legacy sync API
Only after earlier phases land:
- remove broad use of `Instance.current`, `Instance.directory`, `Instance.worktree`, `Instance.project`
- reduce `src/project/instance.ts` to a thin compatibility shim or delete it entirely
- remove the ALS fallback from `InstanceState.context`
## Inventory of direct legacy usage
Direct legacy usage means any source file that still calls one of:
- `Instance.current`
- `Instance.directory`
- `Instance.worktree`
- `Instance.project`
- `Instance.provide(...)`
- `Instance.bind(...)`
- `Instance.restore(...)`
- `Instance.reload(...)`
- `Instance.dispose()` / `Instance.disposeAll()`
Current total: `56` files in `packages/opencode/src`.
### Core bridge and plumbing
These files define or adapt the current bridge. They should change last, after callers have moved.
- `src/project/instance.ts`
- `src/effect/run-service.ts`
- `src/effect/instance-state.ts`
- `src/project/bootstrap.ts`
- `src/config/config.ts`
Migration rule:
- keep these as compatibility glue until the outer boundaries and inner services stop depending on ALS
### HTTP and server boundaries
These are the current request-entry seams that still create or consume instance context through the legacy helper.
- `src/server/routes/instance/middleware.ts`
- `src/server/routes/instance/index.ts`
- `src/server/routes/instance/project.ts`
- `src/server/routes/control/workspace.ts`
- `src/server/routes/instance/file.ts`
- `src/server/routes/instance/experimental.ts`
- `src/server/routes/global.ts`
Migration rule:
- move these to explicit Effect entrypoints that provide `InstanceRef` / `WorkspaceRef`
- do not move these first; first reduce the number of downstream handlers and services that still expect ambient ALS
### CLI and TUI boundaries
These commands still enter an instance through `Instance.provide(...)` or read sync getters directly.
- `src/cli/bootstrap.ts`
- `src/cli/cmd/agent.ts`
- `src/cli/cmd/debug/agent.ts`
- `src/cli/cmd/debug/ripgrep.ts`
- `src/cli/cmd/github.ts`
- `src/cli/cmd/import.ts`
- `src/cli/cmd/mcp.ts`
- `src/cli/cmd/models.ts`
- `src/cli/cmd/plug.ts`
- `src/cli/cmd/pr.ts`
- `src/cli/cmd/providers.ts`
- `src/cli/cmd/stats.ts`
- `src/cli/cmd/tui/attach.ts`
- `src/cli/cmd/tui/plugin/runtime.ts`
- `src/cli/cmd/tui/thread.ts`
- `src/cli/cmd/tui/worker.ts`
Migration rule:
- converge these on one shared `withInstance(...)` Effect entry helper instead of open-coded `Instance.provide(...)`
- after that helper is proven, inline the legacy implementation behind an Effect-native scope provider
### Tool boundary code
These tools mostly use direct getters for path resolution and repo-relative display logic.
- `src/tool/apply_patch.ts`
- `src/tool/bash.ts`
- `src/tool/edit.ts`
- `src/tool/lsp.ts`
- `src/tool/plan.ts`
- `src/tool/read.ts`
- `src/tool/write.ts`
Migration rule:
- expose the current instance as an explicit Effect dependency for tool execution
- keep path logic local; avoid introducing another global singleton for tool state
### Effect services still reading ambient instance state
These modules are already the best near-term migration targets because they are in Effect code but still read sync getters from the legacy helper.
- `src/agent/agent.ts`
- `src/cli/cmd/tui/config/tui-migrate.ts`
- `src/file/index.ts`
- `src/file/watcher.ts`
- `src/format/formatter.ts`
- `src/lsp/client.ts`
- `src/lsp/index.ts`
- `src/lsp/server.ts`
- `src/mcp/index.ts`
- `src/project/vcs.ts`
- `src/provider/provider.ts`
- `src/pty/index.ts`
- `src/session/session.ts`
- `src/session/instruction.ts`
- `src/session/llm.ts`
- `src/session/system.ts`
- `src/sync/index.ts`
- `src/worktree/index.ts`
Migration rule:
- replace direct getter reads with `yield* InstanceState.context` or a yielded `ctx`
- isolate `Instance.bind(...)` callers and convert only the truly callback-driven edges to bridge mode
### Highest-churn hotspots
Current highest direct-usage counts by file:
- `src/file/index.ts` - `18`
- `src/lsp/server.ts` - `14`
- `src/worktree/index.ts` - `12`
- `src/file/watcher.ts` - `9`
- `src/cli/cmd/mcp.ts` - `8`
- `src/format/formatter.ts` - `8`
- `src/tool/apply_patch.ts` - `8`
- `src/cli/cmd/github.ts` - `7`
These files should drive the first measurable burn-down.
## Recommended implementation order
1. Migrate direct getter reads inside Effect services, starting with `file`, `lsp`, `worktree`, `format`, and `session`.
2. Add one shared Effect-native boundary helper for CLI / tool / HTTP entrypoints so we stop open-coding `Instance.provide(...)`.
3. Move experimental `HttpApi` entrypoints to that helper so the new server stack proves the pattern.
4. Convert remaining CLI and tool boundaries.
5. Replace the promise cache with a keyed scoped runtime or keyed layer map.
6. Delete ALS fallback paths once only callback bridges still depend on them.
## Definition of done
This migration is done when all of the following are true:
- new requests and commands enter an instance by providing Effect context, not ALS
- Effect services no longer read `Instance.directory`, `Instance.worktree`, `Instance.project`, or `Instance.current`
- `Instance.provide(...)` is gone from normal request / CLI / tool execution
- per-directory boot and disposal are handled by scoped Effect resources
- `Instance.bind(...)` is either gone or confined to a tiny set of native callback adapters
## Tracker and worktree
Active tracker items:
- `lh7l73` - overall `HttpApi` migration
- `yobwlk` - remove direct `Instance.*` reads inside Effect services
- `7irl1e` - replace `InstanceState` / legacy instance caching with keyed Effect layers
Dedicated worktree for this transition:
- path: `/Users/kit/code/open-source/opencode-worktrees/instance-effect-shift`
- branch: `kit/instance-effect-shift`
Do not add new ambient instance globals. Promise and callback boundaries should either stay in Effect, use `EffectBridge`, or pass the required context explicitly.

View File

@@ -24,10 +24,6 @@ Small follow-ups that do not fit neatly into the main facade, route, tool, or sc
- [ ] `cli/cmd/tui/config/tui.ts` - switch TUI config loading from async `ConfigPaths.*` wrappers to the `ConfigPaths.Service` once that service exists.
- [ ] `cli/cmd/tui/config/tui-migrate.ts` - decide whether to leave this as a plain async module using wrapper functions or effectify it fully after `ConfigPaths.Service` lands.
## Instance cleanup
- [ ] `project/instance.ts` - keep shrinking the legacy ALS / Promise cache after the remaining `Instance.*` callers move over.
## Notes
- Prefer small, semantics-preserving config migrations. Config precedence, legacy key migration, and plugin origin tracking are easy to break accidentally.

View File

@@ -64,13 +64,11 @@ P6 OA
explicit and testable instead of mutable module state.
Shrinks: [`global.ts`](../../../core/src/global.ts) import-time side
effects, mutable `Global.Path` overrides, and its `Flag` dependency.
- `INST` Instance shim — remove ambient `Instance` usage and old ALS
access patterns.
Shrinks: [`src/project/instance.ts`](../../src/project/instance.ts).
- `INST` Instance context — keep project context explicit through Effect refs
and bridge boundaries.
- `BRIDGE` Promise/callback interop — keep bridge helpers, but reduce
legacy ALS coupling.
Shrinks: [`src/effect/bridge.ts`](../../src/effect/bridge.ts)
dependency on [`project/instance.ts`](../../src/project/instance.ts).
Shrinks: ad hoc Promise/callback re-entry code.
- `PROC` AppProcess migration — prefer `AppProcess.Service` over raw
process wrappers.
Shrinks: direct spawn callsites and legacy process helpers.
@@ -221,74 +219,13 @@ Next PR candidates:
## P4: Instance And Bridge
[`project/instance.ts`](../../src/project/instance.ts) is the deletion
target. [`effect/bridge.ts`](../../src/effect/bridge.ts) is not a near-term
deletion target; Promise/callback interop will continue to exist.
Instance context migration is complete for the legacy sync shim. Promise and callback interop continues through [`effect/bridge.ts`](../../src/effect/bridge.ts).
Goal:
Current rules:
- Keep a sanctioned bridge for Promise/callback boundaries.
- Reduce bridge dependence on legacy `Instance.restore` / `Instance.current`.
- Move callers toward `InstanceRef`, `WorkspaceRef`, `InstanceState`, or
explicit context where practical.
- Delete `project/instance.ts` only after ambient Instance coupling is gone.
Important distinction:
- `InstanceState.context`, `InstanceState.directory`, and
`InstanceState.workspaceID` are acceptable inside normal Effect service
code when `InstanceRef` / `WorkspaceRef` are provided by the runtime.
- The deletion blockers are the fallback and callback paths that rely on
ambient ALS: direct `Instance.*` reads, `InstanceState.bind(...)`,
`AppRuntime.runPromise(...)` re-entry from plain JS, and bridge restore
code that installs legacy ALS before invoking callbacks.
Current bottom-up inventory from `dev`:
- Direct `Instance.*` value readers:
[`tool/repo_overview.ts`](../../src/tool/repo_overview.ts),
[`control-plane/adapters/worktree.ts`](../../src/control-plane/adapters/worktree.ts),
[`cli/bootstrap.ts`](../../src/cli/bootstrap.ts).
- `InstanceState.bind(...)` callback boundaries:
[`file/watcher.ts`](../../src/file/watcher.ts) native watcher callback,
[`storage/db.ts`](../../src/storage/db.ts) transaction/effect callbacks,
[`session/llm.ts`](../../src/session/llm.ts) workflow approval callback.
- `AppRuntime.runPromise(...)` / re-entry from plain JS:
[`project/with-instance.ts`](../../src/project/with-instance.ts),
[`project/instance-runtime.ts`](../../src/project/instance-runtime.ts),
[`control-plane/adapters/worktree.ts`](../../src/control-plane/adapters/worktree.ts),
[`cli/effect-cmd.ts`](../../src/cli/effect-cmd.ts), plus global/non-instance
callsites such as CLI upgrade and ACP agent defaults.
- Intentional bridge users to classify, not delete blindly:
workspace adapters in [`control-plane/workspace.ts`](../../src/control-plane/workspace.ts),
MCP, command execution, plugins, pty lifecycle, bus scope cleanup, task
cancellation, and HTTP lifecycle reload/dispose paths.
- Core fallback layer to shrink last:
[`effect/run-service.ts`](../../src/effect/run-service.ts),
[`effect/bridge.ts`](../../src/effect/bridge.ts), and
[`effect/instance-state.ts`](../../src/effect/instance-state.ts).
Recommended PR order:
- [ ] `INST-1` Remove direct `Instance.*` value readers. Start with
`repo_overview`, `worktree` adapter, and `cli/bootstrap`; pass context
explicitly or obtain it from an Effect boundary.
- [ ] `INST-2` Move type-only `InstanceContext` imports from
[`project/instance.ts`](../../src/project/instance.ts) to
[`project/instance-context.ts`](../../src/project/instance-context.ts).
- [ ] `INST-3` Audit each `InstanceState.bind(...)` callback from the inside
out: list what the callback calls (`Bus.publish`, database effects,
permission/session services), then replace ambient capture with explicit
`InstanceRef` / `WorkspaceRef` provision or an `EffectBridge` call.
- [ ] `INST-4` Classify `AppRuntime.runPromise(...)` callsites as global,
instance-scoped with explicit refs, or bridge-required. Eliminate the
instance-scoped callsites that rely on `run-service.attach()` falling
back to `Instance.current`.
- [ ] `INST-5` After consumers are explicit, remove `Instance.current` fallback
from `InstanceState.context` and `run-service.attach()`.
- [ ] `INST-6` Move any remaining `restore` / `bind` compatibility helpers to
the boundary that still needs them, then delete
[`project/instance.ts`](../../src/project/instance.ts).
- Effect services read instance data from `InstanceRef`, `WorkspaceRef`, `InstanceState`, or explicit arguments.
- Plain JavaScript callback boundaries use `EffectBridge` or explicit context arguments.
- Runtime entrypoints must provide refs explicitly when they are instance-scoped.
## Lower Priority Tracks