fix(coding-agent): coerced report_finding string priority to number for reviewer schema
The report_finding tool's priority is exposed as a string enum
("P0"-"P3") for ergonomics, but the reviewer agent and every
custom review agent declare priority as `type: number` in their
JTD output schema. The cast at executor.ts:1473 lied about the
runtime shape, so the auto-injected `findings[].priority` flowed
through as strings and every yield with at least one finding was
rejected with `findings.0.priority: expected number, received string`,
forcing the run into the schema_violation exit path.
Added `toReviewFinding(details)` in tools/review.ts that maps the
priority enum to its numeric ordinal via the existing PRIORITY_INFO
table and use it at the boundary in executor.ts. Render paths still
see the original `ReportFindingDetails` shape (string priority)
through normalizeReportFindings, so display formatting is unaffected.
Fixes #1350
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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<SingleRes
|
||||
// Use final output if available, otherwise accumulated output
|
||||
let rawOutput = finalOutputChunks.length > 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,
|
||||
|
||||
@@ -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<ReportFindingDetails>("report_finding", {
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user