diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 2b6d919a1..1102fba3c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Fixed clipboard image paste (Ctrl+V) silently failing on WSL2 by routing image reads through a `powershell.exe` bridge when WSL interop is detected, since `arboard` returns `ContentNotAvailable` under WSLg ([#1280](https://github.com/can1357/oh-my-pi/issues/1280)) +- Fixed reviewer agent always failing JTD validation with `findings.0.priority: expected number, received string` whenever `report_finding` surfaced a finding; the tool's `"P0"`-`"P3"` priority is now coerced to its numeric ordinal before populating the auto-injected `findings[]` ([#1350](https://github.com/can1357/oh-my-pi/issues/1350)) ## [15.2.4] - 2026-05-22 ### Breaking Changes diff --git a/packages/coding-agent/src/task/executor.ts b/packages/coding-agent/src/task/executor.ts index 739c13349..8ed925a99 100644 --- a/packages/coding-agent/src/task/executor.ts +++ b/packages/coding-agent/src/task/executor.ts @@ -34,6 +34,7 @@ import { truncateTail } from "../session/streaming-output"; import type { ContextFileEntry } from "../tools"; import { jtdToJsonSchema, normalizeSchema } from "../tools/jtd-to-json-schema"; import { ToolAbortError } from "../tools/tool-errors"; +import { type ReportFindingDetails, toReviewFinding } from "../tools/review"; import type { EventBus } from "../utils/event-bus"; import { buildNamedToolChoice } from "../utils/tool-choice"; import type { WorkspaceTree } from "../workspace-tree"; @@ -1470,7 +1471,8 @@ export async function runSubprocess(options: ExecutorOptions): Promise 0 ? finalOutputChunks.join("") : outputChunks.join(""); const yieldItems = progress.extractedToolData?.yield as YieldItem[] | undefined; - const reportFindings = progress.extractedToolData?.report_finding as ReviewFinding[] | undefined; + const reportFindingDetails = progress.extractedToolData?.report_finding as ReportFindingDetails[] | undefined; + const reportFindings: ReviewFinding[] | undefined = reportFindingDetails?.map(toReviewFinding); const finalized = finalizeSubprocessOutput({ rawOutput, exitCode, diff --git a/packages/coding-agent/src/tools/review.ts b/packages/coding-agent/src/tools/review.ts index 75a3d0ad6..9fd19ca72 100644 --- a/packages/coding-agent/src/tools/review.ts +++ b/packages/coding-agent/src/tools/review.ts @@ -15,6 +15,7 @@ import { isRecord } from "@oh-my-pi/pi-utils"; import * as z from "zod/v4"; import type { Theme, ThemeColor } from "../modes/theme/theme"; import { subprocessToolRegistry } from "../task/subprocess-tool-registry"; +import type { ReviewFinding } from "../task/types"; export type FindingPriority = "P0" | "P1" | "P2" | "P3"; export interface FindingPriorityInfo { @@ -186,6 +187,28 @@ export interface SubmitReviewDetails { // Re-export types for external use export type { ReportFindingDetails }; +/** + * Coerce a tool-side `ReportFindingDetails` into the cross-boundary + * `ReviewFinding` shape consumed by the reviewer agent's JTD output schema. + * + * The `report_finding` tool exposes `priority` as a string enum (`"P0".."P3"`) + * for ergonomics, but the bundled reviewer schema (and every custom review + * agent that mirrors it) declares `priority: number`. Without this coercion + * the auto-populated `findings[]` fails JTD validation and every review run + * that surfaces a finding is rejected with `findings.0.priority: expected + * number, received string`. + */ +export function toReviewFinding(details: ReportFindingDetails): ReviewFinding { + return { + title: details.title, + body: details.body, + priority: getPriorityInfo(details.priority).ord, + confidence: details.confidence, + file_path: details.file_path, + line_start: details.line_start, + line_end: details.line_end, + }; +} // Register report_finding handler subprocessToolRegistry.register("report_finding", { diff --git a/packages/coding-agent/test/tools/review.test.ts b/packages/coding-agent/test/tools/review.test.ts index 2ed23104a..5f6beb37d 100644 --- a/packages/coding-agent/test/tools/review.test.ts +++ b/packages/coding-agent/test/tools/review.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "bun:test"; import { subprocessToolRegistry } from "../../src/task/subprocess-tool-registry"; -import { parseReportFindingDetails } from "../../src/tools/review"; +import { finalizeSubprocessOutput } from "../../src/task/executor"; +import { parseReportFindingDetails, toReviewFinding } from "../../src/tools/review"; describe("report_finding subprocess extraction", () => { it("returns undefined for malformed finding details", () => { @@ -58,3 +59,76 @@ describe("report_finding subprocess extraction", () => { ).toBeUndefined(); }); }); + +describe("toReviewFinding", () => { + const base = { + title: "[P0] Example finding", + body: "Details", + confidence: 0.95, + file_path: "/tmp/example.ts", + line_start: 10, + line_end: 12, + } as const; + + it("maps the priority string enum to its numeric ordinal", () => { + expect(toReviewFinding({ ...base, priority: "P0" }).priority).toBe(0); + expect(toReviewFinding({ ...base, priority: "P1" }).priority).toBe(1); + expect(toReviewFinding({ ...base, priority: "P2" }).priority).toBe(2); + expect(toReviewFinding({ ...base, priority: "P3" }).priority).toBe(3); + }); + + it("passes JTD validation against the reviewer agent's numeric priority schema (#1350)", () => { + // Mirrors the bundled reviewer agent's output schema. Before the fix the + // string priority from `report_finding` short-circuited every successful + // review run with `findings.0.priority: expected number, received string`. + const reviewerSchema = { + properties: { + overall_correctness: { enum: ["correct", "incorrect"] }, + explanation: { type: "string" }, + confidence: { type: "number" }, + }, + optionalProperties: { + findings: { + elements: { + properties: { + title: { type: "string" }, + body: { type: "string" }, + priority: { type: "number" }, + confidence: { type: "number" }, + file_path: { type: "string" }, + line_start: { type: "number" }, + line_end: { type: "number" }, + }, + }, + }, + }, + }; + + const result = finalizeSubprocessOutput({ + rawOutput: "", + exitCode: 0, + stderr: "", + doneAborted: false, + signalAborted: false, + yieldItems: [ + { + status: "success", + data: { + overall_correctness: "incorrect", + explanation: "Found one bug", + confidence: 0.9, + }, + }, + ], + reportFindings: [toReviewFinding({ ...base, priority: "P2" })], + outputSchema: reviewerSchema, + }); + + expect(result.exitCode).toBe(0); + expect(result.stderr).toBe(""); + const parsed = JSON.parse(result.rawOutput) as { + findings: Array<{ priority: number }>; + }; + expect(parsed.findings[0].priority).toBe(2); + }); +});