(PR#77 Feature Add) Claude Code Desktop Improvements - #86
austinc3030 wants to merge 23 commits into
Conversation
Pull declares a conflict whenever the remote changed after our last upload
AND the local hash differs from state. For append-only session transcripts
(projects/**/*.jsonl) both conditions are routinely true without any real
divergence: a live Claude Code session appends between two syncs. The content
is never compared, so a fast-forward is indistinguishable from a fork.
Across three synced machines this produced 92 .conflict artifacts in about a
week. Byte analysis showed 91 held no unique data. The 92nd was the dangerous
inversion: the local file was a truncated 41-line copy and its .conflict
counterpart was the complete 251-line transcript, so the conflict rule kept
the damaged file and hid the only intact one under a name Claude Code never
reads.
Pull now classifies the byte relation before declaring a conflict:
equal -> refresh state so it stops re-triggering every pull
remote ahead -> fast-forward by APPENDING the missing tail (never a
rewrite, so a concurrent appender cannot lose lines)
local ahead -> keep local, write no .conflict, leave state stale so the
next push publishes it
diverged -> unchanged: keep local, write .conflict
Push gets the mirror guard. It previously uploaded any file whose hash
differed from state, so a truncated local transcript would overwrite the
fuller bucket copy and propagate the loss to every machine. When a session
transcript has shrunk relative to state, the remote is fetched and the upload
is skipped if local is a prefix of it. The remote round-trip is paid only in
the shrunk case; a non-prefix rewrite still uploads; any fetch error falls
through to a normal upload so the guard can never fail a push. A skip counts
as neither uploaded nor errored.
fetchDecoded is factored out of downloadFile so both paths can inspect remote
plaintext without writing it, and so comparisons are like-for-like (the
remote is resolved to local path form first). The pull path adds no network
cost: the conflict branch it replaces already downloaded the remote to write
the .conflict file.
Refs tawanorg#69, tawanorg#70
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two classes of local-only file are currently uploaded and replicated to every machine. .lock: Claude Code writes a zero-byte lock per task directory (tasks/<id>/.lock). A lock represents a process holding a resource on one machine; replicated elsewhere it is indistinguishable from a lock genuinely held there, and it long outlives the process that made it. Observed: 12 of them, the oldest about four weeks old, syncing across three machines. .conflict.<timestamp>: handleConflict writes the remote copy next to the original, which is inside a synced directory, so the recovery artifact is itself uploaded. Each replica can then be re-detected on another machine and spawn further artifacts. Observed: 92 accumulated in about a week, including five copies of one snapshot minted by successive pulls. Both are now excluded unconditionally, ahead of the user's exclude patterns, because neither has any meaning on another machine under any configuration. The .conflict pattern is anchored on the exact <yyyymmdd>-<hhmmss> format this tool generates rather than matching .conflict. loosely. That matters: an already-tracked file becoming excluded is reported as a deletion and pruned from the bucket on the next push, so a loose pattern would silently delete a user file named e.g. notes.conflict.md from remote storage. Tests pin both directions. Migration note for existing users: the first push after upgrading prunes any already-uploaded locks and conflict artifacts from the bucket. Local copies are untouched. Fixes tawanorg#71 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rom unreachable Head returned the provider's own error verbatim in every adapter, so a caller had no portable way to tell "the object is not in the bucket" from "the bucket is unreachable right now" (timeout, throttle, 5xx). Both surfaced as an opaque error, and the natural reading — treat any Head failure as absent — is exactly the dangerous one. storage.ErrNotFound plus storage.IsNotFound give callers that distinction: each adapter now wraps its own not-found signal (types.NotFound for R2/S3, storage.ErrObjectNotExist for GCS, HTTP 404 for WebDAV) with the sentinel, and leaves every other error untouched. The history merge added in the next commit depends on this: on push it must merge the remote copy into the upload, so a genuinely absent object means "first push, nothing to merge" while a transient failure must abort the upload rather than clobber the bucket's union with local-only content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
history.jsonl is one shared file that every machine appends to. Syncing it
like any other file makes it last-writer-wins: whichever machine pushes last
replaces the bucket copy, silently dropping every prompt the other machines
recorded since, and the /resume picker degrades to whatever that one machine
happened to hold. On pull the same divergence surfaced as a .conflict file,
parking the other machines' history where nothing reads it.
Push now GETs the remote copy, unions it with the local payload, and uploads
the union. Pull appends the lines the local file is missing instead of
overwriting it or declaring a conflict.
Properties that make this safe on a file a live session is writing to:
- Merges only ever APPEND. The local bytes are the verbatim prefix of the
result, so a rewrite from a stale read can never destroy lines appended
during the merge's network round-trip. Unparseable lines (torn writes)
and unknown record shapes survive untouched.
- Idempotent by raw-line dedupe. Remote lines already present byte-for-byte
are dropped, so blank submissions and unknown-shape records — which have
no sessionId+display signature — do not multiply as content cycles
between machines and the bucket. Prompts additionally dedupe by
sessionId+display within historyDedupeWindowMs, the same rule
RebuildHistory uses, with the local line winning so its pastedContents
are kept.
- Aborts rather than clobbers. A missing remote object means first push and
merges nothing, but a transient Head failure or an undecodable remote
aborts the history upload — uploading local-only content would erase the
union. This is what storage.ErrNotFound in the previous commit is for.
- A local delete no longer deletes the bucket copy: the union only grows,
and the next pull restores the file locally.
Builds on the history helpers introduced in PR tawanorg#63 (HistoryEntry, forEachLine,
withinWindow, historyDedupeWindowMs) — tawanorg#63 recovers lost prompts after the
fact, this stops them being lost in the first place.
TestPullDetectsConflicts and TestConflictCreatesConflictFile used
history.jsonl as the vehicle for the generic conflict machinery, which now
never conflicts. They exercise the same assertions against settings.json.
Fixes tawanorg#72
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Once the byte-prefix fast-forward has taken the easy cases, the only apparent
conflict left on a session transcript is a true fork: the same session
advanced on two machines. Writing a .conflict file for that keeps whichever
copy this machine happens to hold and hides the other where Claude Code never
reads it, even though both sides are recoverable.
MergeSessionPayloads unions the two payloads, returning only the lines to
append. The local file is never rewritten, so a live session's concurrent
appends survive and applying the merge is idempotent.
Transcripts mix two record classes with different write semantics:
- uuid-keyed events are immutable DAG nodes linked by parentUuid, so they
union safely. Append order is not a correctness concern: Claude Code
already renders forked DAGs — 782 of 1,180 real transcripts contain
forks — and threads render from the parent links, not line order.
- uuid-less records of type mode, custom-title, ai-title, permission-mode,
last-prompt and worktree-state are MUTABLE state where the last
occurrence in the file wins. Across 12,679 such records in those same
transcripts, not one carries a timestamp, so per-record ordering between
two machines is impossible and cross-machine ordering falls back to
file-level times. Only each side's final value per type is compared, so a
stale intermediate remote record cannot resurrect, and a type the local
file never had is always preserved.
- anything else uuid-less unions by exact-raw-line multiset, the rule that
makes the history merge idempotent.
A merge failure degrades to the legacy keep-local-plus-.conflict path rather
than blocking the pull.
TestPullDivergedJSONLStillConflicts asserted the .conflict outcome this commit
deliberately replaces; it now asserts the merge outcome instead — no
conflicts, local content preserved as a verbatim prefix, the remote-only event
present, and a second pull a byte-for-byte no-op.
Completes tawanorg#69: with the fast-forward and this merge, an append-only transcript
no longer produces .conflict files at all.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The desktop app renders its sidebar from per-machine records at %APPDATA%\Claude\claude-code-sessions\<install-id>\<org-id>\local_*.json, not from ~/.claude/projects — so synced transcripts are resumable from the CLI but invisible in the app. Records now sync under a reserved `_ccd-sessions/<org-id>/` prefix with the machine-specific install-id deliberately excluded from the key, merged last-writer-wins by the record's own lastActivityAt. Notes: - MSIX-packaged installs redirect the Roaming path (probed via the Local\Packages glob). - Trust state is deliberately NOT synced: it gates hook execution. - Records are normalized in transit (sorted keys, machine-local permission fields stripped). - Writes are atomic (temp+rename) — the app reads these files live. - Machines without the desktop app skip the feature silently. Fixes tawanorg#76 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`-q` suppresses all output, including per-file errors, so a hook-driven sync could fail silently on individual files while appearing to succeed. Errors now go to stderr even in quiet mode; normal output is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NormalizeContent/ResolveContent do raw byte-level substitution between a
device's absolute path and the portable ${HOME} token. That's correct for
freeform text (.md/.txt), but most portable content is JSON (session
transcripts, Desktop session records), where a substituted path becomes
part of an already-serialized JSON string. On any device whose home path
contains a JSON string-escape character — a Windows path's backslashes,
concretely — inserting it unescaped corrupts that string's JSON syntax.
The same problem hits the reverse direction: content actually serialized
by encoding/json always doubles backslashes, so a raw-path search can
never match it in the first place.
Found via manual testing: pulling a jumpbox-originated session (home
/home/user) onto Windows (home C:\Users\austi) and resuming it failed
with a generic "No conversation found" error. Root cause was the
resulting transcript: 44 of 59 lines were invalid JSON, each one where a
cwd field or a tool command/result echoing a path had a raw, unescaped
backslash spliced into it.
NormalizeContent/ResolveContent now take a jsonMode bool. In JSON mode,
resolution JSON-escapes the replacement path before insertion, and
normalization matches the path in its JSON-escaped form instead of raw.
Both callers in sync.go (uploadFile/fetchDecoded) pass the new
IsJSONContentPath(relativePath) — true for .json/.jsonl including
history.jsonl, false for .md/.txt, which keep the original raw-substitution
behavior since they're not JSON and must not be escaped.
jsonEscape is a no-op for paths without a backslash or double-quote (every
non-Windows path), so this is a no-op for the common case and only changes
behavior where the bug actually lives.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ccdSessionsDir only ever probed Windows paths (AppData\Roaming and the MSIX Packages redirect), so Desktop session sync silently did nothing on Linux or macOS regardless of whether the app was installed and had real session records sitting right there — pushCCDSessions/ pullCCDSessions both bail out early on an empty ccdDir. Found by testing against an actual Linux Claude Desktop install: `push` completed successfully (0 CCD records, no error) despite four local session pointer files existing in ~/.config/Claude/claude-code-sessions. Adds the macOS (~/Library/Application Support/Claude) and Linux ($XDG_CONFIG_HOME, falling back to ~/.config) cases, matching Electron's own per-OS app-data resolution. The Windows direct-install and MSIX-redirect probing is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Session records carry cwd/originCwd as absolute paths on the machine
that created them. Desktop uses the record's own cwd — not just the
transcript's — to group the session under the right project and locate
it, so pulling a record onto a different OS/user without translating
those fields leaves them pointing at a path that doesn't exist there,
and (per the previous commit) could corrupt the record's JSON outright
when the pulling machine's path needs escaping.
pushCCDSessions now runs the stripped record through NormalizeContent
before hashing/uploading; pullCCDSessions runs the fetched remote
through ResolveContent before comparing against the local copy and
writing it. Both pass jsonMode=true unconditionally — a session record
is always a single JSON object. fetchDecoded's own resolution doesn't
apply here (IsPortableContentPath is false for _ccd-sessions/ keys, by
design — MCP and CCD records both use their own dedicated pull paths
rather than the generic one), so the record content on the wire is
always in normalized (${HOME}-token) form on both sides of the compare.
Verified end-to-end against a real cross-device test (jumpbox Linux ->
Windows): before this and the previous commit, a pulled record kept its
Linux cwd verbatim, and the record content mismatch this uncovered led
directly to finding the JSON-escaping bug fixed in the prior commit.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Desktop session records are deliberately "only grows" (see the doc comments on pushCCDSessions/pullCCDSessions): a normal push or pull never deletes one just because a local copy disappeared, so a session doesn't vanish from a device's sidebar just because another device's file briefly went missing. That's the right default, but it means a record you actually want gone — an orphaned fork with no cliSessionId, a stale duplicate left over from before a project moved, or anything else genuinely broken — has no way to leave the bucket. Deleting it locally on every device doesn't help: the next pull just brings it back from remote. Found this the hard way: cleaning up a couple of stale/broken yolink records required manually deleting them from cloud storage directly via a raw WebDAV request, because there was no tool support for it. Adds Syncer.ForgetCCDSession(ctx, id), matched by exact sessionId (local_<uuid>) or cliSessionId, removing every matching record locally and every matching object on remote — and `claude-sync desktop forget <id>`, gated behind the same type-to-confirm pattern as `reset` (bypassable with --force). Scope is deliberately narrow: it only touches the Desktop-visible pointer, never the underlying conversation transcript under ~/.claude/projects, which already has its own deletion path (delete the file, then push). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
forget requires an exact sessionId or cliSessionId, but there was no way to discover either short of manually inspecting session record files. Adds Syncer.ListCCDSessions(ctx), returning every Desktop session record on remote storage (every device's, not just this machine's, since that's what forget ultimately has to match against) with title, cwd, both ids, and lastActivityAt, sorted newest first — and `claude-sync desktop list` to print it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found immediately by actually using `desktop list` against real data:
every entry showed the raw wire form ("\${HOME}\claude\blink-re")
instead of the machine's actual path. fetchDecoded intentionally
doesn't resolve _ccd-sessions/ content (push/pull need the token form
to compare records consistently), which is correct for them but wrong
for a listing meant to be read by a person.
jsonMode=false, not true: by the time rec.CWD is a Go string,
json.Unmarshal has already stripped JSON escaping, so it's raw text —
resolving with jsonMode=true would JSON-escape the substituted path a
second time (doubled backslashes on Windows).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Push/pull fire up to defaultWorkers (10) concurrent requests at the same host. http.DefaultTransport's MaxIdleConnsPerHost is 2, so most of those requests needed a brand-new TLS handshake every time instead of reusing a connection. Besides the extra latency, a burst of simultaneous new handshakes against the same certificate chain reproducibly triggered a data race in crypto/x509's concurrent policy validation (fatal error: concurrent map writes; also observed as unrelated-looking heap corruption and segfaults in the HTTP/2 read loop under different runs — classic symptoms of the same underlying race surfacing at different points). Keeping enough idle connections around for the whole worker pool means steady-state traffic reuses connections instead of re-handshaking, which avoids the race in practice. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ccdSessionsDir now branches on runtime.GOOS, but the test still asserted against hardcoded Windows-style paths regardless of platform, so it only passed when run on Windows. Build the expected direct-layout path per OS and skip the Windows-only MSIX-fallback assertion elsewhere.
…fresh pointers on pull A Desktop session pointer is only worth syncing when its cliSessionId names a transcript this device actually syncs (respecting excludes) -- otherwise the other side gets a sidebar entry with no transcript behind it. Push and pull now both check ccdSyncedSessionIDs before touching a record. Pull also now leaves a local pointer alone if its mtime is inside the last 5 minutes, on the theory that a very recently touched pointer file means Desktop has that session open on this machine right now, so a pull landing in that window shouldn't race the live app's own write. Ported from feature/desktop-session-sync's PushDesktopSessions/ PullDesktopSessions design, adapted onto this branch's ccdsessions.go.
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness issues in changed code paths (notably stale state timestamps on no-op pushes with CCD uploads, and incomplete ErrNotFound wrapping in WebDAV Head) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR expands claude-sync’s multi-device correctness by adding safer conflict handling/merge semantics for append-only JSONL files (session transcripts + history.jsonl), plus first-class syncing and management of Claude Code Desktop “session pointer” records, and a WebDAV transport tweak to improve connection reuse under concurrency.
Changes:
- Add JSONL fast-forward/merge logic to avoid spurious
.conflictartifacts and prevent truncation clobbers; union-mergehistory.jsonlby appending missing lines. - Add cross-platform Claude Code Desktop session-record sync under
_ccd-sessions/, plusdesktop list/desktop forgetCLI commands. - Introduce a typed
storage.ErrNotFoundsignal and update adapters to wrap provider-specific “not found” errors; increase WebDAV idle connections per host.
File summaries
| File | Description |
|---|---|
| internal/sync/sync.go | Core push/pull changes: debris exclusion, history merge, JSONL conflict resolution, CCD session sync hooks |
| internal/sync/sync_push_pull_test.go | Retarget conflict tests from history.jsonl to a normal file (settings.json) |
| internal/sync/state.go | Add HashBytes; prevent CCD session state entries from being treated as deletions in DetectChanges |
| internal/sync/session_merge.go | Implement append-only merge strategy for diverged session transcripts |
| internal/sync/session_merge_test.go | Tests for transcript merge behavior (DAG integrity, idempotence, state resolution) |
| internal/sync/paths.go | Add JSON-safe path token substitution modes for .json/.jsonl content |
| internal/sync/paths_test.go | Tests for JSON-mode path substitution across OSes and for non-JSON text behavior |
| internal/sync/history_merge.go | Implement union-merge for history.jsonl and append-only application helper |
| internal/sync/history_merge_test.go | Extensive tests for history merge correctness and convergence across “machines” |
| internal/sync/fastforward.go | Add byte-prefix classifier and session JSONL eligibility check |
| internal/sync/fastforward_test.go | Tests for prefix classification, fast-forward, merge vs conflict behavior, and push shrink guard |
| internal/sync/debris_test.go | Tests ensuring .lock and tool-generated .conflict.<ts> artifacts never sync |
| internal/sync/ccdsessions.go | Implement Desktop session record push/pull, store detection, normalization, safety checks |
| internal/sync/ccdsessions_test.go | Tests for CCD store detection, LWW merge, traversal safety, transcript-linked filtering, freshness guard |
| internal/sync/ccdlist.go | Add remote listing API for CCD records with display-friendly cwd resolution |
| internal/sync/ccdlist_test.go | Tests for CCD listing ordering, cwd display resolution, and forget/list interoperability |
| internal/sync/ccdforget.go | Add “forget” operation to remove CCD records locally and remotely by exact id match |
| internal/sync/ccdforget_test.go | Tests for forget behavior and non-destructive matching semantics |
| internal/storage/webdav/webdav.go | Clone transport and increase idle conns per host; wrap not-found for Head |
| internal/storage/storage.go | Add ErrNotFound + IsNotFound helper |
| internal/storage/s3/s3.go | Wrap S3 NotFound errors from Head with storage.ErrNotFound |
| internal/storage/r2/r2.go | Wrap R2 NotFound errors from Head with storage.ErrNotFound |
| internal/storage/gcs/gcs.go | Wrap GCS not-exist errors from Head with storage.ErrNotFound |
| cmd/claude-sync/main.go | Add quiet-mode stderr reporting; add desktop CLI command group (list, forget) |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a potential runtime panic in the WebDAV transport setup (unchecked http.DefaultTransport type assertion) and has misleading documentation about “exact-byte” line matching in the new merge code.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
internal/sync/history_merge.go:24
MergeHistoryPayloadsclaims it dedupes by an "exact-byte multiset match", but the implementation usesforEachLine, whichTrimSpaces each line before counting. That means the match is not truly byte-exact, and the comment is misleading about the dedupe semantics.
// Remote lines are dropped as duplicates when either:
// - an identical raw line exists locally (exact-byte multiset match — this
// is what dedupes blank submissions and unknown-shape records, which have
// no prompt signature and would otherwise multiply on every cycle), or
internal/sync/session_merge.go:50
- This comment says uuid-less records use an "exact-raw-line multiset union", but the merge counts lines via
forEachLinewhichTrimSpaces them first. If the whitespace normalization is intentional, the comment should reflect that; if not, the merge should avoid TrimSpace to truly preserve raw bytes.
// - other uuid-less records (queue-operation, ...): exact-raw-line multiset
// union, the same rule that makes the history merge idempotent.
internal/storage/webdav/webdav.go:56
http.DefaultTransport.(*http.Transport)can panic ifhttp.DefaultTransporthas been overridden with a non-*http.Transport RoundTripper (common in tests or embedding). Using a checked type assertion avoids introducing a runtime panic inNew().
transport := http.DefaultTransport.(*http.Transport).Clone()
transport.MaxIdleConnsPerHost = 32
- Files reviewed: 24/24 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ripts Both were sized for ordinary session files and started failing once a real long-running session grew past them: a 60s http.Client.Timeout (covering the whole request — connect, headers, and full body transfer, not just connection setup) and a 100MB MaxDownloadSize. Push against a genuinely huge transcript (887MB, from a single long-running whathasthischip session; a sibling in the same project hit 930MB) failed with "context deadline exceeded" once the prior Cloudflare 413 cap was worked around — the transfer was simply taking longer than 60s to complete, not stuck. requestTimeout is now 90 minutes (comfortably covers a 2GB transfer even at a conservative ~5 Mbps upload) and MaxDownloadSize is now 2GB, so pulling one of these files on another device doesn't hit the same wall pushing did. Both remain finite: a genuinely stuck connection still fails eventually, and MaxDownloadSize still bounds worst-case memory use for an in-memory download. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
maxDecompressedSize was still capped at 500MB even after storage.MaxDownloadSize was raised to 2GB, so pulling a previously-pushed 843MB transcript failed with "decompressed data exceeds 524288000 bytes limit" despite the download itself succeeding. Raise it to 2GB to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- webdav: Head's PROPFIND-with-zero-results branch returned a plain "object not found" error instead of storage.ErrNotFound, so callers using storage.IsNotFound could misclassify a missing object as an unexpected failure. Only the HTTP 404 branch was wrapped. - webdav: http.DefaultTransport.(*http.Transport).Clone() panics if DefaultTransport has been replaced with a non-*http.Transport RoundTripper (common under test or when embedded). Fall back to a fresh *http.Transport instead of asserting unchecked. - history_merge.go / session_merge.go: doc comments claimed "exact-byte" / "exact-raw-line" multiset matching, but forEachLine TrimSpaces each line before the caller ever sees it, so the dedupe is whitespace- trimmed, not byte-exact. Corrected the comments to describe the actual (intentional, for cross-platform line-ending differences) behavior instead of changing it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
appendHistoryLines currently reads entire existing files into memory just to check a trailing newline, which will be prohibitively expensive for the very large files this PR aims to support.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 1
- Review effort level: Lite
ensureParentDirs's status-code check accepted every possible outcome (201, 405, 409, 404-with-continue, and implicitly anything else too, since only those four cases were ever examined) without returning an error in any of them — so a genuinely failed MKCOL silently fell through to a doomed PUT instead of surfacing. That masked a real race: push uploads with up to defaultWorkers concurrent requests, so the first two files landing in a brand-new directory (e.g. two tool-results files in the same session, or two subagent records) issue concurrent MKCOLs for that same not-yet- existing path. One request can get an unexpected/ambiguous response from the WebDAV server during that race, which the old code silently ignored — observed in practice as "HTTP 404: could not be located" on the follow-up PUT for tool-results/ and subagents/ directories. ensureCollection now serializes the whole check-then-create sequence under a mutex, with a memoized per-path cache so only a genuinely new directory ever issues MKCOL (repeat calls for an already-created path are a lock-and-map-lookup, no network round trip). This eliminates the race at the source instead of trying to interpret ambiguous concurrent responses, and a real MKCOL failure now returns an error that reaches the caller instead of being swallowed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
A cross-platform bug in the new exclusion logic can fail to recognize debris basenames on Windows due to slash-normalized relPaths, risking unintended uploads.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
internal/sync/sync.go:195
- GetLocalFiles normalizes relPath to forward slashes (filepath.ToSlash). Using filepath.Base here is OS-separator dependent and won’t strip directories on Windows when relPath contains '/', so .lock / .conflict artifacts may be uploaded instead of excluded.
internal/sync/sync.go:593 - The comment above localForm is inaccurate: localForm is captured before NormalizeContent, so it’s the on-disk bytes used for state hashing, not necessarily the exact bytes uploaded after path-token normalization.
- Files reviewed: 24/24 changed files
- Comments generated: 0 new
- Review effort level: Lite
- isExcluded: use path.Base instead of filepath.Base since relPath is forward-slash normalized; filepath.Base is backslash-aware on Windows and would fail to strip directories, letting debris slip past the .lock/.conflict exclusion check - appendHistoryLines: avoid reading the whole file into memory just to check the trailing byte; read only the last byte via Stat+ReadAt, since history/transcript files can run into the gigabytes - clarify localForm comment: it's the on-disk bytes used for state hashing, not necessarily the exact bytes uploaded (which may differ after path-token normalization)
There was a problem hiding this comment.
🔵 Needs a closer look
The new desktop commands don’t fully honor the global --quiet flag, and the JSONL fast-forward path does an unnecessary full reread that can significantly increase peak memory/IO for large transcripts.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
cmd/claude-sync/main.go:2790
- The global --quiet flag is documented as "Suppress output", but this command still prints to stdout (both the "no records" message and the record listing). In quiet mode it should perform the operation but return without printing anything.
This issue also appears in the following locations of the same file:
- line 2884
- line 2896
internal/sync/sync.go:859
- In the fast-forward case this rereads the entire file to confirm equality and compute a hash. For large transcripts this can double peak memory (local+remote+final) and add significant IO. Since PrefixRemoteAhead implies we only appended, you can detect concurrent appends cheaply via file size and update state using the already-downloaded remote bytes when sizes match.
cmd/claude-sync/main.go:2887
- In quiet mode this branch still prints "No matching record found" to stdout, which contradicts the global --quiet behavior used elsewhere (push/pull). Consider suppressing this message when quiet is set (still return success).
if len(result.RemovedRemoteKeys) == 0 && len(result.RemovedLocalPaths) == 0 {
fmt.Printf("%s⋯%s No matching record found for %q\n", colorDim, colorReset, id)
return nil
}
cmd/claude-sync/main.go:2898
- This always prints a success summary line even when --quiet is set. That makes scripting harder and is inconsistent with the rest of the CLI's quiet behavior; please guard this output behind !quiet.
fmt.Printf("%s✓%s Removed %d remote record(s), %d local file(s)\n",
colorGreen, colorReset, len(result.RemovedRemoteKeys), len(result.RemovedLocalPaths))
return nil
- Files reviewed: 24/24 changed files
- Comments generated: 0 new
- Review effort level: Lite
- desktop list/forget: honor global --quiet flag; suppress all stdout output (including the 'no records'/'no match' messages and the success summary) when quiet is set, matching push/pull behavior - resolveJSONLConflict fast-forward path: avoid rereading the whole file to verify the append landed cleanly; a size comparison against the already-downloaded remote bytes is enough, since this branch only ever appends and never rewrites
There was a problem hiding this comment.
🔵 Needs a closer look
CCD session sync has correctness issues on pull (state hash form mismatch and Windows pointer-file replacement via os.Rename) that can cause redundant re-uploads and failed record updates.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
internal/sync/ccdsessions.go:252
- ccdWriteAtomic uses os.Rename to replace an existing record. On Windows, os.Rename fails when the destination already exists, so pulls that need to update an existing pointer file can error and leave stale records behind.
internal/sync/ccdsessions.go:496 - pullCCDSessions updates SyncState.Hash using the resolved (local) record bytes, but pushCCDSessions compares against a tokenized form (NormalizeContent). This mismatch will make the same record appear changed after every pull and cause redundant re-uploads/ping-ponging.
- Files reviewed: 24/24 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Thanks for picking up #77 and extending it — the extra ground this covers (Desktop store detection on Linux and macOS, Holding it for now, and the reasons are mostly mechanical rather than about the code: 1. It conflicts with 2. It duplicates part of what just landed. Your 3. It contains #77 verbatim. Your first seven commits are #77's by SHA. The plan is to land #77 first and have this rebase onto it, which keeps attribution with @sjalife and reduces this PR to its own 16 commits — a much more reviewable diff than the current 3,683 lines across 24 files. Sequencing is tracked in #91. So the path forward: #77 lands, you rebase onto Two of your commits look independently valuable and low-risk, and I'd merge either as its own PR without waiting on any of the above:
Splitting those two out would get them in quickly and shrink this PR further. Nothing else needed from you until #77 lands. |
Builds on / supersedes #77 (@sjalife's transcript-conflict + desktop-record sync work). This branch is a strict superset of #77 — its head commit is an ancestor of this branch — plus:
desktop list/desktop forgetCLI commandscliSessionIdhas no locally-synced transcript (avoids empty sidebar entries), and pull leaves a pointer alone if it was touched in the last 5 minutes (avoids racing a live Desktop write)Note for reviewers: since #77 isn't merged yet, this diff includes its commits. Once #77 merges into
main, this PR should shrink to just the additional commits above. Happy to rebase once that happens.