Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/borrowed-ideas.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,14 @@ decoding — returning schema-conformant output. See memory
existing `files-edit` / `salvage-toolcall` tests). **ROI:** high.
- **Done when:** near-miss edits (whitespace/indent drift) apply instead of
failing, and malformed-but-recoverable tool calls parse without a retry.
- **Status (PR #57):** the **edit-on-autoformat** friction (the local model's #1
reported pain — the write-guard reformats after a write, so the next edit's
oldString no longer matches) is addressed: (1) `fuzzyLineReplace` now also
tolerates prettier trailing commas + semicolon drift, so more near-miss edits
just land; (2) a not-found rejection now **inlines the file's current content**
(numbered, ≤400 lines) so the model repairs the stale anchor in the SAME turn
instead of spending one on a re-`read`. BAML-style lenient tool-call parsing and
the create-side ACI echo (#2) remain follow-ups.

---

Expand Down
6 changes: 5 additions & 1 deletion packages/core/src/files/edit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,11 @@ function fuzzyLineReplace(
.trim()
.replace(/['"`]/gu, '"') // unify quote style (straight + the folded curly)
.replace(/\s+/gu, " ") // collapse internal whitespace runs
.replace(/\s*([^\w\s])\s*/gu, "$1"); // drop whitespace AROUND punctuation
.replace(/\s*([^\w\s])\s*/gu, "$1") // drop whitespace AROUND punctuation
.replace(/[;,]+$/u, ""); // drop a line-trailing `,`/`;` (prettier adds trailing
// commas in multiline literals/args and normalizes semicolons — the model
// writes `b` where the formatted file has `b,`, or `foo()` vs `foo();`). The
// unique-window guard still blocks any ambiguous match this might widen into.
// ^ `foo( a, b )`, `foo(a,b)`, and `foo(a, b)` all normalize equal, but two
// identifiers keep their separating space (`const x` never becomes `constx`),
// so the match stays meaningful. Unique-window guard still blocks ambiguity.
Expand Down
52 changes: 50 additions & 2 deletions packages/core/src/loop/tools/file-ops.ts
Original file line number Diff line number Diff line change
Expand Up @@ -643,14 +643,62 @@ export async function doEdit(
edit.edits.length > 1 ? ` (replacement #${result.index + 1})` : "";

const authored = ctx.touched?.has(edit.file.replaceAll("\\", "/")) ?? false;
let help = editFailHelp(edit.file, result, authored);

// A not-found edit is almost always a STALE ANCHOR — the auto-formatter rewrote
// the file after the model's last write, so its oldString no longer matches. The
// harness already has the current bytes, so inline them rather than make the
// model spend a turn re-`read`ing (its #1 reported friction).
if (result.reason === EDIT_FAIL_REASON.notFound) {
const view = await currentFileView(ctx.cwd, edit.file);

help +=
view === null
? ` \`read\` ${edit.file} to see its exact current content, then edit with text copied verbatim.`
: ` Its CURRENT content is below — copy oldString verbatim from it and retry (no need to \`read\`):\n\n${view}`;
}

return reject(
ctx,
`edit:${result.reason}`,
`edit ${edit.file} REJECTED${where}: ${editFailHelp(edit.file, result, authored)}`
`edit ${edit.file} REJECTED${where}: ${help}`
);
}

/** Cap on lines inlined into a not-found edit rejection; above this, fall back to
* advising a `read` so a huge file can't flood the model's context. */
const EDIT_REJECT_MAX_LINES = 400;

/** The file's current content as numbered rows (line number + `HL_LINE_SEP` + text)
* — like `read`'s body but WITHOUT its hashline header (this repairs a `str_replace`
* edit, which anchors on verbatim text, not a line hash). Null if the file is
* missing, too large to inline, or unreadable. Used to repair a stale-anchor edit in
* the SAME turn — the model copies its oldString from the post-format text. Returns null on any I/O error (race, permissions): the edit has
* already failed, so enriching its message must never crash the tool — the caller
* then falls back to advising a `read`. */
async function currentFileView(
cwd: string,
file: string
): Promise<string | null> {
try {
const handle = Bun.file(join(cwd, file));

if (!(await handle.exists())) {
return null;
}

const lines = (await handle.text()).split("\n");

if (lines.length > EDIT_REJECT_MAX_LINES) {
return null;
}

return lines.map((line, i) => `${i + 1}${HL_LINE_SEP}${line}`).join("\n");
} catch {
return null;
}
}
Comment thread
agjs marked this conversation as resolved.

/**
* Turn an edit-failure reason into ACTIONABLE feedback. The bare reason strings
* ("not-found", "missing-file") were fatally ambiguous: a slow local model read
Expand Down Expand Up @@ -681,7 +729,7 @@ function editFailHelp(
? ` Since you created ${file} this session, you may also \`create\` it again to fully rewrite it.`
: " Do NOT use `create` (it already exists).";

return `the file ${file} EXISTS, but your oldString text was not found in it.${rewrite} \`read\` the file to see its exact current contents, then edit with text copied verbatim from it.`;
return `the file ${file} EXISTS, but your oldString text was not found in it (it was likely auto-reformatted after your last write — imports reordered, quotes/commas normalized).${rewrite}`;
}

return result.reason;
Expand Down
42 changes: 42 additions & 0 deletions packages/core/tests/edit-autoformat.e2e.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { test, expect } from "bun:test";
import { mkdtemp, rm } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { formatFile } from "../src/detect-gate";
import { applyEdits } from "../src/files/edit";

// e2e: the write-guard auto-formats a file (real eslint --fix + prettier) right
// after a write, so the model's next edit anchor no longer matches disk. This
// drives the REAL formatter (not a hand-built "formatted" string) and proves a
// pre-format anchor still lands via the widened fuzzy matcher — the local model's
// #1 reported friction. (A structural rewrite the fuzzy can't bridge falls back to
// the inlined-content rejection, covered in files-edit.test.ts.)
test("a pre-format edit anchor survives the real write-guard auto-format", async () => {
const dir = await mkdtemp(join(tmpdir(), "tsforge-fmt-e2e-"));
const file = "demo.ts";
// No trailing comma, no semicolons — the formatter will add both.
const before = "const o = {\n a: 1,\n b: 2\n}\n";

await Bun.write(join(dir, file), before);

try {
await formatFile(dir, file);
const after = await Bun.file(join(dir, file)).text();

// Sanity: the formatter actually reformatted (else the test proves nothing).
expect(after).not.toBe(before);
expect(after).toContain("b: 2,"); // prettier added the trailing comma

// The model's pre-format anchor (`b: 2\n}`, no trailing comma) exact-misses the
// formatted file (`b: 2,\n};`) — it must still apply via the fuzzy fallback,
// NOT reject and force a re-read.
const r = await applyEdits(dir, file, [
{ oldString: " a: 1,\n b: 2\n}", newString: " a: 1,\n b: 999\n}" },
]);

expect(r.ok).toBe(true);
expect(await Bun.file(join(dir, file)).text()).toContain("b: 999");
} finally {
await rm(dir, { recursive: true, force: true });
}
});
70 changes: 70 additions & 0 deletions packages/core/tests/files-edit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { mkdtemp, rm } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { applyEdit, applyEdits } from "../src/files/edit";
import { doEdit } from "../src/loop/tools/file-ops";
import type { IToolContext } from "../src/loop/tools/tool-context";

async function tmp(files: Record<string, string>): Promise<string> {
const dir = await mkdtemp(join(tmpdir(), "tsforge-edit-"));
Expand Down Expand Up @@ -139,6 +141,44 @@ test("fuzzy edit preserves CRLF line endings (no mixed endings) — issue #24",
}
});

test("fuzzy edit tolerates a prettier trailing comma (multiline literal)", async () => {
// Prettier added a trailing comma after `b: 2`. The model's oldString ends the
// window at `b: 2` (no comma), so the EXACT match misses (`b: 2\n}` ≠ `b: 2,\n}`);
// only the comma-tolerant fuzzy path can match.
const dir = await tmp({ "a.ts": "const o = {\n a: 1,\n b: 2,\n};\n" });

try {
const r = await applyEdits(dir, "a.ts", [
{ oldString: " a: 1,\n b: 2\n}", newString: " a: 1,\n b: 3\n}" },
]);

expect(r.ok).toBe(true);
expect(await Bun.file(join(dir, "a.ts")).text()).toContain("b: 3");
} finally {
await rm(dir, { recursive: true, force: true });
}
});

test("fuzzy edit tolerates semicolon-style drift", async () => {
// File has trailing semicolons (prettier); the model's multi-line oldString
// omits them, so the exact match misses and the semicolon-tolerant fuzzy fires.
const dir = await tmp({ "a.ts": "const a = 1;\nconst b = 2;\n" });

try {
const r = await applyEdits(dir, "a.ts", [
{
oldString: "const a = 1\nconst b = 2",
newString: "const a = 1\nconst b = 99",
},
]);

expect(r.ok).toBe(true);
expect(await Bun.file(join(dir, "a.ts")).text()).toContain("const b = 99");
} finally {
await rm(dir, { recursive: true, force: true });
}
});

test("fuzzy delete (empty newString) removes the matched lines, no blank line", async () => {
// Wrong indentation forces the fuzzy path; an empty newString should DELETE
// the matched line, not replace it with a blank line.
Expand Down Expand Up @@ -345,3 +385,33 @@ test("applyEdits sees the result of earlier replacements (sequential)", async ()
await rm(dir, { recursive: true, force: true });
}
});

// A not-found edit (stale anchor after auto-format) inlines the file's CURRENT
// content into the rejection, so the model repairs it in the SAME turn instead
// of spending one on a re-`read` — the model's #1 reported friction.
test("not-found edit rejection carries the file's current content", async () => {
const dir = await tmp({ "a.ts": "const x = 1;\nconst y = 2;\n" });
const ctx: IToolContext = {
cwd: dir,
files: ["**/*.ts"],
task: "t",
report: () => undefined,
};

try {
const msg = await doEdit(
{ file: "a.ts", oldString: "const z = 99;", newString: "const z = 0;" },
ctx
);

expect(msg).toContain("REJECTED");
expect(msg).toContain("CURRENT content is below");
// The actual current lines are present so the model can copy oldString verbatim.
expect(msg).toContain("const x = 1;");
expect(msg).toContain("const y = 2;");
// And it does NOT tell the model to go `read` the file (content is inline).
expect(msg).not.toContain("`read` a.ts to see");
} finally {
await rm(dir, { recursive: true, force: true });
}
});