Repository navigation
fix: translate absolute paths in plugin state files - #73
marioparaschiv wants to merge 4 commits into
Conversation
known_marketplaces.json and installed_plugins.json record absolute install locations, but content translation only covered history.jsonl and projects/, so a pull left them pointing at the pushing device and Claude Code rejected the marketplace. Both go through the path mapper now, decoded as JSON rather than replaced as raw bytes: a Windows path is escaped in the file, so byte replacement would eat an escape and leave the document invalid.
There was a problem hiding this comment.
🟡 Not ready to approve
The updated conflict discovery parses the full filesystem path (not just the filename), which can mis-detect conflicts if any parent directory contains “.conflict.” and produce incorrect OriginalPath values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR extends the sync layer’s “portable path” translation so Claude Code plugin state JSON files (e.g., plugins/known_marketplaces.json, plugins/installed_plugins.json) have absolute install paths normalized/resolved correctly across devices, avoiding invalid JSON from raw byte replacement (notably for escaped Windows paths).
Changes:
- Add JSON-aware path tokenization/resolution for specific plugin state files during upload/download.
- Centralize conflict filename construction/parsing via
ConflictPath/SplitConflictPathand reuse parsing in the CLI. - Add tests covering JSON round-trips, Windows↔Unix translation, and conflict path parsing.
File summaries
| File | Description |
|---|---|
| internal/sync/sync.go | Apply JSON-based path mapping for portable JSON files; introduce shared conflict path helpers. |
| internal/sync/paths.go | Add JSON string-walker mapper and classify plugin state files as portable JSON/content. |
| internal/sync/paths_test.go | Add test coverage for JSON mapping behavior and conflict path helpers. |
| cmd/claude-sync/main.go | Switch conflict discovery to use sync.SplitConflictPath. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| if original, timestamp, ok := sync.SplitConflictPath(path); ok { | ||
| conflicts = append(conflicts, conflictFile{ | ||
| ConflictPath: path, | ||
| OriginalPath: original, | ||
| Timestamp: timestamp, | ||
| }) | ||
| } |
…ResolveFile Fold the parallel JSON path API (NormalizeJSONContent/ResolveJSONContent, IsPortableJSONPath, mapJSONStrings) into NormalizeFile/ResolveFile, which pick JSON-aware vs byte translation internally. Call sites drop the two-way branch and the ordering dependency between the two path predicates. Also scope SplitConflictPath's marker match to the final path segment so a parent directory containing ".conflict." no longer triggers a false match or a wrong original path.
There was a problem hiding this comment.
🟡 Not ready to approve
The JSON rewrite currently re-encodes using default encoding/json behavior (notably HTML escaping) and always re-serializes, which can alter plugin state file bytes beyond the intended path translation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
internal/sync/paths.go:334
mapJSONPathsusesjson.MarshalIndent, which escapes HTML characters (<,>,&) by default. That can change plugin state file contents beyond the intended path translation. Use ajson.EncoderwithSetEscapeHTML(false)instead.
out, err := json.MarshalIndent(walk(doc), "", " ")
if err != nil {
return data
}
return append(out, '\n')
internal/sync/paths.go:298
mapJSONPathsalways re-encodes the JSON (and currently pretty-prints it) even when no string values actually match the mapped prefixes. That will change whitespace/newlines and could create unnecessary churn when syncing these state files. Consider tracking whether any value was rewritten and returning the originaldataunchanged when nothing changed (and decide whether canonical pretty-printing is desired).
func mapJSONPaths(data []byte, from, to string) []byte {
var doc any
if err := json.Unmarshal(data, &doc); err != nil {
return data
}
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The JSON round-trip re-encoded with default settings, which HTML-escaped &, < and > (mangling marketplace repo URLs) and widened integers via float64 decoding. Decode with UseNumber and encode with SetEscapeHTML(false) so only translated path prefixes change.
There was a problem hiding this comment.
🟢 Ready to approve
The change is narrowly scoped to portable path translation, includes unit tests covering the new JSON behavior and conflict parsing, and aligns with the PR’s stated fix.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
ResolveContent spliced local paths into session content with raw byte replacement, producing invalid JSON escapes and mixed separators on Windows (e.g. C:\Users\bob/foo). Route .jsonl (per line) and .json content through JSON-aware translation so inserted paths stay escaped and follow the local separator convention.
|
Also fixes Windows path normalization. On pull, |
|
Thanks for this — and the premise is right, which is the important part. Two things have changed under this PR, though, and together they mean it needs a resubmit rather than a merge. #82 landed and subsumed most of the implementation. It solved the same Windows escaping/separator problem by deriving a content kind from the file extension and canonicalizing tails, rather than by decoding and re-encoding JSON. Of the ~198 lines this PR adds to The decode/re-encode approach has defects that would corrupt data. All of these were reproduced against real
Item 4 is the one I'd call blocking on its own. What's worth keeping. Two pieces survive cleanly and are independent of #82: the three-entry allowlist that brings the plugin files into translation, and Rebased onto #82's content-kind API, that's about 81 lines instead of +460/−23: the allowlist, the conflict-path helpers, and nothing else. The extension-based machinery you wrote is no longer needed because #82 already escapes per content kind. I've filed #93 for the underlying plugin-path gap so it's tracked regardless of what happens to this PR. If you'd rather not redo it, say so and I'll land the minimal version with credit to you — but I'd rather you kept authorship of your own find. |
known_marketplaces.json and installed_plugins.json record absolute install locations, but content translation only covered history.jsonl and projects/, so a pull left them pointing at the pushing device and Claude Code rejected the marketplace.
Both go through the path mapper now, decoded as JSON rather than replaced as raw bytes: a Windows path is escaped in the file, so byte replacement would eat an escape and leave the document invalid.