Parcourir la source

sketch: typed SessionNotFound (design validation)

Demonstrates the typed-errors-at-the-service-layer pattern on a single
method (`Session.Service.get`):

- Define `Session.SessionNotFound` as `Schema.ErrorClass` with
  `httpApiStatus: 404` and a body shape that matches the existing Hono
  `NamedError` envelope (`name: "NotFoundError"`, `data: { message }`).
- Update `Session.Service.Interface.get` and the implementation to fail
  in the E channel via `yield* new SessionNotFound(...)`.
- HttpApi endpoint declares the typed error directly. The handler drops
  its `mapNotFound` wrapper for `get` — Effect HttpApi auto-routes the
  404 status from the schema annotation and serializes the body fields.
- Hono adapter (`runRequest`) bridges the typed error back to the
  legacy `NotFoundError` defect so the existing `ErrorMiddleware` keeps
  rendering 404 + body until that adapter is retired.

End-to-end validation: the unskipped parity test
(`Error JSON shape parity > HttpApi 404 body matches NamedError shape`)
now passes against the HttpApi adapter.

This is a sketch, not for merge — most cascading consumers are
absorbed via `Effect.orDie` (legacy never-E surface preserved). Real
rollout migrates each method/handler explicitly.
Kit Langton il y a 3 mois
Parent
commit
a2bc535367

+ 4 - 1
packages/opencode/src/server/routes/instance/httpapi/groups/session.ts

@@ -123,7 +123,10 @@ export const SessionApi = HttpApi.make("session")
         HttpApiEndpoint.get("get", SessionPaths.get, {
           params: { sessionID: SessionID },
           success: described(Session.Info, "Get session"),
-          error: [HttpApiError.BadRequest, HttpApiError.NotFound],
+          // Sketch: typed `Session.SessionNotFound` declared directly. Effect
+          // HttpApi auto-routes the 404 status (`httpApiStatus` annotation)
+          // and serializes the schema fields (`name`, `data.message`).
+          error: [HttpApiError.BadRequest, Session.SessionNotFound],
         }).annotateMerge(
           OpenApi.annotations({
             identifier: "session.get",

+ 65 - 36
packages/opencode/src/server/routes/instance/httpapi/handlers/session.ts

@@ -38,13 +38,22 @@ import {
   UpdatePayload,
 } from "../groups/session"
 
-const mapNotFound = <A, E, R>(self: Effect.Effect<A, E, R>) =>
+const mapNotFound = <A, E, R>(
+  self: Effect.Effect<A, E, R>,
+): Effect.Effect<A, Exclude<E, Session.SessionNotFound> | HttpApiError.NotFound, R> =>
   self.pipe(
     Effect.catchIf(NotFoundError.isInstance, () => Effect.fail(new HttpApiError.NotFound({}))),
+    // Sketch: bridge typed `SessionNotFound` into the legacy wrapper so
+    // handlers that internally call `session.get` (whose endpoint
+    // declarations have not been migrated yet) keep returning 404.
+    Effect.catchIf(
+      (e: unknown) => e instanceof Session.SessionNotFound,
+      () => Effect.fail(new HttpApiError.NotFound({})),
+    ),
     Effect.catchDefect((error) =>
       NotFoundError.isInstance(error) ? Effect.fail(new HttpApiError.NotFound({})) : Effect.die(error),
     ),
-  )
+  ) as Effect.Effect<A, Exclude<E, Session.SessionNotFound> | HttpApiError.NotFound, R>
 
 export const sessionHandlers = HttpApiBuilder.group(InstanceHttpApi, "session", (handlers) =>
   Effect.gen(function* () {
@@ -78,8 +87,11 @@ export const sessionHandlers = HttpApiBuilder.group(InstanceHttpApi, "session",
       return Object.fromEntries(yield* statusSvc.list())
     })
 
+    // Sketch: typed `SessionNotFound` flows through the E channel — HttpApi
+    // maps the status (404 via `httpApiStatus`) and renders the body via the
+    // schema annotations. No `mapNotFound` wrapper required.
     const get = Effect.fn("SessionHttpApi.get")(function* (ctx: { params: { sessionID: SessionID } }) {
-      return yield* mapNotFound(session.get(ctx.params.sessionID))
+      return yield* session.get(ctx.params.sessionID)
     })
 
     const children = Effect.fn("SessionHttpApi.children")(function* (ctx: { params: { sessionID: SessionID } }) {
@@ -178,20 +190,24 @@ export const sessionHandlers = HttpApiBuilder.group(InstanceHttpApi, "session",
       params: { sessionID: SessionID }
       payload: typeof UpdatePayload.Type
     }) {
-      const current = yield* session.get(ctx.params.sessionID)
-      if (ctx.payload.title !== undefined) {
-        yield* session.setTitle({ sessionID: ctx.params.sessionID, title: ctx.payload.title })
-      }
-      if (ctx.payload.permission !== undefined) {
-        yield* session.setPermission({
-          sessionID: ctx.params.sessionID,
-          permission: Permission.merge(current.permission ?? [], ctx.payload.permission),
-        })
-      }
-      if (ctx.payload.time?.archived !== undefined) {
-        yield* session.setArchived({ sessionID: ctx.params.sessionID, time: ctx.payload.time.archived })
-      }
-      return yield* session.get(ctx.params.sessionID)
+      return yield* mapNotFound(
+        Effect.gen(function* () {
+          const current = yield* session.get(ctx.params.sessionID)
+          if (ctx.payload.title !== undefined) {
+            yield* session.setTitle({ sessionID: ctx.params.sessionID, title: ctx.payload.title })
+          }
+          if (ctx.payload.permission !== undefined) {
+            yield* session.setPermission({
+              sessionID: ctx.params.sessionID,
+              permission: Permission.merge(current.permission ?? [], ctx.payload.permission),
+            })
+          }
+          if (ctx.payload.time?.archived !== undefined) {
+            yield* session.setArchived({ sessionID: ctx.params.sessionID, time: ctx.payload.time.archived })
+          }
+          return yield* session.get(ctx.params.sessionID)
+        }),
+      )
     })
 
     const fork = Effect.fn("SessionHttpApi.fork")(function* (ctx: {
@@ -221,35 +237,48 @@ export const sessionHandlers = HttpApiBuilder.group(InstanceHttpApi, "session",
     })
 
     const share = Effect.fn("SessionHttpApi.share")(function* (ctx: { params: { sessionID: SessionID } }) {
-      yield* shareSvc.share(ctx.params.sessionID).pipe(Effect.mapError(() => new HttpApiError.BadRequest({})))
-      return yield* session.get(ctx.params.sessionID)
+      return yield* mapNotFound(
+        Effect.gen(function* () {
+          yield* shareSvc.share(ctx.params.sessionID).pipe(Effect.mapError(() => new HttpApiError.BadRequest({})))
+          return yield* session.get(ctx.params.sessionID)
+        }),
+      )
     })
 
     const unshare = Effect.fn("SessionHttpApi.unshare")(function* (ctx: { params: { sessionID: SessionID } }) {
-      yield* shareSvc.unshare(ctx.params.sessionID).pipe(Effect.mapError(() => new HttpApiError.BadRequest({})))
-      return yield* session.get(ctx.params.sessionID)
+      return yield* mapNotFound(
+        Effect.gen(function* () {
+          yield* shareSvc.unshare(ctx.params.sessionID).pipe(Effect.mapError(() => new HttpApiError.BadRequest({})))
+          return yield* session.get(ctx.params.sessionID)
+        }),
+      )
     })
 
     const summarize = Effect.fn("SessionHttpApi.summarize")(function* (ctx: {
       params: { sessionID: SessionID }
       payload: typeof SummarizePayload.Type
     }) {
-      yield* revertSvc.cleanup(yield* session.get(ctx.params.sessionID))
-      const messages = yield* session.messages({ sessionID: ctx.params.sessionID })
-      const defaultAgent = yield* agentSvc.defaultAgent()
-      const currentAgent = messages.findLast((message) => message.info.role === "user")?.info.agent ?? defaultAgent
+      return yield* mapNotFound(
+        Effect.gen(function* () {
+          yield* revertSvc.cleanup(yield* session.get(ctx.params.sessionID))
+          const messages = yield* session.messages({ sessionID: ctx.params.sessionID })
+          const defaultAgent = yield* agentSvc.defaultAgent()
+          const currentAgent =
+            messages.findLast((message) => message.info.role === "user")?.info.agent ?? defaultAgent
 
-      yield* compactSvc.create({
-        sessionID: ctx.params.sessionID,
-        agent: currentAgent,
-        model: {
-          providerID: ctx.payload.providerID,
-          modelID: ctx.payload.modelID,
-        },
-        auto: ctx.payload.auto ?? false,
-      })
-      yield* promptSvc.loop({ sessionID: ctx.params.sessionID })
-      return true
+          yield* compactSvc.create({
+            sessionID: ctx.params.sessionID,
+            agent: currentAgent,
+            model: {
+              providerID: ctx.payload.providerID,
+              modelID: ctx.payload.modelID,
+            },
+            auto: ctx.payload.auto ?? false,
+          })
+          yield* promptSvc.loop({ sessionID: ctx.params.sessionID })
+          return true
+        }),
+      )
     })
 
     const prompt = Effect.fn("SessionHttpApi.prompt")(function* (ctx: {

+ 6 - 1
packages/opencode/src/server/routes/instance/httpapi/middleware/workspace-routing.ts

@@ -178,7 +178,12 @@ function routeHttpApiWorkspace<E>(
     const request = yield* HttpServerRequest.HttpServerRequest
     const sessionID = getWorkspaceRouteSessionID(requestURL(request))
     const session = sessionID
-      ? yield* Session.Service.use((svc) => svc.get(sessionID)).pipe(Effect.catchDefect(() => Effect.void))
+      ? yield* Session.Service.use((svc) => svc.get(sessionID)).pipe(
+          // Sketch: also swallow the typed `SessionNotFound` so this routing
+          // probe stays best-effort (matches the previous defect-only catch).
+          Effect.catch(() => Effect.succeed(undefined)),
+          Effect.catchDefect(() => Effect.succeed(undefined)),
+        )
       : undefined
     const plan = yield* planRequest(request, session?.workspaceID)
     return yield* routeWorkspace(client, effect, plan)

+ 20 - 1
packages/opencode/src/server/routes/instance/trace.ts

@@ -1,6 +1,8 @@
 import type { Context } from "hono"
 import { Effect } from "effect"
 import { AppRuntime } from "@/effect/app-runtime"
+import { NotFoundError } from "@/storage/storage"
+import { Session } from "@/session/session"
 
 type AppEnv = Parameters<typeof AppRuntime.runPromise>[0] extends Effect.Effect<any, any, infer R> ? R : never
 
@@ -40,8 +42,25 @@ export function requestAttributes(c: RequestLike): Record<string, string> {
   return attributes
 }
 
+// Bridge typed service errors to the legacy Hono `NamedError` ErrorMiddleware.
+// The HttpApi adapter consumes typed errors directly via schema annotations,
+// but Hono's ErrorMiddleware switches on `instanceof NamedError`. Until the
+// legacy adapter is retired, catch the new typed errors here and rethrow the
+// equivalent NamedError so the existing 404 wiring keeps working.
+const adaptTypedErrors = <A, E, R>(
+  self: Effect.Effect<A, E, R>,
+): Effect.Effect<A, Exclude<E, Session.SessionNotFound>, R> =>
+  self.pipe(
+    Effect.catchIf(
+      (e: unknown) => e instanceof Session.SessionNotFound,
+      (e) => Effect.die(new NotFoundError({ message: (e as Session.SessionNotFound).data.message })),
+    ),
+  ) as unknown as Effect.Effect<A, Exclude<E, Session.SessionNotFound>, R>
+
 export function runRequest<A, E>(name: string, c: Context, effect: Effect.Effect<A, E, AppEnv>) {
-  return AppRuntime.runPromise(effect.pipe(Effect.withSpan(name, { attributes: requestAttributes(c) })))
+  return AppRuntime.runPromise(
+    adaptTypedErrors(effect).pipe(Effect.withSpan(name, { attributes: requestAttributes(c) })),
+  )
 }
 
 export async function jsonRequest<C extends Context, A, E>(

+ 4 - 3
packages/opencode/src/session/prompt.ts

@@ -745,7 +745,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the
           const markReady = ready ? ready.open.pipe(Effect.asVoid) : Effect.void
           const { msg, part, cwd } = yield* Effect.gen(function* () {
             const ctx = yield* InstanceState.context
-            const session = yield* sessions.get(input.sessionID)
+            const session = yield* Effect.orDie(sessions.get(input.sessionID))
             if (session.revert) {
               yield* revert.cleanup(session)
             }
@@ -1369,7 +1369,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the
 
     const prompt: (input: PromptInput) => Effect.Effect<MessageV2.WithParts> = Effect.fn("SessionPrompt.prompt")(
       function* (input: PromptInput) {
-        const session = yield* sessions.get(input.sessionID)
+        const session = yield* Effect.orDie(sessions.get(input.sessionID))
         yield* revert.cleanup(session)
         const message = yield* createUserMessage(input)
         yield* sessions.touch(input.sessionID)
@@ -1402,7 +1402,8 @@ NOTE: At any point in time through this workflow you should feel free to ask the
         const slog = elog.with({ sessionID })
         let structured: unknown | undefined
         let step = 0
-        const session = yield* sessions.get(sessionID)
+        // Sketch: keep public never-E surface; defect on missing session.
+        const session = yield* Effect.orDie(sessions.get(sessionID))
 
         while (true) {
           yield* status.set(sessionID, { type: "busy" })

+ 4 - 4
packages/opencode/src/session/revert.ts

@@ -44,7 +44,7 @@ export const layer = Layer.effect(
       yield* state.assertNotBusy(input.sessionID)
       const all = yield* sessions.messages({ sessionID: input.sessionID })
       let lastUser: MessageV2.User | undefined
-      const session = yield* sessions.get(input.sessionID)
+      const session = yield* Effect.orDie(sessions.get(input.sessionID))
 
       let rev: Session.Info["revert"]
       const patches: Snapshot.Patch[] = []
@@ -89,17 +89,17 @@ export const layer = Layer.effect(
           files: diffs.length,
         },
       })
-      return yield* sessions.get(input.sessionID)
+      return yield* Effect.orDie(sessions.get(input.sessionID))
     })
 
     const unrevert = Effect.fn("SessionRevert.unrevert")(function* (input: { sessionID: SessionID }) {
       log.info("unreverting", input)
       yield* state.assertNotBusy(input.sessionID)
-      const session = yield* sessions.get(input.sessionID)
+      const session = yield* Effect.orDie(sessions.get(input.sessionID))
       if (!session.revert) return session
       if (session.revert.snapshot) yield* snap.restore(session.revert!.snapshot!)
       yield* sessions.clearRevert(input.sessionID)
-      return yield* sessions.get(input.sessionID)
+      return yield* Effect.orDie(sessions.get(input.sessionID))
     })
 
     const cleanup = Effect.fn("SessionRevert.cleanup")(function* (session: Session.Info) {

+ 24 - 4
packages/opencode/src/session/session.ts

@@ -434,7 +434,7 @@ export interface Interface {
   }) => Effect.Effect<Info>
   readonly fork: (input: { sessionID: SessionID; messageID?: MessageID }) => Effect.Effect<Info>
   readonly touch: (sessionID: SessionID) => Effect.Effect<void>
-  readonly get: (id: SessionID) => Effect.Effect<Info>
+  readonly get: (id: SessionID) => Effect.Effect<Info, SessionNotFound>
   readonly setTitle: (input: { sessionID: SessionID; title: string }) => Effect.Effect<void>
   readonly setArchived: (input: { sessionID: SessionID; time?: number }) => Effect.Effect<void>
   readonly setPermission: (input: { sessionID: SessionID; permission: Permission.Ruleset }) => Effect.Effect<void>
@@ -472,6 +472,22 @@ export interface Interface {
   ) => Effect.Effect<Option.Option<MessageV2.WithParts>>
 }
 
+/**
+ * Typed not-found error for `Session.Service.get`.
+ *
+ * The `httpApiStatus: 404` annotation lets the Effect HttpApi auto-route the
+ * status. The schema fields (`name` literal, `data.message`) match Hono's
+ * `NamedError` JSON envelope so SDK consumers see the same body shape on both
+ * adapters.
+ */
+export class SessionNotFound extends Schema.ErrorClass<SessionNotFound>("opencode/Session/NotFound")(
+  {
+    name: Schema.tag("NotFoundError"),
+    data: Schema.Struct({ message: Schema.String }),
+  },
+  { httpApiStatus: 404 },
+) {}
+
 export class Service extends Context.Service<Service, Interface>()("@opencode/Session") {}
 
 export type Patch = Types.DeepMutable<SyncEvent.Event<typeof Event.Updated>["data"]["info"]>
@@ -534,7 +550,7 @@ export const layer: Layer.Layer<Service, never, Bus.Service | Storage.Service |
 
     const get = Effect.fn("Session.get")(function* (id: SessionID) {
       const row = yield* db((d) => d.select().from(SessionTable).where(eq(SessionTable.id, id)).get())
-      if (!row) throw new NotFoundError({ message: `Session not found: ${id}` })
+      if (!row) return yield* new SessionNotFound({ data: { message: `Session not found: ${id}` } })
       return fromRow(row)
     })
 
@@ -556,7 +572,9 @@ export const layer: Layer.Layer<Service, never, Bus.Service | Storage.Service |
 
     const remove: Interface["remove"] = Effect.fnUntraced(function* (sessionID: SessionID) {
       try {
-        const session = yield* get(sessionID)
+        // Sketch: keep `remove`'s legacy never-E surface; convert
+        // `SessionNotFound` to a defect (caught below by the try/catch).
+        const session = yield* Effect.orDie(get(sessionID))
         const kids = yield* children(sessionID)
         for (const child of kids) {
           yield* remove(child.id)
@@ -641,7 +659,9 @@ export const layer: Layer.Layer<Service, never, Bus.Service | Storage.Service |
 
     const fork = Effect.fn("Session.fork")(function* (input: { sessionID: SessionID; messageID?: MessageID }) {
       const ctx = yield* InstanceState.context
-      const original = yield* get(input.sessionID)
+      // Sketch: keep `fork`'s legacy never-E surface; convert SessionNotFound
+      // to a defect. When `fork` itself is migrated, propagate it.
+      const original = yield* Effect.orDie(get(input.sessionID))
       const title = getForkedTitle(original.title)
       const session = yield* createNext({
         directory: ctx.directory,

+ 2 - 2
packages/opencode/test/control-plane/workspace.test.ts

@@ -958,7 +958,7 @@ describe("workspace-old sync state", () => {
 
               yield* eventuallyEffect(
                 Effect.gen(function* () {
-                  expect((yield* sessionSvc.get(session.id)).title).toBe("from history")
+                  expect((yield* Effect.orDie(sessionSvc.get(session.id))).title).toBe("from history")
                 }),
               )
               expect(historyBodies).toEqual([{ [session.id]: historyNextSeq - 1 }])
@@ -1106,7 +1106,7 @@ describe("workspace-old sync state", () => {
 
               yield* eventuallyEffect(
                 Effect.gen(function* () {
-                  expect((yield* sessionSvc.get(session.id)).title).toBe("from sse")
+                  expect((yield* Effect.orDie(sessionSvc.get(session.id))).title).toBe("from sse")
                 }),
               )
               expect(

+ 5 - 1
packages/opencode/test/server/httpapi-parity.test.ts

@@ -113,7 +113,11 @@ describe("404 mapping for missing session", () => {
 // FIXME: unskip when error JSON shape policy is decided + applied (separate PR).
 // ──────────────────────────────────────────────────────────────────────────────
 describe("Error JSON shape parity", () => {
-  test.todo("HttpApi 404 body matches NamedError shape", async () => {
+  // Sketch: validates the typed-error pattern end-to-end on `session.get`. The
+  // service now fails with a typed `SessionNotFound` error annotated
+  // `httpApiStatus: 404`. The endpoint declares the schema, and HttpApi auto-
+  // routes the status + body — no `mapNotFound` wrapper required.
+  test("HttpApi 404 body matches NamedError shape (sketch: session.get)", async () => {
     await using tmp = await tmpdir({ config: { formatter: false, lsp: false } })
 
     const response = await app(true).request("/session/ses_does_not_exist", {