Browse Source

fix(httpapi): return request not found errors (#28693)

Shoubhit Dash 3 months ago
parent
commit
4ce247eaba

+ 6 - 2
packages/opencode/src/permission/index.ts

@@ -100,6 +100,10 @@ export class DeniedError extends Schema.TaggedErrorClass<DeniedError>()("Permiss
   }
 }
 
+export class NotFoundError extends Schema.TaggedErrorClass<NotFoundError>()("Permission.NotFoundError", {
+  requestID: PermissionID,
+}) {}
+
 export type Error = DeniedError | RejectedError | CorrectedError
 
 export const AskInput = Schema.Struct({
@@ -117,7 +121,7 @@ export type ReplyInput = Schema.Schema.Type<typeof ReplyInput>
 
 export interface Interface {
   readonly ask: (input: AskInput) => Effect.Effect<void, Error>
-  readonly reply: (input: ReplyInput) => Effect.Effect<void>
+  readonly reply: (input: ReplyInput) => Effect.Effect<void, NotFoundError>
   readonly list: () => Effect.Effect<ReadonlyArray<Request>>
 }
 
@@ -209,7 +213,7 @@ export const layer = Layer.effect(
     const reply = Effect.fn("Permission.reply")(function* (input: ReplyInput) {
       const { approved, pending } = yield* InstanceState.get(state)
       const existing = pending.get(input.requestID)
-      if (!existing) return
+      if (!existing) return yield* new NotFoundError({ requestID: input.requestID })
 
       pending.delete(input.requestID)
       yield* bus.publish(Event.Replied, {

+ 8 - 4
packages/opencode/src/question/index.ts

@@ -98,6 +98,10 @@ export class RejectedError extends Schema.TaggedErrorClass<RejectedError>()("Que
   }
 }
 
+export class NotFoundError extends Schema.TaggedErrorClass<NotFoundError>()("Question.NotFoundError", {
+  requestID: QuestionID,
+}) {}
+
 interface PendingEntry {
   info: Request
   deferred: Deferred.Deferred<ReadonlyArray<Answer>, RejectedError>
@@ -115,8 +119,8 @@ export interface Interface {
     questions: ReadonlyArray<Info>
     tool?: Tool
   }) => Effect.Effect<ReadonlyArray<Answer>, RejectedError>
-  readonly reply: (input: { requestID: QuestionID; answers: ReadonlyArray<Answer> }) => Effect.Effect<void>
-  readonly reject: (requestID: QuestionID) => Effect.Effect<void>
+  readonly reply: (input: { requestID: QuestionID; answers: ReadonlyArray<Answer> }) => Effect.Effect<void, NotFoundError>
+  readonly reject: (requestID: QuestionID) => Effect.Effect<void, NotFoundError>
   readonly list: () => Effect.Effect<ReadonlyArray<Request>>
 }
 
@@ -180,7 +184,7 @@ export const layer = Layer.effect(
       const existing = pending.get(input.requestID)
       if (!existing) {
         log.warn("reply for unknown request", { requestID: input.requestID })
-        return
+        return yield* new NotFoundError({ requestID: input.requestID })
       }
       pending.delete(input.requestID)
       log.info("replied", { requestID: input.requestID, answers: input.answers })
@@ -197,7 +201,7 @@ export const layer = Layer.effect(
       const existing = pending.get(requestID)
       if (!existing) {
         log.warn("reject for unknown request", { requestID })
-        return
+        return yield* new NotFoundError({ requestID })
       }
       pending.delete(requestID)
       log.info("rejected", { requestID })

+ 18 - 0
packages/opencode/src/server/routes/instance/httpapi/errors.ts

@@ -122,6 +122,24 @@ export class SessionBusyError extends Schema.TaggedErrorClass<SessionBusyError>(
   { httpApiStatus: 409 },
 ) {}
 
+export class QuestionNotFoundError extends Schema.TaggedErrorClass<QuestionNotFoundError>()(
+  "QuestionNotFoundError",
+  {
+    requestID: Schema.String,
+    message: Schema.String,
+  },
+  { httpApiStatus: 404 },
+) {}
+
+export class PermissionNotFoundError extends Schema.TaggedErrorClass<PermissionNotFoundError>()(
+  "PermissionNotFoundError",
+  {
+    requestID: Schema.String,
+    message: Schema.String,
+  },
+  { httpApiStatus: 404 },
+) {}
+
 export class ApiNotFoundError extends Schema.ErrorClass<ApiNotFoundError>("NotFoundError")(
   {
     name: Schema.Literal("NotFoundError"),

+ 2 - 1
packages/opencode/src/server/routes/instance/httpapi/groups/permission.ts

@@ -2,6 +2,7 @@ import { Permission } from "@/permission"
 import { PermissionID } from "@/permission/schema"
 import { Schema } from "effect"
 import { HttpApi, HttpApiEndpoint, HttpApiError, HttpApiGroup, OpenApi } from "effect/unstable/httpapi"
+import { PermissionNotFoundError } from "../errors"
 import { Authorization } from "../middleware/authorization"
 import { InstanceContextMiddleware } from "../middleware/instance-context"
 import { WorkspaceRoutingMiddleware, WorkspaceRoutingQuery } from "../middleware/workspace-routing"
@@ -32,7 +33,7 @@ export const PermissionApi = HttpApi.make("permission")
           query: WorkspaceRoutingQuery,
           payload: ReplyPayload,
           success: described(Schema.Boolean, "Permission processed successfully"),
-          error: [HttpApiError.BadRequest, HttpApiError.NotFound],
+          error: [HttpApiError.BadRequest, PermissionNotFoundError],
         }).annotateMerge(
           OpenApi.annotations({
             identifier: "permission.reply",

+ 3 - 2
packages/opencode/src/server/routes/instance/httpapi/groups/question.ts

@@ -2,6 +2,7 @@ import { Question } from "@/question"
 import { QuestionID } from "@/question/schema"
 import { Schema } from "effect"
 import { HttpApi, HttpApiEndpoint, HttpApiError, HttpApiGroup, OpenApi } from "effect/unstable/httpapi"
+import { QuestionNotFoundError } from "../errors"
 import { Authorization } from "../middleware/authorization"
 import { InstanceContextMiddleware } from "../middleware/instance-context"
 import { WorkspaceRoutingMiddleware, WorkspaceRoutingQuery } from "../middleware/workspace-routing"
@@ -33,7 +34,7 @@ export const QuestionApi = HttpApi.make("question")
           query: WorkspaceRoutingQuery,
           payload: ReplyPayload,
           success: described(Schema.Boolean, "Question answered successfully"),
-          error: [HttpApiError.BadRequest, HttpApiError.NotFound],
+          error: [HttpApiError.BadRequest, QuestionNotFoundError],
         }).annotateMerge(
           OpenApi.annotations({
             identifier: "question.reply",
@@ -45,7 +46,7 @@ export const QuestionApi = HttpApi.make("question")
           params: { requestID: QuestionID },
           query: WorkspaceRoutingQuery,
           success: described(Schema.Boolean, "Question rejected successfully"),
-          error: [HttpApiError.BadRequest, HttpApiError.NotFound],
+          error: [HttpApiError.BadRequest, QuestionNotFoundError],
         }).annotateMerge(
           OpenApi.annotations({
             identifier: "question.reject",

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

@@ -19,7 +19,7 @@ import {
   WorkspaceRoutingQuery,
   WorkspaceRoutingQueryFields,
 } from "../middleware/workspace-routing"
-import { ApiNotFoundError, SessionBusyError } from "../errors"
+import { ApiNotFoundError, PermissionNotFoundError, SessionBusyError } from "../errors"
 import { described } from "./metadata"
 import { QueryBoolean } from "./query"
 
@@ -393,7 +393,7 @@ export const SessionApi = HttpApi.make("session")
           query: WorkspaceRoutingQuery,
           payload: PermissionResponsePayload,
           success: described(Schema.Boolean, "Permission processed successfully"),
-          error: [HttpApiError.BadRequest, ApiNotFoundError],
+          error: [HttpApiError.BadRequest, ApiNotFoundError, PermissionNotFoundError],
         }).annotateMerge(
           OpenApi.annotations({
             identifier: "permission.respond",

+ 17 - 5
packages/opencode/src/server/routes/instance/httpapi/handlers/permission.ts

@@ -3,6 +3,7 @@ import { PermissionID } from "@/permission/schema"
 import { Effect } from "effect"
 import { HttpApiBuilder } from "effect/unstable/httpapi"
 import { InstanceHttpApi } from "../api"
+import { PermissionNotFoundError } from "../errors"
 
 export const permissionHandlers = HttpApiBuilder.group(InstanceHttpApi, "permission", (handlers) =>
   Effect.gen(function* () {
@@ -16,11 +17,22 @@ export const permissionHandlers = HttpApiBuilder.group(InstanceHttpApi, "permiss
       params: { requestID: PermissionID }
       payload: Permission.ReplyBody
     }) {
-      yield* svc.reply({
-        requestID: ctx.params.requestID,
-        reply: ctx.payload.reply,
-        message: ctx.payload.message,
-      })
+      yield* svc
+        .reply({
+          requestID: ctx.params.requestID,
+          reply: ctx.payload.reply,
+          message: ctx.payload.message,
+        })
+        .pipe(
+          Effect.catchTag("Permission.NotFoundError", (error) =>
+            Effect.fail(
+              new PermissionNotFoundError({
+                requestID: String(error.requestID),
+                message: `Permission request not found: ${error.requestID}`,
+              }),
+            ),
+          ),
+        )
       return true
     })
 

+ 26 - 5
packages/opencode/src/server/routes/instance/httpapi/handlers/question.ts

@@ -3,6 +3,7 @@ import { QuestionID } from "@/question/schema"
 import { Effect } from "effect"
 import { HttpApiBuilder } from "effect/unstable/httpapi"
 import { InstanceHttpApi } from "../api"
+import { QuestionNotFoundError } from "../errors"
 
 export const questionHandlers = HttpApiBuilder.group(InstanceHttpApi, "question", (handlers) =>
   Effect.gen(function* () {
@@ -16,15 +17,35 @@ export const questionHandlers = HttpApiBuilder.group(InstanceHttpApi, "question"
       params: { requestID: QuestionID }
       payload: Question.Reply
     }) {
-      yield* svc.reply({
-        requestID: ctx.params.requestID,
-        answers: ctx.payload.answers,
-      })
+      yield* svc
+        .reply({
+          requestID: ctx.params.requestID,
+          answers: ctx.payload.answers,
+        })
+        .pipe(
+          Effect.catchTag("Question.NotFoundError", (error) =>
+            Effect.fail(
+              new QuestionNotFoundError({
+                requestID: String(error.requestID),
+                message: `Question request not found: ${error.requestID}`,
+              }),
+            ),
+          ),
+        )
       return true
     })
 
     const reject = Effect.fn("QuestionHttpApi.reject")(function* (ctx: { params: { requestID: QuestionID } }) {
-      yield* svc.reject(ctx.params.requestID)
+      yield* svc.reject(ctx.params.requestID).pipe(
+        Effect.catchTag("Question.NotFoundError", (error) =>
+          Effect.fail(
+            new QuestionNotFoundError({
+              requestID: String(error.requestID),
+              message: `Question request not found: ${error.requestID}`,
+            }),
+          ),
+        ),
+      )
       return true
     })
 

+ 11 - 1
packages/opencode/src/server/routes/instance/httpapi/handlers/session.ts

@@ -34,6 +34,7 @@ import {
   SummarizePayload,
   UpdatePayload,
 } from "../groups/session"
+import { PermissionNotFoundError } from "../errors"
 import * as SessionError from "./session-errors"
 
 const tryParseJson = (text: string) =>
@@ -356,7 +357,16 @@ export const sessionHandlers = HttpApiBuilder.group(InstanceHttpApi, "session",
       payload: typeof PermissionResponsePayload.Type
     }) {
       yield* requireSession(ctx.params.sessionID)
-      yield* permissionSvc.reply({ requestID: ctx.params.permissionID, reply: ctx.payload.response })
+      yield* permissionSvc.reply({ requestID: ctx.params.permissionID, reply: ctx.payload.response }).pipe(
+        Effect.catchTag("Permission.NotFoundError", (error) =>
+          Effect.fail(
+            new PermissionNotFoundError({
+              requestID: String(error.requestID),
+              message: `Permission request not found: ${error.requestID}`,
+            }),
+          ),
+        ),
+      )
       return true
     })
 

+ 6 - 2
packages/opencode/test/permission/next.test.ts

@@ -1057,10 +1057,14 @@ it.instance(
 )
 
 it.instance(
-  "reply - does nothing for unknown requestID",
+  "reply - fails for unknown requestID",
   () =>
     Effect.gen(function* () {
-      yield* reply({ requestID: PermissionID.make("per_unknown"), reply: "once" })
+      const exit = yield* reply({ requestID: PermissionID.make("per_unknown"), reply: "once" }).pipe(Effect.exit)
+      expect(Exit.isFailure(exit)).toBe(true)
+      if (Exit.isFailure(exit)) {
+        expect(Cause.squash(exit.cause)).toMatchObject({ _tag: "Permission.NotFoundError", requestID: "per_unknown" })
+      }
       expect(yield* list()).toHaveLength(0)
     }),
   { git: true },

+ 22 - 7
packages/opencode/test/question/question.test.ts

@@ -184,11 +184,17 @@ it.instance(
 )
 
 it.instance(
-  "reply - does nothing for unknown requestID",
+  "reply - fails for unknown requestID",
   () =>
-    replyEffect({
-      requestID: QuestionID.make("que_unknown"),
-      answers: [["Option 1"]],
+    Effect.gen(function* () {
+      const exit = yield* replyEffect({
+        requestID: QuestionID.make("que_unknown"),
+        answers: [["Option 1"]],
+      }).pipe(Effect.exit)
+      expect(Exit.isFailure(exit)).toBe(true)
+      if (Exit.isFailure(exit)) {
+        expect(Cause.squash(exit.cause)).toMatchObject({ _tag: "Question.NotFoundError", requestID: "que_unknown" })
+      }
     }),
   { git: true },
 )
@@ -253,9 +259,18 @@ it.instance(
   { git: true },
 )
 
-it.instance("reject - does nothing for unknown requestID", () => rejectEffect(QuestionID.make("que_unknown")), {
-  git: true,
-})
+it.instance(
+  "reject - fails for unknown requestID",
+  () =>
+    Effect.gen(function* () {
+      const exit = yield* rejectEffect(QuestionID.make("que_unknown")).pipe(Effect.exit)
+      expect(Exit.isFailure(exit)).toBe(true)
+      if (Exit.isFailure(exit)) {
+        expect(Cause.squash(exit.cause)).toMatchObject({ _tag: "Question.NotFoundError", requestID: "que_unknown" })
+      }
+    }),
+  { git: true },
+)
 
 // multiple questions tests
 

+ 4 - 12
packages/opencode/test/server/httpapi-exercise/index.ts

@@ -224,9 +224,7 @@ const scenarios: Scenario[] = [
       headers: ctx.headers(),
       body: { reply: "once" },
     }))
-    .json(200, (body) => {
-      check(body === true, "permission reply should return true even when request is no longer pending")
-    }),
+    .json(404, object, "status"),
   http.protected.get("/question", "question.list").json(200, array),
   http.protected
     .post("/question/{requestID}/reply", "question.reply.invalid")
@@ -243,18 +241,14 @@ const scenarios: Scenario[] = [
       headers: ctx.headers(),
       body: { answers: [["Yes"]] },
     }))
-    .json(200, (body) => {
-      check(body === true, "question reply should return true even when request is no longer pending")
-    }),
+    .json(404, object, "status"),
   http.protected
     .post("/question/{requestID}/reject", "question.reject")
     .at((ctx) => ({
       path: route("/question/{requestID}/reject", { requestID: "que_httpapi_reject" }),
       headers: ctx.headers(),
     }))
-    .json(200, (body) => {
-      check(body === true, "question reject should return true even when request is no longer pending")
-    }),
+    .json(404, object, "status"),
   http.protected
     .get("/file", "file.list")
     .seeded((ctx) => ctx.file("hello.txt", "hello\n"))
@@ -1249,9 +1243,7 @@ const scenarios: Scenario[] = [
       headers: ctx.headers(),
       body: { response: "once" },
     }))
-    .json(200, (body) => {
-      check(body === true, "deprecated permission response should return true")
-    }),
+    .json(404, object, "status"),
   http.protected
     .post("/session/{sessionID}/share", "session.share")
     .mutating()

+ 54 - 0
packages/opencode/test/server/httpapi-instance.test.ts

@@ -8,6 +8,8 @@ import { WorkspaceID } from "../../src/control-plane/schema"
 import { ControlPaths } from "../../src/server/routes/instance/httpapi/groups/control"
 import { InstancePaths } from "../../src/server/routes/instance/httpapi/groups/instance"
 import { SessionPaths } from "../../src/server/routes/instance/httpapi/groups/session"
+import { PermissionID } from "../../src/permission/schema"
+import { QuestionID } from "../../src/question/schema"
 import { HttpApiApp } from "../../src/server/routes/instance/httpapi/server"
 import { HEADER as FenceHeader } from "../../src/server/shared/fence"
 import { resetDatabase } from "../fixture/db"
@@ -151,6 +153,58 @@ describe("instance HttpApi", () => {
     }),
   )
 
+  it.live("returns typed not found bodies for missing permission and question requests", () =>
+    Effect.gen(function* () {
+      const dir = yield* tmpdirScoped({ git: true })
+      const request = (path: string, init?: RequestInit) =>
+        Effect.promise(() =>
+          HttpApiApp.webHandler().handler(
+            new Request(`http://localhost${path}`, {
+              ...init,
+              headers: { "x-opencode-directory": dir, "content-type": "application/json", ...init?.headers },
+            }),
+            handlerContext,
+          ),
+        )
+      const permissionID = PermissionID.ascending()
+      const questionReplyID = QuestionID.ascending()
+      const questionRejectID = QuestionID.ascending()
+      const [permission, questionReply, questionReject] = yield* Effect.all(
+        [
+          request(`/permission/${permissionID}/reply`, {
+            method: "POST",
+            body: JSON.stringify({ reply: "once" }),
+          }),
+          request(`/question/${questionReplyID}/reply`, {
+            method: "POST",
+            body: JSON.stringify({ answers: [["Yes"]] }),
+          }),
+          request(`/question/${questionRejectID}/reject`, { method: "POST" }),
+        ],
+        { concurrency: "unbounded" },
+      )
+
+      expect(permission.status).toBe(404)
+      expect(yield* Effect.promise(() => permission.json())).toEqual({
+        _tag: "PermissionNotFoundError",
+        requestID: permissionID,
+        message: `Permission request not found: ${permissionID}`,
+      })
+      expect(questionReply.status).toBe(404)
+      expect(yield* Effect.promise(() => questionReply.json())).toEqual({
+        _tag: "QuestionNotFoundError",
+        requestID: questionReplyID,
+        message: `Question request not found: ${questionReplyID}`,
+      })
+      expect(questionReject.status).toBe(404)
+      expect(yield* Effect.promise(() => questionReject.json())).toEqual({
+        _tag: "QuestionNotFoundError",
+        requestID: questionRejectID,
+        message: `Question request not found: ${questionRejectID}`,
+      })
+    }),
+  )
+
   it.live("serves path and VCS read endpoints", () =>
     Effect.gen(function* () {
       const dir = yield* tmpdirScoped({ git: true })

+ 16 - 0
packages/opencode/test/server/httpapi-public-openapi.test.ts

@@ -157,4 +157,20 @@ describe("PublicApi OpenAPI v2 errors", () => {
       )
     }
   })
+
+  test("documents permission and question not-found errors", () => {
+    const spec = OpenApi.fromApi(PublicApi) as OpenApiSpec
+
+    expect(componentName(responseRef(spec.paths["/permission/{requestID}/reply"]?.post?.responses?.["404"]) ?? "")).toBe(
+      "PermissionNotFoundError",
+    )
+    for (const route of [
+      ["post", "/question/{requestID}/reply"],
+      ["post", "/question/{requestID}/reject"],
+    ] as const) {
+      expect(componentName(responseRef(spec.paths[route[1]]?.[route[0]]?.responses?.["404"]) ?? "")).toBe(
+        "QuestionNotFoundError",
+      )
+    }
+  })
 })

+ 18 - 13
packages/opencode/test/server/httpapi-session.test.ts

@@ -821,19 +821,24 @@ describe("session HttpApi", () => {
           }),
         ).toMatchObject({ id: session.id })
 
-        expect(
-          yield* requestJson<boolean>(
-            pathFor(SessionPaths.permissions, {
-              sessionID: session.id,
-              permissionID: String(PermissionID.ascending()),
-            }),
-            {
-              method: "POST",
-              headers,
-              body: JSON.stringify({ response: "once" }),
-            },
-          ),
-        ).toBe(true)
+        const permissionID = String(PermissionID.ascending())
+        const permission = yield* request(
+          pathFor(SessionPaths.permissions, {
+            sessionID: session.id,
+            permissionID,
+          }),
+          {
+            method: "POST",
+            headers,
+            body: JSON.stringify({ response: "once" }),
+          },
+        )
+        expect(permission.status).toBe(404)
+        expect(yield* responseJson(permission)).toEqual({
+          _tag: "PermissionNotFoundError",
+          requestID: permissionID,
+          message: `Permission request not found: ${permissionID}`,
+        })
       }),
     { git: true, config: { formatter: false, lsp: false } },
   )