Skip to content

fix: make content path mapping separator- and escaping-aware - #82

Merged
tawanorg merged 6 commits into
tawanorg:mainfrom
KritBlade:fix/cross-os-content-path-mapping
Oct 6, 2026
Merged

tawanorg merged 6 commits into
tawanorg:mainfrom
KritBlade:fix/cross-os-content-path-mapping

Conversation

@KritBlade

Copy link
Copy Markdown
Contributor

Problem

Content path translation is not safe across operating systems. NormalizeContent / ResolveContent operate on raw bytes using a single spelling of the mapped path, but .jsonl and .json escape a backslash as a pair. A Windows device therefore never matches its own paths, and two things go wrong.

1. Windows content is never made portable. A transcript stores "cwd":"H:\\Github\\Dev\\my-app", while the mapper looks for H:\Github\Dev. No match, so the content uploads unnormalized and a POSIX device pulling it gets a path it cannot use.

2. Pulling POSIX-authored sessions onto Windows writes invalid JSON. The reverse direction does match, so the remote holds "cwd":"${WORK}/my-app". On pull, a Windows device substitutes the token with a raw backslash path, producing:

{"cwd":"H:\Github\Dev\my-app"}

\G is not a valid JSON escape. The .jsonl no longer parses — invalid character 'G' in string escape code. The same applies to the automatic ${HOME} token, so this reproduces with no path_map configured at all.

Minimal repro against main:

posix, _ := NewPathMapper("/Users/alice", map[string]string{"/Users/alice/work": "WORK"})
win, _ := NewPathMapper(`C:\Users\bob`, map[string]string{`H:\work`: "WORK"})

norm := posix.NormalizeContent([]byte(`{"cwd":"/Users/alice/work/app"}`))
out := win.ResolveContent(norm)
// {"cwd":"H:\work\app"}  -> json.Unmarshal fails

Remote key mapping is unaffected and already correct, because EncodeClaudePath flattens every non-alphanumeric character. Only file content is affected.

Fix

NormalizeContent and ResolveContent now take the file's relative path and derive a content kind from its extension:

  • .json / .jsonl — a backslash is escaped as a pair
  • .md / .txt — a backslash is verbatim

Matched path tails are canonicalized to / in the remote form, so a Windows device and a POSIX device produce byte-identical remote content for the same logical path. On pull the token is rendered with the local separator, escaped for the format being written.

Only the root's native spelling is matched. Windows also accepts C:/like/this, but that form shows up mostly inside quoted error text and tool output; rewriting a device's own prose is worse than leaving one uncommon spelling unmapped. This was not a hypothetical — an early revision matched it and rewrote a PowerShell error message quoted inside a real transcript.

Testing

New internal/sync/paths_crossos_test.go:

  • both operating systems normalize the same logical path to the same remote bytes
  • round trips in both directions, for both content kinds
  • resolved JSON stays parseable, with the decoded cwd checked
  • a Windows root does not over-match a longer sibling directory (H:\Github\Development)

Existing tests keep their original expectations; only the call signature changed.

Also validated outside the test suite against 57 real transcripts on a Windows device: 35 of the 36 files containing a mapped path round-trip byte-identically on the same device, and all 57 stay valid JSON after a POSIX device resolves them. The single exclusion is a transcript quoting a literal ${HOME}, which is the pre-existing token ambiguity already documented on pathToken.

End-to-end on a real R2 bucket across a Windows and a macOS device: 28 files pulled macOS → Windows, 3379 JSONL lines, zero parse failures, ${WORK} and ${HOME} both resolving to the correct local roots.

gofmt and go vet are clean. On Windows six tests fail both before and after this change, all asserting Unix file modes (0600 vs 666); they pass on Linux CI.

Compatibility

Content already on a bucket is untouched and still readable. Windows-authored files uploaded before this change were never normalized, so they carry absolute Windows paths; they become portable the next time that device pushes them. Files whose content was already tokenized resolve correctly under the new code.

Content translation operated on raw bytes using a single spelling of the
mapped path, but .jsonl and .json escape a backslash as a pair. A Windows
device therefore never matched its own paths, and two things went wrong:

- Its content was pushed unnormalized, so it never became portable and a
  POSIX device pulling it got paths it could not use.
- Resolving a token on a Windows device substituted a raw backslash path
  into a JSON string, producing invalid escapes such as \G. Pulling
  POSIX-authored sessions onto Windows wrote .jsonl files that no longer
  parse ("invalid character 'G' in string escape code").

NormalizeContent and ResolveContent now take the file's relative path and
derive a content kind from its extension: .json/.jsonl escape a backslash
as a pair, .md/.txt hold it verbatim. Matched path tails are canonicalized
to "/" in the remote form, so a Windows device and a POSIX device produce
byte-identical remote content, and are rendered with the local separator,
escaped for the target format, on pull.

Only the root's native spelling is matched. Windows also accepts
C:/like/this, but that form shows up mostly inside quoted error text and
tool output, where rewriting a device's own prose would be worse than
leaving one uncommon spelling unmapped.

Remote key mapping is unchanged; it was already separator-agnostic because
EncodeClaudePath flattens every non-alphanumeric character.

Verified against 57 real transcripts from a Windows device: 35 of 36 files
containing a mapped path round-trip byte-identically on the same device,
and all 57 remain valid JSON after a POSIX device resolves them. The one
exclusion is a transcript that quotes a literal ${HOME}, which is the
pre-existing token ambiguity already documented on pathToken.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@KritBlade
KritBlade force-pushed the fix/cross-os-content-path-mapping branch from 867e849 to c6863af Compare August 12, 2026 22:23
@KritBlade

Copy link
Copy Markdown
Contributor Author

Heads up: I opened this before spotting #73, which targets the same underlying bug — its description identifies the same cause, that byte replacement eats a JSON escape and leaves the document invalid. Flagging the overlap so you're not reviewing two PRs without knowing they collide. Both touch internal/sync/paths.go, sync.go and paths_test.go, so they will conflict textually.

I checked out #73 and ran this PR's cross-OS scenarios against it, rewritten to its NormalizeFile / ResolveFile API. It passes all of them:

scenario #73
POSIX → Windows, .jsonl pass
Windows → POSIX, .jsonl pass
Windows → POSIX, .md pass
POSIX → Windows, .md pass
resolved JSON still parses pass
both OSes normalize to the same remote bytes pass
Windows root does not over-match a longer sibling pass

So #73 fixes the corruption independently. The two differ in approach rather than in what they fix:

  • fix: translate absolute paths in plugin state files #73 decodes the document, walks strings, re-encodes. Escaping is correct by construction, and it also covers installed_plugins.json / known_marketplaces.json, which this PR does not.
  • This PR matches at the byte level per content kind, leaving everything outside the mapped span untouched.

Two observations from running them side by side, offered as data rather than argument:

1. The re-encode is not byte-preserving. Same device, no cross-OS involved:

before: {"z":1,"a":"C:\\work\\projects\\app","m":[1,2],"n":1.50}
after : {"a":"C:\\work\\projects\\app","m":[1,2],"n":1.50,"z":1}

Keys come back sorted, since Go marshals map[string]any in key order. UseNumber correctly preserves 1.50, so numbers are safe, but every JSON document that passes through is rewritten — including \u0026\u0026 coming back as &&. In practice every transcript changes bytes and re-uploads on the first sync after the change. Worth a deliberate decision either way; on multi-tens-of-MB session files it is not free.

2. Embedded paths are treated differently. #73 maps a string only when the mapped path is its prefix, so this is left alone:

{"command":"cd /Users/alice/projects/app && ls"}

This PR rewrites it. Neither is obviously right — #73's choice is the more conservative one, and it sidesteps a trap I hit: an earlier revision here also matched C:/forward/slash roots and ended up rewriting a path quoted inside a PowerShell error message in a real transcript. That is why only the root's native spelling is matched now.

Happy to close this in favour of #73 if that is the direction you prefer — no attachment to the implementation. If it is useful, the cross-OS cases in paths_crossos_test.go are written against behaviour rather than internals, so they port to either approach with only the call signature changed, and would cover the .md fallback path and the same-remote-form property that neither PR's existing tests assert.

Context on how this surfaced: syncing one bucket across two Windows machines and a macOS machine, with path_map pointing all three at a shared token. Before the fix, pulling macOS-authored sessions onto Windows produced .jsonl that failed with invalid character 'G' in string escape code. After it, 28 files and 3379 JSONL lines pulled macOS → Windows with zero parse failures.

tawanorg and others added 4 commits October 6, 2026 13:33
The new separator-aware content mapping accepted a backslash as a path
separator for every mapping, not just Windows ones. On a POSIX device a
backslash inside a path is an escape, so a shell command quoted in a
transcript round-tripped lossily:

    cd /Users/alice/My\ Documents   ->   cd /Users/alice/My/ Documents

main does not have this problem, because it only ever rewrote the root
prefix and left the tail alone. Canonicalizing the tail is what made the
tail's backslashes reachable, so the two functions that do it now take
the mapping's platform: a POSIX tail is already "/"-separated and is
returned untouched, while a Windows tail keeps the existing behaviour.

Adds round-trip guards for the escaped-space case in both content kinds,
plus the stability, idempotency and longest-prefix properties the
push/pull cycle depends on.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A mapping was treated as Windows whenever its local path contained a
backslash. A backslash is a legal character in a POSIX directory name, so
a home of /Users/al\ice claimed to be Windows and had its own "/"
separators rewritten on pull:

    {"cwd":"/Users/al\\ice/projects/app"}
    -> {"cwd":"/Users/al\\ice\\projects\\app"}

Test the path shape instead: a drive letter or a UNC share.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two defects found while reviewing the cross-OS content mapping.

renderLocalPath only escaped a backslash for a Windows mapping, but
escaping is a property of the file format, not of the platform. A POSIX
home of /Users/al\ice was matched unescaped (so .jsonl never normalized,
while .md did) and emitted unescaped, expanding ${HOME} into the invalid
escape \i and leaving every pulled transcript unparseable.

isWindowsLocalPath accepts C:/work, which is how a Windows root gets
written in YAML, since "C:\work" is not a valid escape there. The root
then matched with forward slashes while pull emitted backslashes, so a
round trip returned mixed separators. The root is now canonicalized to
the native separator; EncodeClaudePath flattens both, so remote keys are
unchanged, and the length-based longest-prefix ordering is preserved.

Also records the space-in-a-path limitation on pathSegment: a tail stops
at a space, so the remainder keeps the pusher's separators. Admitting a
space into a segment would make the tail swallow prose after a path,
which is worse, so this stays a documented gap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The risk in separator-aware mapping is not the new Windows path, it is the
POSIX content already sitting on every existing bucket. Assert that over a
corpus of realistic transcript fragments the new mapping is byte-identical
to the root-only mapping it replaces, in both content kinds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tawanorg

tawanorg commented Oct 6, 2026

Copy link
Copy Markdown
Owner

Thanks for this — the diagnosis is right and the writeup made it easy to verify. The Windows bug is real, and main genuinely does write invalid JSON when a Windows device pulls a POSIX-authored transcript.

I've pushed four commits to this branch rather than asking for changes, since the fixes are small and I wanted CI green before merging. Review found three defects, two of them introduced by the canonicalization this PR adds.

1. A backslash was treated as a path separator for every mapping, not just Windows ones (420200a)

Canonicalizing the matched tail is what made the tail's backslashes reachable, and on POSIX a backslash inside a path is an escape, not a separator. Any shell command quoted in a transcript round-tripped lossily:

cd /Users/alice/My\ Documents   ->   cd /Users/alice/My/ Documents

main is unaffected because it only ever rewrote the root prefix and left the tail alone. separatorPattern and canonicalizeTail now take the mapping's platform; a POSIX tail is already /-separated and is returned untouched.

2. windows was detected by "contains a backslash" (554c2a8)

A backslash is legal in a POSIX directory name, so a home of /Users/al\ice claimed to be Windows and had its own separators rewritten on pull. Now tested by path shape — a drive letter or a UNC share.

3. Escaping was gated on the platform rather than the file format (4a475e8)

Two halves of the same mistake. renderLocalPath only escaped for Windows mappings, so a POSIX home containing a backslash was matched unescaped — .jsonl silently never normalized while .md did — and expanded ${HOME} into the invalid escape \i, leaving the pulled transcript unparseable. Escaping now follows the content kind; only the separator choice depends on the platform.

The same commit canonicalizes a Windows root given with forward slashes. C:/work/projects is how this gets written in YAML, since "C:\work" is not a valid escape there, and it passed the drive-letter test while keeping / in the root pattern — so pull emitted mixed separators. EncodeClaudePath flattens both separators, so remote keys are unaffected, and the canonicalization preserves length so the longest-prefix ordering is unchanged.

Known limitation, left as-is and now documented on pathSegment

A space is not part of a segment, so a tail stops there and the remainder keeps the pushing device's separators — Application Support on POSIX, My Documents on Windows. Admitting a space into a segment is not the fix: the tail would then swallow the prose after a path, which is the failure you already hit with the PowerShell error message. This is narrower than what's on main today, so it isn't a blocker, but it is still a gap.

Verification

  • gofmt, go vet, golangci-lint clean; full suite and -race pass; CI green (coverage 79%, gate 60%)
  • Added a differential test (f34cc1e) asserting the new mapping is byte-identical to the root-only mapping it replaces across a corpus of realistic transcript fragments in both content kinds. That is the property that matters for content already sitting on existing buckets, and it now fails loudly if it ever stops holding.
  • Added round-trip guards for the escaped-space case, the stability of pull-then-push (so a pull can't produce phantom modifications), idempotency, longest-prefix ordering, and isWindowsLocalPath.

Your original cross-OS tests all still pass unchanged. Merging.

Four more defects from review.

The greedy tail consumes a root nested inside another path as part of one
match, and scanning resumes past it, so "~/a/Users/alice/b" left the
second occurrence absolute where the root-only mapping tokenized it. The
normalize pass now repeats while the literal root is still present, which
a bytes.Contains guard keeps off the hot path.

Both directions called ReplaceAllFunc and then re-ran the regex inside
the callback to recover its submatches, tripling allocations. One
FindAllSubmatchIndex pass gets them directly: 120k allocations and 19.7MB
per 2.7MB of transcript drop to 40k and 10.7MB, matching the root-only
mapping. The residual time is the tail pattern itself, which is the cost
of canonicalizing a tail at all, and stays the same order as the gzip
already in the pipeline.

A trailing backslash was trimmed from POSIX roots too, so a path_map
entry for a directory genuinely named "odd\" rooted at "odd" and captured
the unrelated "odd/" subtree. Trimming is now gated on the path shape,
which is the premise the rest of this work rests on.

The kind loop hardcoded the two kinds while the arrays are sized by
numPathContentKinds, so adding a third would have compiled and then
panicked on a nil regexp. It iterates the full range instead.

Also corrects the pathSegment comment, which named only the space. The
class excludes "@", "(", ")", "+", "~", "=", "," and every non-ASCII rune,
which makes node_modules\@babel\core cross to POSIX with literal
backslashes - common, not a corner. Widening it reaches further into
quoted tool output, so it stays narrow pending a decision on matching
aggressiveness.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tawanorg

tawanorg commented Oct 6, 2026

Copy link
Copy Markdown
Owner

A second review pass against the fixed branch turned up four more, all now fixed in 7ab4e1e, plus one I'm deliberately leaving and tracking separately.

Fixed

  • Nested root occurrences stopped being tokenized. The greedy tail consumes a root nested inside another path as part of one match and scanning resumes past it, so /Users/alice/a/Users/alice/b normalized to ${HOME}/a/Users/alice/b where the root-only mapping gave ${HOME}/a${HOME}/b — the pusher's absolute home went up verbatim. The normalize pass now repeats while the literal root is still present, guarded by bytes.Contains so it stays off the hot path.
  • 2.5× slowdown and 3× allocations on every push and pull. Both directions called ReplaceAllFunc and then re-ran the regex inside the callback to recover its submatches. One FindAllSubmatchIndex pass gets them directly: per 2.7 MB of transcript, 120k allocations / 19.7 MB → 40k / 10.7 MB, which matches the root-only mapping and actually allocates less. The residual ~2× wall time is the tail pattern itself and is irreducible if we canonicalize tails at all; it stays the same order as the gzip already in the pipeline, so it isn't a new bottleneck.
  • A trailing backslash was trimmed from POSIX roots too — exactly the premise isWindowsLocalPath exists to deny. A path_map entry for a directory genuinely named odd\ rooted at odd and swallowed the unrelated odd/ subtree. Trimming is now gated on path shape.
  • The kind loop hardcoded both kinds while normRe/localIn are sized by numPathContentKinds, so adding a third kind would compile and then panic on a nil regexp. It iterates the full range now.

Not fixed — tracked instead

pathSegment is narrower than the comment admitted. I'd written "a space"; it actually excludes @, (, ), +, ~, =, , and every non-ASCII rune. The consequence is worse than a cosmetic mixed separator:

windows push: {"p":"C:\\work\\app\\node_modules\\@babel\\core\\index.js"}
remote:       {"p":"${WORK}/app/node_modules/@babel\\core\\index.js"}
posix pull:   {"p":"/Users/alice/projects/app/node_modules/@babel\core\index.js"}

The backslashes are literal, so the path is wrong rather than oddly spelled, and scoped packages put this in essentially every JS transcript. Same for src\(auth)\page.tsx and non-ASCII directory names.

I'm not widening the class here. It's a judgement call about how far matching should reach into quoted tool output — the thing that already bit you with the PowerShell error message — and it deserves its own change rather than riding along in a merge. The comment on pathSegment now states the real class and the real blast radius, and I've opened a follow-up issue.

Worth being explicit that this is not a regression: on main, Windows-authored content is never normalized at all, so Windows→POSIX is wholly broken today. This PR fixes the primary cwd case and the invalid-JSON corruption in the other direction; the special-character tail is residual, narrower than the status quo, and separable.

CI green on 7ab4e1e. Merging.

@tawanorg
tawanorg merged commit 2b6c25c into tawanorg:main Oct 6, 2026
2 checks passed
github-actions Bot pushed a commit that referenced this pull request Oct 6, 2026
## [1.17.2](v1.17.1...v1.17.2) (2026-10-06)

### Bug Fixes

* make content path mapping separator- and escaping-aware ([#82](#82)) ([2b6c25c](2b6c25c))
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

🎉 This PR is included in version 1.17.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants