fix(agent): stopped arg coercion from corrupting subagent yield payloads

- Added Tool.coerceArguments (default true); false skips every LLM-quirk repair pass (object->string stringification, unrecognized-key deletion, JSON-string parsing) so validation runs verbatim. - YieldTool opts out: its args are the deliverable, and the repair layer silently stringified object payloads into string-typed schema fields while bypassing yield's own validate-and-retry loop. - Losslessly salvaged weak-caller envelopes observed in session traces: type:"result" without the result wrapper finalizes as last-turn, top-level data/error are wrapped, and JSON-string result/data parse before consuming a schema retry.
2026-08-20 04:28:35 +02:00
parent 7e54061cbb
commit 042f27f5e4
9 changed files with 319 additions and 14 deletions
@@ -12,6 +12,7 @@

 ### Changed

+- Unified inline overlay chrome on the rounded-box style used by the model picker/hub and `/settings`: selectors (theme, thinking, queue mode, show images, login/logout, reset usage, session account, plugins, MCP wizard, history search, branch-from-message, sessions, session tree, debug tools) and the `/cleanse`, `/omfg`, `/btw` run panels now render inside a titled `╭─╮│ │╰─╯` box (new shared `OverlayPanel` container) instead of two full-width horizontal rules.
 - `omp cleanse` and the `/cleanse` slash command now render a live interactive status board with running checkers, repair subagents, tool counts, token/cost totals, and live scrollback in both the CLI and interactive terminal modes
 - Replaced the single `compaction.strategy` / `compaction.remoteEnabled` policy with ordered `compaction.methodOrder` preferences. The default now tries OpenAI-compatible server compaction, snapcompact, handoff, shake, then soft compaction; unavailable or failed methods advance through that list.
 - `/settings` rows can now carry a risk note: a warning glyph on the row plus a warning-colored line above the description. `External Thinking` (`externalThinking`, `--external-thinking`) is the first user — providers have flagged the request shape it produces as abuse, up to account-level enforcement, so both the settings entry and `--help` now say so.
@@ -22,6 +23,9 @@

 ### 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 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.
 - Fixed macOS `top`-style single-dash long options in the `top` shell builtin: `top -l 2 -pid 56943 -stats pid,cpu,th,mem,pstate` previously failed with `invalid value 'id' for '--pid <PIDS>'` because clap read `-pid` as `-p id`. Single-dash long spellings now parse, and `-stats` selects and orders output columns using macOS stat keys (`pid`, `cpu`, `th`, `mem`, `pstate`, ...).
 - Fixed a sweep of GNU/BSD compatibility gaps in the built-in shell utilities, found by auditing every builtin against its real counterpart: `timeout` gained `-s`/`-k`/`--preserve-status`/`--foreground`/`-v`, GNU exit codes (124/125/137), signal delivery to the child process group with `-k` SIGKILL escalation, and `timeout 0` disabling the limit; `diff` gained normal-format default output, `-w`/`-b`/`-B`/`-i`/`-x`/`-L`/`-s`/`--strip-trailing-cr`, context format (`-c`/`-C`), bundled flags (`-ru`, `-urN`), timestamped unified headers, and no longer recurses directories without `-r`; `find` fixed inverted `-newerXY` timestamp comparisons, anchored `-regex` to whole paths, and gained BSD `-perm +mode`, `-type f,d` lists, `-size` `T`/`P` suffixes, ISO dates in `-newermt`, and BSD leading flags `-E`/`-x`/`-s`; `date` gained BSD `-r <epoch>`, `-v` adjustments, and `-j -f` strptime parsing, and `-I` no longer swallows a following `+FORMAT`; `tail`/`head` accept obsolete `-N`/`+N` counts at any argv position with any file count, `tail -r -n N` works, and `head` continues past per-file I/O errors with GNU header/separator placement; `rg` resolves `-s`/`-i`/`-S` by last occurrence, accepts `--no-config`/`-j`/`--threads`/`--no-column`, implements `--path-separator`, and emits clean NUL-delimited output under `-0`/`-l0`; `stat` prints integer epochs for `%X`/`%Y`/`%Z` (bash arithmetic on `stat -c %Y` works) and gained BSD `-s`/`-x` output modes plus `-t` time formatting; `cksum` is now registered (multi-algorithm `cksum -a sha256`); `truncate` implements `-o`/`--io-blocks` (previously silently truncated to the raw byte count), accepts `b` (512-byte) suffix and BSD `=` prefix; `sleep`/`timeout` accept `infinity` and keep sub-millisecond precision; `yes` and `errno` accept hyphen-prefixed operands (`yes -n`, `errno -2`); `nohup -- cmd` no longer tries to run `--` (including backgrounded via the brush wrapper); `which` gained BSD `-s` and errors on zero operands; `kill` accepts attached values (`-s9`, `-sKILL`, `-l9`) and maps exit statuses above 128 (`kill -l 137` → `KILL`).
@@ -43,7 +43,11 @@ While work remains, you MUST continue with another tool call — investigate, ed
 Yield protocol:
 - Omit `type` for the normal single terminal structured result in `result.data`.
 - Use non-empty `type: string[]` for incremental, non-terminal sections; calls accumulate by section.
+{{#if outputSchema}}
+- A data-less terminal `type: "result"` only finalizes previously submitted incremental sections; it NEVER substitutes for `result.data`.
+{{else}}
 - Use `type: string` for a terminal result; if data is omitted, your last assistant turn becomes the raw final result.
+{{/if}}

 This is your only way to return a final result. For structured results, you NEVER put JSON in plain text or substitute a text summary for `result.data`.

@@ -0,0 +1,8 @@
+Submit subagent output. Always wrap the payload: `result: { data: <your output> }` for success, `result: { error: "message" }` for failure. `data`/`error` at the top level or a bare payload is invalid.
+
+Omit `type` for the usual single terminal structured result. Pass `type: ["section"]` to submit an incremental, non-terminal section that accumulates.
+{{#if hasOutputSchema}}
+This task declares an output schema: the terminal `result.data` MUST be the full object matching it. A data-less `type: "result"` finalizes previously submitted incremental sections; it is invalid when no sections were submitted — prose in your last turn can never satisfy the schema.
+{{else}}
+Pass `type: "result"` to finalize; when `data` is omitted, your last assistant turn becomes the raw final result.
+{{/if}}
@@ -12,6 +12,8 @@ import {
 	sanitizeSchemaForStrictMode,
 	tryEnforceStrictSchema,
 } from "@oh-my-pi/pi-ai/utils/schema";
+import { prompt } from "@oh-my-pi/pi-utils";
+import yieldDescription from "../prompts/tools/yield.md" with { type: "text" };
 import { subprocessToolRegistry } from "../task/subprocess-tool-registry";
 import type { ToolSession } from ".";
 import { buildOutputValidator, formatAllValidationIssues } from "./output-schema-validator";
@@ -100,6 +102,54 @@ function parseYieldType(value: unknown): string | string[] | undefined {
 	if (isYieldType(value)) return value;
 	throw new Error("type must be a string or non-empty array of strings");
 }
+/** Parse a `{`/`[`-leading JSON string; undefined on non-container or parse failure. */
+function parseJsonContainerString(value: string): unknown {
+	const trimmed = value.trim();
+	if (!(trimmed.startsWith("{") || trimmed.startsWith("["))) return undefined;
+	try {
+		return JSON.parse(trimmed);
+	} catch {
+		return undefined;
+	}
+}
+
+function isPlainRecord(value: unknown): value is Record<string, unknown> {
+	return typeof value === "object" && value !== null && !Array.isArray(value);
+}
+
+/**
+ * Resolve the `result` record from raw yield arguments, losslessly salvaging
+ * the envelope deviations weak tool callers actually produce (observed in
+ * Gemini-flash subagent traces):
+ * - `result` sent as a JSON-encoded string → parsed;
+ * - `data`/`error` at the top level with the `result` wrapper omitted → wrapped;
+ * - `type` present with `result` omitted entirely → `{}` — the tool description
+ *   documents omitted data as last-turn extraction, so an omitted wrapper means
+ *   the same thing.
+ * Returns undefined when no object-shaped result can be recovered; the caller
+ * surfaces the standard retryable format error.
+ */
+function resolveResultRecord(
+	raw: Record<string, unknown>,
+	yieldType: string | string[] | undefined,
+): Record<string, unknown> | undefined {
+	let result = raw.result;
+	if (typeof result === "string") {
+		const parsed = parseJsonContainerString(result);
+		if (isPlainRecord(parsed)) result = parsed;
+	}
+	if (isPlainRecord(result)) return result;
+	if (result === undefined || result === null) {
+		if (Object.hasOwn(raw, "data") || Object.hasOwn(raw, "error")) {
+			const wrapped: Record<string, unknown> = {};
+			if (Object.hasOwn(raw, "data")) wrapped.data = raw.data;
+			if (Object.hasOwn(raw, "error")) wrapped.error = raw.error;
+			return wrapped;
+		}
+		if (yieldType !== undefined) return {};
+	}
+	return undefined;
+}

 /**
 * Render an incremental yield's `type: [...]` labels as a quoted, comma-separated list for
@@ -213,14 +263,19 @@ export class YieldTool implements AgentTool<TSchema, YieldDetails> {
 	readonly name = "yield";
 	readonly approval = "read" as const;
 	readonly label = "Submit Result";
-	readonly description =
-		"Submit subagent output. Omit `type` for the usual final structured result.\n\n" +
-		'Pass `type: ["section"]` to submit an incremental, non-terminal section that accumulates. Pass `type: "result"` to finalize; when `data` is omitted, your last assistant turn becomes the raw final result.\n' +
-		'Use `result: { data: <your output> }` for success, or `result: { error: "message" }` for failure. Keep the `result` wrapper.';
+	description: string;
 	readonly parameters: TSchema;
 	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<string, (value: unknown) => JsonSchemaValidationResult>;
@@ -229,6 +284,7 @@ export class YieldTool implements AgentTool<TSchema, YieldDetails> {
 	#isKnownSection?: (label: string) => boolean;
 	#schemaValidationFailures = 0;
 	#emptyResultFailures = 0;
+	#hasIncrementalSections = false;

 	constructor(session: ToolSession) {
 		let validate: ((value: unknown) => JsonSchemaValidationResult) | undefined;
@@ -304,6 +360,7 @@ export class YieldTool implements AgentTool<TSchema, YieldDetails> {
 		this.#rejectUnknownSections = rejectUnknownSections;
 		this.#knownSectionLabels = knownSectionLabels;
 		this.#isKnownSection = isKnownSection;
+		this.description = prompt.render(yieldDescription, { hasOutputSchema: validate !== undefined });
 		this.parameters = parameters;
 	}

@@ -315,14 +372,13 @@ export class YieldTool implements AgentTool<TSchema, YieldDetails> {
 		_context?: AgentToolContext,
 	): Promise<AgentToolResult<YieldDetails>> {
 		const raw = params as Record<string, unknown>;
-		const rawResult = raw.result;
-		if (!rawResult || typeof rawResult !== "object" || Array.isArray(rawResult)) {
+		const yieldType = parseYieldType(raw.type);
+		const resultRecord = resolveResultRecord(raw, yieldType);
+		if (resultRecord === undefined) {
 			throw new Error(`result must be an object containing either data or error. ${YIELD_RESULT_FORMAT_HINT}`);
 		}
-		const resultRecord = rawResult as Record<string, unknown>;
 		const errorMessage = typeof resultRecord.error === "string" ? resultRecord.error : undefined;
-		const data = resultRecord.data;
-		const yieldType = parseYieldType(raw.type);
+		let data = resultRecord.data;
 		const useLastTurn =
 			errorMessage === undefined && data === undefined && yieldType !== undefined && !("error" in resultRecord);
 		// Incremental array-typed sections carry partial data (one finding, one
@@ -374,15 +430,42 @@ export class YieldTool implements AgentTool<TSchema, YieldDetails> {
 				);
 			}
 		}
+		// A schema-bound terminal last-turn yield with no accumulated sections can
+		// only assemble raw prose, which finalization then rejects post-mortem as a
+		// fatal schema_violation the child can no longer correct. Catch it here as
+		// a retryable error instead. With sections present, a data-less finalize
+		// legitimately closes the incremental flow (assembly keeps the sections).
+		if (status === "success" && useLastTurn && !isIncremental && this.#validate && !this.#hasIncrementalSections) {
+			throw new Error(
+				"This task requires structured output matching the declared schema; a last-turn result cannot satisfy it. " +
+					`Submit the full object: {"result":{"data":<object matching the schema>}}.`,
+			);
+		}
 		if (status === "success" && !useLastTurn) {
 			if (data === null) {
 				throw new Error("data is required when yield indicates success");
 			}
-			const sectionFailure = isIncremental
-				? this.#validateIncrementalSection(yieldType as string[], data)
-				: this.#validate
-					? this.#validate(data)
-					: undefined;
+			const validateData = (value: unknown): JsonSchemaValidationResult | undefined =>
+				isIncremental
+					? this.#validateIncrementalSection(yieldType as string[], value)
+					: this.#validate
+						? this.#validate(value)
+						: undefined;
+			let sectionFailure = validateData(data);
+			if (sectionFailure && !sectionFailure.success && typeof data === "string") {
+				// Lossless recovery: a JSON-encoded payload string parses to exactly
+				// the intended value (executor finalization already parses terminal
+				// yields the same way). Never the reverse — stringifying objects to
+				// fit string-typed fields is silent corruption.
+				const parsed = parseJsonContainerString(data);
+				if (parsed !== undefined) {
+					const revalidated = validateData(parsed);
+					if (revalidated === undefined || revalidated.success) {
+						data = parsed;
+						sectionFailure = revalidated;
+					}
+				}
+			}
 			if (sectionFailure && !sectionFailure.success) {
 				this.#schemaValidationFailures++;
 				if (this.#schemaValidationFailures <= MAX_SCHEMA_RETRIES) {
@@ -401,6 +484,7 @@ export class YieldTool implements AgentTool<TSchema, YieldDetails> {
 		}

 		this.#emptyResultFailures = 0;
+		if (status === "success" && isIncremental) this.#hasIncrementalSections = true;
 		const responseText =
 			status === "aborted"
 				? `Task aborted: ${errorMessage}`
@@ -76,6 +76,142 @@ describe("YieldTool", () => {
 			useLastTurn: true,
 		});
 	});
+	it("finalizes type:'result' with the result wrapper omitted entirely as a last-turn yield", async () => {
+		// Gemini-flash traces: the description invites omitting `data`, and weak
+		// callers omit the whole `result` wrapper with it. Must not bounce with
+		// a retryable format error.
+		const tool = new YieldTool(createSession());
+		const result = await tool.execute("call-wrapperless-last-turn", { type: "result" } as never);
+		expect(result.details).toEqual({
+			data: undefined,
+			status: "success",
+			error: undefined,
+			type: "result",
+			useLastTurn: true,
+		});
+	});
+	it("rejects a schema-bound last-turn finalize with no accumulated sections as retryable", async () => {
+		// Instruction-followed punt observed in Gemini traces: `{type:"result"}`
+		// with a declared output schema. Accepting it terminates the child and
+		// finalization then fails post-mortem with an uncorrectable
+		// schema_violation; the tool must bounce it in-band instead.
+		const tool = new YieldTool(
+			createSession({
+				outputSchema: {
+					type: "object",
+					properties: { summary: { type: "string" } },
+					required: ["summary"],
+				},
+			}),
+		);
+		await expect(tool.execute("call-schema-punt", { type: "result" } as never)).rejects.toThrow(
+			/structured output matching the declared schema/,
+		);
+	});
+
+	it("accepts a data-less finalize after incremental sections even when schema-bound", async () => {
+		const tool = new YieldTool(
+			createSession({
+				outputSchema: {
+					type: "object",
+					properties: { findings: { type: "array", items: { type: "string" } } },
+					required: ["findings"],
+				},
+			}),
+		);
+		const section = await tool.execute("call-section", {
+			type: ["findings"],
+			result: { data: "one finding" },
+		} as never);
+		expect(section.details?.status).toBe("success");
+		const finalize = await tool.execute("call-finalize", { type: "result" } as never);
+		expect(finalize.details).toEqual({
+			data: undefined,
+			status: "success",
+			error: undefined,
+			type: "result",
+			useLastTurn: true,
+		});
+	});
+
+	it("salvages a top-level data payload missing the result wrapper", async () => {
+		const tool = new YieldTool(createSession());
+		const result = await tool.execute("call-unwrapped-data", { data: { ok: true } } as never);
+		expect(result.details).toEqual({ data: { ok: true }, status: "success", error: undefined });
+	});
+
+	it("salvages a top-level error missing the result wrapper", async () => {
+		const tool = new YieldTool(createSession());
+		const result = await tool.execute("call-unwrapped-error", { error: "blocked" } as never);
+		expect(result.details).toEqual({ data: undefined, status: "aborted", error: "blocked" });
+	});
+
+	it("parses a JSON-string result envelope losslessly", async () => {
+		const tool = new YieldTool(createSession());
+		const result = await tool.execute("call-string-envelope", {
+			result: '{"data":{"ok":true}}',
+		} as never);
+		expect(result.details).toEqual({ data: { ok: true }, status: "success", error: undefined });
+	});
+
+	it("parses JSON-string data when the schema rejects the string form", async () => {
+		const tool = new YieldTool(
+			createSession({
+				outputSchema: {
+					type: "object",
+					properties: { n: { type: "number" } },
+					required: ["n"],
+				},
+			}),
+		);
+		const result = await tool.execute("call-string-data", { result: { data: '{"n":4}' } } as never);
+		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.
+		const tool = new YieldTool(
+			createSession({
+				outputSchema: {
+					type: "object",
+					properties: { summary: { type: "string" } },
+					required: ["summary"],
+				},
+			}),
+		);
+		expect(tool.coerceArguments).toBe(false);
+		expect(() =>
+			validateToolArguments(tool as never, {
+				type: "toolCall",
+				id: "call-dict-summary",
+				name: "yield",
+				arguments: { result: { data: { summary: { purge: 13, keep: 20 } } } },
+			}),
+		).toThrow(/summary/);
+	});
+
+	it("verbatim arg validation passes conforming args through unmodified", () => {
+		const tool = new YieldTool(
+			createSession({
+				outputSchema: {
+					type: "object",
+					properties: { summary: { type: "string" } },
+					required: ["summary"],
+				},
+			}),
+		);
+		const args = { result: { data: { summary: "all good" } } };
+		const validated = validateToolArguments(tool as never, {
+			type: "toolCall",
+			id: "call-clean",
+			name: "yield",
+			arguments: args,
+		});
+		expect(validated).toEqual(args);
+	});

 	it("passes array-typed success through as an incremental result", async () => {
 		const tool = new YieldTool(createSession());