Merge pull request #535 from corbitsdev/cl-6900-edit_file-filler-arg-rejections-cause-identical-retry-loops

Treat edit_file filler args as absent for mode selection

Author: TheGreatAxios

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<ToolResult>) {
-    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);
   }
 

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<string, unknown>): 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<string, unknown>): 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, unknown>): 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, unknown>): 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,30 +113,30 @@ export function parseEditFileMode(args: Record<string, unknown>): 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) {
     return parseLineRangeFields(path, new_string, args);
   }
 
   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),
   };