Compare commits

...
Author SHA1 Message Date
abeatrix 32a168e424 zod validate 2026-07-15 08:49:02 +08:00
abeatrix 7cea2d6dbe fix(core): stop read_files results from teaching models an invalid input shape 2026-07-15 06:25:57 +08:00
11 changed files with 415 additions and 33 deletions
+26 -1
View File
@@ -330,6 +330,31 @@ type ToolResultEntry = {
success?: boolean;
};
// read_files queries are JSON object strings ({"path":...,"start_line":...});
// render them as a compact path:start-end label instead of raw JSON.
function toolResultEntryLabel(entry: ToolResultEntry): string {
const query = entry.query ?? "";
if (!query.startsWith("{")) {
return query;
}
try {
const parsed = JSON.parse(query) as {
path?: string;
start_line?: number;
end_line?: number;
};
if (typeof parsed.path !== "string") {
return query;
}
if (parsed.start_line == null && parsed.end_line == null) {
return parsed.path;
}
return `${parsed.path}:${parsed.start_line ?? 1}-${parsed.end_line ?? "EOF"}`;
} catch {
return query;
}
}
function isToolResultArray(value: unknown): value is ToolResultEntry[] {
return (
Array.isArray(value) &&
@@ -379,7 +404,7 @@ function formatRawOutput(output: unknown, fallback: string): string {
function expandToolEvent(toolEvent: ToolEvent): ExpandedToolEvent[] {
if (isToolResultArray(toolEvent.output)) {
return toolEvent.output.map((entry, index) => {
const query = entry.query ?? "";
const query = toolResultEntryLabel(entry);
const title = query ? `${toolEvent.name}: ${query}` : toolEvent.name;
const state: ToolEvent["state"] =
entry.success === false ? "output-error" : toolEvent.state;
@@ -1082,6 +1082,22 @@ function asStringArray(value: unknown): string[] {
);
}
// read_files result queries are JSON object strings ({"path":...,"start_line":...});
// render them as a compact path:start-end label instead of raw JSON.
function formatReadQueryLabel(query: string): string {
if (!query.startsWith("{")) return query;
try {
const parsed = asRecord(JSON.parse(query));
if (typeof parsed?.path !== "string") return query;
const start = parsed.start_line;
const end = parsed.end_line;
if (start == null && end == null) return parsed.path;
return `${parsed.path}:${start ?? 1}-${end ?? "EOF"}`;
} catch {
return query;
}
}
/**
* read_files accepts many input shapes: { files: [{ path }] }, { files: path },
* { file_paths: [...] }, { paths: [...] }, a bare request, an array, or a string.
@@ -1297,10 +1313,9 @@ function buildToolSummary(
return { label: detail, details: [] };
}
const rawQuery = asRecord(result)?.query;
const query =
typeof asRecord(result)?.query === "string"
? (asRecord(result)?.query as string)
: "";
typeof rawQuery === "string" ? formatReadQueryLabel(rawQuery) : "";
const fallback =
query || (inProgress ? `Running ${toolName}` : toolName) || "Tool";
return { label: fallback, details: [fallback] };
+26 -1
View File
@@ -308,6 +308,31 @@ type ToolResultEntry = {
success?: boolean;
};
// read_files queries are JSON object strings ({"path":...,"start_line":...});
// render them as a compact path:start-end label instead of raw JSON.
function toolResultEntryLabel(entry: ToolResultEntry): string {
const query = entry.query ?? "";
if (!query.startsWith("{")) {
return query;
}
try {
const parsed = JSON.parse(query) as {
path?: string;
start_line?: number;
end_line?: number;
};
if (typeof parsed.path !== "string") {
return query;
}
if (parsed.start_line == null && parsed.end_line == null) {
return parsed.path;
}
return `${parsed.path}:${parsed.start_line ?? 1}-${parsed.end_line ?? "EOF"}`;
} catch {
return query;
}
}
function isToolResultArray(value: unknown): value is ToolResultEntry[] {
return (
Array.isArray(value) &&
@@ -357,7 +382,7 @@ function formatRawOutput(output: unknown, fallback: string): string {
function expandToolEvent(toolEvent: ToolEvent): ExpandedToolEvent[] {
if (isToolResultArray(toolEvent.output)) {
return toolEvent.output.map((entry, index) => {
const query = entry.query ?? "";
const query = toolResultEntryLabel(entry);
const title = query ? `${toolEvent.name}: ${query}` : toolEvent.name;
const state: ToolEvent["state"] =
entry.success === false ? "output-error" : toolEvent.state;
@@ -1313,7 +1313,7 @@ describe("default read_files tool", () => {
expect(result).toEqual([
{
query: "/tmp/example.ts:3-5",
query: '{"path":"/tmp/example.ts","start_line":3,"end_line":5}',
result: "selected lines",
success: true,
},
@@ -1428,6 +1428,108 @@ describe("default read_files tool", () => {
);
});
it("accepts filePath/file_path aliases and echoes canonical keys in results", async () => {
const execute = vi.fn(
async (request: { path: string }) => `content:${request.path}`,
);
const tool = createReadFilesTool(execute);
// The exact drift observed in the wild: camelCase key on each entry.
const camelResult = await tool.execute(
{
files: [
{ filePath: "/tmp/client.go", start_line: 1, end_line: 200 },
{ filePath: "/tmp/service.go" },
],
} as never,
{
agentId: "agent-1",
conversationId: "conv-1",
iteration: 1,
},
);
const snakeResult = await tool.execute(
{ files: [{ file_path: "/tmp/queries.go", start_line: 2 }] } as never,
{
agentId: "agent-1",
conversationId: "conv-1",
iteration: 2,
},
);
expect(execute).toHaveBeenNthCalledWith(
1,
{ path: "/tmp/client.go", start_line: 1, end_line: 200 },
expect.objectContaining({ iteration: 1 }),
);
expect(execute).toHaveBeenNthCalledWith(
2,
{ path: "/tmp/service.go" },
expect.objectContaining({ iteration: 1 }),
);
expect(execute).toHaveBeenNthCalledWith(
3,
{ path: "/tmp/queries.go", start_line: 2 },
expect.objectContaining({ iteration: 2 }),
);
expect(camelResult).toEqual([
{
query: '{"path":"/tmp/client.go","start_line":1,"end_line":200}',
result: "content:/tmp/client.go",
success: true,
},
{
query: '{"path":"/tmp/service.go"}',
result: "content:/tmp/service.go",
success: true,
},
]);
expect(snakeResult).toEqual([
{
query: '{"path":"/tmp/queries.go","start_line":2}',
result: "content:/tmp/queries.go",
success: true,
},
]);
});
it("accepts path-key aliases on bare and paths-keyed entries", async () => {
const execute = vi.fn(
async (request: { path: string }) => `content:${request.path}`,
);
const tool = createReadFilesTool(execute);
await tool.execute({ filePath: "/tmp/bare.ts", end_line: 9 } as never, {
agentId: "agent-1",
conversationId: "conv-1",
iteration: 1,
});
await tool.execute(
{ paths: [{ file_path: "/tmp/listed.ts" }, "/tmp/plain.ts"] } as never,
{
agentId: "agent-1",
conversationId: "conv-1",
iteration: 2,
},
);
expect(execute).toHaveBeenNthCalledWith(
1,
{ path: "/tmp/bare.ts", end_line: 9 },
expect.objectContaining({ iteration: 1 }),
);
expect(execute).toHaveBeenNthCalledWith(
2,
{ path: "/tmp/listed.ts" },
expect.objectContaining({ iteration: 2 }),
);
expect(execute).toHaveBeenNthCalledWith(
3,
{ path: "/tmp/plain.ts" },
expect.objectContaining({ iteration: 2 }),
);
});
it("folds orphan range entries into the preceding file entry", async () => {
const execute = vi.fn(
async (request: { path: string }) => `content:${request.path}`,
@@ -1455,6 +1557,19 @@ describe("default read_files tool", () => {
iteration: 2,
},
);
await tool.execute(
{
files: [
{ filePath: "/tmp/aliased.ts" },
{ start_line: 3, end_line: 7 },
],
} as never,
{
agentId: "agent-1",
conversationId: "conv-1",
iteration: 3,
},
);
expect(execute).toHaveBeenNthCalledWith(
1,
@@ -1471,6 +1586,11 @@ describe("default read_files tool", () => {
{ path: "/tmp/b.ts" },
expect.objectContaining({ iteration: 2 }),
);
expect(execute).toHaveBeenNthCalledWith(
4,
{ path: "/tmp/aliased.ts", start_line: 3, end_line: 7 },
expect.objectContaining({ iteration: 3 }),
);
});
it("rejects orphan range entries that cannot be attached to a file entry", async () => {
@@ -1511,7 +1631,7 @@ describe("default read_files tool", () => {
expect(execute).not.toHaveBeenCalled();
});
it("rejects invalid union inputs before calling the executor", async () => {
it("rejects invalid union inputs with a canonical-shape hint before calling the executor", async () => {
const execute = vi.fn(async () => "should not run");
const tool = createReadFilesTool(execute);
@@ -1521,7 +1641,9 @@ describe("default read_files tool", () => {
conversationId: "conv-1",
iteration: 1,
}),
).rejects.toThrow();
).rejects.toThrow(
/Expected input like: \{"files": \[\{"path": "\/absolute\/path\/to\/file\.ts", "start_line": 1, "end_line": 100\}\]\}/,
);
expect(execute).not.toHaveBeenCalled();
});
@@ -1549,7 +1671,7 @@ describe("default read_files tool", () => {
expect(result).toEqual([
{
query: "/tmp/example.ts",
query: '{"path":"/tmp/example.ts"}',
result: "full file",
success: true,
},
@@ -1601,19 +1723,19 @@ describe("default read_files tool", () => {
expect(result).toEqual([
{
query: "/tmp/valid-a.ts:1-2",
query: '{"path":"/tmp/valid-a.ts","start_line":1,"end_line":2}',
result: "content for /tmp/valid-a.ts",
success: true,
},
{
query: "/tmp/reversed.ts:5-3",
query: '{"path":"/tmp/reversed.ts","start_line":5,"end_line":3}',
result: "",
error:
"Invalid file range: start_line must be less than or equal to end_line (received start_line: 5, end_line: 3)",
success: false,
},
{
query: "/tmp/valid-b.ts",
query: '{"path":"/tmp/valid-b.ts"}',
result: "content for /tmp/valid-b.ts",
success: true,
},
@@ -1676,7 +1798,7 @@ describe("zod schema conversion", () => {
path: {
type: "string",
description:
"The absolute file path of a text file to read content from",
"The absolute path of a text file to read content from",
},
start_line: {
anyOf: [{ type: "integer" }, { type: "null" }],
@@ -260,6 +260,9 @@ export function createReadFilesTool(
const validate = validateWithZod(
ReadFilesInputUnionSchema,
coalesceOrphanReadRanges(input),
{
hint: 'Expected input like: {"files": [{"path": "/absolute/path/to/file.ts", "start_line": 1, "end_line": 100}]} — each entry needs "path"; "start_line"/"end_line" are optional',
},
);
let requests: ReadFileRequest[];
if (typeof validate === "string") {
@@ -293,10 +296,13 @@ export function createReadFilesTool(
return Promise.all(
requests.map(async (request): Promise<ToolOperationResult> => {
// The query echoes the request under its canonical input keys
// so results keep reinforcing the shape the model must emit.
const query = formatReadFileQuery(request);
const rangeError = getReadFileRangeError(request);
if (rangeError) {
return {
query: formatReadFileQuery(request),
query,
result: "",
error: `Invalid file range: ${rangeError}`,
success: false,
@@ -310,14 +316,14 @@ export function createReadFilesTool(
`File read timed out after ${timeoutMs}ms`,
);
return {
query: formatReadFileQuery(request),
query,
result: content,
success: true,
};
} catch (error) {
const msg = formatError(error);
return {
query: formatReadFileQuery(request),
query,
result: "",
error: `Error reading file: ${msg}`,
success: false,
@@ -58,14 +58,22 @@ export function withTimeout<T>(
]);
}
/**
* Echo a read request into a result's `query` field as a JSON object string
* (e.g. `{"path":"/a/b.ts","start_line":3,"end_line":5}`). Restating the
* request under its canonical input keys keeps every successful result
* reinforcing the exact shape the model must emit on its next call, unlike
* the previous fused `path:start-end` format which taught an invalid one.
*/
export function formatReadFileQuery(request: ReadFileRequest): string {
const { path, start_line, end_line } = request;
if (start_line == null && end_line == null) {
return path;
const echo: Record<string, string | number> = { path: request.path };
if (request.start_line != null) {
echo.start_line = request.start_line;
}
const start = start_line ?? 1;
const end = end_line ?? "EOF";
return `${path}:${start}-${end}`;
if (request.end_line != null) {
echo.end_line = request.end_line;
}
return JSON.stringify(echo);
}
export function getReadFileRangeError(request: ReadFileRequest): string | null {
@@ -79,6 +87,13 @@ export function getReadFileRangeError(request: ReadFileRequest): string | null {
const READ_RANGE_KEYS = new Set(["start_line", "end_line"]);
/** Path keys accepted on read entries; aliases are normalized to `path` during validation. */
const READ_PATH_KEYS = ["path", "file_path", "filePath"] as const;
function hasReadPathKey(value: object): boolean {
return READ_PATH_KEYS.some((key) => key in value);
}
function isOrphanReadRangeEntry(
value: unknown,
): value is Record<string, unknown> {
@@ -102,7 +117,7 @@ function coalesceOrphanReadRangeEntries(entries: unknown[]): unknown[] {
previous !== null &&
typeof previous === "object" &&
!Array.isArray(previous) &&
"path" in previous &&
hasReadPathKey(previous) &&
Object.keys(entry).every((key) => !(key in previous))
) {
coalesced[coalesced.length - 1] = { ...previous, ...entry };
@@ -14,7 +14,7 @@ export const INPUT_ARG_CHAR_LIMIT = 6000;
*/
const AbsolutePath = z
.string()
.describe("The absolute file path of a text file to read content from");
.describe("The absolute path of a text file to read content from");
export const ReadFileLineRangeSchema = z
.object({
@@ -60,22 +60,46 @@ export const ReadFilesInputSchema = z.object({
),
});
const ReadFileRangeAliasFields = {
start_line: ReadFileLineRangeSchema.shape.start_line,
end_line: ReadFileLineRangeSchema.shape.end_line,
};
/**
* Tolerant per-entry schema for read requests. Some models emit the path
* under `file_path`/`filePath` instead of `path`; normalize those aliases to
* the canonical shape so downstream code only ever sees `path`.
*/
const LooseReadFileRequestSchema = z.union([
ReadFileRequestSchema,
z
.object({ file_path: AbsolutePath, ...ReadFileRangeAliasFields })
.transform(({ file_path, ...rest }) => ({ path: file_path, ...rest })),
z
.object({ filePath: AbsolutePath, ...ReadFileRangeAliasFields })
.transform(({ filePath, ...rest }) => ({ path: filePath, ...rest })),
]);
/**
* Union schema for read_files tool input, allowing either a single string, an array of strings, or the full object schema
*/
export const ReadFilesInputUnionSchema = z.union([
ReadFilesInputSchema,
ReadFileRequestSchema,
z.array(ReadFileRequestSchema),
LooseReadFileRequestSchema,
z.array(LooseReadFileRequestSchema),
z.array(z.string()),
z.string(),
z.object({ files: z.array(z.union([AbsolutePath, ReadFileRequestSchema])) }),
z.object({ files: ReadFileRequestSchema }),
z.object({
files: z.array(z.union([AbsolutePath, LooseReadFileRequestSchema])),
}),
z.object({ files: LooseReadFileRequestSchema }),
z.object({ files: AbsolutePath }),
z.object({ file_paths: z.array(AbsolutePath) }),
z.object({ file_paths: z.string() }),
z.object({ paths: z.array(z.union([AbsolutePath, ReadFileRequestSchema])) }),
z.object({ paths: ReadFileRequestSchema }),
z.object({
paths: z.array(z.union([AbsolutePath, LooseReadFileRequestSchema])),
}),
z.object({ paths: LooseReadFileRequestSchema }),
z.object({ paths: z.string() }),
]);
@@ -170,7 +194,7 @@ export const EditFileInputSchema = z
path: z
.string()
.min(1)
.describe("The absolute file path for the action to be performed on"),
.describe("The absolute path for the action to be performed on"),
old_text: z
.string()
.nullable()
@@ -1545,6 +1545,64 @@ describe("MessageBuilder default-on truncation", () => {
expect(JSON.stringify(result[2].content)).toContain("NEW CONTENT");
});
it("rewrites outdated reads whose query is a JSON object string", () => {
const builder = new MessageBuilder({ minOutdatedRewriteBytes: 0 });
const readUse = (id: string) => ({
role: "assistant" as const,
content: [
{
type: "tool_use" as const,
id,
name: "read_files",
input: {
files: [{ path: "/tmp/a.txt", start_line: 3, end_line: 5 }],
},
},
],
});
const readResult = (id: string, content: string) => ({
role: "user" as const,
content: [
{
type: "tool_result" as const,
tool_use_id: id,
name: "read_files",
content,
},
],
});
const messages: Message[] = [
readUse("call_1"),
readResult(
"call_1",
JSON.stringify([
{
query: '{"path":"/tmp/a.txt","start_line":3,"end_line":5}',
result: "OLD RANGE",
success: true,
},
]),
),
readUse("call_2"),
readResult(
"call_2",
JSON.stringify([
{
query: '{"path":"/tmp/a.txt","start_line":3,"end_line":5}',
result: "NEW RANGE",
success: true,
},
]),
),
];
const result = builder.buildForApi(messages);
const serializedOld = JSON.stringify(result[1].content);
expect(serializedOld).toContain("[outdated - see the latest file content]");
expect(serializedOld).not.toContain("OLD RANGE");
expect(JSON.stringify(result[3].content)).toContain("NEW RANGE");
});
it("truncates unsupported document data blocks nested in structured results", () => {
const builder = new MessageBuilder({
maxToolResultChars: 100,
@@ -863,7 +863,16 @@ export class MessageBuilder {
return typeof value === "number" && Number.isInteger(value) ? value : null;
}
/**
* Read-result queries echo the request as a JSON object string
* (`{"path":...,"start_line":...}`); older transcripts carry the legacy
* fused `path:start-end` format, so both must parse.
*/
private parseReadQuery(query: string): ReadLocator {
const jsonLocator = this.parseJsonReadQuery(query);
if (jsonLocator) {
return jsonLocator;
}
const match = /^(.*):(\d+)-(EOF|\d+)$/.exec(query);
if (!match) {
return { path: query, startLine: null, endLine: null };
@@ -875,6 +884,17 @@ export class MessageBuilder {
};
}
private parseJsonReadQuery(query: string): ReadLocator | undefined {
if (!query.startsWith("{")) {
return undefined;
}
try {
return this.extractLocatorFromReadRequest(JSON.parse(query));
} catch {
return undefined;
}
}
private dedupeReadLocators(locators: ReadLocator[]): ReadLocator[] {
const unique = new Map<string, ReadLocator>();
for (const locator of locators) {
+50
View File
@@ -0,0 +1,50 @@
import { describe, expect, it } from "vitest";
import { z } from "zod";
import { validateWithZod } from "./zod";
const UnionSchema = z.union([
z.object({ files: z.array(z.object({ path: z.string() })) }),
z.string(),
]);
const ObjectSchema = z.object({
path: z.string(),
start_line: z.number().int().optional(),
});
const HINT = 'Expected input like: {"files": [{"path": "/abs/file.ts"}]}';
describe("validateWithZod", () => {
it("returns parsed data on success", () => {
expect(validateWithZod(UnionSchema, "/tmp/a.ts", { hint: HINT })).toBe(
"/tmp/a.ts",
);
});
it("appends the hint on root-level union failures", () => {
expect(() =>
validateWithZod(UnionSchema, { files: [42] }, { hint: HINT }),
).toThrow(`✖ Invalid input. ${HINT}`);
});
it("omits the hint on field-level failures where the message is already specific", () => {
let message = "";
try {
validateWithZod(
ObjectSchema,
{ path: "/tmp/a.ts", start_line: "3" },
{ hint: HINT },
);
} catch (error) {
message = (error as Error).message;
}
expect(message).toContain("start_line");
expect(message).not.toContain(HINT);
});
it("keeps the bare message when no hint is provided", () => {
expect(() => validateWithZod(UnionSchema, { files: [42] })).toThrow(
"✖ Invalid input",
);
});
});
+24 -2
View File
@@ -9,15 +9,37 @@ import { z } from "zod";
/**
* Validate input using a Zod schema
* Throws a formatted error if validation fails
*
* Root-level union failures prettify to a bare "✖ Invalid input" with no
* field-level detail, so callers validating union schemas should pass a
* `hint` describing the canonical input shape (ideally with an example) to
* give the model something actionable to recover with. The hint is appended
* only on such union failures; field-level errors are already specific and
* would only be muddied by restating the whole shape.
*/
export function validateWithZod<T>(schema: z.ZodType<T>, input: unknown): T {
export function validateWithZod<T>(
schema: z.ZodType<T>,
input: unknown,
options?: { hint?: string },
): T {
const result = schema.safeParse(input);
if (!result.success) {
throw new Error(z.prettifyError(result.error));
const message = z.prettifyError(result.error);
throw new Error(
options?.hint && hasRootUnionIssue(result.error)
? `${message}. ${options.hint}`
: message,
);
}
return result.data;
}
function hasRootUnionIssue(error: z.ZodError): boolean {
return error.issues.some(
(issue) => issue.code === "invalid_union" && issue.path.length === 0,
);
}
export function zodToJsonSchema(schema: z.ZodTypeAny): Record<string, unknown> {
return z.toJSONSchema(schema);
}