diff --git a/packages/coding-agent/src/prompts/agents/reviewer.md b/packages/coding-agent/src/prompts/agents/reviewer.md index 934dccd6a..f76da3652 100644 --- a/packages/coding-agent/src/prompts/agents/reviewer.md +++ b/packages/coding-agent/src/prompts/agents/reviewer.md @@ -1,7 +1,7 @@ --- name: reviewer description: "Code review specialist for quality/security analysis" -tools: read, grep, glob, bash, lsp, web_search, ast_grep, report_finding +tools: read, grep, glob, bash, lsp, web_search, ast_grep spawns: explore model: pi/slow thinking-level: high @@ -22,7 +22,7 @@ output: optionalProperties: findings: metadata: - description: Auto-populated from report_finding; don't set manually + description: Populate via incremental `yield` sections under `type: ["findings"]`; don't repeat it in a final payload. elements: properties: title: @@ -60,8 +60,8 @@ Identify bugs the author would want fixed before merge. 1. Run `git diff`, `jj diff --git`, or `gh pr diff ` to view patch 2. Read modified files for full context -3. Call `report_finding` per issue -4. Call `yield` with verdict +3. Record each issue with incremental `yield` using `type: ["findings"]` +4. Record `overall_correctness`, `explanation`, and `confidence` with incremental `yield` sections, then stop so idle finalization assembles the result Bash is read-only: `git diff`, `git log`, `git show`, `jj diff --git`, `gh pr diff`. You NEVER make file edits or trigger builds. @@ -115,7 +115,7 @@ memcpy(buf, data.ptr, data.length); -Each `report_finding` requires: +Each finding uses incremental `yield` with `type: ["findings"]` and `result.data` containing: - `title`: Imperative, ≤80 chars - `body`: One paragraph - `priority`: 0-3 @@ -123,11 +123,12 @@ Each `report_finding` requires: - `file_path`: Path to affected file - `line_start`, `line_end`: Range ≤10 lines, must overlap diff -Final `yield` call (payload under `result.data`): -- `result.data.overall_correctness`: "correct" (no bugs/blockers) or "incorrect" -- `result.data.explanation`: Plain text, 1-3 sentences summarizing verdict. Don't repeat findings (captured via `report_finding`). -- `result.data.confidence`: 0.0-1.0 -- `result.data.findings`: Optional; MUST omit (auto-populated from `report_finding`) +Verdict fields also use incremental `yield` sections: +- `type: ["overall_correctness"]` with `"correct"` (no bugs/blockers) or `"incorrect"` +- `type: ["explanation"]` with a plain-text 1-3 sentence verdict summary +- `type: ["confidence"]` with a 0.0-1.0 confidence value + +Do not emit a separate submit tool call or duplicate `findings` in another payload. Once all sections are recorded, stop and let idle finalization assemble the result. You NEVER output JSON or code blocks. diff --git a/packages/coding-agent/src/prompts/review-custom-request.md b/packages/coding-agent/src/prompts/review-custom-request.md index ade98989b..89e916bff 100644 --- a/packages/coding-agent/src/prompts/review-custom-request.md +++ b/packages/coding-agent/src/prompts/review-custom-request.md @@ -14,8 +14,7 @@ Create exactly **1 reviewer task**. Its assignment MUST include the custom instr Reviewer MUST: 1. Follow the custom instructions below 2. Read the referenced files or workspace context needed to evaluate them -3. Call `report_finding` per issue -4. Call `yield` with verdict when done +3. Use incremental `yield` sections for findings and verdict fields; do NOT call a separate finding tool ### Custom Instructions diff --git a/packages/coding-agent/src/prompts/review-request.md b/packages/coding-agent/src/prompts/review-request.md index b0c001d64..71473c0b8 100644 --- a/packages/coding-agent/src/prompts/review-request.md +++ b/packages/coding-agent/src/prompts/review-request.md @@ -38,8 +38,7 @@ Reviewer MUST: 1. Focus ONLY on assigned files 2. {{#if skipDiff}}{{diffInstruction}}{{else}}MUST use diff hunks below (NEVER re-run git diff){{/if}} 3. {{contextInstruction}} -4. Call `report_finding` per issue -5. Call `yield` with verdict when done +4. Use incremental `yield` sections for findings and verdict fields; do NOT call a separate finding tool {{#if skipDiff}} ### Diff Previews diff --git a/packages/coding-agent/src/prompts/system/subagent-system-prompt.md b/packages/coding-agent/src/prompts/system/subagent-system-prompt.md index f38f2cf45..daebdad7b 100644 --- a/packages/coding-agent/src/prompts/system/subagent-system-prompt.md +++ b/packages/coding-agent/src/prompts/system/subagent-system-prompt.md @@ -50,13 +50,16 @@ Use `irc` only for quick coordination, never long-form content. Address peers by COMPLETION =================================== -No TODO tracking, no progress updates. Execute, call `yield`, done. +No TODO tracking, no progress updates. Execute; report results with `yield`. -While work remains, you MUST continue with another tool call — investigate, edit, run, verify. Save narrative for the final `yield` payload. +While work remains, you MUST continue with another tool call — investigate, edit, run, verify. Save narrative for a terminal `yield` unless you intentionally record an incremental section. -When finished, you MUST call `yield` exactly once. This is like writing to a ticket: provide what is required and close it. +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. +- Use `type: string` for a terminal result; if data is omitted, your last assistant turn becomes the raw final result. -This is your only way to return a result. You NEVER put JSON in plain text, and you NEVER substitute a text summary for the structured `result.data` parameter. +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`. {{#if outputSchema}} Your result MUST match this TypeScript interface: @@ -65,7 +68,7 @@ Your result MUST match this TypeScript interface: ``` {{/if}} -Giving up is a last resort. If truly blocked, you MUST call `yield` exactly once with `result.error` describing what you tried and the exact blocker. +Giving up is a last resort. If truly blocked, you MUST terminal-yield `result.error` describing what you tried and the exact blocker. You NEVER give up due to uncertainty, missing information obtainable via tools or repo context, or needing a design decision you can derive yourself. You MUST keep going until this ticket is closed. This matters. diff --git a/packages/coding-agent/src/prompts/system/subagent-yield-reminder.md b/packages/coding-agent/src/prompts/system/subagent-yield-reminder.md index dfadd4588..9669a2f99 100644 --- a/packages/coding-agent/src/prompts/system/subagent-yield-reminder.md +++ b/packages/coding-agent/src/prompts/system/subagent-yield-reminder.md @@ -1,12 +1,13 @@ Your last turn ended without a tool call, so the session went idle. This is reminder {{retryCount}} of {{maxRetries}}. -Every turn MUST end with a tool call. Pick exactly one of: -1. **Resume the work** — if the assignment is not finished, call the next tool you would have called (edit, write, bash, search, etc.). NEVER yield. NEVER treat this reminder as a forced stop. -2. **Yield with success** — only if the assignment is genuinely complete: call `yield` with the structured payload in `result.data`. -3. **Yield with error** — only if you hit a real, concrete blocker you can name (missing file, unavailable API, contradictory spec). Describe what you tried and the exact blocker. NEVER fabricate a "forced immediate-yield" or "system reminder required termination" reason — this reminder is not a blocker. +Every turn MUST end with a tool call. Pick the first that applies: +1. **Resume the work** — if the assignment is not finished and you are not recording an incremental section, call the next tool you would have called (edit, write, bash, search, etc.). NEVER treat this reminder as a forced stop. +2. **Yield an incremental section** — only when useful for the assignment: call `yield` with non-empty `type: string[]`; matching sections accumulate and the task continues. +3. **Yield with success** — only if the assignment is genuinely complete: call terminal `yield`. Omit `type` for the single final structured result in `result.data`; use `type: string` to finalize from the last assistant turn when data is omitted. +4. **Yield with error** — only if you hit a real, concrete blocker you can name (missing file, unavailable API, contradictory spec). Describe what you tried and the exact blocker. NEVER fabricate a "forced immediate-yield" or "system reminder required termination" reason — this reminder is not a blocker. -Default to option 1 unless the work is actually done or actually blocked. +Default to option 1 unless the work is actually done, actually blocked, or ready for an incremental section. You NEVER end this turn with text only. diff --git a/packages/coding-agent/src/task/executor.ts b/packages/coding-agent/src/task/executor.ts index dbefba996..03cdfbdb8 100644 --- a/packages/coding-agent/src/task/executor.ts +++ b/packages/coding-agent/src/task/executor.ts @@ -71,8 +71,11 @@ import { TASK_SUBAGENT_LIFECYCLE_CHANNEL, TASK_SUBAGENT_PROGRESS_CHANNEL, type TaskToolDetails, + type YieldItem, } from "./types"; +export type { YieldItem } from "./types"; + const MCP_CALL_TIMEOUT_MS = 60_000; /** @@ -517,19 +520,152 @@ function resolveFallbackCompletion(rawOutput: string, outputSchema: unknown): { return { data: candidate }; } -export interface YieldItem { - data?: unknown; - status?: "success" | "aborted"; - error?: string; - /** - * Set by the in-tool yield validator when it exhausted its retry budget - * (MAX_SCHEMA_RETRIES) and accepted a schema-invalid payload anyway. - * `finalizeSubprocessOutput` honors this by serializing the payload and - * surfacing a stderr warning, instead of re-emitting `schema_violation` - * — which would silently swap the subagent's "accepted" view for a - * different, opaque error blob in the parent's view of the result. - */ - schemaOverridden?: boolean; +interface AssembledYieldResult { + data: unknown; + schemaOverridden: boolean; + rawText: boolean; + missingData: boolean; +} + +function isIncrementalYieldType(type: YieldItem["type"]): type is string[] { + return Array.isArray(type) && type.length > 0; +} + +function getYieldLabels(type: YieldItem["type"]): string[] { + if (typeof type === "string") { + const label = type.trim(); + return label ? [label] : []; + } + if (!Array.isArray(type)) return []; + const labels: string[] = []; + for (const value of type) { + if (typeof value !== "string") continue; + const label = value.trim(); + if (label) labels.push(label); + } + return labels; +} + +function resolveYieldPayload( + item: YieldItem, + lastAssistantText: string | undefined, + labels: string[], +): { value: unknown; fromLastAssistantText: boolean; missingData: boolean } { + const hasData = item.data !== undefined; + const shouldUseLastTurn = item.useLastTurn === true || (labels.length > 0 && !hasData); + if (shouldUseLastTurn && lastAssistantText !== undefined) { + return { + value: lastAssistantText, + fromLastAssistantText: true, + missingData: lastAssistantText.length === 0, + }; + } + return { + value: item.data, + fromLastAssistantText: false, + missingData: item.data === undefined || item.data === null, + }; +} + +function appendYieldSection( + sections: Record, + sectionCounts: Map, + label: string, + value: unknown, +): void { + const count = sectionCounts.get(label) ?? 0; + if (count === 0) { + sections[label] = value; + } else if (count === 1) { + sections[label] = [sections[label], value]; + } else { + (sections[label] as unknown[]).push(value); + } + sectionCounts.set(label, count + 1); +} + +/** + * Assemble typed yield calls into the final payload consumed by schema validation. + * + * A non-empty array `type` contributes an incremental section and never decides + * termination by itself. A string `type` with omitted `data` makes the last + * assistant turn the raw terminal result. Other string-typed yields contribute + * the terminal labelled section. Untyped terminal yields keep the historical + * "last yield wins" behavior unless no terminal yield exists, in which case + * accumulated typed sections finalize on idle. + */ +export function assembleYieldResult( + yieldItems: YieldItem[], + lastAssistantText?: string, +): AssembledYieldResult | undefined { + if (yieldItems.length === 0) return undefined; + let terminalItem: YieldItem | undefined; + for (let index = yieldItems.length - 1; index >= 0; index--) { + const item = yieldItems[index]; + if (!item) continue; + if (!isIncrementalYieldType(item.type)) { + terminalItem = item; + break; + } + } + let hasTypedSections = false; + for (const item of yieldItems) { + if (getYieldLabels(item.type).length > 0) { + hasTypedSections = true; + break; + } + } + if (terminalItem && typeof terminalItem.type === "string" && terminalItem.data === undefined) { + const resolved = resolveYieldPayload(terminalItem, lastAssistantText, getYieldLabels(terminalItem.type)); + return { + data: resolved.value, + schemaOverridden: terminalItem.schemaOverridden === true, + rawText: resolved.fromLastAssistantText && typeof resolved.value === "string", + missingData: resolved.missingData, + }; + } + if (!hasTypedSections && terminalItem) { + const resolved = resolveYieldPayload(terminalItem, lastAssistantText, []); + return { + data: resolved.value, + schemaOverridden: terminalItem.schemaOverridden === true, + rawText: resolved.fromLastAssistantText && typeof resolved.value === "string", + missingData: resolved.missingData, + }; + } + + const sections: Record = {}; + const sectionCounts = new Map(); + let schemaOverridden = false; + let missingData = false; + let hasSections = false; + + for (const item of yieldItems) { + if (item.status === "aborted") continue; + schemaOverridden ||= item.schemaOverridden === true; + const labels = getYieldLabels(item.type); + if (labels.length === 0) continue; + const resolved = resolveYieldPayload(item, lastAssistantText, labels); + missingData ||= resolved.missingData; + for (const label of labels) { + appendYieldSection(sections, sectionCounts, label, resolved.value); + hasSections = true; + } + if (!isIncrementalYieldType(item.type)) break; + } + + if (hasSections) { + return { data: sections, schemaOverridden, rawText: false, missingData }; + } + + if (!terminalItem) return undefined; + const resolved = resolveYieldPayload(terminalItem, lastAssistantText, []); + return { + data: resolved.value, + schemaOverridden: terminalItem.schemaOverridden === true, + rawText: resolved.fromLastAssistantText && typeof resolved.value === "string", + missingData: resolved.missingData, + }; } interface FinalizeSubprocessOutputArgs { @@ -541,6 +677,7 @@ interface FinalizeSubprocessOutputArgs { yieldItems?: YieldItem[]; reportFindings?: ReviewFinding[]; outputSchema: unknown; + lastAssistantText?: string; } interface FinalizeSubprocessOutputResult { @@ -583,7 +720,7 @@ function buildSchemaViolationOutcome( export function finalizeSubprocessOutput(args: FinalizeSubprocessOutputArgs): FinalizeSubprocessOutputResult { let { rawOutput, exitCode, stderr } = args; - const { yieldItems, reportFindings, doneAborted, signalAborted, outputSchema } = args; + const { yieldItems, reportFindings, doneAborted, signalAborted, outputSchema, lastAssistantText } = args; let abortedViaYield = false; const hasYield = Array.isArray(yieldItems) && yieldItems.length > 0; const hadFailureBeforeYield = exitCode !== 0 && stderr.trim().length > 0; @@ -600,15 +737,16 @@ export function finalizeSubprocessOutput(args: FinalizeSubprocessOutputArgs): Fi rawOutput = `{"aborted":true,"error":"${lastYield.error || "Unknown error"}"}`; } } else { - const submitData = lastYield?.data; - if (submitData === null || submitData === undefined) { + const assembled = assembleYieldResult(yieldItems, lastAssistantText); + if (!assembled || assembled.missingData) { rawOutput = rawOutput ? `${SUBAGENT_WARNING_NULL_YIELD}\n\n${rawOutput}` : SUBAGENT_WARNING_NULL_YIELD; } else { const { validator, error: schemaError } = buildOutputValidator(outputSchema); - const overridden = lastYield?.schemaOverridden === true; - const completeData = normalizeCompleteData(submitData, reportFindings, validator); + const completeData = assembled.rawText + ? assembled.data + : normalizeCompleteData(assembled.data, reportFindings, validator); const result = - schemaError || overridden + schemaError || assembled.schemaOverridden ? { success: true as const } : (validator?.validate(completeData) ?? { success: true as const }); if (!result.success) { @@ -619,14 +757,17 @@ export function finalizeSubprocessOutput(args: FinalizeSubprocessOutputArgs): Fi exitCode = outcome.exitCode; } else { try { - rawOutput = JSON.stringify(completeData, null, 2) ?? "null"; + rawOutput = + assembled.rawText && typeof completeData === "string" + ? completeData + : (JSON.stringify(completeData, null, 2) ?? "null"); } catch (err) { const errorMessage = err instanceof Error ? err.message : String(err); rawOutput = `{"error":"Failed to serialize yield data: ${errorMessage}"}`; } if (!hadFailureBeforeYield) { exitCode = 0; - stderr = overridden + stderr = assembled.schemaOverridden ? SUBAGENT_WARNING_SCHEMA_OVERRIDDEN : schemaError ? `invalid output schema: ${schemaError}` @@ -1665,6 +1806,7 @@ async function finalizeRunResult(args: FinalizeRunArgs): Promise { yieldItems, reportFindings, outputSchema: args.outputSchema, + lastAssistantText: monitor.lastAssistantSalvageText(), }); } finally { popLoopPhase(); diff --git a/packages/coding-agent/src/task/render.ts b/packages/coding-agent/src/task/render.ts index 459ea0624..b1a5b5df9 100644 --- a/packages/coding-agent/src/task/render.ts +++ b/packages/coding-agent/src/task/render.ts @@ -32,6 +32,7 @@ import { type SubmitReviewDetails, } from "../tools/review"; import { framedBlock, renderStatusLine } from "../tui"; +import { assembleYieldResult } from "./executor"; import { repairDoubleEncodedJsonString } from "./repair-args"; import { subprocessToolRegistry } from "./subprocess-tool-registry"; import type { AgentProgress, SingleResult, TaskItem, TaskParams, TaskToolDetails } from "./types"; @@ -143,6 +144,44 @@ function normalizeReportFindings(value: unknown): ReportFindingDetails[] { return findings; } +function extractIncrementalReviewResult(value: unknown): { summary: SubmitReviewDetails; findings: ReportFindingDetails[] } | undefined { + const yieldItems = normalizeYieldData(value).map(item => ({ + data: item.data, + type: item.type, + status: item.status === "aborted" ? "aborted" : item.status === "success" ? "success" : undefined, + useLastTurn: item.useLastTurn, + })); + const assembled = assembleYieldResult(yieldItems); + const data = assembled?.data; + if (!data || typeof data !== "object" || Array.isArray(data)) return undefined; + const record = data as Record; + const overallCorrectness = record.overall_correctness; + const explanation = record.explanation; + const confidence = record.confidence; + if ( + (overallCorrectness !== "correct" && overallCorrectness !== "incorrect") || + typeof explanation !== "string" || + typeof confidence !== "number" + ) { + return undefined; + } + return { + summary: { + overall_correctness: overallCorrectness, + explanation, + confidence, + }, + findings: normalizeReportFindings(record.findings), + }; +} + +interface RenderYieldItem { + data?: unknown; + type?: string | string[]; + status?: string; + useLastTurn?: boolean; +} + /** * Normalize the `yield` slot of `extractedToolData` into an array of * yield-detail records. The subprocess executor always populates this slot as @@ -153,14 +192,82 @@ function normalizeReportFindings(value: unknown): ReportFindingDetails[] { * A single object is wrapped as a 1-element array so the review verdict still * renders; non-object primitives drop out. */ -function normalizeYieldData(value: unknown): Array<{ data: unknown }> { - if (Array.isArray(value)) { - return value.filter((item): item is { data: unknown } => item !== null && typeof item === "object"); +function normalizeYieldData(value: unknown): RenderYieldItem[] { + const items = Array.isArray(value) ? value : value !== null && typeof value === "object" ? [value] : []; + const normalized: RenderYieldItem[] = []; + for (const item of items) { + if (item === null || typeof item !== "object") continue; + const record = item as Record; + const typeValue = record.type; + let type: RenderYieldItem["type"]; + if (typeof typeValue === "string") { + type = typeValue; + } else if (Array.isArray(typeValue)) { + const labels: string[] = []; + let allLabels = true; + for (const label of typeValue) { + if (typeof label !== "string") { + allLabels = false; + break; + } + labels.push(label); + } + if (allLabels) type = labels; + } + normalized.push({ + data: record.data, + type, + status: typeof record.status === "string" ? record.status : undefined, + useLastTurn: record.useLastTurn === true ? true : undefined, + }); } - if (value !== null && typeof value === "object") { - return [value as { data: unknown }]; + return normalized; +} + +function getRenderYieldLabels(type: RenderYieldItem["type"]): string[] { + if (typeof type === "string") { + const label = type.trim(); + return label ? [label] : []; } - return []; + if (!Array.isArray(type)) return []; + const labels: string[] = []; + for (const value of type) { + const label = value.trim(); + if (label) labels.push(label); + } + return labels; +} + +function formatYieldPreview(item: RenderYieldItem): string { + if (item.useLastTurn === true && item.data === undefined) return "last assistant turn"; + if (item.data === undefined) return "last assistant turn"; + if (typeof item.data === "string") return previewLine(replaceTabs(item.data), 70); + try { + return previewLine(replaceTabs(JSON.stringify(item.data) ?? "null"), 70); + } catch { + return previewLine(replaceTabs(String(item.data)), 70); + } +} + +function renderTypedYieldSections(value: unknown, continuePrefix: string, expanded: boolean, theme: Theme): string[] { + const typedItems: Array<{ item: RenderYieldItem; labels: string[] }> = []; + for (const item of normalizeYieldData(value)) { + const labels = getRenderYieldLabels(item.type); + if (labels.length === 0) continue; + typedItems.push({ item, labels }); + } + const displayCount = expanded ? typedItems.length : 3; + const lines: string[] = []; + for (const { item, labels } of typedItems.slice(-displayCount)) { + const terminal = !Array.isArray(item.type); + const prefix = terminal ? "yield" : "yield+"; + const label = `${prefix}[${labels.join(", ")}]`; + lines.push(`${continuePrefix}${theme.fg("dim", label)}: ${theme.fg("dim", formatYieldPreview(item))}`); + } + if (typedItems.length > displayCount) { + lines.push(`${continuePrefix}${theme.fg("dim", formatMoreItems(typedItems.length - displayCount, "yield"))}`); + } + return lines; } function formatJsonScalar(value: unknown, _theme: Theme): string { @@ -808,10 +915,18 @@ function renderAgentProgress( // Render extracted tool data inline (e.g., review findings) if (progress.extractedToolData) { - // For completed tasks, check for review verdict from yield tool + // For completed tasks, prefer review verdicts assembled from incremental + // yield sections. Fall back to the legacy `report_finding` side-channel. if (progress.status === "completed") { const completeData = normalizeYieldData(progress.extractedToolData.yield); + const incrementalReview = extractIncrementalReviewResult(progress.extractedToolData.yield); const reportFindingData = normalizeReportFindings(progress.extractedToolData.report_finding); + if (incrementalReview) { + lines.push( + ...renderReviewResult(incrementalReview.summary, incrementalReview.findings, continuePrefix, expanded, theme), + ); + return lines; // Review result handles its own rendering + } const reviewData = completeData .map(c => c.data as SubmitReviewDetails) .filter(d => d && typeof d === "object" && "overall_correctness" in d); @@ -825,6 +940,11 @@ function renderAgentProgress( for (const toolName in progress.extractedToolData) { const dataArray = progress.extractedToolData[toolName]; + if (toolName === "yield") { + lines.push(...renderTypedYieldSections(dataArray, continuePrefix, expanded, theme)); + continue; + } + // Handle report_finding with tree formatting if (toolName === "report_finding") { const findings = normalizeReportFindings(dataArray); @@ -1068,23 +1188,28 @@ function renderAgentResult( `${continuePrefix}${theme.fg("error", theme.status.aborted)} ${theme.fg("dim", previewLine(result.abortReason, 80))}`, ); } - // Check for review result (yield with review schema + report_finding) - // Check for review result (yield with review schema + report_finding). + // Check for review result, preferring incremental yield sections and falling + // back to the legacy `report_finding` side-channel. // `normalizeYieldData` guards against a stray non-array `yield` slot — // optional chaining on `.map` only short-circuits on null/undefined and // would otherwise crash the renderer with `TypeError: completeData?.map // is not a function` when the slot is a plain object (see issue #1987). const completeData = normalizeYieldData(result.extractedToolData?.yield); const reportFindingData = normalizeReportFindings(result.extractedToolData?.report_finding); + const incrementalReview = extractIncrementalReviewResult(result.extractedToolData?.yield); - // Extract review verdict from yield tool's data field if it matches SubmitReviewDetails + if (incrementalReview) { + lines.push(...renderReviewResult(incrementalReview.summary, incrementalReview.findings, continuePrefix, expanded, theme)); + return lines; + } + + // Extract review verdict from legacy yield summary objects if present. const reviewData = completeData .map(c => c.data as SubmitReviewDetails) .filter(d => d && typeof d === "object" && "overall_correctness" in d); const submitReviewData = reviewData.length > 0 ? reviewData : undefined; if (submitReviewData) { - // Use combined review renderer const summary = submitReviewData[submitReviewData.length - 1]; const findings = reportFindingData; lines.push(...renderReviewResult(summary, findings, continuePrefix, expanded, theme)); @@ -1092,9 +1217,7 @@ function renderAgentResult( } if (reportFindingData.length > 0) { const hasCompleteData = completeData.length > 0; - const message = hasCompleteData - ? "Review verdict missing expected fields" - : "Review incomplete (yield not called)"; + const message = hasCompleteData ? "Review verdict missing expected fields" : "Review incomplete (yield not called)"; lines.push(`${continuePrefix}${theme.fg("warning", theme.status.warning)} ${theme.fg("dim", message)}`); lines.push(`${continuePrefix}${formatFindingSummary(reportFindingData, theme)}`); lines.push(...renderFindings(reportFindingData, continuePrefix, expanded, theme)); @@ -1105,9 +1228,18 @@ function renderAgentResult( let hasCustomRendering = false; const deferredToolLines: string[] = []; if (result.extractedToolData) { - for (const [toolName, dataArray] of Object.entries(result.extractedToolData)) { + for (const toolName in result.extractedToolData) { + const dataArray = result.extractedToolData[toolName]; + if (toolName === "yield") { + const yieldLines = renderTypedYieldSections(dataArray, continuePrefix, expanded, theme); + if (yieldLines.length > 0) { + hasCustomRendering = true; + lines.push(...yieldLines); + } + continue; + } // Skip review tools - handled above - if (toolName === "yield" || toolName === "report_finding") continue; + if (toolName === "report_finding") continue; const isTaskTool = toolName === "task"; if (isTaskTool && (dataArray as unknown[]).length > 0) { diff --git a/packages/coding-agent/src/task/types.ts b/packages/coding-agent/src/task/types.ts index 47c104d94..58f1db745 100644 --- a/packages/coding-agent/src/task/types.ts +++ b/packages/coding-agent/src/task/types.ts @@ -257,6 +257,23 @@ export interface AgentDefinition { filePath?: string; } +/** Details extracted from a subagent `yield` tool call for final-result assembly and task rendering. */ +export interface YieldItem { + data?: unknown; + status?: "success" | "aborted"; + error?: string; + /** A string label is terminal; a non-empty array of labels is incremental. */ + type?: string | string[]; + /** Resolve this yield's payload from the latest durable assistant text instead of `data`. */ + useLastTurn?: boolean; + /** + * Set by the in-tool yield validator when it exhausted its retry budget and + * accepted schema-invalid data anyway. The executor preserves that override + * during post-mortem validation. + */ + schemaOverridden?: boolean; +} + /** Progress tracking for a single agent */ export interface AgentProgress { index: number; diff --git a/packages/coding-agent/src/tools/review.ts b/packages/coding-agent/src/tools/review.ts index 429cae650..e192bcdcf 100644 --- a/packages/coding-agent/src/tools/review.ts +++ b/packages/coding-agent/src/tools/review.ts @@ -1,9 +1,10 @@ /** - * Review tools - report_finding for structured code review. + * Legacy hidden review-finding tool for agents that have not migrated to + * incremental `yield` sections. * - * Used by the reviewer agent to report findings in a structured way. - * Hidden by default - only enabled when explicitly listed in agent's tools. - * Reviewers finish via `yield` tool with SubmitReviewDetails schema. + * Hidden by default - only enabled when explicitly listed in an agent's tools. + * Reviewers now finish via incremental `yield`; this tool remains for + * compatibility with older or custom review agents. */ // ───────────────────────────────────────────────────────────────────────────── @@ -72,8 +73,13 @@ interface ReportFindingDetails { line_end: number; } -function isFindingPriority(value: unknown): value is FindingPriority { - return value === "P0" || value === "P1" || value === "P2" || value === "P3"; +function normalizeFindingPriority(value: unknown): FindingPriority | undefined { + if (isFindingPriority(value)) return value; + if (value === 0) return "P0"; + if (value === 1) return "P1"; + if (value === 2) return "P2"; + if (value === 3) return "P3"; + return undefined; } export function parseReportFindingDetails(value: unknown): ReportFindingDetails | undefined { @@ -81,7 +87,7 @@ export function parseReportFindingDetails(value: unknown): ReportFindingDetails const title = typeof value.title === "string" ? value.title : undefined; const body = typeof value.body === "string" ? value.body : undefined; - const priority = isFindingPriority(value.priority) ? value.priority : undefined; + const priority = normalizeFindingPriority(value.priority); const confidence = typeof value.confidence === "number" && Number.isFinite(value.confidence) && diff --git a/packages/coding-agent/src/tools/yield.ts b/packages/coding-agent/src/tools/yield.ts index d4969611b..7d4db1ed3 100644 --- a/packages/coding-agent/src/tools/yield.ts +++ b/packages/coding-agent/src/tools/yield.ts @@ -1,7 +1,7 @@ /** - * Submit result tool for structured subagent output. + * Result submission tool for subagent output. * - * Subagents must call this tool to finish and return structured JSON output. + * Subagents can call this tool incrementally or terminally depending on `type`. */ import type { AgentTool, AgentToolContext, AgentToolResult, AgentToolUpdateCallback } from "@oh-my-pi/pi-agent-core"; import type { TSchema } from "@oh-my-pi/pi-ai/types"; @@ -17,9 +17,14 @@ import type { ToolSession } from "."; import { buildOutputValidator, formatAllValidationIssues } from "./output-schema-validator"; export interface YieldDetails { - data: unknown; + /** Successful result payload, or omitted when `useLastTurn` requests last-turn extraction. */ + data?: unknown; status: "success" | "aborted"; error?: string; + /** Optional result section/classification supplied by the yield caller. */ + type?: string | string[]; + /** True when the caller intentionally omitted success data so the executor uses the last assistant turn. */ + useLastTurn?: boolean; /** * Set when the yield tool exhausted its in-tool schema-retry budget * (MAX_SCHEMA_RETRIES) and accepted the data anyway. Surfaced so the @@ -66,33 +71,85 @@ function hasUnresolvedRefs(schema: unknown): boolean { return false; } +const yieldTypeSchema: Record = { + anyOf: [ + { type: "string" }, + { + type: "array", + minItems: 1, + items: { type: "string" }, + }, + ], + description: "Optional result type. A non-empty string array is incremental; a string is terminal.", +}; + +function isYieldType(value: unknown): value is string | string[] { + return ( + typeof value === "string" || + (Array.isArray(value) && value.length > 0 && value.every(item => typeof item === "string")) + ); +} + +function parseYieldType(value: unknown): string | string[] | undefined { + if (value === undefined) return undefined; + if (isYieldType(value)) return value; + throw new Error("type must be a string or non-empty array of strings"); +} + function wrapYieldParameters(dataSchema: Record): Record { + const successResultSchema = { + type: "object", + additionalProperties: false, + description: "task succeeded", + properties: { data: dataSchema }, + required: ["data"], + }; + const errorResultSchema = { + type: "object", + additionalProperties: false, + properties: { + error: { type: "string", description: "error message" }, + }, + required: ["error"], + }; + const lastTurnResultSchema = { + type: "object", + additionalProperties: false, + description: "typed task succeeded; data omitted so the last assistant turn is used", + properties: {}, + required: [], + }; return { type: "object", additionalProperties: false, description: "submit data or error", properties: { + type: yieldTypeSchema, result: { - anyOf: [ - { - type: "object", - additionalProperties: false, - description: "task succeeded", - properties: { data: dataSchema }, - required: ["data"], - }, - { - type: "object", - additionalProperties: false, - properties: { - error: { type: "string", description: "error message" }, - }, - required: ["error"], - }, - ], + anyOf: [successResultSchema, errorResultSchema, lastTurnResultSchema], }, }, required: ["result"], + allOf: [ + { + anyOf: [ + { + type: "object", + properties: { type: yieldTypeSchema }, + required: ["type"], + }, + { + type: "object", + properties: { + result: { + anyOf: [successResultSchema, errorResultSchema], + }, + }, + required: ["result"], + }, + ], + }, + ], }; } @@ -110,9 +167,9 @@ export class YieldTool implements AgentTool { readonly approval = "read" as const; readonly label = "Submit Result"; readonly description = - "Finish the task with structured JSON output. Call exactly once at the end of the task.\n\n" + - 'Pass `result: { data: }` for success, or `result: { error: "message" }` for failure.\n' + - "The `data`/`error` wrapper is required — do not put your output directly in `result`."; + "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: }` for success, or `result: { error: "message" }` for failure. Keep the `result` wrapper.'; readonly parameters: TSchema; strict = true; readonly intent = "omit" as const; @@ -198,15 +255,17 @@ export class YieldTool implements AgentTool { if (!rawResult || typeof rawResult !== "object" || Array.isArray(rawResult)) { throw new Error("result must be an object containing either data or error"); } - const resultRecord = rawResult as Record; const errorMessage = typeof resultRecord.error === "string" ? resultRecord.error : undefined; const data = resultRecord.data; + const yieldType = parseYieldType(raw.type); + const useLastTurn = + errorMessage === undefined && data === undefined && yieldType !== undefined && !("error" in resultRecord); if (errorMessage !== undefined && data !== undefined) { throw new Error("result cannot contain both data and error"); } - if (errorMessage === undefined && data === undefined) { + if (errorMessage === undefined && data === undefined && yieldType === undefined) { throw new Error( 'result must contain either `data` or `error`. Use `{result: {data: }}` for success or `{result: {error: "message"}}` for failure.', ); @@ -214,8 +273,8 @@ export class YieldTool implements AgentTool { const status = errorMessage !== undefined ? "aborted" : "success"; let schemaValidationOverridden = false; - if (status === "success") { - if (data === undefined || data === null) { + if (status === "success" && !useLastTurn) { + if (data === null) { throw new Error("data is required when yield indicates success"); } if (this.#validate) { @@ -245,7 +304,14 @@ export class YieldTool implements AgentTool { : "Result submitted."; return { content: [{ type: "text", text: responseText }], - details: { data, status, error: errorMessage, schemaOverridden: schemaValidationOverridden || undefined }, + details: { + data, + status, + error: errorMessage, + type: yieldType, + useLastTurn: useLastTurn || undefined, + schemaOverridden: schemaValidationOverridden || undefined, + }, }; } } @@ -262,8 +328,21 @@ subprocessToolRegistry.register("yield", { data: record.data, status, error: typeof record.error === "string" ? record.error : undefined, + type: isYieldType(record.type) ? record.type : undefined, + useLastTurn: record.useLastTurn === true ? true : undefined, schemaOverridden: record.schemaOverridden === true ? true : undefined, }; }, - shouldTerminate: event => !event.isError, + shouldTerminate: event => { + if (event.isError) return false; + const details = event.result?.details; + if (!details || typeof details !== "object") return true; + const record = details as Record; + return !( + record.status === "success" && + Array.isArray(record.type) && + record.type.length > 0 && + record.type.every(item => typeof item === "string") + ); + }, }); diff --git a/packages/coding-agent/test/task/executor-warnings.test.ts b/packages/coding-agent/test/task/executor-warnings.test.ts index 4167c75c0..073a80fdf 100644 --- a/packages/coding-agent/test/task/executor-warnings.test.ts +++ b/packages/coding-agent/test/task/executor-warnings.test.ts @@ -191,4 +191,110 @@ describe("subagent warning injection", () => { expect(JSON.parse(result.rawOutput)).toEqual({ verdict: "looks good" }); expect(result.stderr.startsWith("invalid output schema:")).toBe(true); }); + + it("assembles incremental typed yield sections on idle", () => { + const result = finalizeSubprocessOutput({ + rawOutput: "", + exitCode: 0, + stderr: "", + doneAborted: false, + signalAborted: false, + yieldItems: [ + { status: "success", type: ["summary"], data: "first" }, + { status: "success", type: ["summary", "notes"], data: { detail: "second" } }, + ], + outputSchema: { + type: "object", + required: ["summary", "notes"], + properties: { + summary: { type: "array", minItems: 2 }, + notes: { type: "object", required: ["detail"], properties: { detail: { type: "string" } } }, + }, + }, + }); + + expect(result.exitCode).toBe(0); + expect(result.hasYield).toBe(true); + expect(JSON.parse(result.rawOutput)).toEqual({ + summary: ["first", { detail: "second" }], + notes: { detail: "second" }, + }); + }); + + it("validates assembled typed yield data against the output schema", () => { + const result = finalizeSubprocessOutput({ + rawOutput: "", + exitCode: 0, + stderr: "", + doneAborted: false, + signalAborted: false, + yieldItems: [{ status: "success", type: ["summary"], data: "text only" }], + outputSchema: { + type: "object", + required: ["summary", "notes"], + properties: { + summary: { type: "string" }, + notes: { type: "string" }, + }, + }, + }); + + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain("schema_violation"); + expect(JSON.parse(result.rawOutput)).toMatchObject({ + error: "schema_violation", + missingRequired: ["notes"], + }); + }); + + it("uses last assistant text as the raw result for terminal string-typed yields without data", () => { + const result = finalizeSubprocessOutput({ + rawOutput: "", + exitCode: 0, + stderr: "", + doneAborted: false, + signalAborted: false, + yieldItems: [{ status: "success", type: "final" }], + outputSchema: undefined, + lastAssistantText: "final answer from the assistant", + }); + + expect(result.exitCode).toBe(0); + expect(result.rawOutput).toBe("final answer from the assistant"); + }); + + it("lets a terminal string-typed last-turn result override earlier incremental sections", () => { + const result = finalizeSubprocessOutput({ + rawOutput: "", + exitCode: 0, + stderr: "", + doneAborted: false, + signalAborted: false, + yieldItems: [ + { status: "success", type: ["summary"], data: "first" }, + { status: "success", type: "final" }, + ], + outputSchema: undefined, + lastAssistantText: "plain final answer", + }); + + expect(result.exitCode).toBe(0); + expect(result.rawOutput).toBe("plain final answer"); + }); + + it("serializes untyped useLastTurn yield as raw text", () => { + const result = finalizeSubprocessOutput({ + rawOutput: "", + exitCode: 0, + stderr: "", + doneAborted: false, + signalAborted: false, + yieldItems: [{ status: "success", useLastTurn: true }], + outputSchema: undefined, + lastAssistantText: "plain final answer", + }); + + expect(result.exitCode).toBe(0); + expect(result.rawOutput).toBe("plain final answer"); + }); }); diff --git a/packages/coding-agent/test/task/render-yield-shape.test.ts b/packages/coding-agent/test/task/render-yield-shape.test.ts index f6f94113d..0fdf19783 100644 --- a/packages/coding-agent/test/task/render-yield-shape.test.ts +++ b/packages/coding-agent/test/task/render-yield-shape.test.ts @@ -133,4 +133,55 @@ describe("task renderer: malformed yield slot (#1987)", () => { }); expect(text).toContain("correct"); }); + + it("renders typed yield sections compactly in the result branch", async () => { + const text = await renderResultText({ + yield: [ + { type: ["summary"], data: "first note", status: "success" }, + { type: ["summary", "details"], data: { ok: true }, status: "success" }, + { type: "final", data: "done", status: "success" }, + ], + }); + + expect(text).toContain("yield+[summary]"); + expect(text).toContain("yield+[summary, details]"); + expect(text).toContain("yield[final]"); + expect(text).toContain("done"); + }); + + it("renders typed yield sections compactly in the progress branch", async () => { + const text = await renderProgressText({ + yield: { type: ["notes"], useLastTurn: true, status: "success" }, + }); + + expect(text).toContain("yield+[notes]"); + expect(text).toContain("last assistant turn"); + }); + + it("renders reviewer results assembled from incremental yield sections", async () => { + const text = await renderResultText({ + yield: [ + { + type: ["findings"], + data: { + title: "Handle null response", + body: "Null response reaches the formatter and crashes rendering.", + priority: 1, + confidence: 0.8, + file_path: "src/review.ts", + line_start: 42, + line_end: 42, + }, + status: "success", + }, + { type: ["overall_correctness"], data: "incorrect", status: "success" }, + { type: ["explanation"], data: "One bug blocks approval.", status: "success" }, + { type: ["confidence"], data: 0.8, status: "success" }, + ], + }); + + expect(text).toContain("Patch is incorrect"); + expect(text).toContain("Findings:"); + expect(text).toContain("Handle null response"); + }); }); diff --git a/packages/coding-agent/test/tools/yield-extraction.test.ts b/packages/coding-agent/test/tools/yield-extraction.test.ts index 585261bea..298a3fb70 100644 --- a/packages/coding-agent/test/tools/yield-extraction.test.ts +++ b/packages/coding-agent/test/tools/yield-extraction.test.ts @@ -12,11 +12,22 @@ describe("yield subprocess extraction", () => { toolCallId: "call-1", result: { content: [{ type: "text", text: "Result submitted." }], - details: { status: "success", data: { ok: true } }, + details: { + status: "success", + data: { ok: true }, + type: ["notes"], + useLastTurn: true, + }, }, isError: false, }); - expect(data).toEqual({ status: "success", data: { ok: true }, error: undefined }); + expect(data).toEqual({ + status: "success", + data: { ok: true }, + error: undefined, + type: ["notes"], + useLastTurn: true, + }); }); it("ignores malformed yield details without status", () => { @@ -31,4 +42,48 @@ describe("yield subprocess extraction", () => { }); expect(data).toBeUndefined(); }); + + it("classifies terminal and incremental yield completions", () => { + expect(handler?.shouldTerminate).toBeDefined(); + expect( + handler?.shouldTerminate?.({ + toolName: "yield", + toolCallId: "call-terminal-untyped", + result: { + content: [{ type: "text", text: "Result submitted." }], + details: { status: "success", data: { ok: true } }, + }, + isError: false, + }), + ).toBe(true); + expect( + handler?.shouldTerminate?.({ + toolName: "yield", + toolCallId: "call-terminal-string", + result: { + content: [{ type: "text", text: "Result submitted." }], + details: { status: "success", type: "summary", useLastTurn: true }, + }, + isError: false, + }), + ).toBe(true); + expect( + handler?.shouldTerminate?.({ + toolName: "yield", + toolCallId: "call-incremental", + result: { + content: [{ type: "text", text: "Result submitted." }], + details: { status: "success", data: { ok: true }, type: ["notes"] }, + }, + isError: false, + }), + ).toBe(false); + expect( + handler?.shouldTerminate?.({ + toolName: "yield", + toolCallId: "call-tool-error", + isError: true, + }), + ).toBe(false); + }); }); diff --git a/packages/coding-agent/test/tools/yield.test.ts b/packages/coding-agent/test/tools/yield.test.ts index 766593286..8ed4da9f5 100644 --- a/packages/coding-agent/test/tools/yield.test.ts +++ b/packages/coding-agent/test/tools/yield.test.ts @@ -48,6 +48,78 @@ describe("YieldTool", () => { expect(result.details).toEqual({ data: undefined, status: "aborted", error: "blocked" }); }); + it("accepts typed success without data as a last-turn result", async () => { + const tool = new YieldTool(createSession()); + const result = await tool.execute("call-last-turn", { type: "summary", result: {} } as never); + expect(result.details).toEqual({ + data: undefined, + status: "success", + error: undefined, + type: "summary", + useLastTurn: true, + }); + }); + + it("passes array-typed success through as an incremental result", async () => { + const tool = new YieldTool(createSession()); + const result = await tool.execute("call-incremental", { + type: ["notes", "plan"], + result: { data: { step: 1 } }, + } as never); + expect(result.details).toEqual({ + data: { step: 1 }, + status: "success", + error: undefined, + type: ["notes", "plan"], + }); + }); + + 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( + "result must contain either `data` or `error`", + ); + await expect(tool.execute("call-empty-type", { type: [], result: {} } as never)).rejects.toThrow( + "type must be a string or non-empty array of strings", + ); + await expect( + tool.execute("call-null-data", { type: "summary", result: { data: null } } as never), + ).rejects.toThrow("data is required when yield indicates success"); + }); + + it("exposes typed last-turn mode in the argument schema without weakening untyped yield", () => { + const tool = new YieldTool(createSession()); + const parameters = tool.parameters as unknown as Record; + const typeSchema = toRecord(toRecord(parameters.properties).type); + const variants = Array.isArray(typeSchema.anyOf) ? typeSchema.anyOf.map(toRecord) : []; + + expect(variants.map(variant => variant.type)).toEqual(["string", "array"]); + expect(variants[1]?.minItems).toBe(1); + expect(toRecord(variants[1]?.items).type).toBe("string"); + + const toolDefinition: Tool = { + name: tool.name, + description: tool.description, + parameters: tool.parameters, + }; + expect( + validateToolArguments(toolDefinition, { + type: "toolCall", + id: "call-schema-typed", + name: tool.name, + arguments: { type: "summary", result: {} }, + }), + ).toEqual({ type: "summary", result: {} }); + expect(() => + validateToolArguments(toolDefinition, { + type: "toolCall", + id: "call-schema-untyped", + name: tool.name, + arguments: { result: {} }, + }), + ).toThrow(); + }); + it("accepts arbitrary data when outputSchema is null", async () => { const tool = new YieldTool(createSession({ outputSchema: null })); expect(tool.strict).toBe(false);