CS-10618: Implement bidirectional realm sync command#4419
CS-10618: Implement bidirectional realm sync command#4419
Conversation
Add `boxel realm sync` for bidirectional syncing between a local directory and a Boxel realm. Uses a manifest-based approach to detect changes on both sides, with conflict resolution strategies (--prefer-local, --prefer-remote, --prefer-newest). Extracts shared sync-manifest utilities from push command. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use 'update' instead of 'add' in atomic upload when file exists on remote but no manifest is present (first sync with overlapping files) - Add 1s delay in establishBaseline to avoid same-second mtime collisions - Account for realm-default index.json in manifest assertions - Stabilize no-op test by syncing once before checking for no checkpoint Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d0c7f70a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (totalOps === 0) { | ||
| console.log('\nEverything is up to date'); | ||
| if (!this.options.dryRun && !effectiveManifest) { | ||
| // First sync with no changes needed - still write manifest | ||
| await this.writeManifest(localHashes, remoteMtimes); |
There was a problem hiding this comment.
When conflicts are skipped (no --prefer-* strategy), totalOps can still be 0, so this branch reports “Everything is up to date” and writes a first-sync manifest. In that scenario, conflicting local/remote versions get recorded as if they were synchronized, so later runs classify them as unchanged and never surface the conflict again. Skip-success paths should not write a manifest while skippedConflicts is non-empty.
Useful? React with 👍 / 👎.
| const [localFiles, localFilesWithMtimes, remoteMtimes, manifest] = | ||
| await Promise.all([ | ||
| this.getLocalFileList(), | ||
| this.getLocalFileListWithMtimes(), | ||
| this.getRemoteMtimes(), | ||
| loadManifest(this.options.localDir), |
There was a problem hiding this comment.
Sync state is derived from remoteMtimes, but getRemoteMtimes() can legitimately return an empty map when the _mtimes endpoint is unavailable (as handled in RealmSyncBase). With this code path, remote files become invisible to classification and add/update selection, so remote-only files are never pulled and existing remote files may be treated as add operations (causing atomic 409 failures). The command should use getRemoteFileList() as a fallback source of remote existence data.
Useful? React with 👍 / 👎.
| // Phase 6: Update manifest | ||
| if (!this.options.dryRun && !this.hasError) { | ||
| // Recompute hashes for pulled files and update manifest | ||
| const updatedHashes = new Map(localHashes); |
There was a problem hiding this comment.
Rebuilding updatedHashes from current localHashes drops any manifest entries for files that were deleted locally but intentionally not propagated (for example, no --delete and no prefer-local deletion). If another file changes in the same run, this rewrite forgets that pending deletion and the next sync can re-pull the remote copy unexpectedly. Manifest updates should be based on prior manifest state plus executed actions, not only the current local file set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
let's not add this to the repo
There was a problem hiding this comment.
Pull request overview
Adds a new boxel realm sync command to support bidirectional syncing between a local directory and a Boxel realm using a persisted sync manifest, with conflict-resolution flags and integration coverage.
Changes:
- Introduce
realm synccommand implementing bidirectional classification (push/pull/deletes/conflicts) and checkpointing. - Extract shared manifest helpers into
src/lib/sync-manifest.tsand refactorrealm pushto use them. - Add integration tests covering bidirectional sync, conflicts, deletion behavior, dry-run, and manifest updates.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/boxel-cli/src/commands/realm/sync.ts | New bidirectional sync command + conflict strategies, manifest update, and checkpoint creation. |
| packages/boxel-cli/src/lib/sync-manifest.ts | New shared utilities for reading/writing the .boxel-sync.json manifest and hashing files. |
| packages/boxel-cli/src/commands/realm/push.ts | Refactor to use shared sync-manifest utilities instead of inline implementations. |
| packages/boxel-cli/src/commands/realm/index.ts | Register the new sync subcommand under boxel realm. |
| packages/boxel-cli/tests/integration/realm-sync.test.ts | New integration test suite for sync behavior and conflict/deletion scenarios. |
| docs/cs-10618-realm-sync-plan.md | Design/plan document for the new sync command and test coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private determineAction( | ||
| local: SideStatus, | ||
| remote: SideStatus, | ||
| _relativePath: string, | ||
| ): SyncAction { | ||
| // Both unchanged | ||
| if (local === 'unchanged' && remote === 'unchanged') return 'noop'; | ||
|
|
||
| // One side changed, other unchanged | ||
| if (local === 'changed' && remote === 'unchanged') return 'push'; | ||
| if (local === 'unchanged' && remote === 'changed') return 'pull'; | ||
|
|
||
| // One side added, other doesn't exist | ||
| if (local === 'added' && remote === 'unchanged') return 'push'; | ||
| if (local === 'unchanged' && remote === 'added') return 'pull'; | ||
|
|
||
| // Both changed or both added — conflict | ||
| if ( | ||
| (local === 'changed' && remote === 'changed') || | ||
| (local === 'added' && remote === 'added') | ||
| ) { | ||
| return 'conflict'; | ||
| } | ||
|
|
||
| // Deletions | ||
| if (local === 'deleted' && remote === 'unchanged') { | ||
| return this.syncOptions.deleteSync || this.syncOptions.preferLocal | ||
| ? 'push-delete' | ||
| : 'noop'; | ||
| } | ||
| if (local === 'unchanged' && remote === 'deleted') { | ||
| return this.syncOptions.deleteSync || this.syncOptions.preferRemote | ||
| ? 'pull-delete' | ||
| : 'noop'; | ||
| } | ||
|
|
||
| // Delete vs change conflicts | ||
| if (local === 'deleted' && remote === 'changed') return 'conflict'; | ||
| if (local === 'changed' && remote === 'deleted') return 'conflict'; | ||
|
|
||
| // Both deleted | ||
| if (local === 'deleted' && remote === 'deleted') return 'noop'; | ||
|
|
||
| // Added vs deleted (shouldn't normally happen but handle gracefully) | ||
| if (local === 'added' && remote === 'deleted') return 'push'; | ||
| if (local === 'deleted' && remote === 'added') return 'pull'; | ||
|
|
||
| return 'noop'; | ||
| } |
There was a problem hiding this comment.
The action matrix in determineAction() doesn’t handle combinations like local='changed' + remote='added' (or local='added' + remote='changed'), which can occur when remote “in manifest” detection differs from local detection (e.g., manifest missing remoteMtimes entries). These cases currently fall through to noop, silently skipping real changes. Add explicit handling (likely conflict or a deterministic push/pull choice) so unexpected state combinations can’t be ignored.
| // Deletions | ||
| if (local === 'deleted' && remote === 'unchanged') { | ||
| return this.syncOptions.deleteSync || this.syncOptions.preferLocal | ||
| ? 'push-delete' | ||
| : 'noop'; | ||
| } | ||
| if (local === 'unchanged' && remote === 'deleted') { | ||
| return this.syncOptions.deleteSync || this.syncOptions.preferRemote | ||
| ? 'pull-delete' | ||
| : 'noop'; |
There was a problem hiding this comment.
Deletions are executed even when --delete is not set: a local deletion with --prefer-local triggers push-delete, and a remote deletion with --prefer-remote triggers pull-delete. This makes --prefer-* act like an implicit “sync deletions” toggle, which is surprising and risks unintended data loss (and it contradicts the PR description that --delete controls deletion syncing). Consider requiring deleteSync for non-conflict deletions, and limiting --prefer-* to resolving true conflicts only.
| // Deletions | |
| if (local === 'deleted' && remote === 'unchanged') { | |
| return this.syncOptions.deleteSync || this.syncOptions.preferLocal | |
| ? 'push-delete' | |
| : 'noop'; | |
| } | |
| if (local === 'unchanged' && remote === 'deleted') { | |
| return this.syncOptions.deleteSync || this.syncOptions.preferRemote | |
| ? 'pull-delete' | |
| : 'noop'; | |
| // Deletions are only synchronized when deleteSync is explicitly enabled. | |
| // preferLocal / preferRemote should only affect true conflict resolution. | |
| if (local === 'deleted' && remote === 'unchanged') { | |
| return this.syncOptions.deleteSync ? 'push-delete' : 'noop'; | |
| } | |
| if (local === 'unchanged' && remote === 'deleted') { | |
| return this.syncOptions.deleteSync ? 'pull-delete' : 'noop'; |
| const [localFiles, localFilesWithMtimes, remoteMtimes, manifest] = | ||
| await Promise.all([ | ||
| this.getLocalFileList(), | ||
| this.getLocalFileListWithMtimes(), | ||
| this.getRemoteMtimes(), | ||
| loadManifest(this.options.localDir), | ||
| ]); |
There was a problem hiding this comment.
State gathering does two full local traversals (getLocalFileList() and getLocalFileListWithMtimes()) even though mtimes are only needed for --prefer-newest conflict resolution. Consider deriving localFiles from the getLocalFileListWithMtimes() result (or only collecting mtimes when preferNewest is enabled and conflicts exist) to reduce filesystem IO on large directories.
| const [localFiles, localFilesWithMtimes, remoteMtimes, manifest] = | |
| await Promise.all([ | |
| this.getLocalFileList(), | |
| this.getLocalFileListWithMtimes(), | |
| this.getRemoteMtimes(), | |
| loadManifest(this.options.localDir), | |
| ]); | |
| const [localFilesWithMtimes, remoteMtimes, manifest] = await Promise.all([ | |
| this.getLocalFileListWithMtimes(), | |
| this.getRemoteMtimes(), | |
| loadManifest(this.options.localDir), | |
| ]); | |
| const localFiles = new Set(localFilesWithMtimes.keys()); |
| console.log('Testing realm access...'); | ||
| try { | ||
| await this.getRemoteFileList(''); | ||
| } catch (error) { | ||
| console.error('Failed to access realm:', error); | ||
| throw new Error( | ||
| 'Cannot proceed with sync: Authentication or access failed. ' + | ||
| 'Please check your Matrix credentials and realm permissions.', | ||
| ); | ||
| } | ||
| console.log('Realm access verified'); | ||
|
|
||
| // Phase 1: Gather state | ||
| const [localFiles, localFilesWithMtimes, remoteMtimes, manifest] = | ||
| await Promise.all([ | ||
| this.getLocalFileList(), | ||
| this.getLocalFileListWithMtimes(), | ||
| this.getRemoteMtimes(), | ||
| loadManifest(this.options.localDir), | ||
| ]); | ||
|
|
||
| console.log(`Found ${localFiles.size} local files`); | ||
| console.log(`Found ${remoteMtimes.size} remote files`); |
There was a problem hiding this comment.
realm sync currently relies on the realm’s /_mtimes endpoint to discover remote files (getRemoteMtimes()), but RealmSyncBase.getRemoteMtimes() returns an empty map on 404 or fetch errors. In that case this command will treat the remote as having zero files (no pulls, and potentially pushes that overwrite remote-only content). Consider falling back to getRemoteFileList('') (already called for access-check) to at least enumerate remote paths, or fail fast with a clear error when /_mtimes is unavailable so users don’t get a misleading/unsafe sync.
| const inManifest = manifest?.remoteMtimes?.[relativePath] !== undefined; | ||
|
|
||
| if (hasRemote && inManifest) { | ||
| return remoteMtimes.get(relativePath) === | ||
| manifest!.remoteMtimes![relativePath] | ||
| ? 'unchanged' | ||
| : 'changed'; | ||
| } | ||
| if (hasRemote && !inManifest) return 'added'; | ||
| if (!hasRemote && inManifest) return 'deleted'; |
There was a problem hiding this comment.
Remote change classification uses only manifest.remoteMtimes to decide whether a remote file is “in manifest”. If the manifest exists but lacks remoteMtimes (e.g., created when /_mtimes was unavailable), every existing remote file becomes remoteStatus='added', which then interacts badly with the action matrix (e.g., localStatus='changed' + remoteStatus='added' falls through to noop). Consider treating a manifest without remoteMtimes as unusable for remote classification (treat as first sync), or using manifest.files as a secondary “known paths” set and handling the missing-mtime case explicitly (e.g., classify as changed/conflict requiring a strategy).
| const inManifest = manifest?.remoteMtimes?.[relativePath] !== undefined; | |
| if (hasRemote && inManifest) { | |
| return remoteMtimes.get(relativePath) === | |
| manifest!.remoteMtimes![relativePath] | |
| ? 'unchanged' | |
| : 'changed'; | |
| } | |
| if (hasRemote && !inManifest) return 'added'; | |
| if (!hasRemote && inManifest) return 'deleted'; | |
| const manifestRemoteMtime = manifest?.remoteMtimes?.[relativePath]; | |
| const inManifestRemote = manifestRemoteMtime !== undefined; | |
| const inManifestFiles = manifest?.files[relativePath] !== undefined; | |
| const knownInManifest = inManifestRemote || inManifestFiles; | |
| if (hasRemote && inManifestRemote) { | |
| return remoteMtimes.get(relativePath) === manifestRemoteMtime | |
| ? 'unchanged' | |
| : 'changed'; | |
| } | |
| if (hasRemote && knownInManifest) { | |
| // The file path is known from the manifest, but we do not have a | |
| // previous remote mtime to compare against. Treat it as changed rather | |
| // than added so sync planning can resolve it explicitly. | |
| return 'changed'; | |
| } | |
| if (hasRemote && !knownInManifest) return 'added'; | |
| if (!hasRemote && knownInManifest) return 'deleted'; |
Summary
boxel realm synccommand for bidirectional syncing between a local directory and a Boxel realm--prefer-local,--prefer-remote,--prefer-newest--deleteflag to sync deletions and--dry-runfor previewing changesSyncManifest,computeFileHash,loadManifest,saveManifest,pathExists) from push command intosync-manifest.tsTest plan
realm syncintegration tests--prefer-local,--prefer-remote,--prefer-newest--dry-runpreviews without making changesrealm pushstill works after manifest extraction🤖 Generated with Claude Code