Skip to content

fix(conflicts): disable automatic .obsidian LWW mutation (#145) - #156

Merged
psimaker merged 1 commit into
mainfrom
fix/issue-145-disable-auto-obsidian-lww
Aug 16, 2026
Merged

fix(conflicts): disable automatic .obsidian LWW mutation (#145)#156
psimaker merged 1 commit into
mainfrom
fix/issue-145-disable-auto-obsidian-lww

Conversation

@psimaker

@psimaker psimaker commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Foreground and background sync previously invoked a timestamp-based resolver that could delete, replace, or promote .obsidian conflict copies without a contemporaneous user decision.

Remove both automatic call paths, preserve but ignore the legacy preference, retain the gomobile entry point as a non-mutating compatibility no-op, and keep conflicts in the manual review flow.

What could go wrong and why this is safe: an overlooked caller or stale bridge binary could still mutate user bytes. The Go export itself now performs no filesystem operation, issue-numbered tests preserve exact bytes across Mtime, clock-skew, and missing-original cases, the XCFramework was rebuilt, and the complete Go and Xcode suites pass.

Closes #145

What & why

Component(s)

  • go (bridge / Syncthing)
  • ios (app / widget)
  • notify (relay)
  • docs / CI

Testing

  • cd go && make patch && go test -tags noassets ./bridge
  • cd notify && go test ./...
  • iOS build / xcodebuild test
  • Not applicable

Summary

  • Disabled automatic .obsidian conflict resolution in foreground and background sync.
  • Preserved the legacy preference without allowing it to re-enable resolution.
  • Kept AutoResolveStateConflicts as a non-mutating compatibility no-op.
  • Preserved conflict files, metadata, bytes, and modification times for manual review.
  • Updated settings, localization, documentation, and changelog text to describe manual conflict handling.
  • Added regression coverage for timestamp ordering, equal mtimes, clock skew, missing originals, nested paths, and legacy preference states.
  • Rebuilt the XCFramework.
  • Go and Xcode test suites pass.

Foreground and background sync previously invoked a timestamp-based resolver that could delete, replace, or
promote .obsidian conflict copies without a contemporaneous user decision.

Remove both automatic call paths, preserve but ignore the legacy preference, retain the gomobile entry point
as a non-mutating compatibility no-op, and keep conflicts in the manual review flow.

What could go wrong and why this is safe: an overlooked caller or stale bridge binary could still mutate user
bytes. The Go export itself now performs no filesystem operation, issue-numbered tests preserve exact bytes across
Mtime, clock-skew, and missing-original cases, the XCFramework was rebuilt, and the complete Go and Xcode suites
pass.

Closes #145
@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 218fa5d9-e253-459b-b5c3-67deecd9e20f

📥 Commits

Reviewing files that changed from the base of the PR and between 74cc99f and c86bd4c.

📒 Files selected for processing (14)
  • CHANGELOG.md
  • docs/decisions/028-conflicts-require-manual-choice.md
  • docs/sync-filters-ux.md
  • go/bridge/conflicts.go
  • go/bridge/conflicts_test.go
  • ios/VaultSync/Services/BackgroundSyncService.swift
  • ios/VaultSync/Services/SyncBridgeService.swift
  • ios/VaultSync/Services/SyncthingManager.swift
  • ios/VaultSync/Views/SettingsView.swift
  • ios/VaultSync/de.lproj/Localizable.strings
  • ios/VaultSync/en.lproj/Localizable.strings
  • ios/VaultSync/es.lproj/Localizable.strings
  • ios/VaultSync/zh-Hans.lproj/Localizable.strings
  • ios/VaultSyncTests/ConflictAutoResolveTests.swift
💤 Files with no reviewable changes (1)
  • ios/VaultSync/Services/BackgroundSyncService.swift

Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (2)
  • GitHub Check: M5/M6 Syncthing Transfer E2E
  • GitHub Check: Go Tests
🧰 Additional context used
📓 Path-based instructions (11)
**/*.swift

📄 CodeRabbit inference engine (Custom checks)

For Swift background execution changes, pass if work is bounded, cancellation-aware, handles expiration callbacks, and records errors without leaking private vault data. Fail only when background work can continue unbounded, miss cleanup, or violate iOS background execution constraints.

Files:

  • ios/VaultSync/Views/SettingsView.swift
  • ios/VaultSync/Services/SyncBridgeService.swift
  • ios/VaultSyncTests/ConflictAutoResolveTests.swift
  • ios/VaultSync/Services/SyncthingManager.swift
ios/**/*.swift

📄 CodeRabbit inference engine (README.md)

ios/**/*.swift: Build the iOS/iPadOS application with Swift 6 and SwiftUI, targeting iOS/iPadOS 18 or later.
Provide VoiceOver and Dynamic Type support throughout the iOS application.
Support localization in English, German, Spanish, and Simplified Chinese.
Use BGAppRefreshTask and BGContinuedProcessingTask for background processing, with BGContinuedProcessingTask available on iOS 26 or later when supported.
Use silent APNs push notifications through Cloud Relay for server-triggered background wake-ups; do not assume delivery because iOS controls whether and when the app runs.

Files:

  • ios/VaultSync/Views/SettingsView.swift
  • ios/VaultSync/Services/SyncBridgeService.swift
  • ios/VaultSyncTests/ConflictAutoResolveTests.swift
  • ios/VaultSync/Services/SyncthingManager.swift

⚙️ CodeRabbit configuration file

ios/**/*.swift: Focus on Swift 6 strict concurrency, Sendable/MainActor correctness, Task cancellation,
retain cycles, memory pressure, SwiftUI observation state, StoreKit/APNs flows, and iOS background execution limits.
Pay special attention to BGAppRefreshTask and BGContinuedProcessingTask behavior, expiration handling,
bounded work, and cleanup when the app is suspended or terminated.

Files:

  • ios/VaultSync/Views/SettingsView.swift
  • ios/VaultSync/Services/SyncBridgeService.swift
  • ios/VaultSyncTests/ConflictAutoResolveTests.swift
  • ios/VaultSync/Services/SyncthingManager.swift
**/*

⚙️ CodeRabbit configuration file

**/*: VaultSync syncs private Obsidian notes through Syncthing. Treat data loss,
privacy leaks, security regressions, and broken sync behavior as high priority.
Do not nitpick formatting unless it affects maintainability, correctness, or public API clarity.
Flag any accidental logging, telemetry, crash reporting, or network transfer of note contents,
vault paths, filenames with private context, API keys, APNs tokens, relay keys, or security-scoped bookmark data.

Files:

  • ios/VaultSync/Views/SettingsView.swift
  • docs/decisions/028-conflicts-require-manual-choice.md
  • CHANGELOG.md
  • docs/sync-filters-ux.md
  • ios/VaultSync/zh-Hans.lproj/Localizable.strings
  • ios/VaultSync/en.lproj/Localizable.strings
  • ios/VaultSync/es.lproj/Localizable.strings
  • ios/VaultSync/Services/SyncBridgeService.swift
  • ios/VaultSync/de.lproj/Localizable.strings
  • go/bridge/conflicts.go
  • ios/VaultSyncTests/ConflictAutoResolveTests.swift
  • ios/VaultSync/Services/SyncthingManager.swift
  • go/bridge/conflicts_test.go
docs/decisions/**/*

📄 CodeRabbit inference engine (docs/decisions/014-vault-subfolders-override-stray-root-config.md)

docs/decisions/**/*: Classify a connected root as a container, never as a vault-as-root, when it contains at least one direct subdirectory containing .obsidian/, regardless of whether the root itself contains .obsidian/. Use VaultManager.rootIsItselfVault as the single classification source for all sites, including grantAccess, share acceptance, and resolveSharePath.
Do not inspect the contents of the root-level .obsidian/ directory to distinguish a real vault from a stray configuration; use the presence of vault subfolders as the stronger signal.
Maintain regression coverage for root classification, including a root with both a stray root-level .obsidian/ and one or more vault subfolders, using the VaultRootClassificationTests suite.

docs/decisions/**/*: Every Relay provision request must require a locally verified, active Relay entitlement and its signed StoreKit transaction; never send a placeholder when this evidence is unavailable.
Preserve existing local and remote registration evidence across verification failures, network failures, and partial multi-homeserver failures.
Persist verified migration success independently for each homeserver.
Treat pre-migration success flags as indicating that migration is required, not that migration has been verified.
Do not clear registrations before migration; transient migration failures must not disable an otherwise working paid setup.
Do not use one global migration flag; one homeserver's failure must not hide or roll back another homeserver's success.

docs/decisions/**/*: Keep background start, local data progress, upload, download, and full-roundtrip proof as independent fields; never derive a global success flag.
Set only fresh local data progress after a successful file ItemFinished newer than both the check cursor and nanosecond start time within one stable engine generation.
Keep manual results in memory and isolate them per folder and its sole connected peer; preserve partial, unsupported...

Files:

  • docs/decisions/028-conflicts-require-manual-choice.md
docs/decisions/**/*.{go,swift,md}

📄 CodeRabbit inference engine (docs/decisions/022-diagnostics-helper-credentials-and-mutual-pairing.md)

Do not log or persist private/public keys, secrets, QR payloads, TLS pins, identifiers or digests, bindings, nonces, transcript fingerprints, signed bodies, paths, or credential records; update PRIVACY.md before runtime credential transport exists.

Files:

  • docs/decisions/028-conflicts-require-manual-choice.md
docs/**/*

📄 CodeRabbit inference engine (docs/helper-publication-rollout.md)

docs/**/*: Diagnostics is additive and opt-in: without both explicit configuration paths, helper 2.0.2 must retain prior Trigger-v1 behavior and create no diagnostics state.
No released app may call the helper capability as part of this release; publication must not claim upload, download, roundtrip, Relay delivery, APNs delivery, background execution, or vault progress.
Pairing, namespace enablement, authorization, and later app operations must remain separate signed actions; publication must not discover Syncthing, alter its configuration, share folders, transfer trust, or create or adopt namespaces.
Rollback must not delete or rewrite helper credentials, synchronized content, backups, versions, conflicts, peers, remote history, or tombstones; no operation may resume automatically after rollback or recovery.
After publication, perform read-only verification of the public release, exact tag commit, all ten assets and their GitHub SHA-256 digests, both image architectures, OCI digest, attestations, and embedded version; never overwrite published content to conceal failure.
The only supported diagnostics packaging environment is Docker Host-Bind on a standard Linux host with rootful Docker; rootless Docker, Docker Desktop, WSL, NAS/FUSE or remote storage, named volumes, systemd, launchd, and Windows Scheduled Tasks are unsupported.

docs/**/*: a relay rate-limit (HTTP 429) counts as success — it proves the trigger endpoint is reachable
An inactive subscription prints a WARN for the trigger check (WARN Relay trigger endpoint response sanity, followed by relay reports no active subscription for this device — …) without failing
The doctor also reports peer state, always as a WARN (the check name, then an indented reason) and never as a failure
--healthcheck deliberately skips peer state — a legitimately offline peer never makes the container unhealthy.
Silent push wake-ups use a background push that does not need notification permission — b...

Files:

  • docs/decisions/028-conflicts-require-manual-choice.md
  • docs/sync-filters-ux.md
docs/**/*.{md,go,sh}

📄 CodeRabbit inference engine (docs/helper-runtime-packaging-readiness.md)

Diagnostics publication, registry digests, rollout state, and public artifact availability must be established only by the owner-gated workflow and evidence in helper-publication-rollout.md; source text must never claim publication.

Files:

  • docs/decisions/028-conflicts-require-manual-choice.md
  • docs/sync-filters-ux.md
docs/**/*.{go,md}

📄 CodeRabbit inference engine (docs/helper-runtime-packaging-readiness.md)

docs/**/*.{go,md}: Revocation and rollback must preserve immutable authorization history and all retained Syncthing peer, versioning, backup, conflict, remote-history, and tombstone copies; rollback or downgrade must not erase, rewrite, replace, regenerate, or adopt partial state.
Do not claim upload, download, roundtrip, or released-app compatibility evidence from helper-side capability tests; those claims remain unset until the specified app-authored and fresh-device evidence exists.

Files:

  • docs/decisions/028-conflicts-require-manual-choice.md
  • docs/sync-filters-ux.md
**/*.md

⚙️ CodeRabbit configuration file

**/*.md: Review public documentation for technical accuracy, privacy/security claims, App Store-facing wording,
setup correctness, and consistency with the free app plus optional Cloud Relay subscription model.

Files:

  • docs/decisions/028-conflicts-require-manual-choice.md
  • CHANGELOG.md
  • docs/sync-filters-ux.md
go/**/*.go

📄 CodeRabbit inference engine (README.md)

Use Go 1.26 or later for the sync engine and generate the iOS xcframework through gomobile.

Files:

  • go/bridge/conflicts.go
  • go/bridge/conflicts_test.go
go/bridge/**/*.go

⚙️ CodeRabbit configuration file

go/bridge/**/*.go: This code crosses the gomobile Swift-Go boundary. Verify exported signatures use only gomobile-safe primitive types,
preserve the JSON string contract, keep empty-string success conventions intact, and avoid breaking Swift decoding tests.
Review Syncthing lifecycle, locking, error strings, and noassets build assumptions carefully.

Files:

  • go/bridge/conflicts.go
  • go/bridge/conflicts_test.go
🧠 Learnings (3)
📚 Learning: 2026-06-10T18:47:10.724Z
Learnt from: psimaker
Repo: psimaker/vaultsync PR: 38
File: ios/VaultSync/Views/ContentView.swift:605-611
Timestamp: 2026-06-10T18:47:10.724Z
Learning: In the SwiftUI codebase under ios/VaultSync, do not flag missing localization for SwiftUI string literals used as Text("…") or DisclosureGroup("…") titles/labels. In SwiftUI, these string literals are treated as LocalizedStringKey and resolve via the app’s Localizable.strings automatically—so they only need attention if the corresponding key is actually missing. Only require an explicit localization helper (e.g., L10n.tr(…)) when the string is not being passed through SwiftUI’s LocalizedStringKey path (e.g., plain String values provided to non-SwiftUI APIs).

Applied to files:

  • ios/VaultSync/Views/SettingsView.swift
  • ios/VaultSync/Services/SyncBridgeService.swift
  • ios/VaultSync/Services/SyncthingManager.swift
📚 Learning: 2026-07-12T23:04:14.842Z
Learnt from: CR
Repo: psimaker/vaultsync PR: 0
File: docs/decisions/023-diagnostics-namespace-and-least-privilege-access.md:0-0
Timestamp: 2026-07-12T23:04:14.842Z
Learning: Applies to docs/decisions/**/* : Treat conflicts, duplicate or partial artifacts, unexpected entries, and authorization disagreements as conflict/partial; never select a winner, overwrite, or auto-delete ambiguous content.

Applied to files:

  • docs/decisions/028-conflicts-require-manual-choice.md
📚 Learning: 2026-07-12T23:03:04.680Z
Learnt from: psimaker
Repo: psimaker/vaultsync PR: 107
File: ios/VaultSyncTests/DiagnosticsContractTests.swift:39-46
Timestamp: 2026-07-12T23:03:04.680Z
Learning: In iOS Swift tests that use CryptoKit’s `Curve25519.Signing.PrivateKey.signature(for:)` (Ed25519), don’t assert that a generated signature’s bytes exactly match deterministic “golden”/fixture signatures. CryptoKit signatures may be randomized (different but valid for the same key+message). Instead, verify correctness by calling `isValidSignature` (or equivalent) against (1) the golden bytes and (2) the freshly generated signature, and avoid byte-for-byte equality assertions between CryptoKit output and reference vectors.

Applied to files:

  • ios/VaultSyncTests/ConflictAutoResolveTests.swift
🔇 Additional comments (12)
docs/decisions/028-conflicts-require-manual-choice.md (1)

1-9: LGTM!

docs/sync-filters-ux.md (1)

4-5: LGTM!

Also applies to: 151-178

CHANGELOG.md (1)

11-11: LGTM!

go/bridge/conflicts.go (1)

468-470: LGTM!

Also applies to: 492-497

go/bridge/conflicts_test.go (1)

1656-1896: LGTM!

Also applies to: 1898-2040

ios/VaultSync/es.lproj/Localizable.strings (1)

647-649: 📐 Maintainability & Code Quality

Confirm the two new manual-review keys exist in all four catalogs and are used by the settings screen. This cohort supplies only the es and zh-Hans catalogs, so en and de parity and the Swift call sites are unverified.

  • ios/VaultSync/es.lproj/Localizable.strings#L647-L649: confirm the two keys match the exact English key strings used in SettingsView.swift.
  • ios/VaultSync/zh-Hans.lproj/Localizable.strings#L647-L649: confirm the same two keys are present and that the retired auto-resolve keys are removed here and in en and de.

Source: Coding guidelines

ios/VaultSync/Services/SyncthingManager.swift (1)

198-210: LGTM!

Also applies to: 1213-1220

ios/VaultSync/Services/SyncBridgeService.swift (1)

293-296: LGTM!

ios/VaultSync/Views/SettingsView.swift (1)

125-129: LGTM!

ios/VaultSync/en.lproj/Localizable.strings (1)

647-649: LGTM!

ios/VaultSync/de.lproj/Localizable.strings (1)

647-649: LGTM!

ios/VaultSyncTests/ConflictAutoResolveTests.swift (1)

5-89: LGTM!

Also applies to: 121-136


📝 Walkthrough

Walkthrough

VaultSync now leaves .obsidian conflicts unchanged for manual review. The bridge remains as a non-mutating compatibility entry point. iOS sync paths no longer invoke automatic resolution, and the settings UI and tests reflect the retired behavior.

Changes

Manual .obsidian conflict handling

Layer / File(s) Summary
Manual-review policy and compatibility contract
docs/decisions/028-conflicts-require-manual-choice.md, docs/sync-filters-ux.md, CHANGELOG.md
Documentation defines manual conflict review, ignored legacy preferences, unchanged Syncthing retention semantics, and the compatibility bridge behavior.
Non-mutating bridge behavior
go/bridge/conflicts.go, go/bridge/conflicts_test.go
AutoResolveStateConflicts now validates the folder and returns zero resolved copies without modifying files. Tests cover timestamps, nested paths, missing originals, metadata, contents, and error responses.
Retired automatic sync paths
ios/VaultSync/Services/SyncthingManager.swift, ios/VaultSync/Services/BackgroundSyncService.swift, ios/VaultSync/Services/SyncBridgeService.swift
The legacy preference always disables automatic resolution. Polling and background sync no longer perform resolver calls or rescans.
Manual-review UI and safeguards
ios/VaultSync/Views/SettingsView.swift, ios/VaultSync/*lproj/Localizable.strings, ios/VaultSyncTests/ConflictAutoResolveTests.swift
The settings UI and four localization catalogs describe manual review. Swift tests verify preference preservation and the absence of automatic resolver calls.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to c86bd

This PR routes conflicts through manual review and makes the compatibility entry point non-mutating. Localization-key parity should be confirmed during normal review, but no actionable merge-blocking risk remains.

Possibly related issues

Possibly related PRs

Poem

Conflicts stay still, in plain review,
No hidden hand decides what’s true.
The bridge remains, but files endure,
Tests guard each byte and timestamp pure.
Manual choice now leads the way.

🚥 Pre-merge checks | ✅ 7
✅ Passed checks (7 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title uses conventional-commit style and clearly describes disabling automatic .obsidian last-writer-wins mutation.
Linked Issues check ✅ Passed The changes disable automatic resolution, preserve preferences and files, update user-facing text, retain manual review, and add regression coverage for Issue #145.
Out of Scope Changes check ✅ Passed The documented code, tests, localization, and decision-record changes directly support Issue #145 and do not introduce unrelated scope.
No Private Note Leakage ✅ Passed The diff adds no logging, analytics, crash reporting, or network sink. It removes resolver calls; new tests use synthetic fixtures and local source reads, not user data.
Bounded Ios Background Work ✅ Passed The Swift diff only removes resolver work. Existing handlers retain bounded deadlines, cancellation checks, expiration callbacks, bridge cleanup, task completion, and generic error persistence.
Bridge Contract Compatibility ✅ Passed Go and Swift bridge signatures remain unchanged; the {resolved,error} JSON envelope and error conventions remain intact, with regression tests covering valid, unknown, and stopped states.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/issue-145-disable-auto-obsidian-lww

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@psimaker
psimaker merged commit e42079a into main Aug 16, 2026
19 checks passed
@psimaker
psimaker deleted the fix/issue-145-disable-auto-obsidian-lww branch August 16, 2026 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(conflicts): disable automatic .obsidian LWW mutation

1 participant