diff --git a/src/plugins/edit-file-line-range.test.ts b/src/plugins/edit-file-line-range.test.ts index 4875c1f70..83b07f3d9 100644 --- a/src/plugins/edit-file-line-range.test.ts +++ b/src/plugins/edit-file-line-range.test.ts @@ -52,6 +52,65 @@ describe("parseEditFileMode", () => { expect(mode.message).toContain("only one edit mode is allowed"); expect(mode.message).toContain("Omit old_string"); expect(mode.message).toContain("omit start_line/end_line"); + expect(mode.message).toContain("old_string (len 1)"); + expect(mode.message).toContain("start_line=1"); + expect(mode.message).toContain("end_line=1"); + } + }); + + test("treats filler start_line/end_line of 0 as absent and picks substring mode", () => { + const mode = parseEditFileMode({ + path: "a.ts", + old_string: "x", + new_string: "y", + start_line: 0, + end_line: 0, + }); + expect(mode.kind).toBe("substring"); + }); + + test("treats null line fields as absent and picks substring mode", () => { + const mode = parseEditFileMode({ + path: "a.ts", + old_string: "x", + new_string: "y", + start_line: null, + end_line: null, + }); + expect(mode.kind).toBe("substring"); + }); + + test("treats filler empty old_string as absent and picks line-range mode", () => { + const mode = parseEditFileMode({ + path: "a.ts", + old_string: "", + start_line: 2, + end_line: 3, + new_string: "z", + }); + expect(mode.kind).toBe("line_range"); + }); + + test("empty old_string alone gets a helpful error naming what was received", () => { + const mode = parseEditFileMode({ + path: "a.ts", + old_string: "", + new_string: "y", + }); + expect(mode.kind).toBe("invalid"); + if (mode.kind === "invalid") { + expect(mode.message).toContain("old_string is empty"); + expect(mode.message).toContain("old_string (len 0)"); + } + }); + + test("missing both modes reports what was received", () => { + const mode = parseEditFileMode({ path: "a.ts", new_string: "y" }); + expect(mode.kind).toBe("invalid"); + if (mode.kind === "invalid") { + expect(mode.message).toContain("requires old_string"); + expect(mode.message).toContain("no old_string"); + expect(mode.message).toContain("no start_line"); } }); @@ -166,11 +225,9 @@ describe("editFileLineRangePlugin", () => { }); function handler(next: (call: ToolCall, signal: AbortSignal) => Promise) { - const mws = [ - pathEscapePlugin(cwd).middleware!, - editFileLineRangePlugin().middleware!, - verifyPlugin().middleware!, - ]; + const mws = [pathEscapePlugin(cwd), editFileLineRangePlugin(), verifyPlugin()].flatMap((p) => + p.middleware ? [p.middleware] : [], + ); return composeMiddleware(mws, next); } diff --git a/src/plugins/edit-file-line-range.ts b/src/plugins/edit-file-line-range.ts index c18257218..d8dc8e103 100644 --- a/src/plugins/edit-file-line-range.ts +++ b/src/plugins/edit-file-line-range.ts @@ -27,17 +27,42 @@ function optionalInt(value: unknown): number | undefined { : undefined; } +// Models pad the unused mode's fields with fillers (old_string: "", start_line: 0). +// For mode selection a filler counts as absent: "" is never a valid old_string and +// 0/null/non-integers are never valid 1-based lines. Treating them as present made +// filler-padded calls look like "both edit modes" and rejected them (CL-6900). function hasOldStringArg(args: Record): boolean { - return typeof args.old_string === "string"; + return typeof args.old_string === "string" && args.old_string.length > 0; +} + +function validLineNumber(value: unknown): number | undefined { + const n = optionalInt(value); + return n !== undefined && n >= 1 ? n : undefined; } function hasLineRangeArgs(args: Record): boolean { - return optionalInt(args.start_line) !== undefined || optionalInt(args.end_line) !== undefined; + return ( + validLineNumber(args.start_line) !== undefined || validLineNumber(args.end_line) !== undefined + ); +} + +function describeReceived(args: Record): string { + const old = args.old_string; + return [ + typeof old === "string" ? `old_string (len ${old.length})` : "no old_string", + args.start_line === undefined + ? "no start_line" + : `start_line=${JSON.stringify(args.start_line)}`, + args.end_line === undefined ? "no end_line" : `end_line=${JSON.stringify(args.end_line)}`, + ].join(", "); } -const MIXED_MODE_MESSAGE = - "edit_file: received both old_string and start_line/end_line; only one edit mode is allowed. " + - "Omit old_string to use line-range mode, or omit start_line/end_line to use substring mode."; +function mixedModeMessage(args: Record): string { + return ( + `edit_file: received ${describeReceived(args)}; only one edit mode is allowed. ` + + "Omit old_string to use line-range mode, or omit start_line/end_line to use substring mode." + ); +} export function parseLineRangeFields( path: string, @@ -88,7 +113,7 @@ export function parseEditFileMode(args: Record): EditFileModePa // slice, producing a confusing "old_string does not match" error even when the caller // meant plain substring mode (CL-4399). One explicit error beats a wrong guess. if (substring && lineRange) { - return { kind: "invalid", message: MIXED_MODE_MESSAGE }; + return { kind: "invalid", message: mixedModeMessage(args) }; } if (lineRange) { @@ -96,22 +121,22 @@ export function parseEditFileMode(args: Record): EditFileModePa } if (!substring) { + const emptyOldString = typeof args.old_string === "string"; return { kind: "invalid", - message: - "edit_file requires old_string (substring mode) or start_line and end_line (line-range mode)", + message: emptyOldString + ? "edit_file: old_string is empty; provide the exact text to replace (substring mode), " + + "or omit it and send start_line/end_line >= 1 (line-range mode). " + + `Received ${describeReceived(args)}.` + : "edit_file requires old_string (substring mode) or start_line and end_line (line-range mode); " + + `received ${describeReceived(args)}`, }; } - const old_string = String(args.old_string); - if (old_string.length === 0) { - return { kind: "invalid", message: "old_string must not be empty" }; - } - return { kind: "substring", path, - old_string, + old_string: String(args.old_string), new_string, replace_all: Boolean(args.replace_all), };