diff --git a/changelog.d/fixes/13022-desktop-exe-malformed-skill-tool-schemas.md b/changelog.d/fixes/13022-desktop-exe-malformed-skill-tool-schemas.md new file mode 100644 index 0000000000..8765865032 --- /dev/null +++ b/changelog.d/fixes/13022-desktop-exe-malformed-skill-tool-schemas.md @@ -0,0 +1 @@ +- **fix(skills):** repair nested malformed skill-tool schemas (bare property maps, boolean `required: true`) for OpenAI-compatible providers, not just the schema root (#13022) — thanks @ftevxk diff --git a/src/lib/skills/injection.ts b/src/lib/skills/injection.ts index 881a039189..951a65d102 100644 --- a/src/lib/skills/injection.ts +++ b/src/lib/skills/injection.ts @@ -51,6 +51,133 @@ export function decodeSkillToolName(toolName: string): string { } } +// Depth guard mirroring open-sse/services/toolSchemaSanitizer.ts's +// MAX_RECURSION_DEPTH, so a pathological/cyclic-looking nested schema +// submitted by a custom skill (POST /api/skills accepts any z.record shape) +// cannot blow the stack. +const MAX_SCHEMA_REPAIR_DEPTH = 32; + +// JSON Schema keywords whose *value* is itself a schema node/map, not a +// user-declared property — recursing into their children must not treat the +// container itself as a "bare property map" candidate. Mirrors +// open-sse/translator/helpers/geminiHelper.ts's SCHEMA_MAP_KEYS for the +// Gemini-only normalizeMalformedSchemaObjects this mirrors (#12269). +const SCHEMA_MAP_KEYS = new Set(["properties", "$defs", "definitions", "patternProperties"]); + +const SCHEMA_NODE_KEYS = new Set([ + "additionalProperties", + "additionalItems", + "contains", + "default", + "dependencies", + "discriminator", + "else", + "example", + "examples", + "if", + "patternProperties", + "propertyNames", + "then", +]); + +function isSchemaNode(record: Record): boolean { + if (Object.keys(record).some((key) => key.startsWith("x-") || SCHEMA_NODE_KEYS.has(key))) { + return true; + } + if (typeof record.type === "string" || Array.isArray(record.type)) return true; + if (record.properties !== undefined || Array.isArray(record.required)) return true; + if (record.items !== undefined) return true; + if (record.anyOf !== undefined || record.oneOf !== undefined || record.allOf !== undefined) { + return true; + } + return record.$ref !== undefined || record.enum !== undefined || record.const !== undefined; +} + +function isBarePropertyMap(record: Record): boolean { + const keys = Object.keys(record); + if (keys.length === 0 || isSchemaNode(record)) return false; + return keys.every((key) => { + const value = record[key]; + return Boolean(value) && typeof value === "object" && !Array.isArray(value); + }); +} + +// Strips a scalar (non-array) `required` off every property of `record` and, +// only when it was `true`, promotes the property's key onto the parent +// schema's own `required` array (created if absent, deduped if present). +function promoteBooleanRequired(record: Record): void { + const properties = record.properties; + if (!properties || typeof properties !== "object" || Array.isArray(properties)) return; + + const required = Array.isArray(record.required) + ? record.required.filter((field): field is string => typeof field === "string") + : []; + + for (const [name, schema] of Object.entries(properties as Record)) { + if (!schema || typeof schema !== "object" || Array.isArray(schema)) continue; + const child = schema as Record; + if (child.required === true && !required.includes(name)) { + required.push(name); + } + if ("required" in child && !Array.isArray(child.required)) { + delete child.required; + } + } + + if (required.length > 0) { + record.required = required; + } else if (!Array.isArray(record.required)) { + delete record.required; + } +} + +// Repairs the two malformed-schema shapes strict JSON Schema validators +// (agnes/nvidia/DeepSeek and other OpenAI-compatible upstreams) reject, +// recursing into every nested level of a skill's declared input schema — +// not just the root map #11881 already handled: +// 1. A bare property map with no `type`/`properties` wrapper (e.g. +// `{ opts: { limit: { type: "number" } } }`) is lifted into +// `{ type: "object", properties: {...} }`, bottom-up so nested bare +// maps are fixed before their parent is inspected. +// 2. A scalar `required: true` on a property is stripped and promoted onto +// the parent's `required` array instead. +// Mirrors open-sse/translator/helpers/geminiHelper.ts's +// normalizeMalformedSchemaObjects (itself modeled on CLIProxyAPI's function +// of the same name), which already does this for the Gemini/Antigravity +// request-translation path (#12269) — this is the skill-injection-path +// equivalent, additive and independent from that implementation. +function repairMalformedSchema(node: unknown, parentKey?: string, depth = 0): void { + if (!node || typeof node !== "object" || depth > MAX_SCHEMA_REPAIR_DEPTH) return; + + if (Array.isArray(node)) { + for (const item of node) { + repairMalformedSchema(item, parentKey, depth + 1); + } + return; + } + + const record = node as Record; + + for (const [key, value] of Object.entries(record)) { + if (value && typeof value === "object") { + repairMalformedSchema(value, key, depth + 1); + } + } + + if (parentKey === undefined || !SCHEMA_MAP_KEYS.has(parentKey)) { + if (isBarePropertyMap(record)) { + const props = { ...record }; + for (const key of Object.keys(record)) { + delete record[key]; + } + record.type = "object"; + record.properties = props; + } + } + + promoteBooleanRequired(record); +} + // Skills store a flat JSON Schema record ({ "text": { "type": "string" } }), // but Gemini (function_declarations[].parameters) and Anthropic // (input_schema) require a full object schema with a properties wrapper. @@ -60,23 +187,30 @@ function normalizeInputSchema(input: Record): Record; if (typeof input.type === "string") { - return input; + // Already a full object schema (#11881's root case doesn't apply) — but + // it may still carry the deeper #13022 malformations (nested bare + // property maps, per-property boolean `required`) inside `properties`, + // so still recurse; just skip the root-level string-shorthand expansion. + root = { ...input }; + } else { + // Some builtin skills declare property types in shorthand ("content": + // "string" instead of "content": { "type": "string" }). Strict schema + // validators — Zhipu GLM served through opencode-go (upstream error [1210] + // "Invalid API parameter") — reject the shorthand as malformed JSON Schema, + // which 400s every request the skill tools are injected into. Expand string + // values to { type: value }; non-string values pass through untouched. + const properties: Record = {}; + for (const [key, value] of Object.entries(input)) { + properties[key] = typeof value === "string" ? { type: value } : value; + } + root = { type: "object", properties }; } - // Some builtin skills declare property types in shorthand ("content": - // "string" instead of "content": { "type": "string" }). Strict schema - // validators — Zhipu GLM served through opencode-go (upstream error [1210] - // "Invalid API parameter") — reject the shorthand as malformed JSON Schema, - // which 400s every request the skill tools are injected into. Expand string - // values to { type: value }; non-string values pass through untouched. - const properties: Record = {}; - for (const [key, value] of Object.entries(input)) { - properties[key] = typeof value === "string" ? { type: value } : value; - } - return { - type: "object", - properties, - }; + + repairMalformedSchema(root); + return root; } function skillToOpenAI(skill: Skill): OpenAITool { diff --git a/tests/unit/issue-13022-nested-skill-schema.test.ts b/tests/unit/issue-13022-nested-skill-schema.test.ts new file mode 100644 index 0000000000..e50571eb84 --- /dev/null +++ b/tests/unit/issue-13022-nested-skill-schema.test.ts @@ -0,0 +1,88 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-issue-13022-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const coreDb = await import("../../src/lib/db/core.ts"); +const { skillRegistry } = await import("../../src/lib/skills/registry.ts"); +const { injectSkills } = await import("../../src/lib/skills/injection.ts"); + +function resetRegistryState() { + skillRegistry["registeredSkills"].clear(); + skillRegistry["versionCache"].clear(); +} + +test.after(() => { + resetRegistryState(); + coreDb.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + +test("#13022: nested bare property map survives skill injection for OpenAI-format providers (agnes/nvidia/DeepSeek)", async () => { + await skillRegistry.register({ + name: "nested-tool", + version: "1.0.0", + description: "a skill with a nested bare property map, like a local/skillssh author would ship", + schema: { input: { query: "string", opts: { limit: { type: "number" } } }, output: {} }, + handler: "nested-tool-handler", + enabled: true, + apiKeyId: "issue-13022-key", + }); + + const tools = injectSkills({ provider: "openai", apiKeyId: "issue-13022-key" }) as Array<{ + function: { parameters: Record }; + }>; + assert.equal(tools.length, 1); + + const parameters = tools[0].function.parameters; + const properties = parameters.properties as Record>; + + assert.equal( + properties.query.type, + "string", + "#11881 root shorthand expansion should still work" + ); + + const opts = properties.opts; + assert.equal( + opts.type, + "object", + "BUG #13022: nested bare property map missing type:object wrapper" + ); +}); + +test("#13022: per-property boolean required:true survives skill injection for OpenAI-format providers", async () => { + await skillRegistry.register({ + name: "required-bool-tool", + version: "1.0.0", + description: + "a skill declaring required as a boolean on the property, not an array on the schema", + schema: { input: { content: { type: "string", required: true } }, output: {} }, + handler: "required-bool-handler", + enabled: true, + apiKeyId: "issue-13022-key-2", + }); + + const tools = injectSkills({ provider: "openai", apiKeyId: "issue-13022-key-2" }) as Array<{ + function: { parameters: Record }; + }>; + assert.equal(tools.length, 1); + + const parameters = tools[0].function.parameters; + const properties = parameters.properties as Record>; + + assert.equal( + "required" in properties.content, + false, + "BUG #13022: boolean required:true survived on the property" + ); + assert.deepEqual( + parameters.required, + ["content"], + "BUG #13022: boolean required:true was not promoted" + ); +});