Sfoglia il codice sorgente

Fix schema-DSL findings from the second Codex review

InferArgs now produces genuinely optional keys: required/optional
properties are split at the key level (RequiredKeys + mapped `?`), so
{ limit: { type: 'number' } } infers as { limit?: number } and callers
can omit it — previously the key stayed required with `| undefined`.
Array item inference recurses (arrays of objects infer their element
shape instead of Record<string, unknown>), matching the generated
JSON Schema.

Tool execution error reporting handles non-Error throws again:
`throw { message: 'denied' }` reports the message instead of
"[object Object]" (errorMessage helper).

The new schema tests now actually typecheck: schema literals use
`satisfies SchemaSpec` (the standalone-literal widening made
schemaSpecToJsonSchema reject the suite's own examples), and the
ToolSchema probe cast goes through unknown. Tests-and-examples
typechecking is now part of `yarn typecheck` via the new
tsconfig.typecheck.json (resolves vendor packages by their built
declarations, so vendor's relaxed-strictness source stays out of
scope) — vitest never typechecks, so this gate is what catches such
breakage. +4 regression tests (typed omission, array-of-objects
inference both type- and runtime-level, non-Error throw message).
Tianyi Cui 3 mesi fa
parent
commit
ef45ca823a

+ 1 - 1
package.json

@@ -13,7 +13,7 @@
   ],
   "scripts": {
     "build": "tsc -b tsconfig.build.json && tsx scripts/build.ts",
-    "typecheck": "tsc -b tsconfig.build.json",
+    "typecheck": "tsc -b tsconfig.build.json && tsc -p tsconfig.typecheck.json",
     "test": "vitest run",
     "demo": "node --expose-internals --import tsx examples/echo-agent/start.ts"
   },

+ 16 - 2
packages/tools/src/index.ts

@@ -70,6 +70,21 @@ export interface ToolExecutionResult {
   isError: boolean
 }
 
+/**
+ * Best-effort human-readable message from an arbitrary thrown value: Error
+ * instances use `.message`; non-Error objects with a string `message`
+ * property (e.g. `throw { message: 'denied' }`) use it too; everything else
+ * is stringified.
+ */
+function errorMessage(error: unknown): string {
+  if (error instanceof Error) return error.message
+  if (typeof error === 'object' && error !== null
+    && 'message' in error && typeof error.message === 'string') {
+    return error.message
+  }
+  return String(error)
+}
+
 /**
  * Tool registry (`ctx.tools`): tool plugins register definitions; the agent
  * loop executes calls through the `tools/execute` waterfall. The registry
@@ -138,10 +153,9 @@ export class ToolRegistry extends Service {
         const content = await tool.execute(exec.arguments, exec)
         return { callId: exec.callId, content, isError: false }
       } catch (error: unknown) {
-        const message = error instanceof Error ? error.message : String(error)
         return {
           callId: exec.callId,
-          content: [{ type: 'text', text: `Error: ${message}` }],
+          content: [{ type: 'text', text: `Error: ${errorMessage(error)}` }],
           isError: true,
         }
       }

+ 23 - 16
packages/tools/src/schema.ts

@@ -66,34 +66,41 @@ type TypeOf<T extends SchemaType> =
   T extends 'array' ? unknown[] :
   never
 
+/** Flatten an intersection into one object type for readable hovers. */
+type Simplify<T> = { [K in keyof T]: T[K] } & {}
+
+/** Keys of `S` whose prop is marked `required: true`. */
+type RequiredKeys<S extends SchemaSpec> =
+  { [K in keyof S]: S[K] extends { required: true } ? K : never }[keyof S]
+
 /**
- * Infer the TS type of a single {@link SchemaProp}.
- * - `required: true` → required (non-optional)
- * - absent required → optional
- * - `properties` on 'object' → recurse
+ * The VALUE type of one {@link SchemaProp} — optionality is handled at the
+ * key level by {@link InferArgs}, never here.
+ * - `properties` on 'object' → recurse into the nested SchemaSpec
+ * - `items` on 'array' → recurse into the item prop (arrays of objects work)
+ * - otherwise → the primitive for `type`
  */
-type InferProp<P extends SchemaProp> =
-  P extends { type: 'object'; properties: infer Sub extends SchemaSpec } ?
-    // Nested objects with their own SchemaSpec — infer their shape
-    (P extends { required: true } ? InferArgs<Sub> : InferArgs<Sub> | undefined) :
-  P extends { type: 'array'; items: infer Item extends SchemaProp } ?
-    // Arrays: infer item type
-    (P extends { required: true } ? TypeOf<Item['type']>[] : TypeOf<Item['type']>[] | undefined) :
-  // Primitive types
-  (P extends { required: true } ? TypeOf<P['type']> : TypeOf<P['type']> | undefined)
+type InferPropValue<P extends SchemaProp> =
+  P extends { type: 'object'; properties: infer Sub extends SchemaSpec } ? InferArgs<Sub> :
+  P extends { type: 'array'; items: infer Item extends SchemaProp } ? InferPropValue<Item>[] :
+  TypeOf<P['type']>
 
 /**
  * Infer the TS argument type for a complete {@link SchemaSpec}.
  *
+ * Properties marked `required: true` are required keys; all others are
+ * genuinely optional keys (`?`), so callers may omit them entirely.
+ *
  * Example:
  * ```ts
  * type Args = InferArgs<{ path: { type: 'string'; required: true }; limit: { type: 'number' } }>
  * // → { path: string; limit?: number }
  * ```
  */
-export type InferArgs<S extends SchemaSpec> = {
-  [K in keyof S]: InferProp<S[K]>
-}
+export type InferArgs<S extends SchemaSpec> = Simplify<
+  & { [K in RequiredKeys<S>]: InferPropValue<S[K]> }
+  & { [K in Exclude<keyof S, RequiredKeys<S>>]?: InferPropValue<S[K]> }
+>
 
 // ---------------------------------------------------------------------------
 // Runtime conversion: SchemaSpec → JSON Schema

+ 93 - 11
packages/tools/tests/tools.spec.ts

@@ -1,7 +1,10 @@
-import { describe, expect, it } from 'vitest'
+import { describe, expect, expectTypeOf, it } from 'vitest'
 import { Context } from 'cordis'
 import SystemPrompt from '@deepseek-ai/dsh-system-prompt'
-import ToolRegistry, { defineTool, schemaSpecToJsonSchema, ToolExecutionResult } from '@deepseek-ai/dsh-tools'
+import ToolRegistry, {
+  defineTool, schemaSpecToJsonSchema,
+  type InferArgs, type SchemaSpec, type ToolExecutionResult,
+} from '@deepseek-ai/dsh-tools'
 
 async function setup() {
   const ctx = new Context()
@@ -29,9 +32,9 @@ describe('ToolRegistry', () => {
       description: 'echo arguments back',
       parameters: { type: 'object', properties: { text: { type: 'string' } } },
     }])
-    // schemas() result must not leak execute — as any intentional: 'execute'
-    // is deliberately absent from ToolSchema, we're testing it's not there
-    expect((ctx.tools.schemas()[0] as Record<string, unknown>).execute).toBeUndefined()
+    // schemas() result must not leak execute — ToolSchema deliberately has no
+    // 'execute' key, so widen through unknown to probe for the absent property
+    expect((ctx.tools.schemas()[0] as unknown as Record<string, unknown>).execute).toBeUndefined()
 
     const assembly = await ctx.systemPrompt.assemble()
     expect(assembly.tools.map(t => t.name)).toEqual(['echo'])
@@ -123,10 +126,10 @@ describe('ToolRegistry', () => {
 describe('defineTool / schema DSL', () => {
   it('converts SchemaSpec to standard JSON Schema with required array', () => {
     const spec = {
-      path: { type: 'string', required: true as const, description: 'Absolute path' },
+      path: { type: 'string', required: true, description: 'Absolute path' },
       offset: { type: 'number' },
       limit: { type: 'number', description: 'Max lines' },
-    }
+    } satisfies SchemaSpec
     const jsonSchema = schemaSpecToJsonSchema(spec)
     expect(jsonSchema).toEqual({
       type: 'object',
@@ -149,14 +152,14 @@ describe('defineTool / schema DSL', () => {
   it('handles nested object spec', () => {
     const spec = {
       config: {
-        type: 'object' as const,
-        required: true as const,
+        type: 'object',
+        required: true,
         properties: {
-          host: { type: 'string', required: true as const },
+          host: { type: 'string', required: true },
           port: { type: 'number' },
         },
       },
-    }
+    } satisfies SchemaSpec
     const jsonSchema = schemaSpecToJsonSchema(spec)
     expect(jsonSchema).toEqual({
       type: 'object',
@@ -299,3 +302,82 @@ describe('defineTool / schema DSL', () => {
     expect(result.content).toEqual([{ type: 'text', text: '/tmp' }])
   })
 })
+
+describe('schema DSL regressions (Codex review round 2)', () => {
+  it('InferArgs makes non-required keys genuinely optional (omittable)', () => {
+    type Args = InferArgs<{
+      path: { type: 'string'; required: true }
+      limit: { type: 'number' }
+    }>
+    expectTypeOf<Args>().toEqualTypeOf<{ path: string; limit?: number }>()
+    // omitting the optional key is assignable — the actual regression
+    const omitted: Args = { path: '/tmp' }
+    expect(omitted.limit).toBeUndefined()
+  })
+
+  it('InferArgs recurses into array items, including arrays of objects', () => {
+    type Args = InferArgs<{
+      names: { type: 'array'; required: true; items: { type: 'string' } }
+      servers: {
+        type: 'array'
+        items: {
+          type: 'object'
+          properties: {
+            host: { type: 'string'; required: true }
+            port: { type: 'number' }
+          }
+        }
+      }
+    }>
+    expectTypeOf<Args>().toEqualTypeOf<{
+      names: string[]
+      servers?: { host: string; port?: number }[]
+    }>()
+  })
+
+  it('runtime JSON Schema matches the array-of-objects inference', () => {
+    const spec = {
+      servers: {
+        type: 'array',
+        items: {
+          type: 'object',
+          properties: {
+            host: { type: 'string', required: true },
+            port: { type: 'number' },
+          },
+        },
+      },
+    } satisfies SchemaSpec
+    expect(schemaSpecToJsonSchema(spec)).toEqual({
+      type: 'object',
+      properties: {
+        servers: {
+          type: 'array',
+          items: {
+            type: 'object',
+            properties: {
+              host: { type: 'string' },
+              port: { type: 'number' },
+            },
+            required: ['host'],
+          },
+        },
+      },
+    })
+  })
+
+  it('reports messages from non-Error throws (throw { message })', async () => {
+    const ctx = await setup()
+    ctx.tools.register({
+      ...echoTool,
+      name: 'object-thrower',
+      async execute() {
+        // eslint-disable-next-line no-throw-literal — testing non-Error throws
+        throw { message: 'denied by object' }
+      },
+    })
+    const result = await ctx.tools.execute({ callId: 'c1', name: 'object-thrower', arguments: {} })
+    expect(result.isError).toBe(true)
+    expect(result.content[0]).toMatchObject({ text: 'Error: denied by object' })
+  })
+})

+ 28 - 0
tsconfig.typecheck.json

@@ -0,0 +1,28 @@
+{
+  "extends": "./tsconfig.base.json",
+  "compilerOptions": {
+    "noEmit": true,
+    "emitDeclarationOnly": false,
+    "composite": false,
+    "incremental": false,
+    "types": ["node"],
+    "paths": {
+      "cordis": ["./vendor/cordis/lib"],
+      "cosmokit": ["./vendor/cosmokit/lib"],
+      "schemastery": ["./vendor/schemastery/lib"],
+      "@cordisjs/plugin-loader": ["./vendor/loader/lib"],
+      "@cordisjs/plugin-include": ["./vendor/include/lib"],
+      "@cordisjs/plugin-group": ["./vendor/group/lib"],
+      "@cordisjs/plugin-timer": ["./vendor/timer/lib"],
+      "@cordisjs/plugin-hmr": ["./vendor/hmr/lib"],
+      "@cordisjs/plugin-logger-console": ["./vendor/logger-console/lib/shared"],
+      "@deepseek-ai/dsh-llm": ["./packages/llm/src"],
+      "@deepseek-ai/dsh-session": ["./packages/session/src"],
+      "@deepseek-ai/dsh-system-prompt": ["./packages/system-prompt/src"],
+      "@deepseek-ai/dsh-tools": ["./packages/tools/src"],
+      "@deepseek-ai/dsh-agent": ["./packages/agent/src"],
+      "@deepseek-ai/dsh-agent-loop": ["./packages/agent-loop/src"]
+    }
+  },
+  "include": ["packages/*/src", "packages/*/tests", "examples", "scripts"]
+}