Type session not-found errors (#25818)
This commit is contained in:
Kit Langton
329
packages/opencode/specs/effect/errors.md
Normal file
329
packages/opencode/specs/effect/errors.md
Normal file
@@ -0,0 +1,329 @@
|
||||
# Typed error migration
|
||||
|
||||
Plan for moving `packages/opencode` from temporary defect/`NamedError`
|
||||
compatibility toward typed Effect service errors and explicit HTTP error
|
||||
contracts.
|
||||
|
||||
## Goal
|
||||
|
||||
- Expected service failures live on the Effect error channel.
|
||||
- Service interfaces expose those failures in their return types.
|
||||
- Domain errors are authored with Effect Schema so they are reusable by services,
|
||||
tests, HTTP routes, tools, and OpenAPI generation.
|
||||
- HTTP status codes and wire compatibility are handled at the HTTP boundary, not
|
||||
inside service modules.
|
||||
- `Effect.die`, `throw`, `catchDefect`, and global cause inspection are reserved
|
||||
for defects, compatibility bridges, or final fallback behavior.
|
||||
|
||||
## Current State
|
||||
|
||||
- Many migrated services use Effect internally, but expected failures are still a
|
||||
mix of `NamedError.create(...)`, `namedSchemaError(...)`, `class extends Error`,
|
||||
`throw`, and `Effect.die(...)`.
|
||||
- Some services already use `Schema.TaggedErrorClass`, for example `Account`,
|
||||
`Auth`, `Permission`, `Question`, `Installation`, and parts of
|
||||
`Workspace`.
|
||||
- Legacy Hono error handling recognizes `NamedError`, `Session.BusyError`, and a
|
||||
few name-based cases, then emits the legacy `{ name, data }` JSON body.
|
||||
- Effect `HttpApi` only knows how to encode errors that are declared on the
|
||||
endpoint, group, or middleware. Undeclared expected errors become defects and
|
||||
eventually fall through to generic HTTP handling.
|
||||
- The temporary HttpApi error middleware catches defect-wrapped legacy errors to
|
||||
preserve runtime behavior, but it is intentionally a bridge rather than the
|
||||
final model.
|
||||
|
||||
## End State
|
||||
|
||||
Service modules own domain failures.
|
||||
|
||||
```ts
|
||||
export class SessionBusyError extends Schema.TaggedErrorClass<SessionBusyError>()("SessionBusyError", {
|
||||
sessionID: SessionID,
|
||||
message: Schema.String,
|
||||
}) {}
|
||||
|
||||
export type Error = Storage.Error | SessionBusyError
|
||||
|
||||
export interface Interface {
|
||||
readonly get: (id: SessionID) => Effect.Effect<Info, Error>
|
||||
}
|
||||
```
|
||||
|
||||
HTTP modules own transport mapping.
|
||||
|
||||
```ts
|
||||
const get = Effect.fn("SessionHttpApi.get")(function* (ctx: { params: { sessionID: SessionID } }) {
|
||||
return yield* session
|
||||
.get(ctx.params.sessionID)
|
||||
.pipe(
|
||||
Effect.catchTag("StorageNotFoundError", () => new SessionNotFoundHttpError({ sessionID: ctx.params.sessionID })),
|
||||
)
|
||||
})
|
||||
```
|
||||
|
||||
HTTP-visible error schemas carry their own response status through Effect
|
||||
HttpApi's `httpApiStatus` annotation. Prefer `HttpApiSchema.status(...)`, or the
|
||||
equivalent declaration annotation, instead of maintaining a parallel status map.
|
||||
|
||||
```ts
|
||||
export class SessionNotFoundHttpError extends Schema.TaggedErrorClass<SessionNotFoundHttpError>()(
|
||||
"SessionNotFoundHttpError",
|
||||
{
|
||||
sessionID: SessionID,
|
||||
message: Schema.String,
|
||||
},
|
||||
{ httpApiStatus: 404 },
|
||||
) {}
|
||||
```
|
||||
|
||||
Endpoint definitions still declare which HTTP-visible error schemas can be
|
||||
emitted. The status annotation is only used if the error is part of the endpoint,
|
||||
group, or middleware error schema and the handler fails with that error on the
|
||||
typed error channel.
|
||||
|
||||
```ts
|
||||
HttpApiEndpoint.get("get", SessionPaths.get, {
|
||||
success: Session.Info,
|
||||
error: [SessionNotFoundHttpError, SessionBusyHttpError],
|
||||
})
|
||||
```
|
||||
|
||||
The service error and HTTP error may be the same class when the wire shape is a
|
||||
deliberate public contract. They should be different classes when the service
|
||||
error contains internals, low-level causes, retry hints, or anything that should
|
||||
not be exposed to API clients.
|
||||
|
||||
## Rules
|
||||
|
||||
- Use `Schema.TaggedErrorClass` for new expected domain errors.
|
||||
- Include `cause: Schema.optional(Schema.Defect)` only when preserving an
|
||||
underlying unknown failure is useful for logs or callers.
|
||||
- Export a domain-level error union from each service module, for example
|
||||
`export type Error = NotFoundError | BusyError | Storage.Error`.
|
||||
- Put expected errors in service method signatures, for example
|
||||
`Effect.Effect<Result, Service.Error, R>`.
|
||||
- Use `yield* new DomainError(...)` for direct early failures inside
|
||||
`Effect.gen` / `Effect.fn`.
|
||||
- Use `Effect.try({ try, catch })`, `Effect.mapError`, or `Effect.catchTag` to
|
||||
convert external exceptions into domain errors.
|
||||
- Use `HttpApiSchema.status(...)` or `{ httpApiStatus: code }` on HTTP-visible
|
||||
error schemas so Effect `HttpApiBuilder` and OpenAPI generation get the status
|
||||
from the schema itself.
|
||||
- Do not use `Effect.die(...)` for user, IO, validation, missing-resource, auth,
|
||||
provider, worktree, or busy-state failures.
|
||||
- Do not use `catchDefect` to recover expected domain errors. If recovery is
|
||||
needed, the upstream effect should fail with a typed error instead.
|
||||
- Do not make service modules import `HttpApiError`, `HttpServerResponse`, HTTP
|
||||
status codes, or route-specific error schemas.
|
||||
- Keep raw `HttpRouter` routes free to use `HttpServerRespondable` when that is
|
||||
the right transport abstraction, but prefer declared `HttpApi` errors for
|
||||
normal JSON API endpoints.
|
||||
|
||||
## HTTP Boundary Shape
|
||||
|
||||
Create an HttpApi-local error module, likely
|
||||
`src/server/routes/instance/httpapi/errors.ts`.
|
||||
|
||||
That module should provide:
|
||||
|
||||
- Legacy-compatible public schemas for `{ name, data }` error bodies that must
|
||||
remain SDK-compatible during the Hono migration.
|
||||
- Small constructors or mapping helpers for common API errors such as not found,
|
||||
bad request, conflict, and unknown internal errors.
|
||||
- Route-group-specific adapters only when they encode domain-specific public
|
||||
data.
|
||||
- A single place to document which public error shape is legacy-compatible and
|
||||
which shape is new Effect-native API surface.
|
||||
|
||||
Avoid one giant `unknown -> status` mapper. Prefer small, explicit mappers close
|
||||
to the handler or route group.
|
||||
|
||||
```ts
|
||||
const mapSessionError = <A, E, R>(effect: Effect.Effect<A, E, R>) =>
|
||||
effect.pipe(
|
||||
Effect.catchTag("StorageNotFoundError", (error) => new SessionNotFoundHttpError({ message: error.message })),
|
||||
Effect.catchTag("SessionBusyError", (error) => new SessionBusyHttpError({ message: error.message })),
|
||||
)
|
||||
```
|
||||
|
||||
Use built-in `HttpApiError.BadRequest`, `HttpApiError.NotFound`, and related
|
||||
types only when their generated response body and SDK surface are intentionally
|
||||
acceptable. Use a custom schema-backed error when clients need the legacy
|
||||
`{ name, data }` body or a domain-specific error payload.
|
||||
|
||||
## Migration Phases
|
||||
|
||||
### 1. Stabilize The Bridge
|
||||
|
||||
Keep the temporary HttpApi error middleware only as a compatibility bridge while
|
||||
typed errors are introduced.
|
||||
|
||||
- Add tests that prove the bridge catches legacy `NamedError` defects.
|
||||
- Add tests that prove declared HttpApi errors still use the declared endpoint
|
||||
contract.
|
||||
- Stop returning stack traces in unknown HTTP `500` responses; log the full
|
||||
`Cause.pretty(cause)` server-side instead.
|
||||
- Add a comment or TODO that names this plan and states the bridge must shrink
|
||||
as route groups migrate.
|
||||
|
||||
### 2. Define The Shared HTTP Error Helpers
|
||||
|
||||
Add the `httpapi/errors.ts` module before converting route groups.
|
||||
|
||||
- Define a legacy `{ name, data }` body helper for SDK-compatible errors.
|
||||
- Define `UnknownError` for generic internal failures with a safe public message.
|
||||
- Define `BadRequestError` and `NotFoundError` equivalents only if the actual
|
||||
wire body must match the legacy Hono SDK surface.
|
||||
- Put the HTTP status on the public schema with `HttpApiSchema.status(...)` or
|
||||
`{ httpApiStatus: code }`; do not keep a separate name-to-status table.
|
||||
- Keep conversion helpers pure and small. They should not inspect `Cause` or
|
||||
accept `unknown` unless they are final fallback helpers.
|
||||
|
||||
### 3. Convert One Vertical Slice
|
||||
|
||||
Start with session read routes because they already have local `mapNotFound`
|
||||
logic and are heavily covered by existing HttpApi tests.
|
||||
|
||||
- Convert `Session.BusyError` from a plain `Error` to a typed service error, or
|
||||
add a typed wrapper while preserving the old constructor until callers are
|
||||
migrated.
|
||||
- Replace `catchDefect` in `httpapi/handlers/session.ts` with typed error
|
||||
mapping.
|
||||
- Add endpoint error schemas for the affected session endpoints.
|
||||
- Prove behavior with focused tests in `test/server/httpapi-session.test.ts`.
|
||||
- Remove the migrated cases from the global compatibility middleware.
|
||||
|
||||
### 4. Convert Legacy NamedError Domains
|
||||
|
||||
Move legacy `NamedError.create(...)` services to Effect Schema-backed errors in
|
||||
small domain PRs.
|
||||
|
||||
Priority order:
|
||||
|
||||
1. `storage/storage.ts` and `storage/db.ts` not-found errors.
|
||||
2. `worktree/index.ts` `Worktree*` errors.
|
||||
3. `provider/auth.ts` validation failures and `provider/provider.ts` model-not-found errors.
|
||||
4. `mcp/index.ts`, `skill/index.ts`, `lsp/client.ts`, and `ide/index.ts` service errors.
|
||||
5. Config and CLI-only errors after HTTP-facing domains are stable.
|
||||
|
||||
For each domain:
|
||||
|
||||
- Replace `NamedError.create(...)` with `Schema.TaggedErrorClass` when the error
|
||||
is primarily a service error.
|
||||
- Keep or add a separate HTTP error schema when the legacy `{ name, data }` wire
|
||||
shape must remain stable.
|
||||
- Update service interface return types to include the new error union.
|
||||
- Replace `throw new X(...)` inside `Effect.fn` with `yield* new X(...)`.
|
||||
- Replace async exceptions with `Effect.try({ catch })` or explicit `mapError`.
|
||||
- Add service-level tests that assert the error tag and data, not just the HTTP
|
||||
status.
|
||||
|
||||
### 5. Declare HttpApi Errors Group By Group
|
||||
|
||||
For each HttpApi group:
|
||||
|
||||
- Inventory every service call and the typed errors it can return.
|
||||
- Add only the public error schemas that endpoint can actually emit.
|
||||
- Map service errors to HTTP errors in the handler file.
|
||||
- Keep built-in `HttpApiError` only for generic request/validation failures where
|
||||
the generated contract is accepted.
|
||||
- Update `httpapi/public.ts` compatibility transforms only when the generated
|
||||
spec cannot represent the desired source shape directly.
|
||||
- Regenerate the SDK after OpenAPI-visible changes and verify the diff is
|
||||
intentional.
|
||||
|
||||
Suggested route order:
|
||||
|
||||
1. `session` not-found and busy-state reads.
|
||||
2. `experimental` worktree mutations.
|
||||
3. `provider` auth and model selection errors.
|
||||
4. `mcp` OAuth and connection errors.
|
||||
5. Remaining route groups as Hono deletion work progresses.
|
||||
|
||||
### 6. Remove Defect Recovery
|
||||
|
||||
After enough route groups declare their expected errors:
|
||||
|
||||
- Delete `catchDefect` recovery for domain errors.
|
||||
- Delete name-prefix checks such as `error.name.startsWith("Worktree")` from
|
||||
HTTP middleware.
|
||||
- Delete `NamedError` branches from the Effect HttpApi compatibility middleware
|
||||
once no Effect route depends on them.
|
||||
- Leave one final unknown-defect fallback that logs server-side and returns a
|
||||
safe generic `500` body.
|
||||
|
||||
## Inventory Checklist
|
||||
|
||||
Use this checklist when touching a service or route group.
|
||||
|
||||
- [ ] Does the service interface expose every expected failure in the Effect
|
||||
error type?
|
||||
- [ ] Are user-caused, provider-caused, IO, auth, missing-resource, and busy-state
|
||||
failures modeled as typed errors instead of defects?
|
||||
- [ ] Does the service avoid importing HTTP status, `HttpApiError`, or response
|
||||
classes?
|
||||
- [ ] Does the handler map each service error into a declared endpoint error?
|
||||
- [ ] Does the endpoint `error` field include every public error the handler can
|
||||
emit?
|
||||
- [ ] Does OpenAPI/SDK output either stay byte-identical or have an explicitly
|
||||
reviewed diff?
|
||||
- [ ] Do tests cover both service-level error typing and HTTP-level status/body?
|
||||
- [ ] Did the PR remove any now-unneeded case from the temporary compatibility
|
||||
middleware?
|
||||
|
||||
## Testing Requirements
|
||||
|
||||
For service conversions:
|
||||
|
||||
- Test the service method directly with `testEffect(...)`.
|
||||
- Assert on `_tag` or class identity and the structured fields.
|
||||
- Avoid testing by string-matching `Cause.pretty(...)`.
|
||||
|
||||
For HttpApi conversions:
|
||||
|
||||
- Add or update the focused `test/server/httpapi-*.test.ts` file.
|
||||
- Assert status code, content type, and exact JSON body for declared public
|
||||
errors.
|
||||
- Add a regression test that the temporary middleware is no longer needed for the
|
||||
migrated route.
|
||||
- Keep bridge/parity tests aligned with legacy Hono behavior until Hono is
|
||||
deleted or the SDK contract intentionally changes.
|
||||
|
||||
## Verification Commands
|
||||
|
||||
Run from `packages/opencode` unless noted otherwise.
|
||||
|
||||
```bash
|
||||
bun run prettier --write <changed files>
|
||||
bunx oxlint <changed files>
|
||||
bun typecheck
|
||||
bun run test -- test/server/httpapi-session.test.ts
|
||||
```
|
||||
|
||||
Run SDK generation from the repo root when schemas or OpenAPI-visible errors
|
||||
change.
|
||||
|
||||
```bash
|
||||
./packages/sdk/js/script/build.ts
|
||||
```
|
||||
|
||||
## Open Questions
|
||||
|
||||
- Should legacy V1 routes keep `{ name, data }` forever while V2 routes expose a
|
||||
more Effect-native tagged error body?
|
||||
- Should storage not-found remain generic, or should callers map it to
|
||||
domain-specific not-found errors before crossing service boundaries?
|
||||
- Should `namedSchemaError(...)` stay as a long-term public-wire helper, or only
|
||||
as a migration bridge for old `NamedError` contracts?
|
||||
- Which SDK version boundary lets us stop remapping built-in Effect HttpApi error
|
||||
schemas in `httpapi/public.ts`?
|
||||
|
||||
## Success Criteria
|
||||
|
||||
- New service code no longer uses `die` for expected failures.
|
||||
- A route reviewer can read an endpoint definition and see every public error it
|
||||
can return.
|
||||
- The temporary HttpApi error middleware shrinks over time instead of gaining new
|
||||
name-based cases.
|
||||
- Service tests prove domain error types without going through HTTP.
|
||||
- HTTP tests prove status/body contracts without relying on defect recovery.
|
||||
Reference in New Issue
Block a user