From 8fc02b0130718030304748520efc9329dfb5379a Mon Sep 17 00:00:00 2001
From: Kit Langton <kit.langton@gmail.com>
Date: Wed, 20 May 2026 23:06:22 -0400
Subject: [PATCH] refactor(question): tool-arg errors at the boundary, drop
 redundant inner decode (#28570)

---
 packages/opencode/src/question/index.ts       | 59 +++++++-------
 packages/opencode/src/tool/tool.ts            | 31 ++++++--
 .../opencode/test/question/question.test.ts   | 40 ----------
 .../opencode/test/tool/tool-define.test.ts    | 77 +++++++++++++++----
 4 files changed, 114 insertions(+), 93 deletions(-)

diff --git a/packages/opencode/src/question/index.ts b/packages/opencode/src/question/index.ts
index 38acf58e8f..ef871c8aa4 100644
--- a/packages/opencode/src/question/index.ts
+++ b/packages/opencode/src/question/index.ts
@@ -8,16 +8,20 @@ import { QuestionID } from "./schema"
 
 const log = Log.create({ service: "question" })
 
-// Schemas
+// Schemas — these are pure data; nothing checks class identity (see PR
+// description) so they're plain `Schema.Struct` + type alias. That lets
+// `Question.ask` and other internal sites trust the type contract without a
+// re-decode to coerce nested class instances.
 
-export class Option extends Schema.Class<Option>("QuestionOption")({
+export const Option = Schema.Struct({
   label: Schema.String.annotate({
     description: "Display text (1-5 words, concise)",
   }),
   description: Schema.String.annotate({
     description: "Explanation of choice",
   }),
-}) {}
+}).annotate({ identifier: "QuestionOption" })
+export type Option = Schema.Schema.Type<typeof Option>
 
 const base = {
   question: Schema.String.annotate({
@@ -34,48 +38,53 @@ const base = {
   }),
 }
 
-export class Info extends Schema.Class<Info>("QuestionInfo")({
+export const Info = Schema.Struct({
   ...base,
   custom: Schema.optional(Schema.Boolean).annotate({
     description: "Allow typing a custom answer (default: true)",
   }),
-}) {}
+}).annotate({ identifier: "QuestionInfo" })
+export type Info = Schema.Schema.Type<typeof Info>
 
-export class Prompt extends Schema.Class<Prompt>("QuestionPrompt")(base) {}
+export const Prompt = Schema.Struct(base).annotate({ identifier: "QuestionPrompt" })
+export type Prompt = Schema.Schema.Type<typeof Prompt>
 
-export class Tool extends Schema.Class<Tool>("QuestionTool")({
+export const Tool = Schema.Struct({
   messageID: MessageID,
   callID: Schema.String,
-}) {}
+}).annotate({ identifier: "QuestionTool" })
+export type Tool = Schema.Schema.Type<typeof Tool>
 
-export class Request extends Schema.Class<Request>("QuestionRequest")({
+export const Request = Schema.Struct({
   id: QuestionID,
   sessionID: SessionID,
   questions: Schema.Array(Info).annotate({
     description: "Questions to ask",
   }),
   tool: Schema.optional(Tool),
-}) {}
+}).annotate({ identifier: "QuestionRequest" })
+export type Request = Schema.Schema.Type<typeof Request>
 
 export const Answer = Schema.Array(Schema.String).annotate({ identifier: "QuestionAnswer" })
 export type Answer = Schema.Schema.Type<typeof Answer>
 
-export class Reply extends Schema.Class<Reply>("QuestionReply")({
+export const Reply = Schema.Struct({
   answers: Schema.Array(Answer).annotate({
     description: "User answers in order of questions (each answer is an array of selected labels)",
   }),
-}) {}
+}).annotate({ identifier: "QuestionReply" })
+export type Reply = Schema.Schema.Type<typeof Reply>
 
-class Replied extends Schema.Class<Replied>("QuestionReplied")({
+const Replied = Schema.Struct({
   sessionID: SessionID,
   requestID: QuestionID,
   answers: Schema.Array(Answer),
-}) {}
+}).annotate({ identifier: "QuestionReplied" })
 
-class Rejected extends Schema.Class<Rejected>("QuestionRejected")({
+const Rejected = Schema.Struct({
   sessionID: SessionID,
   requestID: QuestionID,
-}) {}
+}).annotate({ identifier: "QuestionRejected" })
 
 export const Event = {
   Asked: BusEvent.define("question.asked", Request),
@@ -146,26 +155,12 @@ export const layer = Layer.effect(
       log.info("asking", { id, questions: input.questions.length })
 
       const deferred = yield* Deferred.make<ReadonlyArray<Answer>, RejectedError>()
-      // Use the Effect-returning decode so a schema failure surfaces as a
-      // typed error the tool wrap can turn into a "rewrite the input" tool
-      // result. The previous `decodeUnknownSync` would throw uncaught, which
-      // crashed the assistant turn for any payload that slipped past the
-      // wrap-level validation (#28438).
-      const info = yield* Schema.decodeUnknownEffect(Request)({
+      const info: Request = {
         id,
         sessionID: input.sessionID,
         questions: input.questions,
         tool: input.tool,
-      }).pipe(
-        Effect.mapError(
-          (error) =>
-            new Error(
-              `The question tool was called with invalid arguments: ${error}.\nPlease rewrite the input so it satisfies the expected schema.`,
-              { cause: error },
-            ),
-        ),
-        Effect.orDie,
-      )
+      }
       pending.set(id, { info, deferred })
       yield* bus.publish(Event.Asked, info)
 
diff --git a/packages/opencode/src/tool/tool.ts b/packages/opencode/src/tool/tool.ts
index a26422d04c..f072773fad 100644
--- a/packages/opencode/src/tool/tool.ts
+++ b/packages/opencode/src/tool/tool.ts
@@ -13,6 +13,24 @@ interface Metadata {
 // TODO: remove this hack
 export type DynamicDescription = (agent: Agent.Info) => Effect.Effect<string>
 
+/**
+ * Raised when the LLM calls a tool with arguments that fail the parameter
+ * schema. This is the canonical "rewrite the input" tool error: the typed
+ * error class makes it matchable upstream, and its `message` getter produces
+ * the model-facing prose that the AI SDK feeds back as the tool result.
+ */
+export class InvalidArgumentsError extends Schema.TaggedErrorClass<InvalidArgumentsError>()(
+  "ToolInvalidArgumentsError",
+  {
+    tool: Schema.String,
+    detail: Schema.String,
+  },
+) {
+  override get message() {
+    return `The ${this.tool} tool was called with invalid arguments: ${this.detail}.\nPlease rewrite the input so it satisfies the expected schema.`
+  }
+}
+
 export type Context<M extends Metadata = Metadata> = {
   sessionID: SessionID
   messageID: MessageID
@@ -99,13 +117,12 @@ function wrap<Parameters extends Schema.Decoder<unknown>, Result extends Metadat
         }
         return Effect.gen(function* () {
           const decoded = yield* decode(args).pipe(
-            Effect.mapError((error) =>
-              toolInfo.formatValidationError
-                ? new Error(toolInfo.formatValidationError(error), { cause: error })
-                : new Error(
-                    `The ${id} tool was called with invalid arguments: ${error}.\nPlease rewrite the input so it satisfies the expected schema.`,
-                    { cause: error },
-                  ),
+            Effect.mapError(
+              (error) =>
+                new InvalidArgumentsError({
+                  tool: id,
+                  detail: toolInfo.formatValidationError ? toolInfo.formatValidationError(error) : String(error),
+                }),
             ),
           )
           const result = yield* execute(decoded as Schema.Schema.Type<Parameters>, ctx)
diff --git a/packages/opencode/test/question/question.test.ts b/packages/opencode/test/question/question.test.ts
index 09ede40068..a5841bd08d 100644
--- a/packages/opencode/test/question/question.test.ts
+++ b/packages/opencode/test/question/question.test.ts
@@ -416,46 +416,6 @@ it.live("pending question rejects on instance dispose", () =>
   }),
 )
 
-// Regression for #28438: when an invalid payload reaches `Question.ask`
-// (one that's missing a required field like `question`), the previous
-// `Schema.decodeUnknownSync` would throw uncaught and crash the whole
-// assistant turn. The fix routes the failure through `Effect.orDie` with a
-// "rewrite the input" Error so the surrounding tool wrap can hand it back to
-// the model as a tool-call error rather than killing the session.
-it.instance(
-  "ask - invalid payload surfaces as a friendly defect, not a thrown SchemaError",
-  () =>
-    Effect.gen(function* () {
-      const exit = yield* askEffect({
-        sessionID: SessionID.make("ses_invalid"),
-        // Cast: bypassing the public type to simulate an upstream caller
-        // (or a future schema divergence) that lets a missing required
-        // field reach the decode boundary.
-        questions: [
-          {
-            header: "Pick mode",
-            options: [
-              { label: "A", description: "x" },
-              { label: "B", description: "y" },
-            ],
-          } as unknown as Question.Info,
-        ],
-      }).pipe(Effect.exit)
-
-      expect(Exit.isFailure(exit)).toBe(true)
-      if (Exit.isFailure(exit)) {
-        const message = exit.cause.toString()
-        // Friendly preamble the AI SDK feeds back to the model so it can retry.
-        expect(message).toContain("invalid arguments")
-        expect(message).toContain("Please rewrite the input")
-        // The exact JSON path pinpointing the missing field, so the model
-        // knows which question and which field to fix.
-        expect(message).toContain(`["questions"][0]["question"]`)
-      }
-    }),
-  { git: true },
-)
-
 it.live("pending question rejects on instance reload", () =>
   Effect.gen(function* () {
     const dir = yield* tmpdirScoped({ git: true })
diff --git a/packages/opencode/test/tool/tool-define.test.ts b/packages/opencode/test/tool/tool-define.test.ts
index ca351cca48..08e0604362 100644
--- a/packages/opencode/test/tool/tool-define.test.ts
+++ b/packages/opencode/test/tool/tool-define.test.ts
@@ -1,5 +1,5 @@
 import { describe, expect } from "bun:test"
-import { Effect, Layer, Schema } from "effect"
+import { Cause, Effect, Exit, Layer, Schema } from "effect"
 import { Agent } from "../../src/agent/agent"
 import { MessageID, SessionID } from "../../src/session/schema"
 import { Tool } from "@/tool/tool"
@@ -10,6 +10,22 @@ const it = testEffect(Layer.mergeAll(Truncate.defaultLayer, Agent.defaultLayer))
 
 const params = Schema.Struct({ input: Schema.String })
 
+function makeCtx(): Tool.Context {
+  return {
+    sessionID: SessionID.descending(),
+    messageID: MessageID.ascending(),
+    agent: "build",
+    abort: new AbortController().signal,
+    messages: [],
+    metadata() {
+      return Effect.void
+    },
+    ask() {
+      return Effect.void
+    },
+  }
+}
+
 function makeTool(id: string, executeFn?: () => void) {
   return {
     description: "test tool",
@@ -79,19 +95,7 @@ describe("Tool.define", () => {
           },
         }),
       )
-      const ctx: Tool.Context = {
-        sessionID: SessionID.descending(),
-        messageID: MessageID.ascending(),
-        agent: "build",
-        abort: new AbortController().signal,
-        messages: [],
-        metadata() {
-          return Effect.void
-        },
-        ask() {
-          return Effect.void
-        },
-      }
+      const ctx = makeCtx()
       const tool = yield* info.init()
       const execute = tool.execute as unknown as (args: unknown, ctx: Tool.Context) => ReturnType<typeof tool.execute>
 
@@ -101,4 +105,49 @@ describe("Tool.define", () => {
       expect(calls).toEqual([{ count: 5 }, { count: 7 }])
     }),
   )
+
+  // Regression for #28438: the wrap is the canonical "untyped → typed" boundary.
+  // When the LLM emits a tool call with a payload that fails the parameter
+  // schema, the wrap must surface a typed `Tool.InvalidArgumentsError` whose
+  // `.message` is the actionable prose the AI SDK feeds back to the model.
+  it.effect("invalid args surface as Tool.InvalidArgumentsError with friendly message and JSON path", () =>
+    Effect.gen(function* () {
+      const parameters = Schema.Struct({
+        questions: Schema.Array(
+          Schema.Struct({
+            question: Schema.String,
+            options: Schema.Array(Schema.String),
+          }),
+        ),
+      })
+      const info = yield* Tool.define(
+        "qtest",
+        Effect.succeed({
+          description: "test tool",
+          parameters,
+          execute() {
+            return Effect.succeed({ title: "ok", output: "ok", metadata: { truncated: false } })
+          },
+        }),
+      )
+      const tool = yield* info.init()
+      const execute = tool.execute as unknown as (args: unknown, ctx: Tool.Context) => ReturnType<typeof tool.execute>
+
+      // Missing required `question` field on the first questions[] entry.
+      const exit = yield* execute({ questions: [{ options: ["a"] }] }, makeCtx()).pipe(Effect.exit)
+      expect(Exit.isFailure(exit)).toBe(true)
+      if (!Exit.isFailure(exit)) return
+
+      // The wrap ends with Effect.orDie, so the failure lives in the cause as a
+      // defect. Recover the typed instance from there.
+      const die = exit.cause.reasons.find(Cause.isDieReason)
+      const error = die?.defect
+      expect(error).toBeInstanceOf(Tool.InvalidArgumentsError)
+      const args = error as Tool.InvalidArgumentsError
+      expect(args.tool).toBe("qtest")
+      expect(args.message).toContain("qtest tool was called with invalid arguments")
+      expect(args.message).toContain("Please rewrite the input")
+      expect(args.message).toContain(`["questions"][0]["question"]`)
+    }),
+  )
 })