fix: normalize paths to prevent duplicate plan steps and lost provenance

[bigknoxy]

Fixes #41

Problem

Path comparisons in intent.ts used raw string equality (!==), so differently-spelled paths for the same file (e.g. absolute vs. relative, ./src/foo.ts vs src/foo.ts) were treated as distinct. This caused:

• Duplicate plan steps in generatePlan — the same file got multiple steps, failing on second application
• Lost provenance — "No edits found" when the query path didn't exactly match the recorded path spelling

Approach

• Added normalizePath() and pathsEqual() in src/core/paths.ts
• All path comparisons in src/core/intent.ts route through these helpers
• Exported utilities for reuse

Tests

• 10 tests in tests/path-normalize.test.ts
• 2 regression tests in tests/intent.test.ts

Sabotage run confirmed

Reverting the fix causes the dedup test to fail; restoring fixes all 236 tests.

No auto-merge — ready for review.

[bigknoxy]

Code Review Summary

Verdict: Comment — solid fix, 2 minor suggestions + 1 small bug to fix.

Strong Points

• Correctness: The normalizePath/pathsEqual abstraction directly fixes the duplicate-steps + lost-provenance bug described in #41. Verified with 2 targeted regression tests, all passing.
• Test coverage: 10 tests in path-normalize.test.ts + 2 regression tests in intent.test.ts. Full suite green (236 pass).
• Scope: Minimal, focused change — routes all path comparisons in intent.ts through the helpers and re-exports them from src/core/index.ts.

Warnings

• src/core/paths.ts:37 — Unused import: relative is imported but never used. Minor, but lets avoid dead imports for cleanliness.
• src/core/paths.ts:37-43 (case-insensitive FS branch) — Potential bug: the slicing uses cwd.length + 1 (original-case length) after a case-insensitive prefix match using lowerCwd/lowerP. If cwd and p differ only in casing such that lowerCwd.length !== cwd.length, the slice will be off by that delta. Since lowerCwd is already computed here, prefer return p.slice(lowerCwd.length + 1) (p keeps its original case, so slicing on lowerCwd.length preserves correctness of the leading characters while using the correct offset). This is an edge case that only bites on case-insensitive filesystems with mixed-case project directories — worth fixing now while the helper is new.

Suggestions

• bun.lock — An unrelated lockfile entry (fdir/picomatch) was added in this diff. If not required by this fix, consider regenerating the lockfile separately to keep the change isolated.
• Consider exporting pathsEqual from the package's public entry (src/core/index.ts) if external consumers will need it; currently only normalizePath and pathsEqual are re-exported — good, but double-check the public surface is intended.

Reviewed by Hermes Agent.

[bigknoxy]

Code Review Summary

Verdict: Comment (Looks solid overall, minor suggestions)

Critical

None found. The path normalization logic correctly handles relative vs. absolute paths, ./ prefixes, ../ traversal, trailing slashes, and case-insensitive filesystems.

Warnings

• src/core/paths.ts:36 -- normalizePath() calls resolve(p) after potentially join(process.cwd(), p). When p is already absolute, join still works correctly, but the double resolution (resolve then normalize) is slightly redundant. Consider simplifying to a single resolve() call, since resolve already handles normalization.
• src/core/paths.ts:49-52 -- The case-insensitive FS check uses process.platform === 'darwin' as a proxy, but Linux with WSL2 (case-insensitive NTFS) or case-insensitive Btrfs mounts would not be detected. If this tool is expected to run on WSL2, this could miss edge cases. Consider using a runtime probe instead.

Suggestions

• src/core/intent.ts:249 -- There is a duplicated normalizePath call pattern across three operations (rename-exported-symbol, remove-parameter, rename-symbol). Consider extracting a shared deduplicateRefFiles(references, definition) helper to reduce repetition.
• tests/path-normalize.test.ts:48 -- The normalizePath('') test expects '' but the function returns '' for falsy input before any processing. Consider adding a null test case as well, since the type signature accepts string | undefined | null.
• src/core/index.ts -- Missing newline at end of file in both original and new version. Suggest adding trailing newline for POSIX compliance.

Looks Good

• Well-structured pathsEqual() helper that handles all edge cases (both undefined, one undefined)
• Tests cover the dedup regression for both rename and remove-parameter operations
• The fix correctly resolves the root cause: path string inequality causing duplicate plan steps
• Good isolation of the normalization logic into a separate module

Reviewed by Hermes Agent

[bigknoxy]

Superseded by #74.

This branch was cut at fb1172e, before src/core/paths.ts existed on main. Main later added that path in #54 and #65 as the write-boundary module — assertWritable, safeWrite, atomicWrite, findProjectRoot, PathDeniedError, and the unoverridable deny-list — now imported by seven modules.

This PR introduces an unrelated file at that same path with new file mode 100644, so merging it as authored would delete every write guard in the codebase. That is what the CONFLICTING status is reporting, and resolving the add/add conflict in the natural direction reproduces the deletion rather than avoiding it.

#74 rebuilds the same fix on current main with the helpers in src/core/path-normalize.ts, plus the review cleanups (dead imports, the inconsistent pathsEqual nullish guard, and the remove-parameter test that can no longer run since #66 refuses that operation). It also covers the two branches this PR left untested.