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.
This commit is contained in:
can1357
2026-05-15 17:47:20 +02:00
parent 29f8e57a46
commit 82c24a3f74
5 changed files with 344 additions and 19 deletions
+4
View File
@@ -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
@@ -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<string>;
seenPairs: Set<string>;
objectIds: WeakMap<object, number>;
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<string, unknown> {
@@ -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 };
}
@@ -64,6 +64,9 @@ function checkNode(node: Json, seen: WeakSet<object>): 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<object>): 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;
}
+16 -6
View File
@@ -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;
+131
View File
@@ -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<string, unknown>;
expect(result.assignment).toBe("do thing");
expect("schema" in result).toBe(true);
expect(result.schema).toBeNull();
});
});