diff --git a/packages/opencode/src/permission/index.ts b/packages/opencode/src/permission/index.ts index cb4d9eef69..1814c5ab2b 100644 --- a/packages/opencode/src/permission/index.ts +++ b/packages/opencode/src/permission/index.ts @@ -100,6 +100,10 @@ export class DeniedError extends Schema.TaggedErrorClass()("Permiss } } +export class NotFoundError extends Schema.TaggedErrorClass()("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 export interface Interface { readonly ask: (input: AskInput) => Effect.Effect - readonly reply: (input: ReplyInput) => Effect.Effect + readonly reply: (input: ReplyInput) => Effect.Effect readonly list: () => Effect.Effect> } @@ -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, { diff --git a/packages/opencode/src/question/index.ts b/packages/opencode/src/question/index.ts index ef871c8aa4..ca52bddc8e 100644 --- a/packages/opencode/src/question/index.ts +++ b/packages/opencode/src/question/index.ts @@ -98,6 +98,10 @@ export class RejectedError extends Schema.TaggedErrorClass()("Que } } +export class NotFoundError extends Schema.TaggedErrorClass()("Question.NotFoundError", { + requestID: QuestionID, +}) {} + interface PendingEntry { info: Request deferred: Deferred.Deferred, RejectedError> @@ -115,8 +119,8 @@ export interface Interface { questions: ReadonlyArray tool?: Tool }) => Effect.Effect, RejectedError> - readonly reply: (input: { requestID: QuestionID; answers: ReadonlyArray }) => Effect.Effect - readonly reject: (requestID: QuestionID) => Effect.Effect + readonly reply: (input: { requestID: QuestionID; answers: ReadonlyArray }) => Effect.Effect + readonly reject: (requestID: QuestionID) => Effect.Effect readonly list: () => Effect.Effect> } @@ -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 }) diff --git a/packages/opencode/src/server/routes/instance/httpapi/errors.ts b/packages/opencode/src/server/routes/instance/httpapi/errors.ts index 004fc7a6e2..f2afe7f0e3 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/errors.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/errors.ts @@ -122,6 +122,24 @@ export class SessionBusyError extends Schema.TaggedErrorClass( { httpApiStatus: 409 }, ) {} +export class QuestionNotFoundError extends Schema.TaggedErrorClass()( + "QuestionNotFoundError", + { + requestID: Schema.String, + message: Schema.String, + }, + { httpApiStatus: 404 }, +) {} + +export class PermissionNotFoundError extends Schema.TaggedErrorClass()( + "PermissionNotFoundError", + { + requestID: Schema.String, + message: Schema.String, + }, + { httpApiStatus: 404 }, +) {} + export class ApiNotFoundError extends Schema.ErrorClass("NotFoundError")( { name: Schema.Literal("NotFoundError"), diff --git a/packages/opencode/src/server/routes/instance/httpapi/groups/permission.ts b/packages/opencode/src/server/routes/instance/httpapi/groups/permission.ts index 5326596d39..103d7aa245 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/groups/permission.ts +++ b/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", diff --git a/packages/opencode/src/server/routes/instance/httpapi/groups/question.ts b/packages/opencode/src/server/routes/instance/httpapi/groups/question.ts index 35cd3314b5..e9e63429db 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/groups/question.ts +++ b/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", diff --git a/packages/opencode/src/server/routes/instance/httpapi/groups/session.ts b/packages/opencode/src/server/routes/instance/httpapi/groups/session.ts index 7f7d8d1118..cd2f3be19c 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/groups/session.ts +++ b/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", diff --git a/packages/opencode/src/server/routes/instance/httpapi/handlers/permission.ts b/packages/opencode/src/server/routes/instance/httpapi/handlers/permission.ts index 2a7b6195df..281e78e3ce 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/handlers/permission.ts +++ b/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 }) diff --git a/packages/opencode/src/server/routes/instance/httpapi/handlers/question.ts b/packages/opencode/src/server/routes/instance/httpapi/handlers/question.ts index 3a4d316179..e9ed8cc891 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/handlers/question.ts +++ b/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 }) diff --git a/packages/opencode/src/server/routes/instance/httpapi/handlers/session.ts b/packages/opencode/src/server/routes/instance/httpapi/handlers/session.ts index 2e3617d146..4d4cce367b 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/handlers/session.ts +++ b/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 }) diff --git a/packages/opencode/test/permission/next.test.ts b/packages/opencode/test/permission/next.test.ts index 1b09c36afd..e969e67ff6 100644 --- a/packages/opencode/test/permission/next.test.ts +++ b/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 }, diff --git a/packages/opencode/test/question/question.test.ts b/packages/opencode/test/question/question.test.ts index a5841bd08d..5f6f87972e 100644 --- a/packages/opencode/test/question/question.test.ts +++ b/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 diff --git a/packages/opencode/test/server/httpapi-exercise/index.ts b/packages/opencode/test/server/httpapi-exercise/index.ts index 8999d51077..66c7bfdef2 100644 --- a/packages/opencode/test/server/httpapi-exercise/index.ts +++ b/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() diff --git a/packages/opencode/test/server/httpapi-instance.test.ts b/packages/opencode/test/server/httpapi-instance.test.ts index 51564a790a..48244b5abd 100644 --- a/packages/opencode/test/server/httpapi-instance.test.ts +++ b/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 }) diff --git a/packages/opencode/test/server/httpapi-public-openapi.test.ts b/packages/opencode/test/server/httpapi-public-openapi.test.ts index 547beb9799..c928f1668e 100644 --- a/packages/opencode/test/server/httpapi-public-openapi.test.ts +++ b/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", + ) + } + }) }) diff --git a/packages/opencode/test/server/httpapi-session.test.ts b/packages/opencode/test/server/httpapi-session.test.ts index a13f986b4d..1e87ddc6b1 100644 --- a/packages/opencode/test/server/httpapi-session.test.ts +++ b/packages/opencode/test/server/httpapi-session.test.ts @@ -821,19 +821,24 @@ describe("session HttpApi", () => { }), ).toMatchObject({ id: session.id }) - expect( - yield* requestJson( - 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 } }, )