Merge branch 'main' into cl-6903-orchestrator-reports-it-looped-it-churned-it-cancelled-with

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),
   };

src/subagent/task-tool.ts

@@ -195,6 +195,51 @@ function taskToolResult(callId: string, content: string): ToolResult {
   return { callId, content, ...(isError ? { isError: true } : {}) };
 }
 
+type RequiredTaskField = "description" | "prompt";
+
+const REQUIRED_TASK_FIELD_HINTS: Record<RequiredTaskField, string> = {
+  description: "a short label for the sub-agent job",
+  prompt: "the actionable goal for the worker",
+};
+
+/** Truncated echo of a received value so the rejection shows what arrived. */
+function receivedFieldPreview(value: string): string {
+  const trimmed = value.trim();
+  return JSON.stringify(trimmed.length > 80 ? `${trimmed.slice(0, 77)}...` : trimmed);
+}
+
+/**
+ * Rejection naming only the actually-bad required fields, echoing the valid
+ * one back. A generic "requires description and prompt" hid which field was
+ * missing, so models retried the identical call verbatim (CL-6901).
+ */
+function requiredTaskFieldsError(
+  args: Record<string, unknown>,
+  bad: readonly RequiredTaskField[],
+): string {
+  const parts = bad.map((name) => {
+    const value = args[name];
+    const hint = REQUIRED_TASK_FIELD_HINTS[name];
+    if (value === undefined) return `is missing ${name} (string): ${hint}`;
+    if (typeof value !== "string") return `has invalid ${name} (must be a string): ${hint}`;
+    return `requires a non-empty ${name}: ${hint}`;
+  });
+  let message = `Error: task ${parts.join(" and ")}.`;
+  const good = (Object.keys(REQUIRED_TASK_FIELD_HINTS) as RequiredTaskField[]).filter(
+    (name) =>
+      !bad.includes(name) &&
+      typeof args[name] === "string" &&
+      (args[name] as string).trim().length > 0,
+  );
+  if (good.length > 0) {
+    const echo = good
+      .map((name) => `${name} ${receivedFieldPreview(args[name] as string)}`)
+      .join(" and ");
+    message += ` Received ${echo} — keep it and add ${bad.join(" and ")}.`;
+  }
+  return message;
+}
+
 export function createTaskTool(deps: TaskToolDeps): AgentTool {
   const run = deps.run;
   const telemetry = deps.telemetry ?? NOOP_TELEMETRY;
@@ -206,10 +251,13 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool {
       const args = call.arguments;
       const parsed = TaskToolArgs(args);
       if (parsed instanceof type.errors) {
-        return taskToolResult(
-          call.id,
-          "Error: task requires description (string) and prompt (string).",
+        const bad = (["description", "prompt"] as const).filter(
+          (name) => typeof args[name] !== "string",
         );
+        if (bad.length === 0) {
+          return taskToolResult(call.id, `Error: task arguments invalid: ${parsed.summary}`);
+        }
+        return taskToolResult(call.id, requiredTaskFieldsError(args, bad));
       }
       const {
         description: rawDesc,
@@ -233,7 +281,10 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool {
       const doNot = rawDoNot?.map((d) => d.trim()).filter((d) => d.length > 0) ?? [];
       const reportFocus = rawReportFocus?.trim();
       if (description.length === 0 || prompt.length === 0) {
-        return taskToolResult(call.id, "Error: task requires a non-empty description and prompt.");
+        const empty = (["description", "prompt"] as const).filter(
+          (name) => (name === "description" ? description : prompt).length === 0,
+        );
+        return taskToolResult(call.id, requiredTaskFieldsError(args, empty));
       }
 
       let provider: SubAgentProvider =

tests/unit/subagent.test.ts

@@ -48,16 +48,48 @@ test("task tool definition requires description and prompt", () => {
   expect(taskToolDefinition.inputSchema.required).toEqual(["description", "prompt"]);
 });
 
-test("handler rejects empty description or prompt", async () => {
+test("handler rejects empty description or prompt, naming only the empty field", async () => {
   const tool = createTaskTool({
     permissionGate: testPermissionGate,
     cwd: "/repo",
     getWorkdirBase: () => "/repo/.ctx",
     provider,
     run: async () => "should not run",
   });
-  expect(await callHandler(tool, { description: "", prompt: "do it" })).toContain("Error:");
-  expect(await callHandler(tool, { description: "label", prompt: "  " })).toContain("Error:");
+  const emptyDesc = await callHandler(tool, { description: "", prompt: "do it" });
+  expect(emptyDesc).toContain("Error: task requires a non-empty description");
+  expect(emptyDesc).toContain('Received prompt "do it"');
+  expect(emptyDesc).not.toContain("non-empty prompt");
+  const emptyPrompt = await callHandler(tool, { description: "label", prompt: "  " });
+  expect(emptyPrompt).toContain("Error: task requires a non-empty prompt");
+  expect(emptyPrompt).toContain('Received description "label" — keep it and add prompt.');
+  expect(emptyPrompt).not.toContain("non-empty description");
+});
+
+test("handler rejects missing required fields, naming only the missing ones", async () => {
+  const tool = createTaskTool({
+    permissionGate: testPermissionGate,
+    cwd: "/repo",
+    getWorkdirBase: () => "/repo/.ctx",
+    provider,
+    run: async () => "should not run",
+  });
+  const missingPrompt = await callHandler(tool, { description: "Add GET /health route" });
+  expect(missingPrompt).toContain(
+    "Error: task is missing prompt (string): the actionable goal for the worker.",
+  );
+  expect(missingPrompt).toContain(
+    'Received description "Add GET /health route" — keep it and add prompt.',
+  );
+  expect(missingPrompt).not.toContain("missing description");
+  const missingDesc = await callHandler(tool, { prompt: "do it" });
+  expect(missingDesc).toContain("Error: task is missing description (string)");
+  expect(missingDesc).toContain('Received prompt "do it" — keep it and add description.');
+  expect(missingDesc).not.toContain("missing prompt");
+  const missingBoth = await callHandler(tool, {});
+  expect(missingBoth).toContain("Error: task is missing description (string)");
+  expect(missingBoth).toContain("is missing prompt (string)");
+  expect(missingBoth).not.toContain("Received");
 });
 
 test("generic leaf gets role-default medium even when parent effort is high", async () => {