Skip to content

Finding #2: Revert to Recommendation is a real engine inverse (unisonRiRevert) + first-class changedFromDefault#12

Merged
bcourbage merged 3 commits into
mainfrom
fix/revert-recommendation
Jul 20, 2026
Merged

Finding #2: Revert to Recommendation is a real engine inverse (unisonRiRevert) + first-class changedFromDefault#12
bcourbage merged 3 commits into
mainfrom
fix/revert-recommendation

Conversation

@bcourbage

@bcourbage bcourbage commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Reconciliation correctness (review Finding #2). "Revert to Unison's Recommendation" previously deleted only the Swift rowOverrides entry; the OCaml direction (set by unisonRiSet*) stayed forced, so synchronization used the reverted-away choice while the badge cleared. This makes Revert a genuine per-row engine inverse, using the exact reset already present in the vendored blob — no blob/patch/upstream-source change (blob stays a57f5c4e).

Why no rescan fallback

Upstream uimacbridge.ml already registers unisonRiRevert (diff.direction <- diff.default_direction) and changedFromDefault — both already in the current blob. Every setter, incl. Force Older/Newer via Recon.setDirection, mutates only diff.direction; default_direction is set once at reconcile time and never reassigned. So resetting to default_direction is the exact, total inverse of all six actions — a coordinator-owned rescan (which would re-run update detection and discard every other row's choices, expansion/selection, and possibly the row set) is unnecessary and strictly worse.

Implementation

  • unison_bridge_ri_revert — shares one helper with the six setters; structured mutation contract (INVALID before mutation / OK / FAILED_DIRTY on any raise once revert began, incl. either readback → engineBecameUncertain). ri_set and revert also return changedFromDefault.
  • changedFromDefault first-class on the C/Swift state item — populated transactionally during scan + Ignore emission (nine callbacks/row, six string dups), updated by every action's result. Never conflates callback failure with false (a raise fails the publication or is DIRTY). Carried from the engine because a skip-requested Conflict renders identically to a default Conflict yet must read as changed (structural compare). It has no initializer default, so every construction site states it explicitly.
  • Revert rewrite — reverts each selected modified leaf in OCaml, resyncs the Swift direction + changedFromDefault, clears the override, redraws leaves/ancestors; FAILED_DIRTY stops the batch (earlier successes stay synced) → restart-required; gated by the reconciliation action gate.
  • Enablement (RowSelectionRules.isRevertible) covers rows that diverge from the default (changedFromDefault — plain First/Second/Merge leave no override) OR carry a Force/Skip override (so a Force equal to the default is still revertible to clear its badge). The obsolete Swift-only clearOverrides helper (which could recreate the original defect) was removed.

Tests & evidence

Engine round-trip: every action → Revert restores the exact recommendation with changedFromDefault == false (each action's returned direction + flag asserted before reverting, so no-op mutations can't pass); action-equals-default and a deterministic Force-equals-default (equal-mtime conflict, engine ignores Older/Newer); Force-changes (distinct mtimes); skip-over-original-conflict via structural comparison; invalid-row no-op; exception injection at the revert/direction/changed readback stages; per-row multi-row success + injected failure; changedFromDefault carried through scan and Ignore reindexing; isRevertible predicate; and a consolidated engine-level Revert smoke.

Post-Revert eligibility is established as split evidence: the engine returns changedFromDefault == false; the pure predicate returns false with no override; and code inspection confirms the window removes the override after a successful Revert.

Verification

  • Debug + Release build green; full suite 526 tests, 0 failures, 1 skipped; app launches cleanly standalone.
  • No new Release test hooks; vendored blob byte-identical (a57f5c4e); vendor//patches/ untouched; docs/manual-test-step2b.md untracked.

Automation limitation (retained)

The window-level Revert batch orchestration (stop-on-DIRTY keeping earlier rows synced, override removal, redraw) lives in the private @MainActor ReconcileWindowController and is not automatable in this suite: the tests do not drive the window controller batch loop, observe visual badges, or observe menu greying. Those are covered by code inspection plus the automated bridge/engine tests and the pure predicate tests. The GUI gestures (folder multi-select, badge/menu rendering) were not automated.

Out of scope

Scan/core cancellation, other review findings, release work, unrelated cleanup.

🤖 Generated with Claude Code

bcourbage and others added 3 commits July 19, 2026 20:11
@bcourbage @claude
Finding #2: make Revert a real engine inverse via unisonRiRevert
161bfbb 
"Revert to Unison's Recommendation" only deleted the Swift override; the OCaml
direction (set by unisonRiSet*) was left forced, so sync used the reverted-away
choice while the badge cleared. Fix it with the exact per-row inverse already in
the vendored blob — no blob/patch/source change.

Engine inverse. Add `unison_bridge_ri_revert`, which applies upstream
`unisonRiRevert` (`diff.direction <- diff.default_direction`). Because every
setter — including Force Older/Newer via `Recon.setDirection` — mutates only
`diff.direction`, and `default_direction` is immutable after the scan, this is
the exact, total inverse of all six actions (First/Second/Skip/Merge/Force). It
follows the structured mutation contract: resolves the revert, direction, and
changed-from-default callbacks before mutating; INVALID for a bad row/missing
callback; FAILED_DIRTY for any raise once the revert began (incl. either
readback), routed to engineBecameUncertain. ri_set/revert share one helper and
now also return `changedFromDefault`.

changedFromDefault is first-class. Added to the C/Swift state item, populated
transactionally during scan and Ignore emission and updated by every direction
action's result. A missing accessor fails the whole publication and a raise in a
mutation is DIRTY, so callback failure is never conflated with `false`. It is
carried (not recomputed in Swift) because a skip-requested Conflict renders
identically to a default Conflict yet must read as changed — only the engine's
structural compare gets that right.

Revert rewrite. `revertSelectionAction` reverts each selected modified leaf in
OCaml, resyncs the Swift direction + changedFromDefault, clears the visual
override, and redraws leaves + ancestors. FAILED_DIRTY stops the batch (earlier
successes stay synced) and enters restart-required. Gated by the reconciliation
action gate. Enablement (extracted `RowSelectionRules.isRevertible`) covers a row
that diverges from the default (changedFromDefault — plain First/Second/Merge,
which leave no override) OR carries a Force/Skip override (so a Force that equals
the default is still revertible to clear its badge).

Tests: six-actions-then-revert restores the exact recommendation; skip over an
original conflict is detected as changed and reverts to the original conflict;
force incl. result-equal-to-default; merge; invalid-row no-op; exception
injection at the revert/direction/changed readback stages; multi-row success +
injected per-row failure; changedFromDefault carried through scan and Ignore
reindexing; and the isRevertible enablement predicate. Corrected the misleading
Revert/clearOverrides comments; updated TODO.md to describe the real engine
transition.

Debug + Release build green; full suite 528 tests, 0 failures, 1 skipped; no new
Release test hooks; vendored blob byte-identical (a57f5c4e).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@bcourbage @claude
Finding #2 amendment: enforce StateItem invariant, drop clearOverride…
d800d51 
…s, harden tests

Review follow-ups on the Revert engine-inverse work.

StateItem invariant: `changedFromDefault` now has NO default in the initializer
(a silent false on a real row would hide a divergence and mis-gate Revert) —
every construction site states it explicitly. Removed the direction-only
`with(direction:)`; a direction change must always supply the matching
`changedFromDefault` atomically via `with(direction:changedFromDefault:)`. Added
tests that `with(progress:)` preserves a true flag and the direction updater
changes both together.

Removed the obsolete `RowSelectionRules.clearOverrides` helper and its tests —
unused, and it preserved a Swift-only "drop the override" API that could recreate
the original Revert defect (clearing the badge without reverting the engine).

Hardened bridge tests: the Force fixture now sets deterministic distinct mtimes
(both.txt) and an equal-mtime conflict row (both-eq.txt); every action asserts
its returned direction AND changedFromDefault before reverting, so a no-op
mutation can't pass; added an explicit action-equals-default case (Second on a
default----> row) and a deterministic Force-equals-default case (equal mtimes →
engine ignores Older/Newer); kept the skip-over-original-conflict structural
test; renamed the multi-row test to state it exercises per-row building blocks,
not the private window batch loop; added a consolidated engine-level Revert
smoke (mixed actions, revert, unrelated row untouched, post-revert not
revertible, restored direction == what sync propagates).

Docs: state-emission accounting corrected to nine callbacks per row (six string
duplications); removed the "modified badge" claim for changedFromDefault (it
gates Revert eligibility).

Debug + Release build green; full suite 526 tests, 0 failures, 1 skipped; app
launches cleanly standalone; no new Release test hooks; vendored blob
byte-identical (a57f5c4e).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@bcourbage @claude
Finding #2: test-accuracy cleanup for Revert (test_k/test_q)
24694c1 
Low-severity test-doc/assertion corrections; no runtime change.

- test_k: fix the doc — the deterministic Force-equals-default case is the
  EQUAL-mtime conflict row (both-eq.txt, engine ignores Older/Newer), not a
  one-sided row (Force on a one-sided row does change direction).
- test_q: match its Force claim — apply a plain direction to only-a, Force to
  the distinct-mtime conflict (both.txt), and Skip to the equal-mtime conflict
  (both-eq.txt); leave only-b untargeted; revert all three and assert restored
  directions + changedFromDefault == false.
- test_q: drop the tautological only-b Swift-snapshot assertion; state that no
  operation targeted that row and that cross-row isolation comes from the
  indexed bridge implementation + the independent per-row tests, not a live
  readback here.
- test_q: describe post-Revert eligibility as SPLIT evidence — engine returns
  changedFromDefault==false; the pure predicate returns false with no override;
  code inspection confirms the window removes the override after a successful
  Revert (not observed by this test).
- test_q: state explicitly it does NOT drive the private window controller or
  observe menu greying/badges.

Debug + Release build green; full suite 526 tests, 0 failures, 1 skipped; no new
Release test hooks; vendored blob byte-identical (a57f5c4e).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@bcourbage bcourbage changed the title Finding #2: Revert to Recommendation performs a real engine inverse Finding #2: Revert to Recommendation is a real engine inverse (unisonRiRevert) + first-class changedFromDefault Jul 20, 2026
@bcourbage
bcourbage marked this pull request as ready for review July 20, 2026 03:45
@bcourbage
bcourbage merged commit 44d74ce into main Jul 20, 2026
1 check passed
@bcourbage bcourbage mentioned this pull request Jul 20, 2026
Merged
bcourbage added a commit that referenced this pull request Jul 20, 2026
@bcourbage @claude
Findings #8/#12: centralize probe lifecycle; deterministic teardown
c106044 
Lifecycle-correction pass for PR #18 (SSH probe safety).

1. Cancel the probe in leaveSession (the common session-leave path), so
   BOTH window-close and the programmatic Stop-during-scan path tear it
   down. Previously Stop-during-scan detached the window delegate before
   closing, so handleWindowClosed (which held the cancel) never fired and
   the probe leaked. Removed the duplicate cancel from handleWindowClosed.

2. runVersionCheckIfNeeded supersedes the old probe BEFORE the
   unison_bridge_get_version() guard, so a replacement open invalidates the
   previous session's probe even if the version lookup then fails.

3. Executor checks cancellation BEFORE launch (never spawns an
   already-abandoned child) and immediately AFTER Process.run().

4. Deterministic termination. New ProbeCanceller fires a registered
   teardown (SIGTERM) SYNCHRONOUSLY inside cancel() rather than on a later
   background poll tick; applicationWillTerminate waits (bounded) on
   Handle.waitUntilFinished so the app doesn't exit mid-teardown. The wait
   loop checks cancellation first so a SIGTERM'd child reports .cancelled,
   not .exited(signal).

5. Active-probe state is cleared on completion only when the delivering
   probe is still current (a newer, superseding probe is never clobbered).

6. Corrected teardown claims: the deadline is measured from just after
   launch (not "launch + I/O + exit"); the final SIGKILL reap-wait result
   is not asserted; terminating ssh does not prove its ProxyCommand/remote
   descendants are reaped.

7. Lifecycle tests: cancel-before-launch never launches; teardown fires
   synchronously on cancel; late registration fires immediately; cancel is
   idempotent (teardown once); wait wakes on cancel / times out; shutdown
   waitUntilFinished completes after cancel.

Debug build + Release compile succeed; full suite 576 tests, 0 failures
(1 pre-existing env skip). Swift-only; vendored blob a57f5c4e and vendor/
patches/ untouched.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bcourbage added a commit that referenced this pull request Jul 20, 2026
@bcourbage
Merge PR #18: lifecycle-owned SSH version probe with correct argv + d…
726b529 
…eterministic teardown (Findings #8/#12)

Findings #8/#12: lifecycle-owned SSH version probe with correct argv and deterministic teardown
This was referenced Jul 20, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@bcourbage