diff --git a/package.json b/package.json index 033c5faf59..1b3ba475b1 100644 --- a/package.json +++ b/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" }, diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index 85dd3b7e01..057f0397f7 100644 --- a/packages/tools/src/index.ts +++ b/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, } } diff --git a/packages/tools/src/schema.ts b/packages/tools/src/schema.ts index 92e3afdb84..66a094a7dc 100644 --- a/packages/tools/src/schema.ts +++ b/packages/tools/src/schema.ts @@ -66,34 +66,41 @@ type TypeOf = T extends 'array' ? unknown[] : never +/** Flatten an intersection into one object type for readable hovers. */ +type Simplify = { [K in keyof T]: T[K] } & {} + +/** Keys of `S` whose prop is marked `required: true`. */ +type RequiredKeys = + { [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 { type: 'object'; properties: infer Sub extends SchemaSpec } ? - // Nested objects with their own SchemaSpec — infer their shape - (P extends { required: true } ? InferArgs : InferArgs | undefined) : - P extends { type: 'array'; items: infer Item extends SchemaProp } ? - // Arrays: infer item type - (P extends { required: true } ? TypeOf[] : TypeOf[] | undefined) : - // Primitive types - (P extends { required: true } ? TypeOf : TypeOf | undefined) +type InferPropValue

= + P extends { type: 'object'; properties: infer Sub extends SchemaSpec } ? InferArgs : + P extends { type: 'array'; items: infer Item extends SchemaProp } ? InferPropValue[] : + TypeOf /** * 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 = { - [K in keyof S]: InferProp -} +export type InferArgs = Simplify< + & { [K in RequiredKeys]: InferPropValue } + & { [K in Exclude>]?: InferPropValue } +> // --------------------------------------------------------------------------- // Runtime conversion: SchemaSpec → JSON Schema diff --git a/packages/tools/tests/tools.spec.ts b/packages/tools/tests/tools.spec.ts index ebca94079a..98ab3332f2 100644 --- a/packages/tools/tests/tools.spec.ts +++ b/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).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).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().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().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' }) + }) +}) diff --git a/tsconfig.typecheck.json b/tsconfig.typecheck.json new file mode 100644 index 0000000000..472560f1ea --- /dev/null +++ b/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"] +}