fix(sync): require explicit consent before a share syncs into a non-empty folder (#54)#58
Conversation
…mpty folder (#54) The accept path guarded only against overlap with configured folders (#45): an unconfigured directory that already held content — a label-named folder under the root, the vault-as-root case, or a numeric-suffix candidate — was synced into silently, merging two content sets and pushing the mix to every offering peer. The manual picker (#52) already refused non-empty targets; the automatic path allowed the same merge implicitly. Per doctrine 002/006 (decision record 007, added here): the automatic pass never merges — the share parks under Pending Shares with an explanation — and an explicit accept gets a confirmation dialog that names the consequence (contents combined and synced to the other devices sharing the vault). Confirming re-runs the accept with consent attached and re-validates everything at confirm time. A recorded manual target (#52) stays exempt as recorded consent, so remove + re-accept recovery keeps working; silent diversion to a suffixed folder was rejected — it would split a legitimate re-link (006's rejected fallback). Enforced in two layers: the pure Swift decision core (resolveAcceptPath, injected listing) and the Go hard floor (AcceptPendingFolder refuses non-empty targets — at most .obsidian — unless the confirmation travels through the new allowNonEmpty parameter; unreadable targets are refused, never assumed empty). Mirror tests on both sides pin the two emptiness rules to identical decisions. Existing empty-vault targets, fresh directories, and all #45 overlap refusals behave exactly as before. Verified here: Go suite green (go test -tags noassets ./bridge, 55s, incl. the new floor tests), gofmt clean, design-token lint green, all four Localizable.strings key counts identical (613). Swift suite not runnable on Linux — pending the macOS test pass. The gomobile API change requires an xcframework rebuild (make xcframework, macOS) before any local Xcode build, test run, or archive.
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughAdds an explicit merge-confirmation gate to prevent syncing a pending share into a non-empty destination folder. Enforcement spans a Go bridge hard floor ( ChangesImplicit merge guard
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ons stay locale-stable (#52) ManualShareTargetTests pins the #52 refusal semantics by asserting English message fragments (path, reason, next step). The test runner inherits the host/simulator language, so on a German-locale machine L10n.tr resolves to de.lproj and 10 assertions fail although the refusal behavior is correct. Pin the language at scheme level (project.yml, test.language: en) instead of weakening assertions or passing -testLanguage per invocation: the local verification command and the CI macOS job both use -scheme VaultSync, so one pin covers both and no command line changes. Test-only scheme option — app behavior, shipped strings, and the #52 assertions are untouched. Verified on a de-DE host: full suite 178/178 green.
1 participant
Closes #54.
The accept path guarded only against overlap with configured folders (#45): an unconfigured directory that already held content — a label-named folder under the root, the vault-as-root case, or a numeric-suffix candidate — was synced into silently, merging two content sets and pushing the mix to every offering peer. The manual picker (#52) already refused non-empty targets; the automatic path allowed the same merge implicitly.
Design (doctrine 002/006, new decision record 007): the automatic pass never merges — the share parks under Pending Shares with an explanation — and an explicit accept gets a confirmation dialog that names the consequence (contents combined and synced to the other devices). Confirming re-runs the accept with consent attached, re-validating everything at confirm time. A recorded manual target (#52) stays exempt as recorded consent, so remove + re-accept recovery keeps working. Silent diversion to a suffixed folder was rejected (006's rejected fallback — it would split a legitimate re-link).
What could go wrong, and why this is safe: enforced in two layers — the pure Swift decision core and the Go hard floor (AcceptPendingFolder refuses non-empty targets unless the confirmation travels through the new allowNonEmpty parameter; unreadable targets are refused, never assumed empty). Mirror tests on both sides pin the two emptiness rules (at most .obsidian) to identical decisions. Empty-vault targets, fresh directories, and all #45 overlap refusals behave exactly as before; overlap refusal always wins over consent.
Verified on Linux: Go suite green incl. the new floor tests (55s), gofmt clean, design-token lint green, all four Localizable.strings in sync (613 keys each). Pending: macOS xcodebuild test pass (CI builds its own xcframework). Note for local checkouts: the gomobile API changed — run 'cd go && make patch && make xcframework' before any local Xcode build, test run, or archive.
Follow-up noted: AddFolder does not have the same non-empty floor; it currently has no callers outside the thin wrapper chain, but deserves the symmetric guard.
This PR prevents pending shares from being silently merged into an existing non-empty folder.
This improves data-safety and privacy by avoiding unintended mixing of vault contents, with no new background behavior beyond the added confirmation gate.