From 14992fbc41ebc77172ee8c20b7f2ddacb0679a35 Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 20 Aug 2026 04:52:49 +0200 Subject: [PATCH] fix(ai): disabled lossy argument repairs on failed union branches - Prevent schema validation from applying lossy repairs such as container stringification and key deletion to values diagnosed within failed `anyOf` and `oneOf` branches. - Remove the `coerceArguments` tool option and let repair provenance dictate whether lossy coercions are allowed. --- packages/ai/CHANGELOG.md | 2 +- packages/ai/src/types.ts | 10 -- .../src/utils/schema/json-schema-validator.ts | 71 +++++++++++---- packages/ai/src/utils/validation.ts | 42 +++++---- .../ai/test/tool-argument-coercion.test.ts | 91 ++++++++++++++----- packages/coding-agent/CHANGELOG.md | 2 +- packages/coding-agent/src/tools/yield.ts | 8 -- .../coding-agent/test/tools/yield.test.ts | 14 +-- 8 files changed, 156 insertions(+), 84 deletions(-) diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index 7919ed5ac..567accbe6 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -5,10 +5,10 @@ ### Added - Added model metadata fields (`context_length`, `max_output_tokens`, `input_modalities`, etc.) to auth gateway model listing responses -- Added `Tool.coerceArguments` (default `true`): setting it `false` opts a tool out of every LLM-quirk argument repair pass (JSON-string parsing, object→string stringification, unrecognized-key dropping, singleton array wrapping) so validation runs verbatim. Tools whose arguments are the deliverable payload — like the subagent `yield` tool — use it to keep lossy repairs from silently corrupting data their own validate-and-retry loop is designed to correct. ### Fixed +- Fixed the tool-argument repair layer applying lossy repairs on union-branch diagnoses: when a value failed every `anyOf`/`oneOf` variant, the first failing branch's issues were treated as authoritative, so object payloads got JSON-stringified into string-typed fields and unrecognized keys were silently deleted — corrupting subagent `yield` payloads (validation "passed" and parents received `summary: "{\"purge\":13,…}"` instead of a retryable error). Issues surfaced from a failed union branch are now marked at every depth and only receive lossless repairs (JSON-string parsing, boolean spellings, scalar coercion); container stringification, key deletion, and singleton-array wrapping require an authoritative non-union diagnosis. - Fixed local OpenAI-compatible servers with strict `chat_template_kwargs` whitelists (e.g. NInfer) failing every Qwen 3.8+ turn with `400 chat_template_kwargs.reasoning_effort is not supported` after the effort routing fix: the reasoning-effort fallback now recognizes a rejection of the kwargs spelling itself, retries with the kwarg stripped while keeping the effort on the standard top-level `reasoning_effort` field (hoisting it there for the kwargs-only vLLM dialect), and remembers the shape for the rest of the session. Value-level rejections and drops now also update the `chat_template_kwargs.reasoning_effort` twin instead of leaving a stale effort for kwargs-reading renderers, and unknown-parameter 400s naming `reasoning_effort` are recognized as effort rejections. ## [17.3.8] - 2026-08-19 diff --git a/packages/ai/src/types.ts b/packages/ai/src/types.ts index 72f5a9dce..d9f0c3c23 100644 --- a/packages/ai/src/types.ts +++ b/packages/ai/src/types.ts @@ -1212,16 +1212,6 @@ export interface Tool { parameters: TParameters; /** If true, tool is strictly typed and validated against the parameters schema before execution */ strict?: boolean; - /** - * If false, argument validation runs verbatim: none of the LLM-quirk - * normalization/repair passes (JSON-string parsing, object→string - * stringification, unrecognized-key dropping, singleton array wrapping) - * are applied. For tools whose arguments ARE the deliverable payload — - * e.g. a subagent's `yield` — lossy repairs silently corrupt data that - * the tool's own validation/retry loop is designed to correct instead. - * Defaults to true. - */ - coerceArguments?: boolean; /** * Optional grammar constraint for OpenAI custom-tool emission. * When set, providers that support grammar-constrained tools (currently only diff --git a/packages/ai/src/utils/schema/json-schema-validator.ts b/packages/ai/src/utils/schema/json-schema-validator.ts index 79a93052e..952038c22 100644 --- a/packages/ai/src/utils/schema/json-schema-validator.ts +++ b/packages/ai/src/utils/schema/json-schema-validator.ts @@ -21,11 +21,11 @@ export interface JsonSchemaValidationIssue { expectedTypes?: string[]; keyword?: string; /** - * Marks issues that originate inside a failed `anyOf` / `oneOf` branch. - * Consumers such as the tool-argument coercion layer use this to avoid - * applying type repairs (e.g. singleton-array wrapping) that would be - * authoritative outside of a combinator but are only one candidate - * branch's expectation here. + * Marks issues surfaced from a failed `anyOf` / `oneOf` branch (at any + * depth). Such a diagnosis is one candidate branch's guess, not + * authoritative: the tool-argument coercion layer keeps lossy repairs + * (container stringification, unrecognized-key deletion, singleton-array + * wrapping) off for these while still applying lossless ones. */ fromUnionBranch?: boolean; } @@ -68,6 +68,35 @@ function getValueIdentity(ctx: ValidationContext, value: object): number { function isJsonObject(value: unknown): value is Record { return typeof value === "object" && value !== null && !Array.isArray(value); } +/** + * Whether `value` matches every `const`/`enum` discriminator property the + * branch declares (with at least one such property present and matching). + * A uniquely tag-selected branch's validation issues are authoritative — the + * model named its intended variant — so the coercion layer may apply lossy + * repairs to them; without a unique tag, branch issues are guesses. + */ +function isTagSelectedBranch(branch: unknown, value: unknown): boolean { + if (!isJsonObject(branch) || !isJsonObject(value)) return false; + const props = branch.properties; + if (!isJsonObject(props)) return false; + let matched = false; + for (const key in props) { + const propSchema = props[key]; + if (!isJsonObject(propSchema)) continue; + const hasConst = Object.hasOwn(propSchema, "const"); + const enumValues = Array.isArray(propSchema.enum) ? propSchema.enum : undefined; + if (!hasConst && !enumValues) continue; + if (!Object.hasOwn(value, key)) return false; + const candidate = value[key]; + if (hasConst) { + if (!areJsonValuesEqual(candidate, propSchema.const)) return false; + } else if (enumValues && !enumValues.some(entry => areJsonValuesEqual(entry, candidate))) { + return false; + } + matched = true; + } + return matched; +} function pushIssue( issues: JsonSchemaValidationIssue[], @@ -239,27 +268,35 @@ function validateSchemaNode( let matches = 0; let firstIssues: JsonSchemaValidationIssue[] | undefined; + let selectedIssues: JsonSchemaValidationIssue[] | undefined; + let selectedCount = 0; for (const branch of branches) { const branchIssues: JsonSchemaValidationIssue[] = []; if (validateSchemaNode(branch, value, path, ctx, branchIssues)) { matches += 1; - } else if (!firstIssues) { - firstIssues = branchIssues; + continue; + } + if (!firstIssues) firstIssues = branchIssues; + if (isTagSelectedBranch(branch, value)) { + selectedCount += 1; + if (selectedCount === 1) selectedIssues = branchIssues; } } const branchValid = keyword === "anyOf" ? matches > 0 : matches === 1; if (!branchValid) { - if (matches === 0 && firstIssues && firstIssues.length > 0) { - // Only tag issues that sit at the combinator's own path as - // union-branch; deeper issues describe a specific field within - // the failed branch and should remain individually repairable. - const unionDepth = path.length; + if (matches === 0 && selectedCount === 1 && selectedIssues && selectedIssues.length > 0) { + // A const/enum discriminator uniquely identifies the intended + // variant, so its diagnosis is authoritative: surface untagged and + // keep every repair (including lossy ones) available. + issues.push(...selectedIssues); + } else if (matches === 0 && firstIssues && firstIssues.length > 0) { + // No variant matched and no tag picks one: everything reported is + // the first failing branch's guess — another variant may accept the + // value as-is. Surface all issues (deep ones remain individually + // repairable by lossless coercions) but mark their provenance so + // lossy repairs (stringify, key deletion, singleton wrap) stay off. for (const branchIssue of firstIssues) { - if (branchIssue.path.length === unionDepth) { - issues.push({ ...branchIssue, fromUnionBranch: true }); - } else { - issues.push(branchIssue); - } + issues.push(branchIssue.fromUnionBranch ? branchIssue : { ...branchIssue, fromUnionBranch: true }); } } else { pushIssue( diff --git a/packages/ai/src/utils/validation.ts b/packages/ai/src/utils/validation.ts index bc39ba5ff..537892596 100644 --- a/packages/ai/src/utils/validation.ts +++ b/packages/ai/src/utils/validation.ts @@ -136,12 +136,20 @@ function tryCoerceBooleanToNumber(value: unknown, expectedTypes: string[]): { va return { value: value ? 1 : 0, changed: true }; } -function tryCoerceString(value: unknown, expectedTypes: string[]): { value: unknown; changed: boolean } { +function tryCoerceString( + value: unknown, + expectedTypes: string[], + allowLossy: boolean, +): { value: unknown; changed: boolean } { if (!expectedTypes.includes("string") || typeof value === "string" || value === null || value === undefined) { return { value, changed: false }; } if (Array.isArray(value) || typeof value === "object") { + // JSON.stringify is irreversible (downstream consumers receive encoded + // text where they expected structure), so it requires an authoritative + // diagnosis — never a union-branch guess. + if (!allowLossy) return { value, changed: false }; try { const stringified = JSON.stringify(value); if (stringified === undefined) return { value, changed: false }; @@ -158,7 +166,16 @@ function tryCoerceString(value: unknown, expectedTypes: string[]): { value: unkn return { value: String(value), changed: true }; } -function tryCoerceForExpectedTypes(value: unknown, expectedTypes: string[]): { value: unknown; changed: boolean } { +/** + * Schema-directed value repair for a single type issue. `allowLossy` gates the + * irreversible repairs (container→string stringification); lossless repairs + * (JSON parsing, boolean spellings, scalar stringification) always apply. + */ +function tryCoerceForExpectedTypes( + value: unknown, + expectedTypes: string[], + allowLossy: boolean, +): { value: unknown; changed: boolean } { if (typeof value === "string") { const parsed = tryParseJsonForTypes(value, expectedTypes); if (parsed.changed) return parsed; @@ -171,7 +188,7 @@ function tryCoerceForExpectedTypes(value: unknown, expectedTypes: string[]): { v const numericCoercion = tryCoerceBooleanToNumber(value, expectedTypes); if (numericCoercion.changed) return numericCoercion; - return tryCoerceString(value, expectedTypes); + return tryCoerceString(value, expectedTypes, allowLossy); } function tryParseLeadingJsonContainer(value: string): unknown | undefined { @@ -1578,7 +1595,12 @@ function coerceArgsFromIssues(args: unknown, issues: FlatIssue[]): { value: unkn let nextArgs: unknown = args; for (const issue of issues) { + // Issues surfaced from a failed union branch are guesses from that + // branch's diagnosis, not authoritative: another variant may accept the + // value as-is. Lossy repairs (key deletion, container stringification, + // singleton wrapping) stay off for them; lossless repairs still apply. if (issue.keyword === "unrecognized") { + if (issue.unionBranch) continue; const previous = nextArgs; nextArgs = deleteValueAtPointer(nextArgs, issue.instancePath); if (nextArgs !== previous) changed = true; @@ -1588,7 +1610,7 @@ function coerceArgsFromIssues(args: unknown, issues: FlatIssue[]): { value: unkn if (issue.expectedTypes.length === 0) continue; const currentValue = getValueAtPointer(nextArgs, issue.instancePath); - const result = tryCoerceForExpectedTypes(currentValue, issue.expectedTypes); + const result = tryCoerceForExpectedTypes(currentValue, issue.expectedTypes, !issue.unionBranch); let coercedValue = result.changed ? result.value : undefined; if ( coercedValue === undefined && @@ -1923,18 +1945,6 @@ export function validateToolArguments(tool: Tool, toolCall: ToolCall): ToolCall[ ); } const ctx = getValidationContext(tool); - // Verbatim mode: the tool opted out of every repair pass because its - // arguments are payload, not plumbing. Validate as-is; on failure the - // caller's lenient path (if any) hands the raw args to the tool, whose - // own error messaging drives the model's retry. - if (tool.coerceArguments === false) { - const verbatim = validateContext(ctx, originalArgs); - if (verbatim.success) return verbatim.value as ToolCall["arguments"]; - const errors = verbatim.messages.join("\n") || "Unknown validation error"; - throw new AIError.ValidationError( - `Validation failed for tool "${toolCall.name}":\n${errors}\n\nReceived arguments:\n${JSON.stringify(truncateArgsForError(originalArgs), null, 2)}`, - ); - } const { json } = ctx; // Always normalize first — strip null/string "null" from optional fields, diff --git a/packages/ai/test/tool-argument-coercion.test.ts b/packages/ai/test/tool-argument-coercion.test.ts index 08f41b5d5..53d42447d 100644 --- a/packages/ai/test/tool-argument-coercion.test.ts +++ b/packages/ai/test/tool-argument-coercion.test.ts @@ -56,48 +56,91 @@ describe("Tool argument coercion", () => { expect(result.payload).toBe('{"a":1,"nested":["x"]}'); }); - it("coerceArguments: false rejects object values for string fields instead of stringifying", () => { + it("does not stringify container values diagnosed inside a failed union branch", () => { + // Regression: a subagent yield payload with an object in a string-typed + // schema field sat under the tool's `anyOf` wrapper; the stringify repair + // fired on that branch's guess, so validation "passed" and downstream + // consumers received encoded text instead of a retryable error. const tool: Tool = { - name: "verbatim", + name: "union-string", description: "", - coerceArguments: false, - parameters: type({ payload: type("string") }), + parameters: { + type: "object", + additionalProperties: false, + properties: { + payload: { anyOf: [{ type: "string" }, { type: "number" }] }, + }, + required: ["payload"], + } as never, }; expect(() => validateToolArguments(tool, { type: "toolCall", - id: "call-verbatim-object", - name: "verbatim", + id: "call-union-object", + name: "union-string", arguments: { payload: { a: 1 } }, }), ).toThrow(/payload/); }); - it("coerceArguments: false still passes conforming args and rejects invalid JSON buffers", () => { + it("does not delete unrecognized keys diagnosed inside a failed union branch", () => { const tool: Tool = { - name: "verbatim-ok", + name: "union-closed", description: "", - coerceArguments: false, - parameters: type({ payload: type("string") }), + parameters: { + type: "object", + additionalProperties: false, + properties: { + op: { + anyOf: [ + { + type: "object", + additionalProperties: false, + properties: { kind: { type: "string" }, value: { type: "number" } }, + required: ["kind", "value"], + }, + { type: "string" }, + ], + }, + }, + required: ["op"], + } as never, + }; + + // `extra` fails the closed object variant; deleting it would silently + // drop payload data on a branch guess. Must surface as a validation error. + expect(() => + validateToolArguments(tool, { + type: "toolCall", + id: "call-union-extra-key", + name: "union-closed", + arguments: { op: { kind: "set", value: 1, extra: "keep me" } }, + }), + ).toThrow(/op/); + }); + + it("still applies lossless repairs inside union branches", () => { + const tool: Tool = { + name: "union-lossless", + description: "", + parameters: { + type: "object", + additionalProperties: false, + properties: { + payload: { anyOf: [{ type: "number" }, { type: "boolean" }] }, + }, + required: ["payload"], + } as never, }; const result = validateToolArguments(tool, { type: "toolCall", - id: "call-verbatim-ok", - name: "verbatim-ok", - arguments: { payload: "fine" }, - }) as { payload: string }; - expect(result.payload).toBe("fine"); - - expect(() => - validateToolArguments(tool, { - type: "toolCall", - id: "call-verbatim-parse-error", - name: "verbatim-ok", - arguments: { __parseError: "Unexpected token", __rawJson: '{"payload": ' }, - }), - ).toThrow(/not valid JSON/); + id: "call-union-numeric-string", + name: "union-lossless", + arguments: { payload: "300" }, + }) as { payload: number }; + expect(result.payload).toBe(300); }); it("stringifies array values when schema expects string", () => { diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 3fec9e5a8..2bfe11504 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -24,7 +24,7 @@ ### Fixed -- Fixed subagent structured returns being silently corrupted by the generic tool-argument repair layer: the `yield` tool's parameters embed the caller's output schema, so the repair passes could JSON-stringify object payloads into string-typed fields (parents received `summary: "{\"purge\":13,…}"` instead of prose) and drop unrecognized keys — bypassing yield's own validate-and-retry loop. `yield` now opts out via `coerceArguments: false`, so mismatches surface as retryable schema errors and accepted payloads arrive verbatim. +- Fixed subagent structured returns being silently corrupted by the tool-argument repair layer: the `yield` tool's parameters embed the caller's output schema under an `anyOf` wrapper, and lossy repairs fired on union-branch guesses — JSON-stringifying object payloads into string-typed fields (parents received `summary: "{\"purge\":13,…}"` instead of prose) and deleting unrecognized keys — bypassing yield's own validate-and-retry loop. The repair layer (pi-ai) now restricts union-branch diagnoses to lossless repairs, so mismatches surface as retryable schema errors and accepted payloads arrive verbatim. - Fixed the `yield` tool bouncing common weak-caller envelope shapes with `result must be an object containing either data or error` retries (the dominant structured-output failure in Gemini-flash subagent traces): `type: "result"` with the `result` wrapper omitted entirely now finalizes as the documented last-turn yield, top-level `data`/`error` payloads missing the wrapper are salvaged, and `result` or `data` sent as a JSON-encoded string is parsed losslessly (mirroring executor finalization) before consuming a schema retry. - Fixed the `yield` tool's instructions teaching weak callers the wrong call shape for structured tasks: the description led with `Pass type:"result" to finalize; when data is omitted, your last assistant turn becomes the raw final result` before ever stating the `result: { data }` wrapper, producing wrapper-less `{type:"result"}` punts and top-level `data:` payloads in Gemini-flash traces. The description (now `prompts/tools/yield.md`) and the subagent system prompt lead with the wrapper contract and only advertise last-turn extraction when no output schema is declared; a schema-bound last-turn finalize with no accumulated incremental sections is rejected in-band as a retryable error instead of terminating the child into an uncorrectable post-mortem `schema_violation`. - Fixed a prompt cancelled during turn setup (Esc while the pre-stream spinner is up, after dispatch had started) vanishing entirely: it was never persisted to the session — so the `/tree` and `/branch` selectors had nothing to rewind to — and was not returned to the editor either, while its optimistic transcript row kept lingering. A prompt dropped before reaching the agent (abort or usage-preflight denial racing setup) is now handed back: the stale transcript row is removed and the typed text and image attachments are restored to the editor for editing. diff --git a/packages/coding-agent/src/tools/yield.ts b/packages/coding-agent/src/tools/yield.ts index b5c140b28..bb8048f81 100644 --- a/packages/coding-agent/src/tools/yield.ts +++ b/packages/coding-agent/src/tools/yield.ts @@ -268,14 +268,6 @@ export class YieldTool implements AgentTool { strict = true; readonly intent = "omit" as const; lenientArgValidation = true; - /** - * The args ARE the subagent's deliverable: the generic repair layer's lossy - * coercions (object→string stringification, unrecognized-key deletion) would - * silently corrupt the payload and bypass this tool's own validate-and-retry - * loop. Verbatim validation + `lenientArgValidation` routes every mismatch - * through `execute()`'s salvage/retry messaging instead. - */ - coerceArguments = false; readonly #validate?: (value: unknown) => JsonSchemaValidationResult; readonly #validateSection?: ReadonlyMap JsonSchemaValidationResult>; diff --git a/packages/coding-agent/test/tools/yield.test.ts b/packages/coding-agent/test/tools/yield.test.ts index cca096672..e0b234667 100644 --- a/packages/coding-agent/test/tools/yield.test.ts +++ b/packages/coding-agent/test/tools/yield.test.ts @@ -168,11 +168,12 @@ describe("YieldTool", () => { expect(result.details).toEqual({ data: { n: 4 }, status: "success", error: undefined }); }); - it("verbatim arg validation rejects object payloads in string-typed fields instead of stringifying", () => { - // Regression: the generic tool-arg repair layer used to JSON.stringify an - // object submitted for a string-typed schema field, so validation - // "passed" and the parent received `summary: "{\"purge\":13,…}"` instead - // of a retry prompt. `coerceArguments = false` must surface the mismatch. + it("arg validation rejects object payloads in string-typed fields instead of stringifying", () => { + // Regression: the repair layer used to JSON.stringify an object submitted + // for a string-typed schema field even though the diagnosis came from a + // failed `anyOf` branch of the yield wrapper, so validation "passed" and + // the parent received `summary: "{\"purge\":13,…}"` instead of a retry + // prompt. Union-branch diagnoses must not trigger lossy repairs. const tool = new YieldTool( createSession({ outputSchema: { @@ -182,7 +183,6 @@ describe("YieldTool", () => { }, }), ); - expect(tool.coerceArguments).toBe(false); expect(() => validateToolArguments(tool as never, { type: "toolCall", @@ -193,7 +193,7 @@ describe("YieldTool", () => { ).toThrow(/summary/); }); - it("verbatim arg validation passes conforming args through unmodified", () => { + it("arg validation passes conforming args through unmodified", () => { const tool = new YieldTool( createSession({ outputSchema: {