fix(toolpath-convo): strip leading slash in unified_diff headers#43
Merged
Conversation
|
🔍 Preview deployed: https://fb9414f9.toolpath.pages.dev |
eliothedeman
commented
Apr 22, 2026
Collaborator
Author
eliothedeman
left a comment
There was a problem hiding this comment.
Summary
Targeted, correct fix for #36. unified_diff now strips the leading / so absolute paths stop emitting --- a//abs/path.rs. Both --- and +++ headers use the trimmed display, and the misleading Claude test is corrected rather than deleted.
Notes
crates/toolpath-convo/src/derive.rs:381—trim_start_matches('/')strips all leading slashes, not just one. The issue only spec'd stripping a single/, but this is the safer choice (nonstandard//fooinputs also normalize) andtest_unified_diff_strips_leading_slash_on_absolute_pathenforces the!contains("a//")invariant. Worth noting in case anyone expectsPath::new-style "preserve//root" semantics, but I'd keep it as-is.- Edge cases behave reasonably: empty string and bare
/both produce--- a/\n+++ b/\n— same as before for the empty case, strictly better for/. Relative paths pass through untouched (covered bytest_unified_diff_preserves_relative_path). crates/toolpath-claude/src/derive.rs:1619— good that the updated test asserts both headers plus the negative!raw.contains("a//"); that's what keeps the canonicalised-bug from reappearing.- Scope is clean: no version bump (correct — public signature and semantics for the common relative-path case are unchanged), no CHANGELOG entry (consistent with the file's version-scoped pattern), no refactoring drift.
- Risk: a downstream consumer that specifically parses
a//abs/...would break, but that already means they worked around our bug. The PR body'spatch(1)framing is right — this makes output correct per git conventions.
Verdict
LGTM (commenting rather than approving — GitHub blocks self-approval). Matches the issue spec, has regression coverage on both sides of the crate boundary, and the doc comment on unified_diff now explains why the trim happens so a future reader won't undo it.
`unified_diff(path, ...)` previously emitted `--- a//abs/file.rs` / `+++ b//abs/file.rs` when `path` already started with `/` (common for Claude's `tool_use.input.file_path`, which carries absolute paths). Git-style `a/` and `b/` prefixes already denote the repo root, so the doubled slash breaks `patch(1)` and any other tool that parses the header. Trim a single leading `/` before splicing into the header. Add a regression test in `toolpath-convo::derive::tests` and update the `toolpath-claude::derive::test_derive_path_edit_tool_emits_unified_diff` assertion, which had been canonicalising the broken output. The frontend diff renderer splits on lines and does not parse the header, so no UI breakage. Internal-only; no API change. Closes #36
7ae44f8 to
783aa51
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #36
Summary
unified_diff(path, ...)previously emitted--- a//abs/file.rs/+++ b//abs/file.rswheneverpathalready started with/(common for Claude'stool_use.input.file_path, which carries absolute paths like/Users/.../src/login.rs). Git-stylea/andb/prefixes already denote the repo root, so the doubled slash breakspatch(1)and any other tool that parses the header.The fix trims a single leading
/before splicing into the header. Thetoolpath-claudetest that asserted on the broken form ("--- a//src/login.rs") has been updated to match — it had been canonicalising the bug.Changes
crates/toolpath-convo/src/derive.rs::unified_diff— strip leading/viatrim_start_matches('/'); doc comment updated to explain the rationale.crates/toolpath-convo/src/derive.rs— addtest_unified_diff_strips_leading_slash_on_absolute_pathandtest_unified_diff_preserves_relative_pathregression tests.crates/toolpath-claude/src/derive.rs::test_derive_path_edit_tool_emits_unified_diff— assert the correct headers and explicitly reject anya//substring.Risk
Low. The frontend diff renderer (
toolpath-desktop) splits on lines and does not parse the header, so no UI breakage. External consumers piping.path.jsondiffs intopatch(1)benefit directly. No public API change; no version bump.Test plan
cargo test -p toolpath-convo -p toolpath-claudecargo clippy -p toolpath-convo -p toolpath-claude -- -D warnings