diff --git a/packages/opencode/specs/effect/error-boundaries-plan.md b/packages/opencode/specs/effect/error-boundaries-plan.md new file mode 100644 index 0000000000..763bf5ea5e --- /dev/null +++ b/packages/opencode/specs/effect/error-boundaries-plan.md @@ -0,0 +1,235 @@ +# Error Boundaries Plan + +Plan for removing `NamedError` as connective tissue while keeping public +wire contracts stable. + +## Desired Shape + +```text +Domain/service error + Schema.TaggedErrorClass + - catchable with catchTag / catchTags + - appears in service method error type + - no HTTP status + - no toObject() + +HTTP public error + Schema.ErrorClass / TaggedErrorClass with httpApiStatus + - endpoint-declared public contract + - owns legacy { name, data } only when that is the SDK wire shape + +CLI/user rendering + FormatError and small format helpers + - converts domain errors to text + - preserves useful structured fields + +Session/model-visible error + first-class session/message error schema or helper + - owns { name, data } event/message shape + - not a service error class +``` + +The important rule: a service error should not also be the HTTP body, CLI +formatter, and session event body. Each seam adapts the error into the +shape it owns. + +## Concrete Example: Provider Model Not Found + +Before: + +```ts +export const ModelNotFoundError = NamedError.create("ProviderModelNotFoundError", { + providerID: ProviderID, + modelID: ModelID, + suggestions: Schema.optional(Schema.Array(Schema.String)), +}) +``` + +Problems: + +- Throwing it inside `Effect.fn` made it behave like a defect unless a + compatibility bridge caught it. +- HTTP middleware knew that this one domain error should be a `400`. +- Callers read `.data.*`, which couples them to the legacy `{ name, data }` + wire shape. + +After: + +```ts +export class ModelNotFoundError extends Schema.TaggedErrorClass()("ProviderModelNotFoundError", { + providerID: ProviderID, + modelID: ModelID, + suggestions: Schema.optional(Schema.Array(Schema.String)), + cause: Schema.optional(Schema.Defect), +}) {} + +export interface Interface { + readonly getModel: (providerID: ProviderID, modelID: ModelID) => Effect.Effect +} +``` + +Boundary adapters: + +```text +CLI +└─ FormatError sees _tag ProviderModelNotFoundError -> nice text + +Session prompt +└─ catch ModelNotFoundError -> publish Session.Event.Error as message/session wire shape + +HTTP route +└─ catch ModelNotFoundError -> declared BadRequest public API error when the endpoint needs it + +HTTP middleware +└─ no Provider.ModelNotFoundError knowledge +``` + +## Refining Known Promise Failures + +Use `EffectPromise.refineRejection(...)` when a Promise boundary can reject +with many unknown values, but only one or two rejection classes are expected +domain failures. Unknown rejections stay defects; the helper maps only known +rejection shapes to typed errors. + +```ts +const language = + yield * + EffectPromise.refineRejection( + async () => loadFromProvider(), + (cause) => (cause instanceof NoSuchModelError ? new ModelNotFoundError({ providerID, modelID, cause }) : undefined), + ) +``` + +Use this when the Promise can genuinely reject and most rejection values are +still defects for the current module. Use `Effect.tryPromise({ try, catch })` +when every rejection should become the same expected error type. Use +`Effect.promise(...)` only when rejection means a defect and you do not need +to refine known rejection classes. + +## Helper Modules We Probably Want + +Add helpers only when repeated call sites prove the seam is real. + +### HTTP API Errors + +Likely location: `src/server/routes/instance/httpapi/errors.ts`. + +Purpose: + +- construct public HTTP error bodies +- preserve legacy `{ name, data }` where needed +- attach `httpApiStatus` + +Good helpers: + +```ts +notFound(message) +badRequest(message) +unknown() +``` + +Avoid: + +```ts +mapAnyDomainError(error) +``` + +That recreates the giant middleware mapper problem. + +### Session / Message Error Wire Helpers + +Likely location: near `src/session/message-error.ts` or a new narrow +module such as `src/session/event-error.ts`. + +Purpose: + +- construct the `{ name, data }` shape used by `Session.Event.Error` and + assistant message errors +- replace `new NamedError.Unknown(...).toObject()` call sites +- keep model-visible error bodies separate from service/domain errors + +Good helpers: + +```ts +unknown(message) +agentNotFound(agent, available) +commandNotFound(command, available) +modelNotFound(error: Provider.ModelNotFoundError) +``` + +### CLI Formatters + +Likely location: `src/cli/error.ts` until repetition demands domain-local +format helpers. + +Purpose: + +- produce human-readable terminal messages from typed errors +- support old `{ name, data }` shapes only while compatibility is needed + +## Migration Queue + +### Remove Domain Knowledge From HTTP Middleware + +- [x] Storage not found no longer maps through defect fallback. +- [x] Worktree expected errors moved to typed errors. +- [x] Provider auth expected errors moved to typed errors. +- [x] Provider model not found no longer needs an HTTP middleware status + special case. +- [ ] Convert `Session.BusyError` and map it at route boundaries. +- [ ] Delete the broad `NamedError` middleware branch once no route relies + on defect-wrapped legacy domain errors. +- [ ] Keep one final unknown-defect fallback that logs `Cause.pretty(cause)` + and returns a safe `500` body. + +### Remaining `NamedError.create(...)` Service Errors + +These should become `Schema.TaggedErrorClass` when touched: + +- [ ] `src/provider/provider.ts` — `ProviderInitError`. +- [ ] `src/storage/db.ts` — database `NotFoundError`. +- [ ] `src/mcp/index.ts` — `MCPFailed`. +- [ ] `src/skill/index.ts` — `SkillInvalidError`, + `SkillNameMismatchError`. +- [ ] `src/lsp/client.ts` — `LSPInitializeError`. +- [ ] `src/ide/index.ts` — install errors. +- [ ] `src/config/error.ts`, `src/config/config.ts`, + `src/config/markdown.ts` — config errors. These already render well + in the CLI, so migrate carefully and preserve diagnostics. + +### Session / Message Wire Errors + +These are not ordinary service errors. They mostly build `{ name, data }` +objects for model-visible/session-visible output. + +- [ ] Add a first-class session/message error wire helper. +- [ ] Replace `new NamedError.Unknown(...).toObject()` in + `src/session/prompt.ts`. +- [ ] Replace `new NamedError.Unknown(...).toObject()` in config/skill/plugin + session event publishing. +- [ ] Move `src/session/message-error.ts` and `src/session/message-v2.ts` + away from `NamedError.create(...)` once the wire helper exists. +- [ ] Update retry/message tests to assert the wire schema/helper output, + not `NamedError` instances. + +### CLI Rendering + +- [x] Tagged config errors render with useful diagnostics. +- [x] Provider model not found renders from both old `{ name, data }` and + new `_tag` shapes. +- [ ] Add typed render cases as more `NamedError.create(...)` domains move + to `Schema.TaggedErrorClass`. +- [ ] Eventually remove old-shape compatibility branches when no callers can + produce them. + +## PR Checklist + +For each migrated error: + +- [ ] Domain error is `Schema.TaggedErrorClass`. +- [ ] Service method exposes the typed error in its error channel. +- [ ] No service error has `toObject()` just for compatibility. +- [ ] CLI, HTTP, and session/message adapters each own their output shape. +- [ ] HTTP middleware gets smaller or stays unchanged. +- [ ] Focused tests cover the domain error and any public rendering/wire + shape touched by the PR. diff --git a/packages/opencode/specs/effect/errors.md b/packages/opencode/specs/effect/errors.md index 5266ca5101..69298bde5c 100644 --- a/packages/opencode/specs/effect/errors.md +++ b/packages/opencode/specs/effect/errors.md @@ -4,6 +4,9 @@ This note expands the `ERR`, `RENDER`, and `HTTP` tracks from [`todo.md`](./todo.md). It is the current reference for expected failures, typed service errors, and HTTP error boundaries. +For the migration architecture and queue, see +[`error-boundaries-plan.md`](./error-boundaries-plan.md). + ## Goal - Expected service failures live on the Effect error channel. diff --git a/packages/opencode/specs/effect/todo.md b/packages/opencode/specs/effect/todo.md index 9261811c38..0c10f12275 100644 --- a/packages/opencode/specs/effect/todo.md +++ b/packages/opencode/specs/effect/todo.md @@ -2,8 +2,9 @@ Short roadmap for Effect cleanup in `packages/opencode`. -Current patterns and examples live in [`guide.md`](./guide.md). Test -migration rules live in +Current patterns and examples live in [`guide.md`](./guide.md). Error +boundary migration details live in +[`error-boundaries-plan.md`](./error-boundaries-plan.md). Test migration rules live in [`test/EFFECT_TEST_MIGRATION.md`](../../test/EFFECT_TEST_MIGRATION.md). Older deep-dive notes in this directory may still be useful, but treat this roadmap and the guide as the current entry points. diff --git a/packages/opencode/src/agent/agent.ts b/packages/opencode/src/agent/agent.ts index 1e4d7e1563..ce6cf30b6d 100644 --- a/packages/opencode/src/agent/agent.ts +++ b/packages/opencode/src/agent/agent.ts @@ -62,11 +62,14 @@ export interface Interface { readonly generate: (input: { description: string model?: { providerID: ProviderID; modelID: ModelID } - }) => Effect.Effect<{ - identifier: string - whenToUse: string - systemPrompt: string - }> + }) => Effect.Effect< + { + identifier: string + whenToUse: string + systemPrompt: string + }, + Provider.ModelNotFoundError + > } type State = Omit diff --git a/packages/opencode/src/cli/error.ts b/packages/opencode/src/cli/error.ts index 2bb0cb51fd..ffb0e0cfba 100644 --- a/packages/opencode/src/cli/error.ts +++ b/packages/opencode/src/cli/error.ts @@ -1,5 +1,6 @@ import { NamedError } from "@opencode-ai/core/util/error" import { errorFormat } from "@/util/error" +import { isRecord } from "@/util/record" interface ErrorLike { name?: string @@ -10,10 +11,6 @@ interface ErrorLike { type ConfigIssue = { message: string; path: string[] } -function isRecord(input: unknown): input is Record { - return typeof input === "object" && input !== null -} - function isTaggedError(error: unknown, tag: string): boolean { return isRecord(error) && error._tag === tag } @@ -61,11 +58,13 @@ export function FormatError(input: unknown) { } // ProviderModelNotFoundError: { providerID: string, modelID: string, suggestions?: string[] } - if (NamedError.hasName(input, "ProviderModelNotFoundError")) { - const data = (input as ErrorLike).data - const suggestions = Array.isArray(data?.suggestions) ? data.suggestions.filter((x) => typeof x === "string") : [] + const providerModelNotFound = configData(input, "ProviderModelNotFoundError") + if (providerModelNotFound) { + const suggestions = Array.isArray(providerModelNotFound.suggestions) + ? providerModelNotFound.suggestions.filter((x) => typeof x === "string") + : [] return [ - `Model not found: ${data?.providerID}/${data?.modelID}`, + `Model not found: ${providerModelNotFound.providerID}/${providerModelNotFound.modelID}`, ...(suggestions.length ? ["Did you mean: " + suggestions.join(", ")] : []), `Try: \`opencode models\` to list available models`, `Or check your config (opencode.json) provider/model names`, diff --git a/packages/opencode/src/effect/promise.ts b/packages/opencode/src/effect/promise.ts new file mode 100644 index 0000000000..d7918796ad --- /dev/null +++ b/packages/opencode/src/effect/promise.ts @@ -0,0 +1,17 @@ +import { Cause, Effect } from "effect" + +export function refineRejection( + evaluate: (signal: AbortSignal) => PromiseLike, + refine: (cause: unknown) => E | undefined, +) { + return Effect.tryPromise(evaluate).pipe( + Effect.catch((error) => { + const cause = Cause.isUnknownError(error) ? error.cause : error + const refined = refine(cause) + if (refined !== undefined) return Effect.fail(refined) + return Effect.die(cause) + }), + ) +} + +export * as EffectPromise from "./promise" diff --git a/packages/opencode/src/provider/provider.ts b/packages/opencode/src/provider/provider.ts index d4d28088d9..cbf879620c 100644 --- a/packages/opencode/src/provider/provider.ts +++ b/packages/opencode/src/provider/provider.ts @@ -21,6 +21,7 @@ import { pathToFileURL } from "url" import { Effect, Layer, Context, Schema, Types } from "effect" import { EffectBridge } from "@/effect/bridge" import { InstanceState } from "@/effect/instance-state" +import { EffectPromise } from "@/effect/promise" import { AppFileSystem } from "@opencode-ai/core/filesystem" import { isRecord } from "@/util/record" import { optionalOmitUndefined } from "@opencode-ai/core/schema" @@ -956,11 +957,24 @@ export function defaultModelIDs sort(Object.values(item.models))[0].id) } +export class ModelNotFoundError extends Schema.TaggedErrorClass()("ProviderModelNotFoundError", { + providerID: ProviderID, + modelID: ModelID, + suggestions: Schema.optional(Schema.Array(Schema.String)), + cause: Schema.optional(Schema.Defect), +}) { + static isInstance(input: unknown): input is ModelNotFoundError { + return input instanceof ModelNotFoundError + } +} + +export type Error = ModelNotFoundError + export interface Interface { readonly list: () => Effect.Effect> readonly getProvider: (providerID: ProviderID) => Effect.Effect - readonly getModel: (providerID: ProviderID, modelID: ModelID) => Effect.Effect - readonly getLanguage: (model: Model) => Effect.Effect + readonly getModel: (providerID: ProviderID, modelID: ModelID) => Effect.Effect + readonly getLanguage: (model: Model) => Effect.Effect readonly closest: ( providerID: ProviderID, query: string[], @@ -1591,14 +1605,14 @@ const layer: Layer.Layer< if (!provider) { const available = Object.keys(s.providers) const matches = fuzzysort.go(providerID, available, { limit: 3, threshold: -10000 }) - throw new ModelNotFoundError({ providerID, modelID, suggestions: matches.map((m) => m.target) }) + return yield* new ModelNotFoundError({ providerID, modelID, suggestions: matches.map((m) => m.target) }) } const info = provider.models[modelID] if (!info) { const available = Object.keys(provider.models) const matches = fuzzysort.go(modelID, available, { limit: 3, threshold: -10000 }) - throw new ModelNotFoundError({ providerID, modelID, suggestions: matches.map((m) => m.target) }) + return yield* new ModelNotFoundError({ providerID, modelID, suggestions: matches.map((m) => m.target) }) } return info }) @@ -1609,11 +1623,10 @@ const layer: Layer.Layer< const key = `${model.providerID}/${model.id}` if (s.models.has(key)) return s.models.get(key)! - return yield* Effect.promise(async () => { - const provider = s.providers[model.providerID] - const sdk = await resolveSDK(model, s, envs) - - try { + const provider = s.providers[model.providerID] + return yield* EffectPromise.refineRejection( + async () => { + const sdk = await resolveSDK(model, s, envs) const language = s.modelLoaders[model.providerID] ? await s.modelLoaders[model.providerID](sdk, model.api.id, { ...provider.options, @@ -1622,18 +1635,12 @@ const layer: Layer.Layer< : sdk.languageModel(model.api.id) s.models.set(key, language) return language - } catch (e) { - if (e instanceof NoSuchModelError) - throw new ModelNotFoundError( - { - modelID: model.id, - providerID: model.providerID, - }, - { cause: e }, - ) - throw e - } - }) + }, + (cause) => + cause instanceof NoSuchModelError + ? new ModelNotFoundError({ modelID: model.id, providerID: model.providerID, cause }) + : undefined, + ) }) const closest = Effect.fn("Provider.closest")(function* (providerID: ProviderID, query: string[]) { @@ -1653,7 +1660,7 @@ const layer: Layer.Layer< if (cfg.small_model) { const parsed = parseModel(cfg.small_model) - return yield* getModel(parsed.providerID, parsed.modelID) + return yield* getModel(parsed.providerID, parsed.modelID).pipe(Effect.orDie) } const s = yield* InstanceState.get(state) @@ -1681,22 +1688,22 @@ const layer: Layer.Layer< const candidates = Object.keys(provider.models).filter((m) => m.includes(item)) const globalMatch = candidates.find((m) => m.startsWith("global.")) - if (globalMatch) return yield* getModel(providerID, ModelID.make(globalMatch)) + if (globalMatch) return yield* getModel(providerID, ModelID.make(globalMatch)).pipe(Effect.orDie) const region = provider.options?.region if (region) { const regionPrefix = region.split("-")[0] if (regionPrefix === "us" || regionPrefix === "eu") { const regionalMatch = candidates.find((m) => m.startsWith(`${regionPrefix}.`)) - if (regionalMatch) return yield* getModel(providerID, ModelID.make(regionalMatch)) + if (regionalMatch) return yield* getModel(providerID, ModelID.make(regionalMatch)).pipe(Effect.orDie) } } const unprefixed = candidates.find((m) => !crossRegionPrefixes.some((p) => m.startsWith(p))) - if (unprefixed) return yield* getModel(providerID, ModelID.make(unprefixed)) + if (unprefixed) return yield* getModel(providerID, ModelID.make(unprefixed)).pipe(Effect.orDie) } else { for (const model of Object.keys(provider.models)) { - if (model.includes(item)) return yield* getModel(providerID, ModelID.make(model)) + if (model.includes(item)) return yield* getModel(providerID, ModelID.make(model)).pipe(Effect.orDie) } } } @@ -1771,12 +1778,6 @@ export function parseModel(model: string) { } } -export const ModelNotFoundError = NamedError.create("ProviderModelNotFoundError", { - providerID: ProviderID, - modelID: ModelID, - suggestions: Schema.optional(Schema.Array(Schema.String)), -}) - export const InitError = NamedError.create("ProviderInitError", { providerID: ProviderID, }) diff --git a/packages/opencode/src/server/routes/instance/httpapi/middleware/error.ts b/packages/opencode/src/server/routes/instance/httpapi/middleware/error.ts index bb75f6602c..0a126abb2d 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/middleware/error.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/middleware/error.ts @@ -1,6 +1,4 @@ -import { Provider } from "@/provider/provider" import { Session } from "@/session/session" -import { iife } from "@/util/iife" import { NamedError } from "@opencode-ai/core/util/error" import * as Log from "@opencode-ai/core/util/log" import { Cause, Effect } from "effect" @@ -24,14 +22,7 @@ export const errorLayer = HttpRouter.middleware<{ handles: unknown }>()((effect) log.error("failed", { error, cause: Cause.pretty(cause) }) if (error instanceof NamedError) { - return Effect.succeed( - HttpServerResponse.jsonUnsafe(error.toObject(), { - status: iife(() => { - if (error instanceof Provider.ModelNotFoundError) return 400 - return 500 - }), - }), - ) + return Effect.succeed(HttpServerResponse.jsonUnsafe(error.toObject(), { status: 500 })) } if (error instanceof Session.BusyError) { return Effect.succeed( diff --git a/packages/opencode/src/session/compaction.ts b/packages/opencode/src/session/compaction.ts index 3340e0fbe9..fabdc01f09 100644 --- a/packages/opencode/src/session/compaction.ts +++ b/packages/opencode/src/session/compaction.ts @@ -390,8 +390,8 @@ export const layer: Layer.Layer< const agent = yield* agents.get("compaction") const model = agent.model - ? yield* provider.getModel(agent.model.providerID, agent.model.modelID) - : yield* provider.getModel(userMessage.model.providerID, userMessage.model.modelID) + ? yield* provider.getModel(agent.model.providerID, agent.model.modelID).pipe(Effect.orDie) + : yield* provider.getModel(userMessage.model.providerID, userMessage.model.modelID).pipe(Effect.orDie) const cfg = yield* config.get() const history = compactionPart && messages.at(-1)?.info.id === input.parentID ? messages.slice(0, -1) : messages const prior = completedCompactions(history) @@ -519,7 +519,9 @@ export const layer: Layer.Layer< { sessionID: input.sessionID, agent: userMessage.agent, - model: yield* provider.getModel(userMessage.model.providerID, userMessage.model.modelID), + model: yield* provider + .getModel(userMessage.model.providerID, userMessage.model.modelID) + .pipe(Effect.orDie), provider: { source: info.source, info, diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index cae0dd3845..93b26669ea 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -1056,15 +1056,15 @@ NOTE: At any point in time through this workflow you should feel free to ask the if (Exit.isSuccess(exit)) return exit.value const err = Cause.squash(exit.cause) if (Provider.ModelNotFoundError.isInstance(err)) { - const hint = err.data.suggestions?.length ? ` Did you mean: ${err.data.suggestions.join(", ")}?` : "" + const hint = err.suggestions?.length ? ` Did you mean: ${err.suggestions.join(", ")}?` : "" yield* bus.publish(Session.Event.Error, { sessionID, error: new NamedError.Unknown({ - message: `Model not found: ${err.data.providerID}/${err.data.modelID}.${hint}`, + message: `Model not found: ${err.providerID}/${err.modelID}.${hint}`, }).toObject(), }) } - return yield* Effect.failCause(exit.cause) + return yield* Effect.die(err) }) const currentModel = Effect.fnUntraced(function* (sessionID: SessionID) { @@ -1107,7 +1107,9 @@ NOTE: At any point in time through this workflow you should feel free to ask the const same = ag.model && model.providerID === ag.model.providerID && model.modelID === ag.model.modelID const full = !input.variant && ag.variant && same - ? yield* provider.getModel(model.providerID, model.modelID).pipe(Effect.catchDefect(() => Effect.void)) + ? yield* provider + .getModel(model.providerID, model.modelID) + .pipe(Effect.catchIf(Provider.ModelNotFoundError.isInstance, () => Effect.succeed(undefined))) : undefined const variant = input.variant ?? (ag.variant && full?.variants?.[ag.variant] ? ag.variant : undefined) diff --git a/packages/opencode/test/cli/error.test.ts b/packages/opencode/test/cli/error.test.ts index 6c5ab265b3..63e04695d1 100644 --- a/packages/opencode/test/cli/error.test.ts +++ b/packages/opencode/test/cli/error.test.ts @@ -64,6 +64,23 @@ describe("cli.error", () => { expect(formatted).toContain("Check your network, proxy, or VPN configuration and try again.") }) + test("formats legacy and tagged provider model errors the same way", () => { + const data = { + providerID: "anthropic", + modelID: "claude-sonet-4", + suggestions: ["claude-sonnet-4"], + } + const expected = [ + "Model not found: anthropic/claude-sonet-4", + "Did you mean: claude-sonnet-4", + "Try: `opencode models` to list available models", + "Or check your config (opencode.json) provider/model names", + ].join("\n") + + expect(FormatError({ name: "ProviderModelNotFoundError", data })).toBe(expected) + expect(FormatError({ _tag: "ProviderModelNotFoundError", ...data })).toBe(expected) + }) + test("formats cancelled UI errors as empty output", () => { expect(FormatError(new UI.CancelledError())).toBe("") }) diff --git a/packages/opencode/test/provider/provider.test.ts b/packages/opencode/test/provider/provider.test.ts index 2270418beb..afdd420bc7 100644 --- a/packages/opencode/test/provider/provider.test.ts +++ b/packages/opencode/test/provider/provider.test.ts @@ -1571,8 +1571,8 @@ test("ModelNotFoundError includes suggestions for typos", async () => { await getModel(ProviderID.anthropic, ModelID.make("claude-sonet-4")) // typo: sonet instead of sonnet expect(true).toBe(false) // Should not reach here } catch (e: any) { - expect(e.data.suggestions).toBeDefined() - expect(e.data.suggestions.length).toBeGreaterThan(0) + expect(e.suggestions).toBeDefined() + expect(e.suggestions.length).toBeGreaterThan(0) } }, }) @@ -1597,8 +1597,8 @@ test("ModelNotFoundError for provider includes suggestions", async () => { await getModel(ProviderID.make("antropic"), ModelID.make("claude-sonnet-4")) // typo: antropic expect(true).toBe(false) // Should not reach here } catch (e: any) { - expect(e.data.suggestions).toBeDefined() - expect(e.data.suggestions).toContain("anthropic") + expect(e.suggestions).toBeDefined() + expect(e.suggestions).toContain("anthropic") } }, })