Repository navigation
Conversation
…d of conflict markers
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
ocavuebot
left a comment
There was a problem hiding this comment.
Review by Claude Fable 5.1 (Anthropic model claude-fable-5-1). Static review of the diff and the call chains it touches, against the sync design (total merge, archive before any non-exact result, one sync channel per graph). No local tests run; CI was green at review time.
Reviewed head 3d30467807a897c1075564b35ff61dc53d29d902 (one commit on git_N1_total_merge).
Verdict: needs changes (one P1: the collision fold overwrites the canonical note without archiving it). I also flag a plan-level issue with rule 7 that this PR implemented as specified.
What I confirmed: Resolution::Marked is gone and grep whole_note_markers|stacked_whole_note_markers|Resolution::Marked is empty; rule 7 keeps the line-disjoint tail guard; the edit-conflict pass archives every version before the ladder runs, so rule 4 and rule 8 losers are archived there; the sweep records merge pairs for Reconciled like for Merged; needs_review is kept but never pushed; conflict_merge_text maps Reconciled to Unmergeable { content: theirs }, and with #1464 that path saves the buffer as a copy beside the note, so no buffer is lost; clippy -D warnings is green on Linux.
Doc nits (not inline, unchanged lines): conflict/mod.rs:36-43 still says “Marker labels for a merge” for THIS_DEVICE / OTHER_DEVICE and “provenance that drives ordering and marker labels” for ConflictSide; the labels are diff3 labels and diagnostics only now.
| Resolution::Merged { content } => (content, false), | ||
| Resolution::Marked { content } => (content, true), | ||
| let merged = match resolution { | ||
| Resolution::AlreadyResolved => canonical_content.clone(), |
There was a problem hiding this comment.
[P1] fold_duplicate archives only the duplicate, then writes a Reconciled result over the canonical, whose exact bytes then exist nowhere.
Before this PR rule 8 wrote markers, so the canonical’s text stayed inside the file. Now rule 8 interleaves words. The canonical is not an NSFileVersion (a collision duplicate is a separate file, not a version), and .reflect/ does not sync, so there is no copy of it on any device. This breaks the invariant the Resolution doc states (callers archive both sides before writing Reconciled).
Trigger: daily/2026-07-04.md = - shared\n- mac wording\n- common tail\n, daily/2026-07-04 2.md = - shared\n- phone wording\n- common tail\n. base: None, the union guard refuses (both tails contain - common tail), rule 8 returns Reconciled, the canonical is overwritten. The PR’s own test overlapping_collision_bodies_merge_into_the_canonical runs exactly this path and does not check for a canonical archive.
Fix: archive_version(root, canonical_rel, None, <canonical modified_ms>, canonical_content.as_bytes()) before ladder::resolve (archive first, like archived_sides), and assert two archive entries in that test.
There was a problem hiding this comment.
Confirmed and fixed in ca4308a: fold_duplicate builds both ConflictSides first and archives each (canonical under its own path, duplicate under its own) before ladder::resolve. The collision test now asserts one archive entry under each path.
| if let Some(content) = merge_frontmatter_only(input.base, &first, &second) { | ||
| return Ok(Resolution::Merged { content }); | ||
| } | ||
|
|
There was a problem hiding this comment.
[P2, plan gap: the PR did what the plan says] Rule 7 is base-blind and now applies to every note. With a base, a deletion on one side next to an append on the other resurrects the deleted lines as an exact Merged result that no caller archives.
Trigger: base a\nb\nc\n, A = a\nb\n (deleted c), B = a\nb\nc\nd\n (appended d). diff3 conflicts (adjacent hunks; git merge-file -p exits 1), rule 6 does not apply, rule 7: A’s tail is empty, so append_union returns B whole and c is back. After N6 the editor will adopt this silently; after P3.2 a pull does the same. Before this PR the exposure was limited to daily/ and creation collisions.
The new test whole_note_rewrites_on_both_sides_keep_both_lines (base line\n, sides mac\n / phone\n) pins this base-blind behavior for a rewrite that is not an append.
Suggested plan change: when base is Some, apply rule 7 only if the shared prefix covers the base’s non-blank lines (both sides still start with the base). Otherwise fall to rule 8, whose callers archive. Not a blocker for this PR alone, but it should be decided before P3.2 and N6 widen the exposure.
There was a problem hiding this comment.
Agreed, and done in ca4308a. append_union now takes the base: when one exists, both sides must still start with it (trailing blank lines trimmed), otherwise the union refuses and rule 8 takes over, whose callers archive. Without a base the common-prefix rule is unchanged.
Consequences, all in this PR: your trigger is pinned as a_deletion_beside_an_append_does_not_come_back_as_an_append (result a\nb\nd\n, Reconciled); whole_note_rewrites_on_both_sides_keep_both_lines now expects Reconciled with both words; the sweep's three-device rewrite test now checks words rather than phrases, because the total merge interleaves a line every device rewrote (wording from ipad mac phone) instead of stacking three lines. That is the trade-off of the stricter rule: a whole-line rewrite on both sides is word-merged and archived rather than line-unioned and not archived. The daily-note case (both sides append to the synced base) is untouched.
…on appends only to the base A collision fold archived only the duplicate, then wrote a `Reconciled` result over the canonical, whose exact bytes then existed nowhere. Rule 7 was base-blind: a deletion on one side next to an append on the other came back as an exact `Merged` result that no caller archives. With a base, both sides must still start with it; otherwise the total merge (rule 8) takes over and its callers archive.
Summary
Stacked on #1477 (
total::merge). With this PR the app never writes conflict markers again. Every conflict the iCloud sweep or the editor's buffer merge sees ends in a usable note.The ladder (
conflict/ladder.rs) keeps its exact rules and replaces the two marker rules:Reconciled)daily/and creation collisions onlytotal::merge(Reconciled): both sides' words kept, no markersResolution::MarkedbecomesResolution::Reconciled: a result where a side's text may be dropped (rule 4) or interleaved (rule 8). The iCloud sweep already archives every involved version under.reflect/conflict-archive/before it writes, so it treatsMergedandReconciledalike.needs_reviewis still reported but never pushed by the sweep.Removed with the markers:
markers.rskeeps onlycontains_conflict_markers(rule 3 still protects notes an older version left marked).whole_note_markers,stacked_markers,marker_labelare gone.merge3::Diff3Outcome→Option<String>.ConflictInput::{path, creation_collision}: no rule keys on the path any more.resolve_many, the n-way marker fold insweep.rs: sides fold pairwise through the ladder, oldest first.ShadowStore::clear_merge_pair(no caller).MergeTextKind::Conflictedis no longer produced. The TS schema still accepts it;conflict_merge_textreports a merge that could only be reconciled asunmergeable(it has no graph root to archive into), and the editor keeps the buffer as it does today. The editor-side cleanup (pathargument,conflictedbranch) comes in the follow-up editor PR.Tests
cargo test -p reflect-open: 411 passed. Ladder, merge3, sweep, andconflict_merge_texttests rewritten for the new outcomes, including the three-device folds and the marked-side case.cargo clippy --all-targets -- -D warningsclean.Written by Claude Fable 5.1.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by Sourcery
Replace conflict-marker generation with deterministic reconciliation that preserves overlapping edits and archives the original versions.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: