From eecadf35b20ac72d509513bef5949d36c887354d Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:15:17 +0800 Subject: [PATCH] fix(verification): unify client schema traversal and reference checks Share ordered schema positions between Gemini literal checks, DeepSeek regex checks, and catalog array-shape checks. Reuse the canonical keyword and local reference owners; keep unreferenced instance literals opaque and unresolved external or dynamic targets unknown. Gemini and catalog checks now catch constraints in schemas reached through references into examples. Preserve source paths, resource scopes, cycles and regex occurrence counts. Align fixture first-error selection with the native Gemini check and use consistent bracketed array locations in diagnostics. Validation: 55 focused tests passed, including full/compact MCP catalogs; typecheck, lint, discovery, and Node source-loader checks passed. The old Gemini scanner reproduced the local-reference false negative. Installed Gemini and DeepSeek clients were not run. --- scripts/lib/client-schema-patterns.mts | 50 ++----- scripts/lib/client-schema-positions.mts | 92 ++++++++++++ scripts/lib/gemini-schema-literals.mts | 27 ++++ scripts/verify-gemini-client.mjs | 50 ++----- .../clientSchemaItemConstraints.test.ts | 57 ++++++++ .../verifiers/clientSchemaPatterns.test.ts | 10 ++ .../verifiers/geminiSchemaLiterals.test.ts | 122 ++++++++++++++++ tests/fixtures/mcpClientSchemaConstraints.ts | 133 ++++++------------ 8 files changed, 368 insertions(+), 173 deletions(-) create mode 100644 scripts/lib/client-schema-positions.mts create mode 100644 scripts/lib/gemini-schema-literals.mts create mode 100644 tests/conformance/verifiers/clientSchemaItemConstraints.test.ts create mode 100644 tests/conformance/verifiers/geminiSchemaLiterals.test.ts diff --git a/scripts/lib/client-schema-patterns.mts b/scripts/lib/client-schema-patterns.mts index 2ac721492a..0afed6f172 100644 --- a/scripts/lib/client-schema-patterns.mts +++ b/scripts/lib/client-schema-patterns.mts @@ -1,48 +1,22 @@ -import { JsonSchemaReferences } from "../../src/contracts/jsonSchemaReferences.js"; -import { - schemaArrays, - schemaChildren, - schemaMaps, -} from "../../src/contracts/jsonSchemaKeywords.js"; - -const isSchemaObject = ( - value: unknown, -): value is Readonly> => - value !== null && typeof value === "object" && !Array.isArray(value); +import { clientSchemaPositions } from "./client-schema-positions.mjs"; /** Check forwarded regex constraints in native plain/Unicode/Unicode-sets modes. */ export const assertClientSchemaPatterns = ( schema: unknown, ): readonly string[] => { - if (!isSchemaObject(schema)) return []; const patterns: string[] = []; - const seen = new Set(); - const checkPattern = (pattern: string): void => { + for (const position of clientSchemaPositions(schema)) { + const pattern = + position.kind === "pattern-property" + ? position.pattern + : position.kind === "keyword" && + position.keyword === "pattern" && + typeof position.value === "string" + ? position.value + : undefined; + if (pattern === undefined) continue; for (const flags of ["", "u", "v"]) new RegExp(pattern, flags); patterns.push(pattern); - }; - const visit = (value: unknown): void => { - if (!isSchemaObject(value) || seen.has(value)) return; - seen.add(value); - for (const [key, child] of Object.entries(value)) { - if (key === "pattern" && typeof child === "string") checkPattern(child); - else if (schemaMaps.has(key) && isSchemaObject(child)) { - for (const [name, nested] of Object.entries(child)) { - if (key === "patternProperties") checkPattern(name); - visit(nested); - } - } else if ( - (schemaArrays.has(key) || schemaChildren.has(key)) && - Array.isArray(child) - ) { - for (const nested of child) visit(nested); - } else if (schemaChildren.has(key)) visit(child); - } - }; - visit(schema); - // Literal instance values stay opaque unless the reference owner resolves a - // local target into them. External/dynamic/ambiguous targets remain unknown. - for (const { target } of new JsonSchemaReferences(schema).references()) - if (target.kind === "target") visit(target.value); + } return patterns; }; diff --git a/scripts/lib/client-schema-positions.mts b/scripts/lib/client-schema-positions.mts new file mode 100644 index 0000000000..59c0bada03 --- /dev/null +++ b/scripts/lib/client-schema-positions.mts @@ -0,0 +1,92 @@ +import { JsonSchemaReferences } from "../../src/contracts/jsonSchemaReferences.js"; +import { + schemaArrays, + schemaChildren, + schemaMaps, +} from "../../src/contracts/jsonSchemaKeywords.js"; + +type SchemaObject = Readonly>; +export type ClientSchemaPosition = + | { + readonly kind: "schema"; + readonly schema: SchemaObject; + readonly path: string; + } + | { + readonly kind: "keyword"; + readonly keyword: string; + readonly value: unknown; + readonly path: string; + } + | { + readonly kind: "pattern-property"; + readonly pattern: string; + readonly path: string; + }; + +const isSchemaObject = (value: unknown): value is SchemaObject => + value !== null && typeof value === "object" && !Array.isArray(value); + +const referenceTargetPath = ( + root: SchemaObject, + tokens: readonly string[], + path: string, +): string => { + let value: unknown = root; + for (const token of tokens) { + path += Array.isArray(value) ? `[${token}]` : `.${token}`; + // The reference owner already established this exact pointer's target. + value = Reflect.get(value as object, token); + } + return path; +}; + +/** Ordered schema positions for native-client checks; instance literals remain opaque. */ +export function* clientSchemaPositions( + schema: unknown, +): IterableIterator { + if (!isSchemaObject(schema)) return; + const seen = new Set(); + const paths = new WeakMap(); + function* visit( + value: unknown, + path: string, + ): IterableIterator { + if (!isSchemaObject(value) || seen.has(value)) return; + seen.add(value); + paths.set(value, path); + yield { kind: "schema", schema: value, path }; + for (const [keyword, child] of Object.entries(value)) { + const childPath = `${path}.${keyword}`; + yield { kind: "keyword", keyword, value: child, path: childPath }; + if (schemaMaps.has(keyword) && isSchemaObject(child)) { + for (const [name, nested] of Object.entries(child)) { + const nestedPath = `${childPath}.${name}`; + if (keyword === "patternProperties") + yield { kind: "pattern-property", pattern: name, path: nestedPath }; + yield* visit(nested, nestedPath); + } + } else if ( + (schemaArrays.has(keyword) || schemaChildren.has(keyword)) && + Array.isArray(child) + ) { + for (const [index, nested] of child.entries()) + yield* visit(nested, `${childPath}[${index}]`); + } else if (schemaChildren.has(keyword)) yield* visit(child, childPath); + } + } + yield* visit(schema, "$"); + // Resolve after ordinary positions so an existing inline diagnostic keeps + // its encounter order. External/dynamic/ambiguous targets stay unknown. + for (const { scope, target } of new JsonSchemaReferences( + schema, + ).references()) { + if (target.kind !== "target") continue; + const scopePath = paths.get(scope.root); + if (scopePath !== undefined) + yield* visit( + target.value, + referenceTargetPath(scope.root, target.path, scopePath), + ); + } +} diff --git a/scripts/lib/gemini-schema-literals.mts b/scripts/lib/gemini-schema-literals.mts new file mode 100644 index 0000000000..1bcf481428 --- /dev/null +++ b/scripts/lib/gemini-schema-literals.mts @@ -0,0 +1,27 @@ +import { clientSchemaPositions } from "./client-schema-positions.mjs"; + +export interface GeminiUnsupportedLiteral { + readonly path: string; + readonly keyword: "const" | "enum"; + readonly reason: "boolean" | "empty-string"; +} + +/** Gemini's declaration policy, including schemas selected by resolved local references. */ +export const geminiUnsupportedLiteral = ( + schema: unknown, +): GeminiUnsupportedLiteral | undefined => { + for (const position of clientSchemaPositions(schema)) { + if (position.kind !== "keyword") continue; + const { keyword, value, path } = position; + if (keyword === "const") { + if (typeof value === "boolean") + return { path, keyword, reason: "boolean" }; + if (value === "") return { path, keyword, reason: "empty-string" }; + } else if (keyword === "enum" && Array.isArray(value)) { + if (value.some((item) => typeof item === "boolean")) + return { path, keyword, reason: "boolean" }; + if (value.includes("")) return { path, keyword, reason: "empty-string" }; + } + } + return undefined; +}; diff --git a/scripts/verify-gemini-client.mjs b/scripts/verify-gemini-client.mjs index 8ed5d94fab..9837ad53dd 100644 --- a/scripts/verify-gemini-client.mjs +++ b/scripts/verify-gemini-client.mjs @@ -5,11 +5,6 @@ import { tmpdir } from "node:os"; import { join, resolve, sep } from "node:path"; import { fileURLToPath } from "node:url"; import Ajv2020 from "ajv/dist/2020.js"; -import { - schemaArrays, - schemaChildren, - schemaMaps, -} from "../dist/contracts/jsonSchemaKeywords.js"; import { TOOL_CONTRACTS, toolContract, @@ -27,6 +22,12 @@ import { } from "./verify/clients/result-discovery.mjs"; import { createVerifierRun, completeVerifierRun } from "./lib/verifier-run.mjs"; import { createGeminiModelFixture } from "./verify/clients/gemini-model-fixture.mjs"; +import { importVerifierSource } from "./lib/verifier-source-loader.mjs"; + +const { geminiUnsupportedLiteral } = await importVerifierSource( + new URL("./lib/gemini-schema-literals.mts", import.meta.url).href, + import.meta.url, +); const repo = fileURLToPath(new URL("..", import.meta.url)); const runtimeRoot = process.env.REA_VERIFY_RUNTIME_ROOT ?? repo; @@ -200,41 +201,6 @@ await withVerifierCleanup(async () => { strict: false, validateFormats: false, }); - const unsupportedLiteralPath = (schema, path = "$", seen = new Set()) => { - if (!schema || typeof schema !== "object" || seen.has(schema)) - return undefined; - seen.add(schema); - for (const [key, child] of Object.entries(schema)) { - const childPath = `${path}.${key}`; - if (key === "const" && (typeof child === "boolean" || child === "")) - return childPath; - if ( - key === "enum" && - Array.isArray(child) && - child.some((item) => typeof item === "boolean" || item === "") - ) - return childPath; - const schemaChildrenAtKey = - schemaMaps.has(key) && - child && - typeof child === "object" && - !Array.isArray(child) - ? Object.entries(child).map(([name, item]) => [ - `${childPath}.${name}`, - item, - ]) - : schemaArrays.has(key) && Array.isArray(child) - ? child.map((item, index) => [`${childPath}[${index}]`, item]) - : schemaChildren.has(key) - ? [[childPath, child]] - : []; - for (const [nestedPath, nested] of schemaChildrenAtKey) { - const found = unsupportedLiteralPath(nested, nestedPath, seen); - if (found !== undefined) return found; - } - } - return undefined; - }; const decodeGeminiResult = (text) => { if ( text.startsWith("\n") && @@ -277,9 +243,9 @@ await withVerifierCleanup(async () => { schemaValidator.validateSchema(tool.parametersJsonSchema), `${tool.name}: invalid advertised JSON Schema: ${JSON.stringify(schemaValidator.errors)}`, ); - const unsupportedLiteral = unsupportedLiteralPath( + const unsupportedLiteral = geminiUnsupportedLiteral( tool.parametersJsonSchema, - ); + )?.path; assert.equal( unsupportedLiteral, undefined, diff --git a/tests/conformance/verifiers/clientSchemaItemConstraints.test.ts b/tests/conformance/verifiers/clientSchemaItemConstraints.test.ts new file mode 100644 index 0000000000..75a81cd695 --- /dev/null +++ b/tests/conformance/verifiers/clientSchemaItemConstraints.test.ts @@ -0,0 +1,57 @@ +import { expect, it } from "vitest"; +import { + arrayItemSchemaError, + strictGatewayItemSchemaErrors, +} from "../../fixtures/mcpClientSchemaConstraints.js"; + +it("checks client array requirements when a reference selects literal-held schemas", () => { + const literal = { examples: [{ type: "array" }] }; + expect(arrayItemSchemaError(literal, "tool")).toBeUndefined(); + expect( + arrayItemSchemaError({ ...literal, $ref: "#/examples/0" }, "tool"), + ).toBe("tool.examples[0]: tool parameters array type must have items"); + expect( + arrayItemSchemaError( + { + $ref: "#/examples/0", + examples: [{ type: "array", items: { type: "string" } }], + }, + "tool", + ), + ).toBeUndefined(); +}); + +it.each([ + { + selected: { type: "object", items: { type: "string" } }, + error: 'tool.examples[0].items is beside type "object"', + }, + { + selected: { + type: "array", + items: { anyOf: [{ type: "string" }, { type: "number" }] }, + }, + error: "tool.examples[0].items is an untyped union", + }, +])( + "checks referenced gateway item constraints: $error", + ({ selected, error }) => { + const literal = { examples: [selected] }; + expect(strictGatewayItemSchemaErrors(literal, "tool")).toEqual([]); + expect( + strictGatewayItemSchemaErrors( + { ...literal, $ref: "#/examples/0" }, + "tool", + ), + ).toEqual([error]); + expect( + strictGatewayItemSchemaErrors( + { + $ref: "#/examples/0", + examples: [{ type: "array", items: { type: "string" } }], + }, + "tool", + ), + ).toEqual([]); + }, +); diff --git a/tests/conformance/verifiers/clientSchemaPatterns.test.ts b/tests/conformance/verifiers/clientSchemaPatterns.test.ts index caec43e797..5b8027aa86 100644 --- a/tests/conformance/verifiers/clientSchemaPatterns.test.ts +++ b/tests/conformance/verifiers/clientSchemaPatterns.test.ts @@ -95,3 +95,13 @@ it("keeps native Unicode-mode failures and repeated pattern occurrences visible" }), ).toEqual(["^same", "^same"]); }); + +it("keeps pattern encounter order while visiting a shared schema object only once", () => { + const shared = { pattern: "nested$" }; + expect( + assertClientSchemaPatterns({ + patternProperties: { "^first": shared, "^second": shared }, + $ref: "#/patternProperties/^first", + }), + ).toEqual(["^first", "nested$", "^second"]); +}); diff --git a/tests/conformance/verifiers/geminiSchemaLiterals.test.ts b/tests/conformance/verifiers/geminiSchemaLiterals.test.ts new file mode 100644 index 0000000000..35e8a47bd4 --- /dev/null +++ b/tests/conformance/verifiers/geminiSchemaLiterals.test.ts @@ -0,0 +1,122 @@ +import { expect, it } from "vitest"; +import { Ajv2020 } from "ajv/dist/2020.js"; +import { geminiUnsupportedLiteral } from "../../../scripts/lib/gemini-schema-literals.mjs"; +import { clientSchemaPositions } from "../../../scripts/lib/client-schema-positions.mjs"; +import { unsupportedInputLiteralSchemaError } from "../../fixtures/mcpClientSchemaConstraints.js"; + +it("checks an explicitly referenced example as a schema while leaving unreferenced examples opaque", () => { + const literal = { examples: [{ const: false }] }; + expect(geminiUnsupportedLiteral(literal)).toBeUndefined(); + const schema = { ...literal, $ref: "#/examples/0" }; + const ajv = new Ajv2020({ strict: false, validateFormats: false }); + expect(ajv.validateSchema(schema)).toBe(true); + const validate = ajv.compile(schema); + expect(validate(false)).toBe(true); + expect(validate(true)).toBe(false); + expect(geminiUnsupportedLiteral(schema)).toEqual({ + path: "$.examples[0].const", + keyword: "const", + reason: "boolean", + }); + expect(unsupportedInputLiteralSchemaError(schema, "tool")).toBe( + "tool.examples[0].const is boolean", + ); +}); + +it("preserves the inline schema keyword encounter order used by the native Gemini verifier", () => { + const schema = { properties: { early: { enum: [""] } }, const: false }; + expect(geminiUnsupportedLiteral(schema)).toEqual({ + path: "$.properties.early.enum", + keyword: "enum", + reason: "empty-string", + }); + expect(unsupportedInputLiteralSchemaError(schema, "tool")).toBe( + "tool.properties.early.enum contains an empty string", + ); +}); + +it("resolves nested local references against their own resource while retaining root-relative diagnostic locations", () => { + const schema = { + $id: "https://schemas.example/root.json", + $defs: { + child: { + $id: "child.json", + $ref: "#/examples/0", + examples: [{ $ref: "#/default" }], + default: { enum: ["named", ""] }, + }, + }, + }; + expect(geminiUnsupportedLiteral(schema)).toEqual({ + path: "$.$defs.child.default.enum", + keyword: "enum", + reason: "empty-string", + }); + expect(unsupportedInputLiteralSchemaError(schema, "tool")).toBe( + "tool.$defs.child.default.enum contains an empty string", + ); +}); + +it("terminates reference cycles and reports the first newly reached constraint at its source path", () => { + const schema = { + $ref: "#/examples/0", + examples: [ + { $ref: "#/examples/1" }, + { $ref: "#/examples/0", properties: { chosen: { const: "" } } }, + ], + }; + expect(geminiUnsupportedLiteral(schema)).toEqual({ + path: "$.examples[1].properties.chosen.const", + keyword: "const", + reason: "empty-string", + }); + expect( + [...clientSchemaPositions(schema)].filter( + (position) => position.kind === "schema", + ), + ).toHaveLength(4); +}); + +it("uses decoded reference tokens and preserves the existing spelling of property names in diagnostic paths", () => { + const schema = { + examples: [{ properties: { "a/b~name": { const: false } } }], + $ref: "#/examples/0/properties/a~1b~0name", + }; + expect(geminiUnsupportedLiteral(schema)?.path).toBe( + "$.examples[0].properties.a/b~name.const", + ); +}); + +it("leaves external, dynamic, anchor and ambiguous resource references unknown", () => { + for (const reference of [ + { $ref: "https://schemas.example/external.json" }, + { $ref: "#anchor" }, + { $dynamicRef: "#/examples/0" }, + ]) + expect( + geminiUnsupportedLiteral({ ...reference, examples: [{ const: false }] }), + ).toBeUndefined(); + expect( + geminiUnsupportedLiteral({ + $id: "https://schemas.example/root.json", + $defs: { + first: { + $id: "child.json", + $ref: "#/examples/0", + examples: [{ const: false }], + }, + second: { $id: "child.json", examples: [{ const: false }] }, + }, + }), + ).toBeUndefined(); +}); + +it("keeps literal-policy diagnostic prose for const and enum failures", () => { + for (const [schema, message] of [ + [{ const: false }, "tool.const is boolean"], + [{ const: "" }, "tool.const is an empty string"], + [{ enum: [false, ""] }, "tool.enum contains a boolean"], + [{ enum: [""] }, "tool.enum contains an empty string"], + ] as const) + expect(unsupportedInputLiteralSchemaError(schema, "tool")).toBe(message); +}); diff --git a/tests/fixtures/mcpClientSchemaConstraints.ts b/tests/fixtures/mcpClientSchemaConstraints.ts index 42b496c93a..9c1fb262f1 100644 --- a/tests/fixtures/mcpClientSchemaConstraints.ts +++ b/tests/fixtures/mcpClientSchemaConstraints.ts @@ -1,8 +1,5 @@ -import { - schemaArrays, - schemaChildren, - schemaMaps, -} from "../../src/contracts/jsonSchemaKeywords.js"; +import { geminiUnsupportedLiteral } from "../../scripts/lib/gemini-schema-literals.mjs"; +import { clientSchemaPositions } from "../../scripts/lib/client-schema-positions.mjs"; const isRecord = (value: unknown): value is Record => typeof value === "object" && value !== null && !Array.isArray(value); @@ -12,30 +9,14 @@ export const arrayItemSchemaError = ( schema: unknown, path: string, ): string | undefined => { - if (!isRecord(schema)) return undefined; - const admitsArrays = - schema.type === "array" || - (Array.isArray(schema.type) && schema.type.includes("array")); - if (admitsArrays && !schema.items) - return `${path}: tool parameters array type must have items`; - for (const [key, value] of Object.entries(schema)) { - const children = - schemaMaps.has(key) && isRecord(value) - ? Object.entries(value).map( - ([name, child]) => [`${key}.${name}`, child] as const, - ) - : (schemaArrays.has(key) || schemaChildren.has(key)) && - Array.isArray(value) - ? value.map( - (child: unknown, index) => [`${key}.${index}`, child] as const, - ) - : schemaChildren.has(key) - ? [[key, value] as const] - : []; - for (const [suffix, child] of children) { - const error = arrayItemSchemaError(child, `${path}.${suffix}`); - if (error !== undefined) return error; - } + for (const position of clientSchemaPositions(schema)) { + if (position.kind !== "schema") continue; + const value = position.schema; + const admitsArrays = + value.type === "array" || + (Array.isArray(value.type) && value.type.includes("array")); + if (admitsArrays && !value.items) + return `${path}${position.path.slice(1)}: tool parameters array type must have items`; } return undefined; }; @@ -45,38 +26,17 @@ export const unsupportedInputLiteralSchemaError = ( schema: unknown, path: string, ): string | undefined => { - if (!isRecord(schema)) return undefined; - if (typeof schema.const === "boolean") return `${path}.const is boolean`; - if (schema.const === "") return `${path}.const is an empty string`; - if ( - Array.isArray(schema.enum) && - schema.enum.some((value) => typeof value === "boolean") - ) - return `${path}.enum contains a boolean`; - if (Array.isArray(schema.enum) && schema.enum.includes("")) - return `${path}.enum contains an empty string`; - for (const [key, value] of Object.entries(schema)) { - const children = - schemaMaps.has(key) && isRecord(value) - ? Object.entries(value).map( - ([name, child]) => [`${key}.${name}`, child] as const, - ) - : schemaArrays.has(key) && Array.isArray(value) - ? value.map( - (child: unknown, index) => [`${key}[${index}]`, child] as const, - ) - : schemaChildren.has(key) - ? [[key, value] as const] - : []; - for (const [suffix, child] of children) { - const error = unsupportedInputLiteralSchemaError( - child, - `${path}.${suffix}`, - ); - if (error !== undefined) return error; - } - } - return undefined; + const finding = geminiUnsupportedLiteral(schema); + if (finding === undefined) return undefined; + const reason = + finding.reason === "boolean" + ? finding.keyword === "const" + ? "is boolean" + : "contains a boolean" + : finding.keyword === "const" + ? "is an empty string" + : "contains an empty string"; + return `${path}${finding.path.slice(1)} ${reason}`; }; /** @@ -89,36 +49,23 @@ export const strictGatewayItemSchemaErrors = ( schema: unknown, path = "$", ): readonly string[] => { - if (!isRecord(schema)) return []; - const here: string[] = []; - if (schema.items !== undefined && schema.type !== "array") - here.push(`${path}.items is beside type ${JSON.stringify(schema.type)}`); - const items = schema.items; - if ( - schema.type === "array" && - isRecord(items) && - items.type === undefined && - (Array.isArray(items.oneOf) || Array.isArray(items.anyOf)) - ) - here.push(`${path}.items is an untyped union`); - return [ - ...here, - ...Object.entries(schema).flatMap(([key, value]) => { - if (schemaMaps.has(key) && isRecord(value)) - return Object.entries(value).flatMap(([name, child]) => - strictGatewayItemSchemaErrors(child, `${path}.${key}.${name}`), - ); - if (schemaArrays.has(key) && Array.isArray(value)) - return value.flatMap((child: unknown, index) => - strictGatewayItemSchemaErrors(child, `${path}.${key}[${index}]`), - ); - if (schemaChildren.has(key)) - return Array.isArray(value) - ? value.flatMap((child: unknown, index) => - strictGatewayItemSchemaErrors(child, `${path}.${key}[${index}]`), - ) - : strictGatewayItemSchemaErrors(value, `${path}.${key}`); - return []; - }), - ]; + const errors: string[] = []; + for (const position of clientSchemaPositions(schema)) { + if (position.kind !== "schema") continue; + const value = position.schema; + const selectedPath = `${path}${position.path.slice(1)}`; + if (value.items !== undefined && value.type !== "array") + errors.push( + `${selectedPath}.items is beside type ${JSON.stringify(value.type)}`, + ); + const items = value.items; + if ( + value.type === "array" && + isRecord(items) && + items.type === undefined && + (Array.isArray(items.oneOf) || Array.isArray(items.anyOf)) + ) + errors.push(`${selectedPath}.items is an untyped union`); + } + return errors; };