🤖 fix: enforce single guard for file_edit_insert

Generated with

Author: ammar-agent

src/services/tools/file_edit_insert.test.ts

@@ -80,7 +80,7 @@ describe("file_edit_insert tool", () => {
     }
   });
 
-  it("fails when before/after disagree", async () => {
+  it("fails when both before and after are provided", async () => {
     const tool = createTestTool(testDir);
     const args: FileEditInsertToolArgs = {
       file_path: path.relative(testDir, testFilePath),
@@ -92,7 +92,7 @@ describe("file_edit_insert tool", () => {
     const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditInsertToolResult;
     expect(result.success).toBe(false);
     if (!result.success) {
-      expect(result.error).toContain("Guard mismatch");
+      expect(result.error).toContain("only one of before or after");
     }
   });
 

src/services/tools/file_edit_insert.ts

@@ -23,6 +23,27 @@ interface InsertOperationFailure {
   note?: string;
 }
 
+interface InsertContentOptions {
+  before?: string;
+  after?: string;
+  lineOffset?: number;
+}
+
+interface GuardResolutionSuccess {
+  success: true;
+  index: number;
+}
+
+function guardFailure(error: string): InsertOperationFailure {
+  return {
+    success: false,
+    error,
+    note: READ_AND_RETRY_NOTE,
+  };
+}
+
+type GuardAnchors = Pick<InsertContentOptions, "before" | "after">;
+
 export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration) => {
   return tool({
     description: TOOL_DEFINITIONS.file_edit_insert.description,
@@ -108,18 +129,20 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration)
 function insertContent(
   originalContent: string,
   contentToInsert: string,
-  { before, after, lineOffset }: { before?: string; after?: string; lineOffset?: number }
+  options: InsertContentOptions
 ): InsertOperationSuccess | InsertOperationFailure {
+  const { before, after, lineOffset } = options;
+
+  if (before !== undefined && after !== undefined) {
+    return guardFailure("Provide only one of before or after (not both).");
+  }
+
   if (before !== undefined || after !== undefined) {
     return insertWithGuards(originalContent, contentToInsert, { before, after });
   }
 
   if (lineOffset === undefined) {
-    return {
-      success: false,
-      error: "line_offset must be provided when before/after guards are omitted.",
-      note: READ_AND_RETRY_NOTE,
-    };
+    return guardFailure("line_offset must be provided when before/after guards are omitted.");
   }
 
   return insertWithLineOffset(originalContent, contentToInsert, lineOffset);
@@ -128,46 +151,17 @@ function insertContent(
 function insertWithGuards(
   originalContent: string,
   contentToInsert: string,
-  { before, after }: { before?: string; after?: string }
+  anchors: GuardAnchors
 ): InsertOperationSuccess | InsertOperationFailure {
-  let anchorIndex: number | undefined;
-
-  if (before !== undefined) {
-    const beforeIndexResult = findUniqueSubstringIndex(originalContent, before, "before");
-    if (!beforeIndexResult.success) {
-      return beforeIndexResult;
-    }
-    anchorIndex = beforeIndexResult.index + before.length;
-  }
-
-  if (after !== undefined) {
-    const afterIndexResult = findUniqueSubstringIndex(originalContent, after, "after");
-    if (!afterIndexResult.success) {
-      return afterIndexResult;
-    }
-
-    if (anchorIndex === undefined) {
-      anchorIndex = afterIndexResult.index;
-    } else if (anchorIndex !== afterIndexResult.index) {
-      return {
-        success: false,
-        error:
-          "Guard mismatch: before and after substrings do not point to the same insertion point.",
-        note: READ_AND_RETRY_NOTE,
-      };
-    }
-  }
-
-  if (anchorIndex === undefined) {
-    return {
-      success: false,
-      error: "Unable to determine insertion point from guards.",
-      note: READ_AND_RETRY_NOTE,
-    };
+  const anchorResult = resolveGuardAnchor(originalContent, anchors);
+  if (!anchorResult.success) {
+    return anchorResult;
   }
 
   const newContent =
-    originalContent.slice(0, anchorIndex) + contentToInsert + originalContent.slice(anchorIndex);
+    originalContent.slice(0, anchorResult.index) +
+    contentToInsert +
+    originalContent.slice(anchorResult.index);
 
   return {
     success: true,
@@ -180,49 +174,62 @@ function findUniqueSubstringIndex(
   haystack: string,
   needle: string,
   label: "before" | "after"
-): InsertOperationFailure | { success: true; index: number } {
+): GuardResolutionSuccess | InsertOperationFailure {
   const firstIndex = haystack.indexOf(needle);
   if (firstIndex === -1) {
-    return {
-      success: false,
-      error: `Guard mismatch: unable to find ${label} substring in the current file.`,
-      note: READ_AND_RETRY_NOTE,
-    };
+    return guardFailure(`Guard mismatch: unable to find ${label} substring in the current file.`);
   }
 
   const secondIndex = haystack.indexOf(needle, firstIndex + needle.length);
   if (secondIndex !== -1) {
-    return {
-      success: false,
-      error: `Guard mismatch: ${label} substring matched multiple times. Provide a more specific string.`,
-      note: READ_AND_RETRY_NOTE,
-    };
+    return guardFailure(
+      `Guard mismatch: ${label} substring matched multiple times. Provide a more specific string.`
+    );
   }
 
   return { success: true, index: firstIndex };
 }
 
+function resolveGuardAnchor(
+  originalContent: string,
+  { before, after }: GuardAnchors
+): GuardResolutionSuccess | InsertOperationFailure {
+  if (before !== undefined) {
+    const beforeIndexResult = findUniqueSubstringIndex(originalContent, before, "before");
+    if (!beforeIndexResult.success) {
+      return beforeIndexResult;
+    }
+    return { success: true, index: beforeIndexResult.index + before.length };
+  }
+
+  if (after !== undefined) {
+    const afterIndexResult = findUniqueSubstringIndex(originalContent, after, "after");
+    if (!afterIndexResult.success) {
+      return afterIndexResult;
+    }
+    return { success: true, index: afterIndexResult.index };
+  }
+
+  return guardFailure("Unable to determine insertion point from guards.");
+}
+
 function insertWithLineOffset(
   originalContent: string,
   contentToInsert: string,
   lineOffset: number
 ): InsertOperationSuccess | InsertOperationFailure {
-  const lines = originalContent.split("\n");
-  if (lineOffset > lines.length) {
+  const lineCount = countLines(originalContent);
+  if (lineOffset > lineCount) {
     return {
       success: false,
-      error: `line_offset ${lineOffset} is beyond file length (${lines.length} lines)`,
-      note: `${EDIT_FAILED_NOTE_PREFIX} The file has ${lines.length} lines. ${NOTE_READ_FILE_RETRY}`,
+      error: `line_offset ${lineOffset} is beyond file length (${lineCount} lines)`,
+      note: `${EDIT_FAILED_NOTE_PREFIX} The file has ${lineCount} lines. ${NOTE_READ_FILE_RETRY}`,
     };
   }
 
   const insertionIndex = getIndexForLineOffset(originalContent, lineOffset);
   if (insertionIndex === null) {
-    return {
-      success: false,
-      error: `Unable to compute insertion point for line_offset ${lineOffset}.`,
-      note: READ_AND_RETRY_NOTE,
-    };
+    return guardFailure(`Unable to compute insertion point for line_offset ${lineOffset}.`);
   }
 
   const newContent =
@@ -264,3 +271,17 @@ function getIndexForLineOffset(content: string, lineOffset: number): number | nu
 
   return null;
 }
+
+function countLines(content: string): number {
+  if (content.length === 0) {
+    return 1;
+  }
+
+  let count = 1;
+  for (const char of content) {
+    if (char === "\n") {
+      count += 1;
+    }
+  }
+  return count;
+}

src/utils/tools/toolDefinitions.ts

@@ -112,6 +112,7 @@ export const TOOL_DEFINITIONS = {
     description:
       "Insert content into a file using either a line offset or substring guards. " +
       "Provide at least one of before/after/line_offset so the operation is anchored. " +
+      "When using guards, supply exactly one of before or after (not both). " +
       `Optional before/after substrings must uniquely match surrounding content. ${TOOL_EDIT_WARNING}`,
     schema: z
       .object({
@@ -148,7 +149,11 @@ export const TOOL_DEFINITIONS = {
             "Provide at least one of line_offset, before, or after to anchor the insertion point.",
           path: ["line_offset"],
         }
-      ),
+      )
+      .refine((data) => !(data.before !== undefined && data.after !== undefined), {
+        message: "Provide only one of before or after (not both).",
+        path: ["before"],
+      }),
   },
   propose_plan: {
     description: