From 4810f47db9cbb6901d67ea68914e9ce9453857dc Mon Sep 17 00:00:00 2001
From: roboomp <omp@can.ac>
Date: Tue, 30 Jun 2026 06:20:03 +0000
Subject: [PATCH] fix(yield): validate incremental sections per-label to
 prevent fatal schema_violation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The yield tool's per-call schema validator was skipped entirely for incremental
yields (`type: ["<label>"]`), so when a subagent emitted a non-conforming value
for a known section (e.g. DeepSeek-v4-pro returning "Correct"/"correct."/"approved"
for the reviewer's `overall_correctness` enum), the call succeeded locally and
the model got no retry feedback. The mismatch only surfaced post-mortem in
`finalizeSubprocessOutput` as a fatal `schema_violation` — the parent agent
lost the entire result, with no recourse for the subagent to fix it.

Build a per-label sub-validator map alongside the full-schema validator: each
entry validates one section's `data` against its top-level property's sub-schema
(items schema for array-typed labels like `findings`). The yield tool runs this
map for incremental yields and routes failures through the same MAX_SCHEMA_RETRIES
budget the terminal path uses, so the model sees up to three corrective retries
and the existing schema-override safety net accepts the value with
SUBAGENT_WARNING_SCHEMA_OVERRIDDEN after exhaustion. Unknown labels remain
unconstrained so scratchpad/streaming sections still pass.

Fixes #3870
---
 packages/coding-agent/CHANGELOG.md            |   4 +
 .../src/tools/output-schema-validator.ts      |  38 ++++++
 packages/coding-agent/src/tools/yield.ts      |  67 +++++++---
 .../tools/output-schema-validator.test.ts     |  30 +++++
 .../coding-agent/test/tools/yield.test.ts     | 125 ++++++++++++++++++
 5 files changed, 249 insertions(+), 15 deletions(-)

diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md
index c44e8b2da..2587b61bf 100644
--- a/packages/coding-agent/CHANGELOG.md
+++ b/packages/coding-agent/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## [Unreleased]
 
+### Fixed
+
+- Fixed reviewer/`task` subagents whose incremental `yield` (`type: ["overall_correctness"]`, `type: ["findings"]`, …) carried a value that mismatched the matching property's sub-schema being silently accepted and then post-mortem rejected with `schema_violation` — opaquely swapping the agent's accepted output for an error blob. The yield tool now validates each incremental section's `data` against its top-level property's sub-schema (items schema for array-typed labels) and surfaces the same retry feedback as terminal yields, so models like `deepseek-v4-pro` that emit `"Correct"`/`"correct."`/`"approved"` for an enum field get up to three corrective retries; the existing `MAX_SCHEMA_RETRIES` override then accepts the value with `SUBAGENT_WARNING_SCHEMA_OVERRIDDEN` instead of losing the entire result. Unknown labels stay unconstrained ([#3870](https://github.com/can1357/oh-my-pi/issues/3870)).
+
 ## [16.2.7] - 2026-06-30
 
 ### Breaking Changes
diff --git a/packages/coding-agent/src/tools/output-schema-validator.ts b/packages/coding-agent/src/tools/output-schema-validator.ts
index 4041793e1..21edc37e2 100644
--- a/packages/coding-agent/src/tools/output-schema-validator.ts
+++ b/packages/coding-agent/src/tools/output-schema-validator.ts
@@ -21,6 +21,16 @@ export interface OutputValidator {
 	validate(value: unknown): JsonSchemaValidationResult;
 	/** Top-level required property names. Empty if the schema has no `required` array at root. */
 	readonly requiredFields: readonly string[];
+	/**
+	 * Per-label validators for incremental yields (`type: ["<label>"]`). Each entry validates the
+	 * `data` payload of a single section against the matching top-level property's sub-schema —
+	 * array-typed properties (e.g. `findings`) use the items schema since each yield contributes
+	 * one element, while scalar properties use the property schema directly. Unknown labels (not
+	 * top-level properties) have no entry and skip per-call validation. Lets the yield tool give
+	 * the model retry feedback on a section as soon as it arrives, instead of deferring every
+	 * mismatch to the parent's post-mortem `schema_violation`.
+	 */
+	readonly validateSection: ReadonlyMap<string, (value: unknown) => JsonSchemaValidationResult>;
 }
 
 export interface BuildOutputValidatorResult {
@@ -72,10 +82,38 @@ export function buildOutputValidator(schema: unknown): BuildOutputValidatorResul
 		validator: {
 			requiredFields: required,
 			validate: value => validateJsonSchemaValue(jsonSchemaRecord, value),
+			validateSection: buildSectionValidators(jsonSchemaRecord),
 		},
 	};
 }
 
+/**
+ * Build per-top-level-property validators for incremental yields.
+ *
+ * Each entry validates the `data` payload of one `type: ["<label>"]` section against the
+ * matching property's sub-schema — array-typed properties (e.g. `findings`, derived from JTD
+ * `elements`) use the items schema since each yield contributes one element, while scalar
+ * properties use the property schema directly. Unknown labels (anything not declared as a
+ * top-level property) are deliberately omitted so user-defined section labels still pass.
+ */
+function buildSectionValidators(
+	jsonSchema: Record<string, unknown>,
+): ReadonlyMap<string, (value: unknown) => JsonSchemaValidationResult> {
+	const validators = new Map<string, (value: unknown) => JsonSchemaValidationResult>();
+	const properties = jsonSchema.properties;
+	if (properties === null || typeof properties !== "object") return validators;
+	for (const [label, raw] of Object.entries(properties as Record<string, unknown>)) {
+		if (raw === null || typeof raw !== "object") continue;
+		const propRecord = raw as Record<string, unknown>;
+		const sectionSchema =
+			propRecord.type === "array" && propRecord.items !== undefined && propRecord.items !== null
+				? (propRecord.items as Record<string, unknown>)
+				: propRecord;
+		validators.set(label, value => validateJsonSchemaValue(sectionSchema, value));
+	}
+	return validators;
+}
+
 /** Produce the executor's headline+missing-required summary from a failed validation. */
 export function summarizeValidationFailure(
 	result: JsonSchemaValidationResult,
diff --git a/packages/coding-agent/src/tools/yield.ts b/packages/coding-agent/src/tools/yield.ts
index 3f2dfac0d..be5559b56 100644
--- a/packages/coding-agent/src/tools/yield.ts
+++ b/packages/coding-agent/src/tools/yield.ts
@@ -98,6 +98,16 @@ function parseYieldType(value: unknown): string | string[] | undefined {
 	throw new Error("type must be a string or non-empty array of strings");
 }
 
+/**
+ * Render an incremental yield's `type: [...]` labels as a quoted, comma-separated list for
+ * model-facing retry messages — keeps the failed section labelled even when the yield carried
+ * multiple labels at once.
+ */
+function formatYieldLabels(labels: readonly string[]): string {
+	if (labels.length === 0) return '""';
+	return labels.map(label => `"${label}"`).join(", ");
+}
+
 /**
  * Expand a plain-object `data` schema into a strict union that ALSO accepts each
  * top-level section value (and array element) on its own. Agents that yield
@@ -202,10 +212,12 @@ export class YieldTool implements AgentTool<TSchema, YieldDetails> {
 	lenientArgValidation = true;
 
 	readonly #validate?: (value: unknown) => JsonSchemaValidationResult;
+	readonly #validateSection?: ReadonlyMap<string, (value: unknown) => JsonSchemaValidationResult>;
 	#schemaValidationFailures = 0;
 
 	constructor(session: ToolSession) {
 		let validate: ((value: unknown) => JsonSchemaValidationResult) | undefined;
+		let validateSection: ReadonlyMap<string, (value: unknown) => JsonSchemaValidationResult> | undefined;
 		let parameters: TSchema;
 
 		try {
@@ -217,6 +229,7 @@ export class YieldTool implements AgentTool<TSchema, YieldDetails> {
 			} = buildOutputValidator(session.outputSchema);
 			if (validator) {
 				validate = value => validator.validate(value);
+				validateSection = validator.validateSection;
 			}
 
 			const schemaHint = formatSchema(normalizedSchema ?? session.outputSchema);
@@ -266,6 +279,7 @@ export class YieldTool implements AgentTool<TSchema, YieldDetails> {
 		}
 
 		this.#validate = validate;
+		this.#validateSection = validateSection;
 		this.parameters = parameters;
 	}
 
@@ -307,22 +321,25 @@ export class YieldTool implements AgentTool<TSchema, YieldDetails> {
 			if (data === null) {
 				throw new Error("data is required when yield indicates success");
 			}
-			if (this.#validate && !isIncremental) {
-				const parsed = this.#validate(data);
-				if (!parsed.success) {
-					this.#schemaValidationFailures++;
-					if (this.#schemaValidationFailures <= MAX_SCHEMA_RETRIES) {
-						const remaining = MAX_SCHEMA_RETRIES - this.#schemaValidationFailures;
-						const retryHint =
-							remaining > 0
-								? ` Call yield again with the corrected shape — ${remaining} retry attempt(s) remain before the schema constraint is dropped.`
-								: " Call yield again with the corrected shape — this is the final retry before the schema constraint is dropped.";
-						throw new Error(
-							`Output does not match schema: ${formatAllValidationIssues(parsed.issues)}.${retryHint}`,
-						);
-					}
-					schemaValidationOverridden = true;
+			const sectionFailure = isIncremental
+				? this.#validateIncrementalSection(yieldType as string[], data)
+				: this.#validate
+					? this.#validate(data)
+					: undefined;
+			if (sectionFailure && !sectionFailure.success) {
+				this.#schemaValidationFailures++;
+				if (this.#schemaValidationFailures <= MAX_SCHEMA_RETRIES) {
+					const remaining = MAX_SCHEMA_RETRIES - this.#schemaValidationFailures;
+					const retryHint =
+						remaining > 0
+							? ` Call yield again with the corrected shape — ${remaining} retry attempt(s) remain before the schema constraint is dropped.`
+							: " Call yield again with the corrected shape — this is the final retry before the schema constraint is dropped.";
+					const scope = isIncremental ? `Section ${formatYieldLabels(yieldType as string[])}` : "Output";
+					throw new Error(
+						`${scope} does not match schema: ${formatAllValidationIssues(sectionFailure.issues)}.${retryHint}`,
+					);
 				}
+				schemaValidationOverridden = true;
 			}
 		}
 
@@ -344,6 +361,26 @@ export class YieldTool implements AgentTool<TSchema, YieldDetails> {
 			},
 		};
 	}
+
+	/**
+	 * Validate the `data` payload of an incremental yield (`type: ["<label>", …]`) against
+	 * the matching property's sub-validator. Returns the first failure across all known labels,
+	 * or `undefined` when no label is recognised (user-defined section labels stay loose) or
+	 * when all known labels accept the value. Lets the model see the same retry feedback that
+	 * the terminal-yield path already produces, instead of leaking the mismatch through to
+	 * the parent's post-mortem `schema_violation`.
+	 */
+	#validateIncrementalSection(labels: string[], data: unknown): JsonSchemaValidationResult | undefined {
+		const subValidators = this.#validateSection;
+		if (!subValidators || subValidators.size === 0) return undefined;
+		for (const label of labels) {
+			const sub = subValidators.get(label);
+			if (!sub) continue;
+			const parsed = sub(data);
+			if (!parsed.success) return parsed;
+		}
+		return undefined;
+	}
 }
 
 // Register subprocess tool handler for extraction + termination.
diff --git a/packages/coding-agent/test/tools/output-schema-validator.test.ts b/packages/coding-agent/test/tools/output-schema-validator.test.ts
index 0657e2d8e..a89b50c8b 100644
--- a/packages/coding-agent/test/tools/output-schema-validator.test.ts
+++ b/packages/coding-agent/test/tools/output-schema-validator.test.ts
@@ -69,6 +69,36 @@ describe("buildOutputValidator", () => {
 			"required",
 		]);
 	});
+
+	it("exposes per-label sub-validators that accept items (not whole arrays) for elements properties", () => {
+		const { validator } = buildOutputValidator({
+			properties: {
+				overall_correctness: { enum: ["correct", "incorrect"] },
+				explanation: { type: "string" },
+			},
+			optionalProperties: {
+				findings: {
+					elements: {
+						properties: { title: { type: "string" }, body: { type: "string" } },
+					},
+				},
+			},
+		});
+		expect(validator).toBeDefined();
+		const sections = validator?.validateSection;
+		expect(sections).toBeDefined();
+		// Scalar enum: per-section validator enforces the enum directly.
+		expect(sections?.get("overall_correctness")?.("correct").success).toBe(true);
+		expect(sections?.get("overall_correctness")?.("Correct").success).toBe(false);
+		// String property: any string passes, non-strings fail.
+		expect(sections?.get("explanation")?.("ok").success).toBe(true);
+		expect(sections?.get("explanation")?.(123).success).toBe(false);
+		// Array property: each section validates ONE item against the items schema, not the whole array.
+		expect(sections?.get("findings")?.({ title: "t", body: "b" }).success).toBe(true);
+		expect(sections?.get("findings")?.([{ title: "t", body: "b" }]).success).toBe(false);
+		// Unknown labels have no validator so user-defined sections stay loose.
+		expect(sections?.has("scratchpad")).toBe(false);
+	});
 });
 describe("summarizeValidationFailure", () => {
 	it("returns an empty summary when the result is a success", () => {
diff --git a/packages/coding-agent/test/tools/yield.test.ts b/packages/coding-agent/test/tools/yield.test.ts
index 1c2bdc24c..b05640ea1 100644
--- a/packages/coding-agent/test/tools/yield.test.ts
+++ b/packages/coding-agent/test/tools/yield.test.ts
@@ -121,6 +121,131 @@ describe("YieldTool", () => {
 		});
 	});
 
+	it("validates incremental sections against per-label sub-schemas with retry feedback", async () => {
+		// Regression for issue #3870: DeepSeek emits `type: ["overall_correctness"]` with
+		// non-enum values like "Correct" or "approved". The yield tool used to skip
+		// validation for incremental yields entirely, so the model got no retry feedback
+		// and the parent saw a fatal `schema_violation` post-mortem.
+		const tool = new YieldTool(
+			createSession({
+				outputSchema: {
+					properties: {
+						overall_correctness: { enum: ["correct", "incorrect"] },
+						explanation: { type: "string" },
+						confidence: { type: "number" },
+					},
+					optionalProperties: {
+						findings: {
+							elements: { properties: { title: { type: "string" }, body: { type: "string" } } },
+						},
+					},
+				},
+			}),
+		);
+
+		// Attempt 1: off-enum value rejected with the section label in the error.
+		await expect(
+			tool.execute("call-bad-1", { type: ["overall_correctness"], result: { data: "Correct" } } as never),
+		).rejects.toThrow(/Section "overall_correctness" does not match schema.*2 retry attempt\(s\) remain/);
+
+		// Attempt 2 and 3 advertise dwindling retries; attempt 3 names this as the last one.
+		await expect(
+			tool.execute("call-bad-2", { type: ["overall_correctness"], result: { data: "correct." } } as never),
+		).rejects.toThrow(/1 retry attempt\(s\) remain/);
+		await expect(
+			tool.execute("call-bad-3", { type: ["overall_correctness"], result: { data: "approved" } } as never),
+		).rejects.toThrow(/this is the final retry/);
+
+		// 4th invalid yield is accepted with schemaOverridden so the parent still gets a result.
+		const overrideResult = await tool.execute("call-bad-4", {
+			type: ["overall_correctness"],
+			result: { data: "still-wrong" },
+		} as never);
+		expect(overrideResult.details?.schemaOverridden).toBe(true);
+
+		// A fresh tool accepts a valid enum value without ticking the counter.
+		const fresh = new YieldTool(
+			createSession({
+				outputSchema: {
+					properties: {
+						overall_correctness: { enum: ["correct", "incorrect"] },
+						explanation: { type: "string" },
+						confidence: { type: "number" },
+					},
+				},
+			}),
+		);
+		const valid = await fresh.execute("call-good", {
+			type: ["overall_correctness"],
+			result: { data: "correct" },
+		} as never);
+		expect(valid.details).toEqual({
+			data: "correct",
+			status: "success",
+			error: undefined,
+			type: ["overall_correctness"],
+			useLastTurn: undefined,
+			schemaOverridden: undefined,
+		});
+	});
+
+	it("validates incremental items for array-typed labels against the element schema", async () => {
+		// Each `type: ["findings"]` yield is one finding; the per-call validator runs against the
+		// items schema (not the array schema), so a missing required field surfaces immediately
+		// instead of being swallowed by the post-mortem assembly.
+		const tool = new YieldTool(
+			createSession({
+				outputSchema: {
+					optionalProperties: {
+						findings: {
+							elements: {
+								properties: {
+									title: { type: "string" },
+									body: { type: "string" },
+								},
+							},
+						},
+					},
+				},
+			}),
+		);
+
+		const accepted = await tool.execute("call-finding-ok", {
+			type: ["findings"],
+			result: { data: { title: "bug", body: "details" } },
+		} as never);
+		expect(accepted.details?.data).toEqual({ title: "bug", body: "details" });
+
+		await expect(
+			tool.execute("call-finding-missing", {
+				type: ["findings"],
+				result: { data: { title: "only-title" } },
+			} as never),
+		).rejects.toThrow(/Section "findings" does not match schema.*body/);
+	});
+
+	it("leaves user-defined section labels unconstrained", async () => {
+		// Labels that are not top-level properties in the output schema have no per-call
+		// validator — they're scratchpad/streaming sections the agent invents at runtime and
+		// must not be rejected.
+		const tool = new YieldTool(
+			createSession({
+				outputSchema: {
+					properties: {
+						overall_correctness: { enum: ["correct", "incorrect"] },
+						explanation: { type: "string" },
+						confidence: { type: "number" },
+					},
+				},
+			}),
+		);
+		const result = await tool.execute("call-scratchpad", {
+			type: ["scratchpad"],
+			result: { data: { anything: "goes", n: 3 } },
+		} as never);
+		expect(result.details?.data).toEqual({ anything: "goes", n: 3 });
+	});
+
 	it("rejects missing success data unless a yield type requests last-turn mode", async () => {
 		const tool = new YieldTool(createSession());
 		await expect(tool.execute("call-untyped-empty", { result: {} } as never)).rejects.toThrow(