Merge pull request #575 from corbitsdev/cl-6962-edit-and-write-tools-should-return-the-changed-region-so-no

Return the changed region from edit/write/delete/apply_patch

Author: TheGreatAxios

docs/PRODUCT.md

@@ -92,7 +92,7 @@ is the direct, explicit resume path.
   - Paths outside the workspace and writes under the session state root still ask; mutating MCP and unknown tools still prompt.
 
 - **Path sandboxing** — Tool path arguments are resolved against the working directory; paths that escape it are blocked unless `--dangerously-skip-permissions` / `/yolo` is on (secret-guard and authz hard denies still apply).
-- **Write verification** — After every write/edit the file is re-read and compared to confirm the change actually landed.
+- **Write verification** — After every write/edit the file is re-read and compared to confirm the change actually landed; the result returned to the model (and shown to the operator) includes a bounded diff of the changed region — `write_file`, `edit_file`, `delete_file`, and each op inside `apply_patch` — so a follow-up `read_file` is never needed just to confirm an edit landed. A whole-file rewrite's diff is truncated (and says so) rather than blowing the result size cap.
 
 ## Slash Commands (TUI)
 

src/agent/apply-patch-diff.test.ts

@@ -0,0 +1,131 @@
+import { describe, expect, test } from "bun:test";
+import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { createPosixTools } from "@intx/tools-posix";
+import { createToolRunner } from "@intx/agent";
+import type { AgentTool } from "@intx/agent";
+
+import { createCodexToolProxies, type CodexRunTool } from "./codex-tool-proxies.js";
+import { buildCorePosixToolPlugins } from "./posix-tool-plugins.js";
+import { createPermissionGate } from "../permission/gate.js";
+
+/**
+ * apply_patch forwards each op through the same posixTools.run chain the rest
+ * of the agent uses (see tools.ts), so verify-plugin's and delete-file-plugin's
+ * diffs surface here too without any apply_patch-specific plumbing.
+ */
+async function invokeApplyPatch(tools: AgentTool[], input: string) {
+  const runner = createToolRunner(tools);
+  return runner.run(
+    { id: "call-1", name: "apply_patch", arguments: { input } },
+    new AbortController().signal,
+  );
+}
+
+async function makeApplyPatch(cwd: string): Promise<AgentTool[]> {
+  const gate = createPermissionGate({
+    approvals: [],
+    interactive: false,
+    skipPermissions: true,
+    auto: false,
+    cwd,
+  });
+  const posixTools = createPosixTools({
+    cwd,
+    plugins: buildCorePosixToolPlugins({ cwd, permissionGate: gate }),
+  });
+  const runTool: CodexRunTool = async (name, args) => {
+    // read_file's real tool output is cat -n formatted (line numbers), which
+    // is not the raw content applyUpdateHunks needs — in production this
+    // means Update File hunks generally fail to match context (filed as
+    // CL-6966, Urgent; not this issue's bug to fix). This stub bypasses that
+    // known defect by returning raw content, so the "Update File shows the
+    // diff" test below is NOT proof that apply_patch Update works end to end
+    // — it only proves the diff-surfacing added here is correct once the op
+    // succeeds. Add File / Delete File below do not depend on read_file and
+    // are real, unstubbed coverage.
+    if (name === "read_file") {
+      const path = String((args as { path?: unknown }).path ?? "");
+      try {
+        return { content: await readFile(join(cwd, path), "utf8") };
+      } catch (err) {
+        return { content: err instanceof Error ? err.message : String(err), isError: true };
+      }
+    }
+    const result = await posixTools.run(
+      { id: "codex-proxy", name, arguments: args },
+      new AbortController().signal,
+    );
+    return {
+      content: typeof result.content === "string" ? result.content : JSON.stringify(result.content),
+      ...(result.isError === true ? { isError: true } : {}),
+    };
+  };
+  return createCodexToolProxies({
+    isCodex: true,
+    runTool,
+    runManageTasks: async () => ({ content: "ok" }),
+  });
+}
+
+describe("apply_patch surfaces the changed region", () => {
+  test("Update File shows the diff", async () => {
+    const cwd = await mkdtemp(join(tmpdir(), "apply-patch-diff-"));
+    try {
+      await writeFile(join(cwd, "a.txt"), "line1\nworld\nline3\n");
+      const tools = await makeApplyPatch(cwd);
+      const input = [
+        "*** Begin Patch",
+        "*** Update File: a.txt",
+        "@@",
+        " line1",
+        "-world",
+        "+universe",
+        " line3",
+        "*** End Patch",
+      ].join("\n");
+
+      const result = await invokeApplyPatch(tools, input);
+
+      expect(result.isError).not.toBe(true);
+      expect(String(result.content)).toContain("-world");
+      expect(String(result.content)).toContain("+universe");
+    } finally {
+      await rm(cwd, { recursive: true, force: true });
+    }
+  });
+
+  test("Delete File shows the removed content", async () => {
+    const cwd = await mkdtemp(join(tmpdir(), "apply-patch-diff-"));
+    try {
+      await writeFile(join(cwd, "gone.txt"), "bye\n");
+      const tools = await makeApplyPatch(cwd);
+      const input = ["*** Begin Patch", "*** Delete File: gone.txt", "*** End Patch"].join("\n");
+
+      const result = await invokeApplyPatch(tools, input);
+
+      expect(result.isError).not.toBe(true);
+      expect(String(result.content)).toContain("-bye");
+    } finally {
+      await rm(cwd, { recursive: true, force: true });
+    }
+  });
+
+  test("Add File shows the added content", async () => {
+    const cwd = await mkdtemp(join(tmpdir(), "apply-patch-diff-"));
+    try {
+      const tools = await makeApplyPatch(cwd);
+      const input = ["*** Begin Patch", "*** Add File: new.txt", "+hello", "*** End Patch"].join(
+        "\n",
+      );
+
+      const result = await invokeApplyPatch(tools, input);
+
+      expect(result.isError).not.toBe(true);
+      expect(String(result.content)).toContain("+hello");
+    } finally {
+      await rm(cwd, { recursive: true, force: true });
+    }
+  });
+});

src/plugins/change-diff.test.ts

@@ -0,0 +1,68 @@
+import { describe, expect, test } from "bun:test";
+import { formatChangeDiff, MAX_DIFF_CHARS } from "./change-diff.js";
+
+describe("formatChangeDiff", () => {
+  test("returns undefined when content is unchanged", () => {
+    expect(formatChangeDiff("a.txt", "same\n", "same\n")).toBeUndefined();
+  });
+
+  test("small edit produces a unified diff with context", () => {
+    const before = "line1\nline2\nline3\nline4\nline5\n";
+    const after = "line1\nline2\nCHANGED\nline4\nline5\n";
+
+    const diff = formatChangeDiff("a.txt", before, after);
+
+    expect(diff).toBeDefined();
+    expect(diff).toContain("--- a.txt");
+    expect(diff).toContain("+++ a.txt");
+    expect(diff).toContain("-line3");
+    expect(diff).toContain("+CHANGED");
+    expect(diff).toContain(" line2");
+    expect(diff).toContain(" line4");
+  });
+
+  test("whole-file rewrite is bounded by the char cap and says so", () => {
+    const before = "old content\n".repeat(2000);
+    const after = "new content\n".repeat(2000);
+
+    const diff = formatChangeDiff("a.txt", before, after);
+
+    expect(diff).toBeDefined();
+    // The cap must hold exactly — the truncation note is reserved WITHIN
+    // maxChars, not appended after it.
+    expect(diff!.length).toBeLessThanOrEqual(MAX_DIFF_CHARS);
+    expect(diff).toContain("truncated");
+  });
+
+  test("truncation note never pushes the result past the cap at a small boundary", () => {
+    // A tiny maxChars stresses the fixed-point loop in truncate(): the note's
+    // own length (which depends on the digit counts it reports) must still
+    // fit within the cap it is describing.
+    const before = "a\n".repeat(50);
+    const after = "b\n".repeat(50);
+
+    for (const maxChars of [50, 80, 120, 200]) {
+      const diff = formatChangeDiff("a.txt", before, after, maxChars);
+      expect(diff).toBeDefined();
+      expect(diff!.length).toBeLessThanOrEqual(maxChars);
+    }
+  });
+
+  test("very large files skip full LCS and report a bounded summary", () => {
+    const before = Array.from({ length: 3000 }, (_, i) => `l${i}`).join("\n");
+    const after = Array.from({ length: 3000 }, (_, i) => `m${i}`).join("\n");
+
+    const diff = formatChangeDiff("big.txt", before, after);
+
+    expect(diff).toBeDefined();
+    expect(diff).toContain("large change");
+    expect(diff).toContain("exceeds");
+    expect(diff!.length).toBeLessThanOrEqual(MAX_DIFF_CHARS);
+  });
+
+  test("deletion (after is empty) shows removed lines", () => {
+    const diff = formatChangeDiff("gone.txt", "keep me\n", "");
+    expect(diff).toBeDefined();
+    expect(diff).toContain("-keep me");
+  });
+});