From 9844ebd305fc1244f85b4c88b2225d648323e944 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Sat, 15 Aug 2026 00:04:02 +0000 Subject: [PATCH 1/3] feat(agent-cli): validate tool input against the declared schema Both tool executors now check each invocation's input against the tool's declared inputSchema before dispatch and throw a ToolInputValidationError listing every mismatch, instead of forwarding unchecked input to handlers. Co-Authored-By: Claude Fable 5 --- packages/agent-cli/src/commands/router.ts | 6 + packages/agent-cli/src/core/validate-input.ts | 229 +++++++++++++ packages/agent-cli/src/executor.ts | 3 + packages/agent-cli/src/index.ts | 1 + .../tests/tool-input-validation.test.ts | 319 ++++++++++++++++++ 5 files changed, 558 insertions(+) create mode 100644 packages/agent-cli/src/core/validate-input.ts create mode 100644 packages/agent-cli/tests/tool-input-validation.test.ts diff --git a/packages/agent-cli/src/commands/router.ts b/packages/agent-cli/src/commands/router.ts index b11ccbeb0..0ebcf5d6a 100644 --- a/packages/agent-cli/src/commands/router.ts +++ b/packages/agent-cli/src/commands/router.ts @@ -557,6 +557,11 @@ interface ToolCatalogEntry { } interface InProcessToolEntry { + /** + * Same declaration the catalog exposes, so the in-process executor validates + * tool input exactly like the spawned-CLI executor does. + */ + inputSchema: ToolCatalogEntry['inputSchema']; execute: (context: { dataFile: string; input: Record; @@ -691,6 +696,7 @@ export function getInProcessToolExecutors(): Record< for (const def of Object.values(subcommands)) { if (!def.toolName) continue; tools[def.toolName] = { + inputSchema: toolInputSchema, sourcePagination: getSourcePaginationConfig(def.options), execute: async ({ dataFile, input }) => { setDataFilePath(dataFile); diff --git a/packages/agent-cli/src/core/validate-input.ts b/packages/agent-cli/src/core/validate-input.ts new file mode 100644 index 000000000..db1688840 --- /dev/null +++ b/packages/agent-cli/src/core/validate-input.ts @@ -0,0 +1,229 @@ +import type { JsonSchema } from './types'; + +/** + * Tool input validation. + * + * The supported schema dialect is exactly what the tool catalog emits from + * `OptionDef` (see `optionToJsonSchema` / `buildInputSchema` in + * `commands/router.ts`): an object schema carrying `properties`, an optional + * `required` list, `additionalProperties`, and per-property `type` (a single + * name or a list of names), `enum`, `minimum`, and `maximum`. `items` is + * handled as well so array properties declared by hand-written + * `ToolDefinition`s are checked too. + * + * Two deliberate choices keep the executor from being stricter than the CLI it + * mirrors: + * + * - Unknown keywords are ignored instead of rejected, so a richer schema never + * fails closed. + * - Extra properties are accepted unless a schema explicitly declares + * `additionalProperties: false`. The catalog declares + * `additionalProperties: true` because `parseSubcommandOptions` and + * `appendToolSpecificOptions` silently drop options a subcommand does not + * declare, so rejecting unknown keys here would refuse input the CLI itself + * accepts. + */ +export interface ToolInputValidationIssue { + /** Dotted path to the offending value, empty for the input object itself. */ + path: string; + message: string; +} + +/** + * Thrown before dispatch when a tool input does not match the tool's declared + * `inputSchema`. Throwing matches how the executor already reports pre-dispatch + * failures (unknown tool, unparsable control values), so the CLI keeps wrapping + * it in the usual `{ ok: false, error }` envelope, while `issues` exposes the + * structured detail. + */ +export class ToolInputValidationError extends Error { + readonly toolName: string; + readonly issues: ToolInputValidationIssue[]; + + constructor(toolName: string, issues: ToolInputValidationIssue[]) { + super( + `Invalid input for rsdoctor tool ${toolName}: ${issues + .map((issue) => issue.message) + .join('; ')}`, + ); + this.name = 'ToolInputValidationError'; + this.toolName = toolName; + this.issues = issues; + } +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +function matchesSchemaType(value: unknown, type: unknown): boolean { + if (Array.isArray(type)) { + return type.some((entry) => matchesSchemaType(value, entry)); + } + + switch (type) { + case 'array': + return Array.isArray(value); + case 'boolean': + return typeof value === 'boolean'; + case 'integer': + return typeof value === 'number' && Number.isInteger(value); + case 'null': + return value === null; + case 'number': + return typeof value === 'number' && Number.isFinite(value); + case 'object': + return isRecord(value); + case 'string': + return typeof value === 'string'; + default: + return true; + } +} + +function describeSchemaType(type: unknown): string { + return Array.isArray(type) ? type.map(String).join(' or ') : String(type); +} + +function describeValueType(value: unknown): string { + if (value === null) { + return 'null'; + } + if (Array.isArray(value)) { + return 'array'; + } + return typeof value; +} + +function formatValue(value: unknown): string { + return typeof value === 'string' ? JSON.stringify(value) : String(value); +} + +function describeLabel(path: string): string { + return path === '' ? 'input' : `"${path}"`; +} + +function joinPath(path: string, key: string): string { + return path === '' ? key : `${path}.${key}`; +} + +function collectIssues( + value: unknown, + schema: unknown, + path: string, + issues: ToolInputValidationIssue[], +): void { + if (!isRecord(schema)) { + return; + } + + const label = describeLabel(path); + + if (schema.type !== undefined && !matchesSchemaType(value, schema.type)) { + issues.push({ + path, + message: `${label} must be of type ${describeSchemaType( + schema.type, + )}, received ${describeValueType(value)}`, + }); + return; + } + + if (Array.isArray(schema.enum) && !schema.enum.includes(value)) { + issues.push({ + path, + message: `${label} must be one of ${schema.enum + .map(formatValue) + .join(', ')}, received ${formatValue(value)}`, + }); + } + + if (typeof value === 'number') { + if (typeof schema.minimum === 'number' && value < schema.minimum) { + issues.push({ + path, + message: `${label} must be >= ${schema.minimum}, received ${value}`, + }); + } + if (typeof schema.maximum === 'number' && value > schema.maximum) { + issues.push({ + path, + message: `${label} must be <= ${schema.maximum}, received ${value}`, + }); + } + } + + if (Array.isArray(value)) { + if (isRecord(schema.items)) { + value.forEach((entry, index) => { + collectIssues(entry, schema.items, `${path}[${index}]`, issues); + }); + } + return; + } + + if (!isRecord(value)) { + return; + } + + const properties = isRecord(schema.properties) ? schema.properties : {}; + + if (Array.isArray(schema.required)) { + for (const key of schema.required) { + if (typeof key === 'string' && !(key in value)) { + issues.push({ + path: joinPath(path, key), + message: `${label} is missing required property "${key}"`, + }); + } + } + } + + for (const [key, entry] of Object.entries(value)) { + const propertyPath = joinPath(path, key); + const propertySchema = properties[key]; + + if (propertySchema !== undefined) { + collectIssues(entry, propertySchema, propertyPath, issues); + continue; + } + + if (schema.additionalProperties === false) { + issues.push({ + path: propertyPath, + message: `${describeLabel(propertyPath)} is not an allowed property`, + }); + continue; + } + + if (isRecord(schema.additionalProperties)) { + collectIssues(entry, schema.additionalProperties, propertyPath, issues); + } + } +} + +/** + * Validates `input` against a tool's declared `inputSchema` and throws a + * {@link ToolInputValidationError} listing every mismatch. + * + * `undefined` (or any non-object) input is reported as a type mismatch rather + * than defaulted to `{}`: `ToolExecutionRequest.input` declares the field as + * required, and the executor previously crashed on a missing one. Callers that + * treat "no arguments" as valid should keep passing `{}`. + */ +export function validateToolInput( + toolName: string, + input: unknown, + schema: JsonSchema | undefined, +): void { + if (schema === undefined) { + return; + } + + const issues: ToolInputValidationIssue[] = []; + collectIssues(input, schema, '', issues); + + if (issues.length > 0) { + throw new ToolInputValidationError(toolName, issues); + } +} diff --git a/packages/agent-cli/src/executor.ts b/packages/agent-cli/src/executor.ts index 1d1eb32b4..fdc499555 100644 --- a/packages/agent-cli/src/executor.ts +++ b/packages/agent-cli/src/executor.ts @@ -10,6 +10,7 @@ import { applyToolResultControls, splitToolInputControls, } from './core/result-controls'; +import { validateToolInput } from './core/validate-input'; import { getInProcessToolExecutors } from './commands'; const execFileAsync = promisify(execFile); @@ -44,6 +45,7 @@ export function createRsdoctorCliToolExecutor({ return { async execute(request: ToolExecutionRequest): Promise { const tool = getToolByName(tools, request.toolName); + validateToolInput(request.toolName, request.input, tool.inputSchema); const { controls, passthroughInput, paginateResult } = splitToolInputControls(request.input, { sourcePagination: tool.sourcePagination, @@ -79,6 +81,7 @@ export function createInProcessRsdoctorCliToolExecutor(): ToolExecutor { if (!tool) { throw new Error(`Unknown rsdoctor tool: ${request.toolName}`); } + validateToolInput(request.toolName, request.input, tool.inputSchema); const { controls, passthroughInput, paginateResult } = splitToolInputControls(request.input, { sourcePagination: tool.sourcePagination, diff --git a/packages/agent-cli/src/index.ts b/packages/agent-cli/src/index.ts index 11d4e8478..84e1ee6e7 100644 --- a/packages/agent-cli/src/index.ts +++ b/packages/agent-cli/src/index.ts @@ -1,4 +1,5 @@ export * from './cli'; export * from './commands'; export * from './core/types'; +export * from './core/validate-input'; export * from './executor'; diff --git a/packages/agent-cli/tests/tool-input-validation.test.ts b/packages/agent-cli/tests/tool-input-validation.test.ts new file mode 100644 index 000000000..fd376ca6b --- /dev/null +++ b/packages/agent-cli/tests/tool-input-validation.test.ts @@ -0,0 +1,319 @@ +import { describe, expect, it } from '@rstest/core'; + +import { getToolCatalog } from '../src/commands'; +import type { ToolDefinition } from '../src/core/types'; +import { ToolInputValidationError } from '../src/core/validate-input'; +import { + createInProcessRsdoctorCliToolExecutor, + createRsdoctorCliToolExecutor, +} from '../src/executor'; + +interface Harness { + execute: (input: unknown) => Promise; + commands: string[][]; +} + +function createHarness(inputSchema: ToolDefinition['inputSchema']): Harness { + const commands: string[][] = []; + const executor = createRsdoctorCliToolExecutor({ + tools: [ + { + name: 'schema_tool', + description: 'test tool', + inputSchema, + buildCommand: () => ['schema-tool'], + }, + ], + runCommand: async (command) => { + commands.push(command); + return JSON.stringify({ ok: true, data: { called: true } }); + }, + }); + + return { + commands, + execute: (input: unknown) => + executor.execute({ + toolName: 'schema_tool', + input: input as Record, + dataFile: '/tmp/demo.json', + }), + }; +} + +async function expectValidationError( + run: () => Promise, +): Promise { + let caught: unknown; + try { + await run(); + } catch (error) { + caught = error; + } + + expect(caught).toBeInstanceOf(ToolInputValidationError); + return caught as ToolInputValidationError; +} + +describe('tool input validation', () => { + it('passes valid input through unchanged', async () => { + const harness = createHarness({ + type: 'object', + properties: { + id: { type: 'string' }, + limit: { type: 'integer', minimum: 1, maximum: 10 }, + }, + required: ['id'], + additionalProperties: false, + }); + + const result = await harness.execute({ id: 'main', limit: 5 }); + + expect(result).toEqual({ ok: true, data: { called: true } }); + expect(harness.commands).toEqual([['schema-tool']]); + }); + + it('accepts an empty object for a tool without required fields', async () => { + const harness = createHarness({ + type: 'object', + properties: { id: { type: 'string' } }, + additionalProperties: false, + }); + + await expect(harness.execute({})).resolves.toEqual({ + ok: true, + data: { called: true }, + }); + }); + + it('rejects missing required properties before dispatch', async () => { + const harness = createHarness({ + type: 'object', + properties: { id: { type: 'string' } }, + required: ['id'], + additionalProperties: false, + }); + + const error = await expectValidationError(() => harness.execute({})); + + expect(error.toolName).toBe('schema_tool'); + expect(error.issues).toEqual([ + { path: 'id', message: 'input is missing required property "id"' }, + ]); + expect(error.message).toBe( + 'Invalid input for rsdoctor tool schema_tool: input is missing required property "id"', + ); + expect(harness.commands).toEqual([]); + }); + + it('rejects wrong primitive types', async () => { + const harness = createHarness({ + type: 'object', + properties: { limit: { type: 'integer' } }, + additionalProperties: false, + }); + + const error = await expectValidationError(() => + harness.execute({ limit: '10' }), + ); + + expect(error.issues).toEqual([ + { + path: 'limit', + message: '"limit" must be of type integer, received string', + }, + ]); + expect(harness.commands).toEqual([]); + }); + + it('rejects values outside declared numeric bounds', async () => { + const harness = createHarness({ + type: 'object', + properties: { limit: { type: 'integer', minimum: 1, maximum: 10 } }, + additionalProperties: false, + }); + + const error = await expectValidationError(() => + harness.execute({ limit: 99 }), + ); + + expect(error.issues).toEqual([ + { path: 'limit', message: '"limit" must be <= 10, received 99' }, + ]); + }); + + it('rejects unknown enum values', async () => { + const harness = createHarness({ + type: 'object', + properties: { + category: { type: 'string', enum: ['cjs', 'barrel'] }, + }, + additionalProperties: false, + }); + + const error = await expectValidationError(() => + harness.execute({ category: 'esm' }), + ); + + expect(error.issues).toEqual([ + { + path: 'category', + message: '"category" must be one of "cjs", "barrel", received "esm"', + }, + ]); + }); + + it('rejects wrong array item types', async () => { + const harness = createHarness({ + type: 'object', + properties: { + fields: { type: 'array', items: { type: 'string' } }, + }, + additionalProperties: false, + }); + + const error = await expectValidationError(() => + harness.execute({ fields: ['id', 2] }), + ); + + expect(error.issues).toEqual([ + { + path: 'fields[1]', + message: '"fields[1]" must be of type string, received number', + }, + ]); + }); + + it('rejects non-object input where an object is required', async () => { + const harness = createHarness({ + type: 'object', + properties: {}, + additionalProperties: true, + }); + + const undefinedInput = await expectValidationError(() => + harness.execute(undefined), + ); + expect(undefinedInput.issues).toEqual([ + { path: '', message: 'input must be of type object, received undefined' }, + ]); + + const arrayInput = await expectValidationError(() => harness.execute([])); + expect(arrayInput.issues).toEqual([ + { path: '', message: 'input must be of type object, received array' }, + ]); + + expect(harness.commands).toEqual([]); + }); + + it('rejects unknown properties only when the schema forbids them', async () => { + const strict = createHarness({ + type: 'object', + properties: {}, + additionalProperties: false, + }); + + const error = await expectValidationError(() => + strict.execute({ nope: 1 }), + ); + expect(error.issues).toEqual([ + { path: 'nope', message: '"nope" is not an allowed property' }, + ]); + + const open = createHarness({ + type: 'object', + properties: {}, + additionalProperties: true, + }); + await expect(open.execute({ nope: 1 })).resolves.toEqual({ + ok: true, + data: { called: true }, + }); + }); + + it('reports every mismatch in a single error', async () => { + const harness = createHarness({ + type: 'object', + properties: { + id: { type: 'string' }, + limit: { type: 'integer', minimum: 1 }, + }, + required: ['id'], + additionalProperties: false, + }); + + const error = await expectValidationError(() => + harness.execute({ limit: 0, extra: true }), + ); + + expect(error.issues.map((issue) => issue.path)).toEqual([ + 'id', + 'limit', + 'extra', + ]); + }); + + it('validates catalog controls for the spawned cli executor', async () => { + const commands: string[][] = []; + const executor = createRsdoctorCliToolExecutor({ + tools: getToolCatalog(), + runCommand: async (command) => { + commands.push(command); + return JSON.stringify({ ok: true, data: {} }); + }, + }); + + const error = await expectValidationError(() => + executor.execute({ + toolName: 'chunks_list', + input: { page: 'two', pageSize: 5000 }, + dataFile: '/tmp/demo.json', + }), + ); + + expect(error.toolName).toBe('chunks_list'); + expect(error.issues).toEqual([ + { + path: 'page', + message: '"page" must be of type integer, received string', + }, + { + path: 'pageSize', + message: '"pageSize" must be <= 1000, received 5000', + }, + ]); + expect(commands).toEqual([]); + }); + + it('validates catalog controls for the in-process executor', async () => { + const executor = createInProcessRsdoctorCliToolExecutor(); + + const error = await expectValidationError(() => + executor.execute({ + toolName: 'build_summary', + input: { page: 'two' }, + dataFile: '/nonexistent/rsdoctor-data.json', + }), + ); + + expect(error.toolName).toBe('build_summary'); + expect(error.issues).toEqual([ + { + path: 'page', + message: '"page" must be of type integer, received string', + }, + ]); + }); + + it('keeps rejecting unknown tools before validating input', async () => { + const executor = createInProcessRsdoctorCliToolExecutor(); + + await expect( + executor.execute({ + toolName: 'not_a_tool', + input: undefined as unknown as Record, + dataFile: '/tmp/demo.json', + }), + ).rejects.toThrow('Unknown rsdoctor tool: not_a_tool'); + }); +}); From 054226adb2103f44ca04a964af0cba2679f6b486 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Sat, 15 Aug 2026 01:09:40 +0000 Subject: [PATCH 2/3] fix(agent-cli): reject unknown tool input properties --- packages/agent-cli/src/commands/router.ts | 74 +++++--- .../agent-cli/src/core/result-controls.ts | 8 +- packages/agent-cli/src/core/validate-input.ts | 163 +++++++++++++++--- packages/agent-cli/src/executor.ts | 9 +- .../tests/tool-input-validation.test.ts | 162 ++++++++++++++++- 5 files changed, 356 insertions(+), 60 deletions(-) diff --git a/packages/agent-cli/src/commands/router.ts b/packages/agent-cli/src/commands/router.ts index 0ebcf5d6a..bd5bb1864 100644 --- a/packages/agent-cli/src/commands/router.ts +++ b/packages/agent-cli/src/commands/router.ts @@ -547,6 +547,7 @@ interface ToolCatalogEntry { inputSchema: { type: 'object'; properties: Record; + required?: string[]; additionalProperties: boolean; }; buildCommand: (context: { @@ -605,29 +606,6 @@ function appendToolSpecificOptions( return nextCommand; } -const toolInputSchema = { - type: 'object' as const, - properties: { - filter: { - type: ['string', 'array'], - description: - 'Optional field filter. Supports comma-separated paths like "items.id,items.name".', - }, - page: { - type: 'integer', - minimum: 1, - description: 'Optional page number for response pagination.', - }, - pageSize: { - type: 'integer', - minimum: 1, - maximum: 1000, - description: 'Optional page size for response pagination.', - }, - } as Record, - additionalProperties: true, -}; - export function getToolCatalog(): ToolCatalogEntry[] { const tools: ToolCatalogEntry[] = []; for (const [group, subcommands] of Object.entries(SUBCOMMANDS)) { @@ -636,7 +614,7 @@ export function getToolCatalog(): ToolCatalogEntry[] { tools.push({ name: def.toolName, description: def.toolDescription ?? def.description, - inputSchema: toolInputSchema, + inputSchema: buildToolInputSchema(def.options), sourcePagination: getSourcePaginationConfig(def.options), buildCommand: ({ dataFile, input }) => appendToolSpecificOptions( @@ -679,7 +657,7 @@ export function describeRunSubcommands(): Array<{ toolName: def.toolName, path: `${group}.${subcommand}`, description: def.toolDescription ?? def.description, - args: toolInputSchema as Record, + args: buildToolInputSchema(def.options) as Record, }); } } @@ -696,7 +674,7 @@ export function getInProcessToolExecutors(): Record< for (const def of Object.values(subcommands)) { if (!def.toolName) continue; tools[def.toolName] = { - inputSchema: toolInputSchema, + inputSchema: buildToolInputSchema(def.options), sourcePagination: getSourcePaginationConfig(def.options), execute: async ({ dataFile, input }) => { setDataFilePath(dataFile); @@ -741,6 +719,50 @@ function buildInputSchema(options: OptionDef[]): Record { return schema; } +function buildToolInputSchema( + options: OptionDef[], +): ToolCatalogEntry['inputSchema'] { + const properties: Record = { + filter: { + type: ['string', 'array'], + description: + 'Optional field filter. Supports comma-separated paths like "items.id,items.name".', + }, + page: { + type: 'integer', + minimum: 1, + description: 'Optional page number for response pagination.', + }, + pageSize: { + type: 'integer', + minimum: 1, + maximum: 1000, + description: 'Optional page size for response pagination.', + }, + }; + const required: string[] = []; + + for (const option of options) { + const name = option.name.replace(/^--/, ''); + if (name === 'page-number' || name === 'page-size') { + continue; + } + if (properties[name] === undefined) { + properties[name] = optionToJsonSchema(option); + } + if (option.required) { + required.push(name); + } + } + + return { + type: 'object', + properties, + ...(required.length > 0 ? { required } : {}), + additionalProperties: false, + }; +} + export function describeCommandSchema(commandPath: string): unknown { const [group, subcommand] = commandPath.split('.'); if (!group || !subcommand) { diff --git a/packages/agent-cli/src/core/result-controls.ts b/packages/agent-cli/src/core/result-controls.ts index 5dc15bf16..d97848550 100644 --- a/packages/agent-cli/src/core/result-controls.ts +++ b/packages/agent-cli/src/core/result-controls.ts @@ -10,7 +10,11 @@ interface ParsedControls { paginateResult: boolean; } -const CONTROL_KEYS = new Set(['filter', 'page', 'pageSize']); +export const TOOL_INPUT_CONTROL_KEYS: ReadonlySet = new Set([ + 'filter', + 'page', + 'pageSize', +]); function parsePositiveInteger( value: unknown, @@ -231,7 +235,7 @@ export function splitToolInputControls( const passthroughInput: Record = {}; for (const [key, value] of Object.entries(input)) { - if (CONTROL_KEYS.has(key)) { + if (TOOL_INPUT_CONTROL_KEYS.has(key)) { continue; } if ( diff --git a/packages/agent-cli/src/core/validate-input.ts b/packages/agent-cli/src/core/validate-input.ts index db1688840..8c55661ec 100644 --- a/packages/agent-cli/src/core/validate-input.ts +++ b/packages/agent-cli/src/core/validate-input.ts @@ -3,13 +3,12 @@ import type { JsonSchema } from './types'; /** * Tool input validation. * - * The supported schema dialect is exactly what the tool catalog emits from - * `OptionDef` (see `optionToJsonSchema` / `buildInputSchema` in - * `commands/router.ts`): an object schema carrying `properties`, an optional - * `required` list, `additionalProperties`, and per-property `type` (a single - * name or a list of names), `enum`, `minimum`, and `maximum`. `items` is - * handled as well so array properties declared by hand-written - * `ToolDefinition`s are checked too. + * The supported schema dialect covers the shared controls and `OptionDef` + * properties emitted by the tool catalog: an object schema carrying + * `properties`, an optional `required` list, `additionalProperties`, and + * per-property `type` (a single name or a list of names), `enum`, `minimum`, + * and `maximum`. `items` is handled as well so array properties declared by + * hand-written `ToolDefinition`s are checked too. * * Two deliberate choices keep the executor from being stricter than the CLI it * mirrors: @@ -17,11 +16,8 @@ import type { JsonSchema } from './types'; * - Unknown keywords are ignored instead of rejected, so a richer schema never * fails closed. * - Extra properties are accepted unless a schema explicitly declares - * `additionalProperties: false`. The catalog declares - * `additionalProperties: true` because `parseSubcommandOptions` and - * `appendToolSpecificOptions` silently drop options a subcommand does not - * declare, so rejecting unknown keys here would refuse input the CLI itself - * accepts. + * `additionalProperties: false`. Catalog schemas are strict so misspelled + * programmatic inputs are rejected before dispatch. */ export interface ToolInputValidationIssue { /** Dotted path to the offending value, empty for the input object itself. */ @@ -56,6 +52,17 @@ function isRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value); } +function parseFiniteNumber(value: unknown): number | undefined { + if (typeof value === 'number') { + return Number.isFinite(value) ? value : undefined; + } + if (typeof value !== 'string' || value.trim() === '') { + return undefined; + } + const parsed = Number(value); + return Number.isFinite(parsed) ? parsed : undefined; +} + function matchesSchemaType(value: unknown, type: unknown): boolean { if (Array.isArray(type)) { return type.some((entry) => matchesSchemaType(value, entry)); @@ -66,12 +73,14 @@ function matchesSchemaType(value: unknown, type: unknown): boolean { return Array.isArray(value); case 'boolean': return typeof value === 'boolean'; - case 'integer': - return typeof value === 'number' && Number.isInteger(value); + case 'integer': { + const parsed = parseFiniteNumber(value); + return parsed !== undefined && Number.isInteger(parsed); + } case 'null': return value === null; case 'number': - return typeof value === 'number' && Number.isFinite(value); + return parseFiniteNumber(value) !== undefined; case 'object': return isRecord(value); case 'string': @@ -96,7 +105,67 @@ function describeValueType(value: unknown): string { } function formatValue(value: unknown): string { - return typeof value === 'string' ? JSON.stringify(value) : String(value); + if (typeof value === 'string') { + return JSON.stringify(value); + } + if (value !== null && typeof value === 'object') { + try { + return JSON.stringify(value); + } catch { + return String(value); + } + } + return String(value); +} + +function jsonValuesEqual( + left: unknown, + right: unknown, + seen = new WeakMap(), +): boolean { + if (left === right) { + return true; + } + + if (Array.isArray(left) || Array.isArray(right)) { + if (!Array.isArray(left) || !Array.isArray(right)) { + return false; + } + const previous = seen.get(left); + if (previous !== undefined) { + return previous === right; + } + seen.set(left, right); + return ( + left.length === right.length && + left.every((entry, index) => jsonValuesEqual(entry, right[index], seen)) + ); + } + + if (!isRecord(left) || !isRecord(right)) { + return false; + } + const previous = seen.get(left); + if (previous !== undefined) { + return previous === right; + } + seen.set(left, right); + const leftKeys = Object.keys(left); + const rightKeys = Object.keys(right); + return ( + leftKeys.length === rightKeys.length && + leftKeys.every( + (key) => + Object.prototype.hasOwnProperty.call(right, key) && + jsonValuesEqual(left[key], right[key], seen), + ) + ); +} + +function schemaHasNumericType(type: unknown): boolean { + return Array.isArray(type) + ? type.includes('integer') || type.includes('number') + : type === 'integer' || type === 'number'; } function describeLabel(path: string): string { @@ -112,6 +181,7 @@ function collectIssues( schema: unknown, path: string, issues: ToolInputValidationIssue[], + options: ToolInputValidationOptions, ): void { if (!isRecord(schema)) { return; @@ -129,7 +199,18 @@ function collectIssues( return; } - if (Array.isArray(schema.enum) && !schema.enum.includes(value)) { + const numericValue = + typeof value === 'number' || schemaHasNumericType(schema.type) + ? parseFiniteNumber(value) + : undefined; + if ( + Array.isArray(schema.enum) && + !schema.enum.some( + (entry) => + jsonValuesEqual(entry, value) || + (numericValue !== undefined && jsonValuesEqual(entry, numericValue)), + ) + ) { issues.push({ path, message: `${label} must be one of ${schema.enum @@ -138,17 +219,17 @@ function collectIssues( }); } - if (typeof value === 'number') { - if (typeof schema.minimum === 'number' && value < schema.minimum) { + if (numericValue !== undefined) { + if (typeof schema.minimum === 'number' && numericValue < schema.minimum) { issues.push({ path, - message: `${label} must be >= ${schema.minimum}, received ${value}`, + message: `${label} must be >= ${schema.minimum}, received ${formatValue(value)}`, }); } - if (typeof schema.maximum === 'number' && value > schema.maximum) { + if (typeof schema.maximum === 'number' && numericValue > schema.maximum) { issues.push({ path, - message: `${label} must be <= ${schema.maximum}, received ${value}`, + message: `${label} must be <= ${schema.maximum}, received ${formatValue(value)}`, }); } } @@ -156,7 +237,13 @@ function collectIssues( if (Array.isArray(value)) { if (isRecord(schema.items)) { value.forEach((entry, index) => { - collectIssues(entry, schema.items, `${path}[${index}]`, issues); + collectIssues( + entry, + schema.items, + `${path}[${index}]`, + issues, + options, + ); }); } return; @@ -170,7 +257,10 @@ function collectIssues( if (Array.isArray(schema.required)) { for (const key of schema.required) { - if (typeof key === 'string' && !(key in value)) { + if ( + typeof key === 'string' && + (!(key in value) || value[key] === undefined) + ) { issues.push({ path: joinPath(path, key), message: `${label} is missing required property "${key}"`, @@ -180,15 +270,20 @@ function collectIssues( } for (const [key, entry] of Object.entries(value)) { + if (entry === undefined) { + continue; + } const propertyPath = joinPath(path, key); const propertySchema = properties[key]; if (propertySchema !== undefined) { - collectIssues(entry, propertySchema, propertyPath, issues); + collectIssues(entry, propertySchema, propertyPath, issues, options); continue; } - if (schema.additionalProperties === false) { + const isAllowedExecutorProperty = + path === '' && options.allowedAdditionalProperties?.has(key) === true; + if (schema.additionalProperties === false && !isAllowedExecutorProperty) { issues.push({ path: propertyPath, message: `${describeLabel(propertyPath)} is not an allowed property`, @@ -197,11 +292,22 @@ function collectIssues( } if (isRecord(schema.additionalProperties)) { - collectIssues(entry, schema.additionalProperties, propertyPath, issues); + collectIssues( + entry, + schema.additionalProperties, + propertyPath, + issues, + options, + ); } } } +export interface ToolInputValidationOptions { + /** Executor-level properties accepted in addition to a strict tool schema. */ + allowedAdditionalProperties?: ReadonlySet; +} + /** * Validates `input` against a tool's declared `inputSchema` and throws a * {@link ToolInputValidationError} listing every mismatch. @@ -215,13 +321,14 @@ export function validateToolInput( toolName: string, input: unknown, schema: JsonSchema | undefined, + options: ToolInputValidationOptions = {}, ): void { if (schema === undefined) { return; } const issues: ToolInputValidationIssue[] = []; - collectIssues(input, schema, '', issues); + collectIssues(input, schema, '', issues, options); if (issues.length > 0) { throw new ToolInputValidationError(toolName, issues); diff --git a/packages/agent-cli/src/executor.ts b/packages/agent-cli/src/executor.ts index fdc499555..642697a61 100644 --- a/packages/agent-cli/src/executor.ts +++ b/packages/agent-cli/src/executor.ts @@ -9,6 +9,7 @@ import type { import { applyToolResultControls, splitToolInputControls, + TOOL_INPUT_CONTROL_KEYS, } from './core/result-controls'; import { validateToolInput } from './core/validate-input'; import { getInProcessToolExecutors } from './commands'; @@ -45,7 +46,9 @@ export function createRsdoctorCliToolExecutor({ return { async execute(request: ToolExecutionRequest): Promise { const tool = getToolByName(tools, request.toolName); - validateToolInput(request.toolName, request.input, tool.inputSchema); + validateToolInput(request.toolName, request.input, tool.inputSchema, { + allowedAdditionalProperties: TOOL_INPUT_CONTROL_KEYS, + }); const { controls, passthroughInput, paginateResult } = splitToolInputControls(request.input, { sourcePagination: tool.sourcePagination, @@ -81,7 +84,9 @@ export function createInProcessRsdoctorCliToolExecutor(): ToolExecutor { if (!tool) { throw new Error(`Unknown rsdoctor tool: ${request.toolName}`); } - validateToolInput(request.toolName, request.input, tool.inputSchema); + validateToolInput(request.toolName, request.input, tool.inputSchema, { + allowedAdditionalProperties: TOOL_INPUT_CONTROL_KEYS, + }); const { controls, passthroughInput, paginateResult } = splitToolInputControls(request.input, { sourcePagination: tool.sourcePagination, diff --git a/packages/agent-cli/tests/tool-input-validation.test.ts b/packages/agent-cli/tests/tool-input-validation.test.ts index fd376ca6b..c142b7c3d 100644 --- a/packages/agent-cli/tests/tool-input-validation.test.ts +++ b/packages/agent-cli/tests/tool-input-validation.test.ts @@ -106,7 +106,7 @@ describe('tool input validation', () => { expect(harness.commands).toEqual([]); }); - it('rejects wrong primitive types', async () => { + it('rejects non-numeric strings for numeric types', async () => { const harness = createHarness({ type: 'object', properties: { limit: { type: 'integer' } }, @@ -114,7 +114,7 @@ describe('tool input validation', () => { }); const error = await expectValidationError(() => - harness.execute({ limit: '10' }), + harness.execute({ limit: 'ten' }), ); expect(error.issues).toEqual([ @@ -126,6 +126,30 @@ describe('tool input validation', () => { expect(harness.commands).toEqual([]); }); + it('accepts numeric strings and applies numeric bounds to them', async () => { + const harness = createHarness({ + type: 'object', + properties: { + count: { type: 'integer', minimum: 1, maximum: 10 }, + ratio: { type: 'number', minimum: 0, maximum: 1 }, + choice: { type: 'integer', enum: [1, 2] }, + }, + additionalProperties: false, + }); + + await expect( + harness.execute({ count: '2', ratio: '0.5', choice: '2' }), + ).resolves.toEqual({ ok: true, data: { called: true } }); + + const error = await expectValidationError(() => + harness.execute({ count: '11', ratio: '-0.1' }), + ); + expect(error.issues).toEqual([ + { path: 'count', message: '"count" must be <= 10, received "11"' }, + { path: 'ratio', message: '"ratio" must be >= 0, received "-0.1"' }, + ]); + }); + it('rejects values outside declared numeric bounds', async () => { const harness = createHarness({ type: 'object', @@ -163,6 +187,35 @@ describe('tool input validation', () => { ]); }); + it('compares structured enum values by JSON value', async () => { + const harness = createHarness({ + type: 'object', + properties: { + config: { type: 'object', enum: [{ mode: 'fast' }] }, + }, + additionalProperties: false, + }); + + await expect( + harness.execute({ config: { mode: 'fast' } }), + ).resolves.toEqual({ ok: true, data: { called: true } }); + }); + + it('preserves string enum values in numeric union types', async () => { + const harness = createHarness({ + type: 'object', + properties: { + choice: { type: ['string', 'integer'], enum: ['2', 3] }, + }, + additionalProperties: false, + }); + + await expect(harness.execute({ choice: '2' })).resolves.toEqual({ + ok: true, + data: { called: true }, + }); + }); + it('rejects wrong array item types', async () => { const harness = createHarness({ type: 'object', @@ -231,6 +284,38 @@ describe('tool input validation', () => { }); }); + it('treats undefined required properties as missing', async () => { + const harness = createHarness({ + type: 'object', + properties: { id: { type: 'string' } }, + required: ['id'], + additionalProperties: false, + }); + + const error = await expectValidationError(() => + harness.execute({ id: undefined }), + ); + + expect(error.issues).toEqual([ + { path: 'id', message: 'input is missing required property "id"' }, + ]); + expect(harness.commands).toEqual([]); + }); + + it('allows executor controls alongside a strict custom tool schema', async () => { + const harness = createHarness({ + type: 'object', + properties: { id: { type: 'string' } }, + required: ['id'], + additionalProperties: false, + }); + + await expect( + harness.execute({ id: 'main', filter: '', page: '1', pageSize: '2' }), + ).resolves.toEqual({ ok: true, data: { called: true } }); + expect(harness.commands).toEqual([['schema-tool']]); + }); + it('reports every mismatch in a single error', async () => { const harness = createHarness({ type: 'object', @@ -285,6 +370,60 @@ describe('tool input validation', () => { expect(commands).toEqual([]); }); + it('rejects unknown catalog properties instead of silently ignoring them', async () => { + const commands: string[][] = []; + const executor = createRsdoctorCliToolExecutor({ + tools: getToolCatalog(), + runCommand: async (command) => { + commands.push(command); + return JSON.stringify({ ok: true, data: {} }); + }, + }); + + const error = await expectValidationError(() => + executor.execute({ + toolName: 'chunks_list', + input: { pageNumber: 2 }, + dataFile: '/tmp/demo.json', + }), + ); + + expect(error.issues).toEqual([ + { + path: 'pageNumber', + message: '"pageNumber" is not an allowed property', + }, + ]); + expect(commands).toEqual([]); + }); + + it('catalog schemas retain declared tool options while rejecting extras', () => { + const catalog = getToolCatalog(); + const optimize = catalog.find((tool) => tool.name === 'bundle_optimize'); + const sideEffects = catalog.find( + (tool) => tool.name === 'tree_shaking_side_effects', + ); + + expect(optimize?.inputSchema).toMatchObject({ + properties: { + filter: expect.any(Object), + page: expect.any(Object), + pageSize: expect.any(Object), + step: { type: 'integer', enum: [1, 2] }, + }, + additionalProperties: false, + }); + expect(sideEffects?.inputSchema).toMatchObject({ + properties: { + category: { + type: 'string', + enum: ['cjs', 'barrel', 'side-effects', 'dynamic-import'], + }, + }, + additionalProperties: false, + }); + }); + it('validates catalog controls for the in-process executor', async () => { const executor = createInProcessRsdoctorCliToolExecutor(); @@ -305,6 +444,25 @@ describe('tool input validation', () => { ]); }); + it('rejects unknown catalog properties in the in-process executor', async () => { + const executor = createInProcessRsdoctorCliToolExecutor(); + + const error = await expectValidationError(() => + executor.execute({ + toolName: 'build_summary', + input: { pageNumber: 2 }, + dataFile: '/nonexistent/rsdoctor-data.json', + }), + ); + + expect(error.issues).toEqual([ + { + path: 'pageNumber', + message: '"pageNumber" is not an allowed property', + }, + ]); + }); + it('keeps rejecting unknown tools before validating input', async () => { const executor = createInProcessRsdoctorCliToolExecutor(); From c74c9f23426c03a9f447edddf23267ea2281b121 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Wed, 26 Aug 2026 23:02:46 +0000 Subject: [PATCH 3/3] refactor(agent-cli): validate tool input with ajv --- packages/agent-cli/package.json | 3 + packages/agent-cli/src/core/validate-input.ts | 397 ++++++------------ .../tests/tool-input-validation.test.ts | 55 ++- pnpm-lock.yaml | 4 + 4 files changed, 176 insertions(+), 283 deletions(-) diff --git a/packages/agent-cli/package.json b/packages/agent-cli/package.json index fd1828b5f..38b7f65f2 100644 --- a/packages/agent-cli/package.json +++ b/packages/agent-cli/package.json @@ -37,5 +37,8 @@ "publishConfig": { "access": "public", "registry": "https://registry.npmjs.org/" + }, + "dependencies": { + "ajv": "^8.20.0" } } diff --git a/packages/agent-cli/src/core/validate-input.ts b/packages/agent-cli/src/core/validate-input.ts index 8c55661ec..055f97500 100644 --- a/packages/agent-cli/src/core/validate-input.ts +++ b/packages/agent-cli/src/core/validate-input.ts @@ -1,37 +1,12 @@ +import Ajv, { type ErrorObject, type ValidateFunction } from 'ajv'; + import type { JsonSchema } from './types'; -/** - * Tool input validation. - * - * The supported schema dialect covers the shared controls and `OptionDef` - * properties emitted by the tool catalog: an object schema carrying - * `properties`, an optional `required` list, `additionalProperties`, and - * per-property `type` (a single name or a list of names), `enum`, `minimum`, - * and `maximum`. `items` is handled as well so array properties declared by - * hand-written `ToolDefinition`s are checked too. - * - * Two deliberate choices keep the executor from being stricter than the CLI it - * mirrors: - * - * - Unknown keywords are ignored instead of rejected, so a richer schema never - * fails closed. - * - Extra properties are accepted unless a schema explicitly declares - * `additionalProperties: false`. Catalog schemas are strict so misspelled - * programmatic inputs are rejected before dispatch. - */ export interface ToolInputValidationIssue { - /** Dotted path to the offending value, empty for the input object itself. */ path: string; message: string; } -/** - * Thrown before dispatch when a tool input does not match the tool's declared - * `inputSchema`. Throwing matches how the executor already reports pre-dispatch - * failures (unknown tool, unparsable control values), so the CLI keeps wrapping - * it in the usual `{ ok: false, error }` envelope, while `issues` exposes the - * structured detail. - */ export class ToolInputValidationError extends Error { readonly toolName: string; readonly issues: ToolInputValidationIssue[]; @@ -48,289 +23,173 @@ export class ToolInputValidationError extends Error { } } -function isRecord(value: unknown): value is Record { - return typeof value === 'object' && value !== null && !Array.isArray(value); +export interface ToolInputValidationOptions { + allowedAdditionalProperties?: ReadonlySet; } -function parseFiniteNumber(value: unknown): number | undefined { - if (typeof value === 'number') { - return Number.isFinite(value) ? value : undefined; +const ajv = new Ajv({ + allErrors: true, + allowUnionTypes: true, + strict: false, +}); + +const validatorCache = new WeakMap< + JsonSchema, + Map | undefined, ValidateFunction> +>(); + +function buildValidationSchema( + schema: JsonSchema, + allowedAdditionalProperties?: ReadonlySet, +): JsonSchema { + if ( + schema.additionalProperties !== false || + allowedAdditionalProperties === undefined + ) { + return schema; } - if (typeof value !== 'string' || value.trim() === '') { - return undefined; + + const properties = { ...schema.properties }; + for (const property of allowedAdditionalProperties) { + properties[property] ??= {}; } - const parsed = Number(value); - return Number.isFinite(parsed) ? parsed : undefined; + + return { ...schema, properties }; } -function matchesSchemaType(value: unknown, type: unknown): boolean { - if (Array.isArray(type)) { - return type.some((entry) => matchesSchemaType(value, entry)); +function getValidator( + schema: JsonSchema, + allowedAdditionalProperties?: ReadonlySet, +): ValidateFunction { + let validators = validatorCache.get(schema); + if (validators === undefined) { + validators = new Map(); + validatorCache.set(schema, validators); } - switch (type) { - case 'array': - return Array.isArray(value); - case 'boolean': - return typeof value === 'boolean'; - case 'integer': { - const parsed = parseFiniteNumber(value); - return parsed !== undefined && Number.isInteger(parsed); - } - case 'null': - return value === null; - case 'number': - return parseFiniteNumber(value) !== undefined; - case 'object': - return isRecord(value); - case 'string': - return typeof value === 'string'; - default: - return true; + let validate = validators.get(allowedAdditionalProperties); + if (validate === undefined) { + validate = ajv.compile( + buildValidationSchema(schema, allowedAdditionalProperties), + ); + validators.set(allowedAdditionalProperties, validate); } + return validate; } -function describeSchemaType(type: unknown): string { - return Array.isArray(type) ? type.map(String).join(' or ') : String(type); -} - -function describeValueType(value: unknown): string { - if (value === null) { - return 'null'; - } - if (Array.isArray(value)) { - return 'array'; - } - return typeof value; +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); } -function formatValue(value: unknown): string { - if (typeof value === 'string') { - return JSON.stringify(value); - } - if (value !== null && typeof value === 'object') { - try { - return JSON.stringify(value); - } catch { - return String(value); +function normalizeNumericStrings(value: unknown, schema: unknown): unknown { + if (!isRecord(schema)) return value; + + const types = Array.isArray(schema.type) ? schema.type : [schema.type]; + const acceptsString = types.includes('string'); + if (typeof value === 'string' && !acceptsString && value.trim() !== '') { + const parsed = Number(value); + if ( + Number.isFinite(parsed) && + (types.includes('number') || + (types.includes('integer') && Number.isInteger(parsed))) + ) { + return parsed; } } - return String(value); -} -function jsonValuesEqual( - left: unknown, - right: unknown, - seen = new WeakMap(), -): boolean { - if (left === right) { - return true; + if (Array.isArray(value)) { + return value.map((item) => normalizeNumericStrings(item, schema.items)); } - if (Array.isArray(left) || Array.isArray(right)) { - if (!Array.isArray(left) || !Array.isArray(right)) { - return false; - } - const previous = seen.get(left); - if (previous !== undefined) { - return previous === right; - } - seen.set(left, right); - return ( - left.length === right.length && - left.every((entry, index) => jsonValuesEqual(entry, right[index], seen)) - ); - } + if (!isRecord(value)) return value; - if (!isRecord(left) || !isRecord(right)) { - return false; - } - const previous = seen.get(left); - if (previous !== undefined) { - return previous === right; - } - seen.set(left, right); - const leftKeys = Object.keys(left); - const rightKeys = Object.keys(right); - return ( - leftKeys.length === rightKeys.length && - leftKeys.every( - (key) => - Object.prototype.hasOwnProperty.call(right, key) && - jsonValuesEqual(left[key], right[key], seen), - ) + const properties = isRecord(schema.properties) ? schema.properties : {}; + const additionalProperties = isRecord(schema.additionalProperties) + ? schema.additionalProperties + : undefined; + + return Object.fromEntries( + Object.entries(value).map(([key, item]) => [ + key, + normalizeNumericStrings(item, properties[key] ?? additionalProperties), + ]), ); } -function schemaHasNumericType(type: unknown): boolean { - return Array.isArray(type) - ? type.includes('integer') || type.includes('number') - : type === 'integer' || type === 'number'; +function decodePointerSegment(segment: string): string { + return segment.replaceAll('~1', '/').replaceAll('~0', '~'); } -function describeLabel(path: string): string { - return path === '' ? 'input' : `"${path}"`; +function getPathSegments(instancePath: string): string[] { + return instancePath + .split('/') + .slice(1) + .map((segment) => decodePointerSegment(segment)); } -function joinPath(path: string, key: string): string { - return path === '' ? key : `${path}.${key}`; +function formatPath(segments: string[]): string { + return segments.reduce( + (path, segment) => + /^\d+$/.test(segment) + ? `${path}[${segment}]` + : path === '' + ? segment + : `${path}.${segment}`, + '', + ); } -function collectIssues( - value: unknown, - schema: unknown, - path: string, - issues: ToolInputValidationIssue[], - options: ToolInputValidationOptions, -): void { - if (!isRecord(schema)) { - return; - } +function describeLabel(path: string): string { + return path === '' ? 'input' : `"${path}"`; +} - const label = describeLabel(path); +function formatIssue(error: ErrorObject): ToolInputValidationIssue { + const segments = getPathSegments(error.instancePath); + const basePath = formatPath(segments); - if (schema.type !== undefined && !matchesSchemaType(value, schema.type)) { - issues.push({ - path, - message: `${label} must be of type ${describeSchemaType( - schema.type, - )}, received ${describeValueType(value)}`, - }); - return; + if (error.keyword === 'required') { + return { + path: formatPath([...segments, String(error.params.missingProperty)]), + message: `${describeLabel(basePath)} ${error.message ?? 'is invalid'}`, + }; } - const numericValue = - typeof value === 'number' || schemaHasNumericType(schema.type) - ? parseFiniteNumber(value) - : undefined; - if ( - Array.isArray(schema.enum) && - !schema.enum.some( - (entry) => - jsonValuesEqual(entry, value) || - (numericValue !== undefined && jsonValuesEqual(entry, numericValue)), - ) - ) { - issues.push({ + if (error.keyword === 'additionalProperties') { + const path = formatPath([ + ...segments, + String(error.params.additionalProperty), + ]); + return { path, - message: `${label} must be one of ${schema.enum - .map(formatValue) - .join(', ')}, received ${formatValue(value)}`, - }); - } - - if (numericValue !== undefined) { - if (typeof schema.minimum === 'number' && numericValue < schema.minimum) { - issues.push({ - path, - message: `${label} must be >= ${schema.minimum}, received ${formatValue(value)}`, - }); - } - if (typeof schema.maximum === 'number' && numericValue > schema.maximum) { - issues.push({ - path, - message: `${label} must be <= ${schema.maximum}, received ${formatValue(value)}`, - }); - } - } - - if (Array.isArray(value)) { - if (isRecord(schema.items)) { - value.forEach((entry, index) => { - collectIssues( - entry, - schema.items, - `${path}[${index}]`, - issues, - options, - ); - }); - } - return; - } - - if (!isRecord(value)) { - return; - } - - const properties = isRecord(schema.properties) ? schema.properties : {}; - - if (Array.isArray(schema.required)) { - for (const key of schema.required) { - if ( - typeof key === 'string' && - (!(key in value) || value[key] === undefined) - ) { - issues.push({ - path: joinPath(path, key), - message: `${label} is missing required property "${key}"`, - }); - } - } + message: `${describeLabel(path)} is not an allowed property`, + }; } - for (const [key, entry] of Object.entries(value)) { - if (entry === undefined) { - continue; - } - const propertyPath = joinPath(path, key); - const propertySchema = properties[key]; - - if (propertySchema !== undefined) { - collectIssues(entry, propertySchema, propertyPath, issues, options); - continue; - } - - const isAllowedExecutorProperty = - path === '' && options.allowedAdditionalProperties?.has(key) === true; - if (schema.additionalProperties === false && !isAllowedExecutorProperty) { - issues.push({ - path: propertyPath, - message: `${describeLabel(propertyPath)} is not an allowed property`, - }); - continue; - } - - if (isRecord(schema.additionalProperties)) { - collectIssues( - entry, - schema.additionalProperties, - propertyPath, - issues, - options, - ); - } - } + return { + path: basePath, + message: `${describeLabel(basePath)} ${error.message ?? 'is invalid'}`, + }; } -export interface ToolInputValidationOptions { - /** Executor-level properties accepted in addition to a strict tool schema. */ - allowedAdditionalProperties?: ReadonlySet; -} - -/** - * Validates `input` against a tool's declared `inputSchema` and throws a - * {@link ToolInputValidationError} listing every mismatch. - * - * `undefined` (or any non-object) input is reported as a type mismatch rather - * than defaulted to `{}`: `ToolExecutionRequest.input` declares the field as - * required, and the executor previously crashed on a missing one. Callers that - * treat "no arguments" as valid should keep passing `{}`. - */ export function validateToolInput( toolName: string, input: unknown, schema: JsonSchema | undefined, options: ToolInputValidationOptions = {}, ): void { - if (schema === undefined) { - return; - } - - const issues: ToolInputValidationIssue[] = []; - collectIssues(input, schema, '', issues, options); - - if (issues.length > 0) { - throw new ToolInputValidationError(toolName, issues); - } + if (schema === undefined) return; + + const validate = getValidator(schema, options.allowedAdditionalProperties); + if (validate(normalizeNumericStrings(input, schema))) return; + + throw new ToolInputValidationError( + toolName, + (validate.errors ?? []) + .toSorted( + (left, right) => + Number(left.keyword === 'additionalProperties') - + Number(right.keyword === 'additionalProperties'), + ) + .map(formatIssue), + ); } diff --git a/packages/agent-cli/tests/tool-input-validation.test.ts b/packages/agent-cli/tests/tool-input-validation.test.ts index 5f879a19d..db8db0849 100644 --- a/packages/agent-cli/tests/tool-input-validation.test.ts +++ b/packages/agent-cli/tests/tool-input-validation.test.ts @@ -98,10 +98,10 @@ describe('tool input validation', () => { expect(error.toolName).toBe('schema_tool'); expect(error.issues).toEqual([ - { path: 'id', message: 'input is missing required property "id"' }, + { path: 'id', message: "input must have required property 'id'" }, ]); expect(error.message).toBe( - 'Invalid input for rsdoctor tool schema_tool: input is missing required property "id"', + "Invalid input for rsdoctor tool schema_tool: input must have required property 'id'", ); expect(harness.commands).toEqual([]); }); @@ -120,7 +120,7 @@ describe('tool input validation', () => { expect(error.issues).toEqual([ { path: 'limit', - message: '"limit" must be of type integer, received string', + message: '"limit" must be integer', }, ]); expect(harness.commands).toEqual([]); @@ -145,8 +145,8 @@ describe('tool input validation', () => { harness.execute({ count: '11', ratio: '-0.1' }), ); expect(error.issues).toEqual([ - { path: 'count', message: '"count" must be <= 10, received "11"' }, - { path: 'ratio', message: '"ratio" must be >= 0, received "-0.1"' }, + { path: 'count', message: '"count" must be <= 10' }, + { path: 'ratio', message: '"ratio" must be >= 0' }, ]); }); @@ -162,7 +162,7 @@ describe('tool input validation', () => { ); expect(error.issues).toEqual([ - { path: 'limit', message: '"limit" must be <= 10, received 99' }, + { path: 'limit', message: '"limit" must be <= 10' }, ]); }); @@ -182,11 +182,38 @@ describe('tool input validation', () => { expect(error.issues).toEqual([ { path: 'category', - message: '"category" must be one of "cjs", "barrel", received "esm"', + message: '"category" must be equal to one of the allowed values', }, ]); }); + it('enforces standard JSON Schema string constraints', async () => { + const harness = createHarness({ + type: 'object', + properties: { + id: { type: 'string', minLength: 3, pattern: '^[a-z]+$' }, + }, + required: ['id'], + additionalProperties: false, + }); + + const error = await expectValidationError(() => + harness.execute({ id: 'A' }), + ); + + expect(error.issues).toEqual([ + { + path: 'id', + message: '"id" must NOT have fewer than 3 characters', + }, + { + path: 'id', + message: '"id" must match pattern "^[a-z]+$"', + }, + ]); + expect(harness.commands).toEqual([]); + }); + it('compares structured enum values by JSON value', async () => { const harness = createHarness({ type: 'object', @@ -232,7 +259,7 @@ describe('tool input validation', () => { expect(error.issues).toEqual([ { path: 'fields[1]', - message: '"fields[1]" must be of type string, received number', + message: '"fields[1]" must be string', }, ]); }); @@ -248,12 +275,12 @@ describe('tool input validation', () => { harness.execute(undefined), ); expect(undefinedInput.issues).toEqual([ - { path: '', message: 'input must be of type object, received undefined' }, + { path: '', message: 'input must be object' }, ]); const arrayInput = await expectValidationError(() => harness.execute([])); expect(arrayInput.issues).toEqual([ - { path: '', message: 'input must be of type object, received array' }, + { path: '', message: 'input must be object' }, ]); expect(harness.commands).toEqual([]); @@ -297,7 +324,7 @@ describe('tool input validation', () => { ); expect(error.issues).toEqual([ - { path: 'id', message: 'input is missing required property "id"' }, + { path: 'id', message: "input must have required property 'id'" }, ]); expect(harness.commands).toEqual([]); }); @@ -360,11 +387,11 @@ describe('tool input validation', () => { expect(error.issues).toEqual([ { path: 'page', - message: '"page" must be of type integer, received string', + message: '"page" must be integer', }, { path: 'pageSize', - message: '"pageSize" must be <= 1000, received 5000', + message: '"pageSize" must be <= 1000', }, ]); expect(commands).toEqual([]); @@ -439,7 +466,7 @@ describe('tool input validation', () => { expect(error.issues).toEqual([ { path: 'page', - message: '"page" must be of type integer, received string', + message: '"page" must be integer', }, ]); }); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index af7bc7bc1..acf30e69a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -445,6 +445,10 @@ importers: version: 24.12.3 packages/agent-cli: + dependencies: + ajv: + specifier: ^8.20.0 + version: 8.20.0 devDependencies: '@rslib/core': specifier: 1.0.0-beta.3