fix(filesystem): improve edit_file error with nearest-match diagnostics

[Christian-Sidak]

Summary

When edit_file fails to find a match (neither exact nor whitespace-flexible), the error message currently just dumps the oldText with no context about what the file actually contains. This makes it hard for both agents and humans to diagnose why the match failed.

This PR improves the error to include:

• File path in the error message
• Closest near-match location with 1-indexed line numbers
• Match percentage (how many lines matched after trimming)
• Line-by-line comparison showing which lines matched and which diverged, with both the actual and expected content

Example error after this change:

Could not find match for edit in /path/to/file.ts
Closest match (75% of lines) at lines 42-45:
   line 42: "  const foo = bar;"
 ! line 43: "  baz();"
     expected: "  baz(x);"
   line 44: "  return result;"
   line 45: "}"

Edit oldText:
  const foo = bar;
  baz(x);
  return result;
}

No changes to the actual matching/replacement logic -- this is purely diagnostic improvement.

Fixes #2034

Test plan

• All 146 existing tests pass
• Updated test name and assertion to match new error format
• Verified error includes file path and diagnostic context on mismatch

[Christian-Sidak]

Friendly bump -- would love a review when someone has a chance. Happy to address any feedback.

[Christian-Sidak]

Closing due to inactivity. Happy to reopen if there's interest.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 45s —— View job

---

PR Review: fix(filesystem): improve edit_file error with nearest-match diagnostics

• Read changed files and git diff
• Analyze implementation quality
• Check tests
• Post review feedback

---

Overall: This is a well-targeted quality-of-life improvement for agent/human debugging. The feature is correctly scoped (error paths only), and the diagnostic output format is sensible. There are a few issues worth addressing before merging.

---

src/filesystem/lib.ts

Issue 1 — Redundant variable declarations (code smell)

oldLines and contentLines are already declared in the outer edit-loop block at lines 215–216. The new code inside if (!matchFound) re-declares them as new const bindings (shadowing the outer ones). This works in JavaScript/TypeScript since it's a child block scope, but it splits the same strings twice for no reason and is confusing.

// Already exists in outer scope:
const oldLines = normalizedOld.split('\n');
const contentLines = modifiedContent.split('\n');
...
if (!matchFound) {
  // These shadow the outer variables — unnecessary
  const oldLines = normalizedOld.split('\n');    // ← redundant
  const contentLines = modifiedContent.split('\n'); // ← redundant

The fix is to simply remove the two const re-declarations inside the if block and reuse the outer variables.

Fix this →

Issue 2 — Weak test coverage for the new diagnostic

The test update only changes the error message string assertion from 'Could not find exact match for edit' to 'Could not find match for edit'. It does not test any of the new diagnostic content (file path, line numbers, match percentage, or line-by-line diff).

The new logic has meaningful branches:

• bestScore > 0 → shows closest match with percentage and diff
• bestScore === 0 → shows "No similar content found" fallback

Neither branch is exercised by the existing test (the test uses 'nonexistent line' as oldText in a ~20-line mock file, which would produce a score > 0 from partial matches against other content). Consider adding:

it('error includes file path and nearest-match diagnostics', async () => {
  const edits = [{ oldText: 'nonexistent line', newText: 'replacement' }];
  await expect(applyFileEdits('/test/file.txt', edits, false))
    .rejects.toThrow(/Could not find match for edit in \/test\/file\.txt/);
});

it('error reports no similar content when oldText is completely foreign', async () => {
  const edits = [{ oldText: 'XXXXXXXXXXXXXXXXXXXXXXXX', newText: 'y' }];
  await expect(applyFileEdits('/test/file.txt', edits, false))
    .rejects.toThrow(/No similar content found/);
});

Fix this →

---

Minor observations (non-blocking)

• JSON.stringify for line output — Using JSON.stringify(line) makes whitespace visible (e.g., " const foo = bar;"). This is intentional and smart for whitespace mismatch debugging.
• O(n²) complexity — The near-match scan is quadratic in file size, but since it's only hit on error paths, this is an acceptable trade-off. No action needed.
• Edge case: oldText longer than file — The for loop guard i <= contentLines.length - oldLines.length handles this correctly (the loop doesn't run, bestScore === 0, fallback message shown). Good.

---

Summary

The core idea is solid and useful. Two things to address before merging:

1. Remove the redundant oldLines/contentLines declarations inside the if block.
2. Strengthen the test to actually verify the diagnostic content (file path, at minimum).

The PR was closed due to inactivity — worth reopening and addressing these small items.