From 11f85b4f88e62bbf066c77ff0f064be159e0fdf5 Mon Sep 17 00:00:00 2001
From: Tianyi Cui <53024+tianyicui@users.noreply.github.com>
Date: Sat, 13 Jun 2026 23:11:48 +0800
Subject: [PATCH] fix(tools): address Codex review of arg validation (PR 1)

- enum membership now checked uniformly for all SchemaTypes, mirroring the
  converter which emits `enum` regardless of type (was string-only)
- checkValue switch ends in assertNever per the closed-union convention
- sync the adding-a-tool cookbook to the validate-for-you behavior
- soften ADR 0011's property-test claim (RFC 001 not yet landed)
---
 docs/adr/0011-runtime-arg-validation.md |  2 +-
 docs/cookbook/adding-a-tool.md          |  2 +-
 packages/tools/src/schema.ts            | 22 +++++++++++++++-------
 packages/tools/tests/tools.spec.ts      | 12 ++++++++++++
 4 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/docs/adr/0011-runtime-arg-validation.md b/docs/adr/0011-runtime-arg-validation.md
index 98b8b81e46..90bb69dc3f 100644
--- a/docs/adr/0011-runtime-arg-validation.md
+++ b/docs/adr/0011-runtime-arg-validation.md
@@ -15,6 +15,6 @@ The validator mirrors `schemaSpecToJsonSchema` semantics exactly — same struct
 ## Consequences
 
 - The model gets actionable feedback on its own malformed calls instead of an opaque crash, closing the gap between `InferArgs`'s promise and runtime reality.
-- The validator and `InferArgs` must stay in agreement; that drift risk is closed by a property test (RFC 001) generating args that satisfy `InferArgs` and asserting they pass `validateArgs`.
+- The validator and `InferArgs` must stay in agreement; that drift risk is to be closed by a property test (RFC 001, not yet landed) generating args that satisfy `InferArgs` and asserting they pass `validateArgs`. Until then the agreement rests on the example tests and the shared converter structure.
 - `ToolArgsError` is a plain `Error` with a `code` field for now; if a harness-wide error taxonomy lands it becomes a subclass without changing callers that read `.message`.
 - Validation cost is negligible next to a model call.
diff --git a/docs/cookbook/adding-a-tool.md b/docs/cookbook/adding-a-tool.md
index ec84d12e2a..431e473df0 100644
--- a/docs/cookbook/adding-a-tool.md
+++ b/docs/cookbook/adding-a-tool.md
@@ -32,7 +32,7 @@ Registration is effect-based: disposing the plugin fiber unregisters the tool (w
 
 ## Rules of the execute() contract
 
-- **Validate args at runtime.** `defineTool`'s `InferArgs` typing is compile-time only; at runtime `arguments` is whatever JSON the model emitted. Check every field; throw a descriptive Error for bad input.
+- **Args are validated for you.** `defineTool` validates the model-generated `arguments` against the `SchemaSpec` before `execute` runs (type, required keys, enum membership, nested objects/arrays — ADR 0011), so inside `execute` the args already match `InferArgs`. You still hand-check value constraints the DSL can't express (non-empty strings, positive numbers, cross-field rules); throw a descriptive Error for those. Raw JSON-Schema tools registered directly (MCP) are NOT validated by the harness — they validate their own input.
 - **Throwing means isError.** The registry catches anything `execute()` throws and returns `{isError: true}` to the model. Use that for infrastructure failures (bad input, spawn errors, aborts) — but REPORT domain failures in the result text instead (e.g. tool-bash returns `[exit code: 9]` with `isError: false`: the model decides what a failing command means).
 - **Honor `exec.signal`.** Cancel in-flight work when it fires.
 - **Use `exec.agent` for async notifications.** `agent.inject(content, {source: {kind: 'plugin', plugin: '<name>'}})` appends durable context the NEXT model request sees — it is not a wake-up (an idle agent stays idle). Guard against disposed agents (try/catch).
diff --git a/packages/tools/src/schema.ts b/packages/tools/src/schema.ts
index 39d978da3e..577a200504 100644
--- a/packages/tools/src/schema.ts
+++ b/packages/tools/src/schema.ts
@@ -20,6 +20,7 @@
  */
 
 import type { ContentBlock } from '@deepseek-ai/dsh-llm'
+import { assertNever } from '@deepseek-ai/dsh-llm'
 import type { ToolDefinition, ToolExecution } from './index.ts'
 
 // ---------------------------------------------------------------------------
@@ -202,16 +203,15 @@ function checkValue(prop: SchemaProp, value: unknown, path: string): string[] {
   switch (prop.type) {
     case 'string': {
       if (typeof value !== 'string') return [`"${path}" must be a string`]
-      if (prop.enum && !prop.enum.includes(value)) {
-        return [`"${path}" must be one of ${JSON.stringify(prop.enum)}`]
-      }
-      return []
+      break
     }
     case 'number': {
-      return typeof value === 'number' ? [] : [`"${path}" must be a number`]
+      if (typeof value !== 'number') return [`"${path}" must be a number`]
+      break
     }
     case 'boolean': {
-      return typeof value === 'boolean' ? [] : [`"${path}" must be a boolean`]
+      if (typeof value !== 'boolean') return [`"${path}" must be a boolean`]
+      break
     }
     case 'object': {
       if (!isPlainObject(value)) return [`"${path}" must be an object`]
@@ -225,8 +225,16 @@ function checkValue(prop: SchemaProp, value: unknown, path: string): string[] {
       const items = prop.items
       return value.flatMap((el, i) => checkValue(items, el, `${path}[${i}]`))
     }
-    // No default: SchemaType is a closed union; every case is handled above.
+    default: return assertNever(prop.type, 'validateArgs')
   }
+  // Enum membership, checked uniformly: the converter emits `enum` for any
+  // type ([prop.enum]), so the validator must too. `enum` is `string[]`, so a
+  // non-string value can never be a member — it falls out here, consistent
+  // with the schema the model was given.
+  if (prop.enum && !(prop.enum as unknown[]).includes(value)) {
+    return [`"${path}" must be one of ${JSON.stringify(prop.enum)}`]
+  }
+  return []
 }
 
 /** Collect violations for an object value against a {@link SchemaSpec}. */
diff --git a/packages/tools/tests/tools.spec.ts b/packages/tools/tests/tools.spec.ts
index 410b3519b7..4eaf222687 100644
--- a/packages/tools/tests/tools.spec.ts
+++ b/packages/tools/tests/tools.spec.ts
@@ -623,6 +623,18 @@ describe('validateArgs (RFC 005 part 1)', () => {
     expect(validateArgs(spec, { color: 'blue' })).toEqual(['"color" must be one of ["red","green"]'])
   })
 
+  it('checks enum uniformly with the converter (enum on a non-string prop)', () => {
+    // The converter emits `enum` regardless of type; the validator must agree.
+    // `enum` is string[], so a number value can never be a member.
+    const spec = { n: { type: 'number', enum: ['1', '2'] } } as unknown as SchemaSpec
+    expect(validateArgs(spec, { n: 1 })).toEqual(['"n" must be one of ["1","2"]'])
+  })
+
+  it('rejects an unknown SchemaType at runtime (assertNever guard)', () => {
+    const spec = { x: { type: 'weird' } } as unknown as SchemaSpec
+    expect(() => validateArgs(spec, { x: 1 })).toThrow(/unreachable variant.*validateArgs/)
+  })
+
   it('recurses into nested objects (and an object without properties only type-checks)', () => {
     const spec = {
       config: {