From 82c24a3f74a8db90d5cdcf178600c8d5a31df1ca Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 17:47:20 +0200 Subject: [PATCH] fix(ai/schema): enforce conditional keywords, fix $ref recursion, preserve Zod root extras - JSON Schema validator now enforces propertyNames, patternProperties, dependentRequired, dependencies, if/then/else, contains, and prefixItems instead of silently accepting values that violate them; unevaluated* still permissive but warns once. - Recursive $ref no longer short-circuits to true on revisit: cycle detection keys on (ref, value-identity) with a primitive depth cap, so nested sub-schema violations are caught. - Meta-validator now structurally validates if/then/else/dependencies sub-schemas and accepts draft-07 dependencies as either schemas or string-array dependent keys. - Wire-schema null normalization no longer strips null-valued unknown root fields before preserveUnknownRootFields snapshots them, so task.simple and similar callers still see disallowed null arguments. --- packages/ai/CHANGELOG.md | 4 + .../src/utils/schema/json-schema-validator.ts | 192 ++++++++++++++++-- .../ai/src/utils/schema/meta-validator.ts | 14 ++ packages/ai/src/utils/validation.ts | 22 +- packages/ai/test/schema-strict-mode.test.ts | 131 ++++++++++++ 5 files changed, 344 insertions(+), 19 deletions(-) diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index 439424525..5d66a0357 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -39,6 +39,10 @@ - Fixed OpenAI-completions duplicate Kimi tool calls when a single chunk delivers both leaked markers and a structured `delta.tool_calls`; the healer now strips visible markers but discards its synthesized calls so structured payloads remain the single source of truth - Fixed Kimi tool-call healer synthesizing a bogus empty call when assistant text mentions a literal `<|tool_call_end|>` (or `<|tool_call_begin|>` / `<|tool_call_argument_begin|>`) outside an active `<|tool_calls_section_begin|>…<|tool_calls_section_end|>` section; the tokens now survive as text - Fixed OpenAI-completions ignoring per-request `StreamOptions.streamFirstEventTimeoutMs` when configuring the underlying OpenAI SDK HTTP timeout, causing slow-before-headers providers to be aborted at the env default before the wrapping watchdog armed +- Fixed JSON Schema validator silently accepting values that violate `propertyNames`, `patternProperties`, `dependentRequired`, `dependencies`, `if`/`then`/`else`, `contains`, and `prefixItems`; the in-tree validator now enforces these keywords instead of falling through. `unevaluatedProperties`/`unevaluatedItems` remain permissive but log a one-time warning so tool authors are not surprised. +- Fixed recursive `$ref` schemas being treated as universally valid: the validator previously short-circuited on the second occurrence of any ref it had already seen, so nested values violating the referenced sub-schema passed. Cycle detection now keys on (ref, value-identity) pairs with a depth cap for primitive values, so genuine sub-tree violations are still caught. +- Fixed JSON Schema meta-validator accepting malformed `if`/`then`/`else` and `dependencies` keywords; each conditional sub-schema is now structurally validated and draft-07 `dependencies` accepts either a schema or a string array of dependent keys. +- Fixed Zod-emitted wire schemas dropping null-valued unknown root fields before `preserveUnknownRootFields` could snapshot them, so callers like `task.simple` no longer lose a `schema: null` argument and downstream rejection paths fire as intended. ## [15.0.2] - 2026-05-15 ### Fixed diff --git a/packages/ai/src/utils/schema/json-schema-validator.ts b/packages/ai/src/utils/schema/json-schema-validator.ts index 3a75253f3..c6bf82dec 100644 --- a/packages/ai/src/utils/schema/json-schema-validator.ts +++ b/packages/ai/src/utils/schema/json-schema-validator.ts @@ -1,3 +1,4 @@ +import { logger } from "@oh-my-pi/pi-utils"; import { areJsonValuesEqual } from "./equality"; export interface JsonSchemaValidationIssue { @@ -12,9 +13,34 @@ export interface JsonSchemaValidationResult { issues: JsonSchemaValidationIssue[]; } +/** + * Cycle bookkeeping for recursive `$ref` schemas. We track pairs of (resolved + * ref, value identity) rather than refs alone: returning `true` for every + * nested occurrence of a ref previously allowed recursive schemas to silently + * validate values they should have rejected. For primitive values we fall back + * to a depth counter capped at MAX_REF_DEPTH so a self-referential schema can + * still bottom out without infinite recursion. + */ interface ValidationContext { root: unknown; - seenRefs: Set; + seenPairs: Set; + objectIds: WeakMap; + nextObjectId: { value: number }; + refDepth: number; +} + +const MAX_REF_DEPTH = 64; + +/** Module-level guard so the unevaluatedItems/unevaluatedProperties warning fires once per process. */ +let seenUnevaluatedWarning = false; + +function getValueIdentity(ctx: ValidationContext, value: object): number { + let id = ctx.objectIds.get(value); + if (id !== undefined) return id; + id = ctx.nextObjectId.value; + ctx.nextObjectId.value += 1; + ctx.objectIds.set(value, id); + return id; } function isJsonObject(value: unknown): value is Record { @@ -111,15 +137,28 @@ function validateSchemaNode( const ref = schema.$ref; if (typeof ref === "string") { - if (ctx.seenRefs.has(ref)) return true; const resolved = resolveLocalRef(ctx.root, ref); if (resolved === undefined) { pushIssue(issues, path, `unresolved reference ${ref}`, { keyword: "$ref" }); return false; } - ctx.seenRefs.add(ref); + // Cycle detection: for object/array values we key on (ref, value-identity) + // so the same schema applied to a different value still recurses; only an + // exact (schema, value) repeat short-circuits as a true cycle. For + // primitives we fall back to a depth counter so self-referential schemas + // without a base case still terminate. + let pairKey: string | undefined; + if (value !== null && typeof value === "object") { + pairKey = `${ref}:${getValueIdentity(ctx, value)}`; + if (ctx.seenPairs.has(pairKey)) return true; + ctx.seenPairs.add(pairKey); + } else { + if (ctx.refDepth >= MAX_REF_DEPTH) return true; + ctx.refDepth += 1; + } const ok = validateSchemaNode(resolved, value, path, ctx, issues); - ctx.seenRefs.delete(ref); + if (pairKey !== undefined) ctx.seenPairs.delete(pairKey); + else ctx.refDepth -= 1; return ok; } @@ -191,6 +230,33 @@ function validateSchemaNode( } } + // if/then/else: validate the if-branch silently; based on its outcome, + // validate against then/else. Each sub-schema is treated as a schema node + // (no requirement that branches be objects). This is a minimal correct + // semantic — schemas where the if-branch references properties only present + // after applying then will still resolve consistently for the LLM-emitted + // shapes we encounter. + if ("if" in schema) { + const ifIssues: JsonSchemaValidationIssue[] = []; + const ifOk = validateSchemaNode(schema.if, value, path, ctx, ifIssues); + const branch = ifOk ? schema.then : schema.else; + if (branch !== undefined) { + valid = validateSchemaNode(branch, value, path, ctx, issues) && valid; + } + } + + // `unevaluatedProperties` / `unevaluatedItems` require tracking which + // keys/indices were "evaluated" by sibling keywords across composed + // schemas — expensive bookkeeping we do not implement. Warn once so tool + // authors who rely on them know the keyword is silently permissive in + // this validator. + if (("unevaluatedProperties" in schema || "unevaluatedItems" in schema) && !seenUnevaluatedWarning) { + seenUnevaluatedWarning = true; + logger.warn( + "JSON Schema unevaluatedProperties/unevaluatedItems are not enforced by the in-tree validator; treating as permissive", + ); + } + if (isJsonObject(value)) { valid = validateObjectKeywords(schema, value, path, ctx, issues) && valid; } @@ -237,6 +303,70 @@ function validateObjectKeywords( } const known = new Set(Object.keys(properties)); + if (isJsonObject(schema.patternProperties)) { + for (const [pattern, patternSchema] of Object.entries(schema.patternProperties)) { + let re: RegExp; + try { + re = new RegExp(pattern); + } catch { + pushIssue(issues, path, `invalid patternProperties regex ${pattern}`, { keyword: "patternProperties" }); + valid = false; + continue; + } + for (const [key, entry] of Object.entries(value)) { + if (!re.test(key)) continue; + known.add(key); + valid = validateSchemaNode(patternSchema, entry, [...path, key], ctx, issues) && valid; + } + } + } + + if (isJsonObject(schema.dependentRequired)) { + for (const [key, deps] of Object.entries(schema.dependentRequired)) { + if (!(key in value)) continue; + if (!Array.isArray(deps)) continue; + for (const dep of deps) { + if (typeof dep !== "string") continue; + if (!(dep in value)) { + pushIssue(issues, [...path, dep], `is required when "${key}" is present`, { + keyword: "dependentRequired", + }); + valid = false; + } + } + } + } + + if (isJsonObject(schema.dependentSchemas)) { + for (const [key, depSchema] of Object.entries(schema.dependentSchemas)) { + if (!(key in value)) continue; + valid = validateSchemaNode(depSchema, value, path, ctx, issues) && valid; + } + } + + // Draft-07 `dependencies`: each entry is either a schema (validate value + // when key present) or a string[] of additional required keys. + if (isJsonObject(schema.dependencies)) { + for (const [key, dep] of Object.entries(schema.dependencies)) { + if (!(key in value)) continue; + if (Array.isArray(dep)) { + for (const required of dep) { + if (typeof required !== "string") continue; + if (!(required in value)) { + pushIssue(issues, [...path, required], `is required when "${key}" is present`, { + keyword: "dependencies", + }); + valid = false; + } + } + } else if (dep !== undefined) { + valid = validateSchemaNode(dep, value, path, ctx, issues) && valid; + } + } + } + + // `known` includes property names and any keys matched by patternProperties + // above, so additionalProperties only governs the genuine leftovers. const additional = schema.additionalProperties; if (additional === false) { for (const key of Object.keys(value)) { @@ -289,20 +419,30 @@ function validateArrayKeywords( } } + // Tuple validation: when `prefixItems` is present (draft 2020-12) it gives + // per-index schemas, and `items` then applies to the remainder. When + // `items` is itself an array (draft-07 tuple form), we keep the legacy + // behavior and validate by index. + const prefixItems = Array.isArray(schema.prefixItems) ? schema.prefixItems : undefined; const items = schema.items; - if (Array.isArray(items)) { - const limit = Math.min(items.length, value.length); + const tupleSchemas = prefixItems ?? (Array.isArray(items) ? items : undefined); + if (tupleSchemas) { + const limit = Math.min(tupleSchemas.length, value.length); for (let i = 0; i < limit; i += 1) { - valid = validateSchemaNode(items[i], value[i], [...path, i], ctx, issues) && valid; + valid = validateSchemaNode(tupleSchemas[i], value[i], [...path, i], ctx, issues) && valid; } - if (schema.additionalItems === false && value.length > items.length) { - for (let i = items.length; i < value.length; i += 1) { + // Items after the tuple prefix: + // - draft-07 tuple form (items: array): `additionalItems` governs the rest. + // - draft 2020-12 (prefixItems + items: schema): `items` validates the rest. + const remainderSchema = prefixItems && !Array.isArray(items) ? items : (schema.additionalItems as unknown); + if (remainderSchema === false && value.length > tupleSchemas.length) { + for (let i = tupleSchemas.length; i < value.length; i += 1) { pushIssue(issues, [...path, i], "must not be present", { keyword: "additionalItems" }); valid = false; } - } else if (schema.additionalItems !== undefined && schema.additionalItems !== true) { - for (let i = items.length; i < value.length; i += 1) { - valid = validateSchemaNode(schema.additionalItems, value[i], [...path, i], ctx, issues) && valid; + } else if (remainderSchema !== undefined && remainderSchema !== true) { + for (let i = tupleSchemas.length; i < value.length; i += 1) { + valid = validateSchemaNode(remainderSchema, value[i], [...path, i], ctx, issues) && valid; } } } else if (items !== undefined) { @@ -311,6 +451,26 @@ function validateArrayKeywords( } } + if (schema.contains !== undefined) { + const minContains = typeof schema.minContains === "number" ? schema.minContains : 1; + const maxContains = typeof schema.maxContains === "number" ? schema.maxContains : Infinity; + let count = 0; + for (let i = 0; i < value.length; i += 1) { + const containsIssues: JsonSchemaValidationIssue[] = []; + if (validateSchemaNode(schema.contains, value[i], [...path, i], ctx, containsIssues)) { + count += 1; + } + } + if (count < minContains) { + pushIssue(issues, path, `must contain at least ${minContains} matching item(s)`, { keyword: "contains" }); + valid = false; + } + if (count > maxContains) { + pushIssue(issues, path, `must contain at most ${maxContains} matching item(s)`, { keyword: "maxContains" }); + valid = false; + } + } + return valid; } @@ -386,7 +546,13 @@ function validateNumberKeywords( export function validateJsonSchemaValue(schema: unknown, value: unknown): JsonSchemaValidationResult { const issues: JsonSchemaValidationIssue[] = []; - const success = validateSchemaNode(schema, value, [], { root: schema, seenRefs: new Set() }, issues); + const success = validateSchemaNode( + schema, + value, + [], + { root: schema, seenPairs: new Set(), objectIds: new WeakMap(), nextObjectId: { value: 0 }, refDepth: 0 }, + issues, + ); return { success, issues }; } diff --git a/packages/ai/src/utils/schema/meta-validator.ts b/packages/ai/src/utils/schema/meta-validator.ts index 578a4c6a6..5e808e29c 100644 --- a/packages/ai/src/utils/schema/meta-validator.ts +++ b/packages/ai/src/utils/schema/meta-validator.ts @@ -64,6 +64,9 @@ function checkNode(node: Json, seen: WeakSet): boolean { if (key in node && !checkSchemaArray(node[key], seen)) return false; } if ("not" in node && !checkNode(node.not, seen)) return false; + for (const key of ["if", "then", "else"] as const) { + if (key in node && !checkNode(node[key], seen)) return false; + } for (const key of ["properties", "patternProperties", "$defs", "definitions"] as const) { if (key in node && !checkSchemaMap(node[key], seen)) return false; @@ -112,6 +115,17 @@ function checkNode(node: Json, seen: WeakSet): boolean { } } + // Draft-07 `dependencies`: each entry is either a schema or a string[]. + if ("dependencies" in node) { + const value = node.dependencies; + if (!isPlainObject(value)) return false; + for (const entry of Object.values(value)) { + if (Array.isArray(entry)) { + if (!entry.every(item => typeof item === "string")) return false; + } else if (!checkNode(entry, seen)) return false; + } + } + if ("enum" in node) { if (!Array.isArray(node.enum) || node.enum.length === 0 || !hasUniqueJsonValues(node.enum)) return false; } diff --git a/packages/ai/src/utils/validation.ts b/packages/ai/src/utils/validation.ts index eaee090fd..3ed3f1907 100644 --- a/packages/ai/src/utils/validation.ts +++ b/packages/ai/src/utils/validation.ts @@ -485,7 +485,11 @@ function branchMatchesSchema(branch: unknown, value: unknown): boolean { return isJsonSchemaValueValid(branch, value); } -function normalizeOptionalNullsForSchema(schema: unknown, value: unknown): { value: unknown; changed: boolean } { +function normalizeOptionalNullsForSchema( + schema: unknown, + value: unknown, + isRoot = true, +): { value: unknown; changed: boolean } { if (value === null || value === undefined) return { value, changed: false }; if (schema === null || typeof schema !== "object") return { value, changed: false }; @@ -498,7 +502,7 @@ function normalizeOptionalNullsForSchema(schema: unknown, value: unknown): { val let changedCandidate: { value: unknown; changed: true } | null = null; for (const branch of branches) { - const normalized = normalizeOptionalNullsForSchema(branch, value); + const normalized = normalizeOptionalNullsForSchema(branch, value, isRoot); if (!normalized.changed) continue; if (branchMatchesSchema(branch, normalized.value)) { @@ -523,7 +527,7 @@ function normalizeOptionalNullsForSchema(schema: unknown, value: unknown): { val let changed = false; let nextValue: unknown = value; for (const branch of schemaObject.allOf) { - const normalized = normalizeOptionalNullsForSchema(branch, nextValue); + const normalized = normalizeOptionalNullsForSchema(branch, nextValue, isRoot); if (!normalized.changed) continue; nextValue = normalized.value; changed = true; @@ -540,7 +544,7 @@ function normalizeOptionalNullsForSchema(schema: unknown, value: unknown): { val let changed = false; let nextValue = value; for (let i = 0; i < value.length; i += 1) { - const normalized = normalizeOptionalNullsForSchema(itemSchema, value[i]); + const normalized = normalizeOptionalNullsForSchema(itemSchema, value[i], false); if (!normalized.changed) continue; if (!changed) { nextValue = [...value]; @@ -603,7 +607,7 @@ function normalizeOptionalNullsForSchema(schema: unknown, value: unknown): { val continue; } } - const normalized = normalizeOptionalNullsForSchema(propertySchema, currentValue); + const normalized = normalizeOptionalNullsForSchema(propertySchema, currentValue, false); if (!normalized.changed) continue; if (!changed) { @@ -619,7 +623,13 @@ function normalizeOptionalNullsForSchema(schema: unknown, value: unknown): { val // these the same as null on known optional fields is a safer fallback. Keys // with non-null unknown values are left intact so genuine schema mistakes // still surface as validation errors. - if (schemaObject.additionalProperties === false) { + // + // At the ROOT level we deliberately keep unknown null-valued keys intact: + // Zod-emitted wire schemas always set `additionalProperties: false`, but the + // post-validation `preserveUnknownRootFields` pass re-attaches root extras + // so callers can observe (and reject) hallucinated fields. Stripping here + // would erase the field before that snapshot, hiding the rejection signal. + if (!isRoot && schemaObject.additionalProperties === false) { const knownKeys = new Set(Object.keys(properties)); for (const key of Object.keys(nextValue)) { if (knownKeys.has(key)) continue; diff --git a/packages/ai/test/schema-strict-mode.test.ts b/packages/ai/test/schema-strict-mode.test.ts index d0180af0b..9bf053b3a 100644 --- a/packages/ai/test/schema-strict-mode.test.ts +++ b/packages/ai/test/schema-strict-mode.test.ts @@ -1,10 +1,14 @@ import { describe, expect, it } from "bun:test"; +import type { Tool, ToolCall } from "@oh-my-pi/pi-ai/types"; import { enforceStrictSchema, + isJsonSchemaValueValid, + isValidJsonSchema, sanitizeSchemaForStrictMode, tryEnforceStrictSchema, zodToWireSchema, } from "@oh-my-pi/pi-ai/utils/schema"; +import { validateToolArguments } from "@oh-my-pi/pi-ai/utils/validation"; import * as z from "zod/v4"; describe("sanitizeSchemaForStrictMode", () => { @@ -503,3 +507,130 @@ describe("tryEnforceStrictSchema", () => { expect(updateTasks.required).toEqual(["content", "status", "notes"]); }); }); + +describe("json-schema validator unsupported-keyword regressions", () => { + it("rejects values with keys that fail the propertyNames schema", () => { + const schema = { + type: "object", + propertyNames: { type: "string", pattern: "^[a-z]+$" }, + }; + expect(isJsonSchemaValueValid(schema, { abc: 1 })).toBe(true); + expect(isJsonSchemaValueValid(schema, { "BAD-KEY": 1 })).toBe(false); + }); + + it("rejects patternProperties mismatches", () => { + const schema = { + type: "object", + patternProperties: { "^id_": { type: "number" } }, + }; + expect(isJsonSchemaValueValid(schema, { id_a: 1 })).toBe(true); + expect(isJsonSchemaValueValid(schema, { id_a: "not-a-number" })).toBe(false); + }); + + it("rejects values that violate dependentRequired", () => { + const schema = { + type: "object", + dependentRequired: { credit_card: ["billing_address"] }, + }; + expect(isJsonSchemaValueValid(schema, { credit_card: "x", billing_address: "y" })).toBe(true); + expect(isJsonSchemaValueValid(schema, { credit_card: "x" })).toBe(false); + }); + + it("applies then when if matches and rejects missing required fields", () => { + const schema = { + type: "object", + properties: { kind: { type: "string" }, extra: { type: "string" } }, + if: { properties: { kind: { const: "a" } }, required: ["kind"] }, + // biome-ignore lint/suspicious/noThenProperty: JSON Schema if/then/else keyword + then: { required: ["extra"] }, + }; + expect(isJsonSchemaValueValid(schema, { kind: "b" })).toBe(true); + expect(isJsonSchemaValueValid(schema, { kind: "a", extra: "ok" })).toBe(true); + expect(isJsonSchemaValueValid(schema, { kind: "a" })).toBe(false); + }); + + it("validates contains against array elements", () => { + const schema = { type: "array", contains: { type: "number" } }; + expect(isJsonSchemaValueValid(schema, ["a", 1])).toBe(true); + expect(isJsonSchemaValueValid(schema, ["a", "b"])).toBe(false); + }); + + it("validates prefixItems by index", () => { + const schema = { type: "array", prefixItems: [{ type: "string" }, { type: "number" }] }; + expect(isJsonSchemaValueValid(schema, ["x", 1])).toBe(true); + expect(isJsonSchemaValueValid(schema, [1, "x"])).toBe(false); + }); + + it("recursively validates nested values through self-referential $ref", () => { + const schema = { + $ref: "#/definitions/Node", + definitions: { + Node: { + type: "object", + properties: { + name: { type: "string" }, + child: { $ref: "#/definitions/Node" }, + }, + required: ["name"], + additionalProperties: false, + }, + }, + }; + // Nested shape conforming — should validate. + expect(isJsonSchemaValueValid(schema, { name: "root", child: { name: "leaf" } })).toBe(true); + // Nested child violates the inner shape: previously short-circuited to true + // because the second occurrence of the $ref was treated as a seen ref. + expect(isJsonSchemaValueValid(schema, { name: "root", child: { name: 123 } })).toBe(false); + expect(isJsonSchemaValueValid(schema, { name: "root", child: { child: { name: "x" } } })).toBe(false); + }); +}); + +describe("meta-validator conditional keywords", () => { + it("accepts well-formed if/then/else", () => { + expect( + isValidJsonSchema({ + type: "object", + if: { properties: { kind: { const: "a" } } }, + // biome-ignore lint/suspicious/noThenProperty: JSON Schema if/then/else keyword + then: { required: ["extra"] }, + else: { required: ["other"] }, + }), + ).toBe(true); + }); + + it("rejects malformed if (must be a schema, not an array)", () => { + expect(isValidJsonSchema({ type: "object", if: [] })).toBe(false); + }); + + it("rejects malformed then", () => { + // biome-ignore lint/suspicious/noThenProperty: JSON Schema if/then/else keyword + expect(isValidJsonSchema({ type: "object", then: "not-a-schema" })).toBe(false); + }); + + it("accepts draft-07 dependencies as schemas or string arrays", () => { + expect(isValidJsonSchema({ type: "object", dependencies: { a: ["b"], c: { type: "object" } } })).toBe(true); + expect(isValidJsonSchema({ type: "object", dependencies: { a: 1 } })).toBe(false); + expect(isValidJsonSchema({ type: "object", dependencies: { a: [1] } })).toBe(false); + }); +}); + +describe("Zod root extras preserved through normalize", () => { + it("retains a null-valued unknown root key after tool-argument validation so downstream rejection still triggers", () => { + const tool: Tool = { + name: "simple_tool", + description: "", + parameters: z.object({ assignment: z.string() }), + }; + const toolCall: ToolCall = { + type: "toolCall", + id: "call-zod-null-root", + name: "simple_tool", + arguments: { assignment: "do thing", schema: null }, + }; + + const result = validateToolArguments(tool, toolCall) as Record; + expect(result.assignment).toBe("do thing"); + expect("schema" in result).toBe(true); + expect(result.schema).toBeNull(); + }); +});