fix(ui): stop a legacy-sourced copy overwriting the stored room identity

[sanity]

Problem

reconcile_room_present's diverged-identity arm keeps local unconditionally and never consults rank, while the merge path (#527, #590) does. The save path contains zero rank reads. So an identity loaded from an older delegate generation can be CAS-written over the current delegate's slot, destroying self_sk — which cannot be re-derived from the network.

Reachability (#588 route 2): #592's get_key_index turns an unparseable index into an empty one, while handle_get_request and handle_get_versioned_request read secrets directly and never consult the index. A room therefore becomes invisible to ListRequest while remaining fully readable by key — which routes the loader into ProbeLegacy, whose only guard is a localStorage flag that reads false forever in the sandboxed opaque-origin iframe. A legacy generation then answers with the old identity and the re-save overwrites the stored one.

No code path blocks that chain, and the terminal step was executed rather than reasoned about (stored_self_sk_survived=false). Whether the index actually corrupts in the field is unestablished — quota exhaustion was investigated and cleanly ruled out (the quota check precedes the write; the write is tmp+rename atomic). So this is cheap insurance against an unrecoverable loss, not a response to an observed incident.

Approach

Why not "keep the higher-ranked identity"

That phrasing does not survive contact with the code, and the reason is #590's session boundary again: there is only ONE rank in existence here. MergeRanks is never persisted and RoomSlot::Present carries no provenance, so there is nothing to compare the stored slot against. Any implementation quietly becomes "compare local's rank to a constant", and the constant is then the entire fix.

What is decidable is whether the copy we hold is legacy-sourced. If it is, the stored slot was written by the current delegate, which outranks every legacy generation by construction, so refusing is always right. Refusing costs room_state, which the network can re-derive. Writing costs self_sk, which nothing can.

The default IS the fix

An absent rank entry does not mean "unknown, assume the worst". record_identity_source deliberately records nothing for a room that is already present and unranked — exactly how an in-session create or import looks — so absent must read as SOURCE_RANK_AUTHORITATIVE. unwrap_or(0) would make every locally created room overwritable by any legacy copy: strictly worse than having no guard.

It is extracted into identity_rank_for_save and pinned, because the first mutation run showed unwrap_or(0) SURVIVING — the behavioural tests call reconcile_room_present directly with an explicit rank and never exercised the caller's default. Same untested-half pattern that produced four findings in #593.

The rank is read once before the per-room loop, not inside the CAS closure: the closure re-runs per retry and this is the same non-reentrant mutex that produced a near-deadlock in #590's drain.

Testing

Four-case matrix driving the real function against real slot bytes:

case	expected
legacy rank + differing identity	Ok(None), stored self_sk intact
current-delegate rank + differing	writes local (#420 unchanged)
absent rank + differing	writes local (#414 unchanged)
legacy rank + matching identity	normal merge, still persists

The last case is the one that catches an over-broad guard that would silently stop persisting every migrated room. Plus a test that Absent/Tombstone paths never consult rank, so round-9 rejoin behaviour is untouched.

Mutations verified red: unwrap_or(0); <= for <; comparison reversed; compared against SOURCE_RANK_AUTHORITATIVE rather than current_delegate_source_rank(); ranks.tombstone substituted for ranks.identity (compiles — both are HashMap<RoomKey, u32>); guard deleted; helper ignoring the registry entirely.

839 tests green, clippy clean. Also drops an unused import introduced by #593.

Residual — NOT closed

The design rests on "the stored slot was written by the current delegate, so it outranks any legacy generation". True for slots written by this client, but a slot written by an older River build that itself held a legacy-sourced identity violates it, and detecting that requires persisted provenance — a much larger change. This closes route 2; it does not make the branch correct in general.

Sequencing

This is a precondition for #588's seal, not a follow-up. Repairing the seal closes route 2 but opens route 1 (an interrupted migration can then set the in-progress flag), and route 1's trigger — a tab closed mid-migration — is far more common than index corruption. Landing the seal first would trade a rare trigger for a common one.

Design and test matrix were specified by an independent review lens before implementation.

Refs #588, #592

[AI-assisted - Claude]

[sanity]

Review result: blocking regression found — parking this PR

Two independent blind reviewers read this branch (adversarial lens + code-first; a data-loss lens was attempted repeatedly and never produced a report, so it is not counted as a clearance). One blocking finding, which I then verified against the code myself.

The guard's stated premise does not hold during the legacy fan-out

The comment at ui/src/components/app/chat_delegate.rs:4944 justifies the refusal with:

"If it did, the stored slot was written by the current delegate, which outranks every legacy generation by construction, so refusing is always right."

That conflates which delegate key the bytes live under with which generation supplied the identity inside them. During migration, ui/src/components/app/freenet_api/response_handler.rs:1891 fires a save_rooms_to_delegate() for every responding legacy generation, so the current delegate's slot is routinely written by the legacy migration itself. RoomSlot::Present carries no provenance, so the guard cannot tell the two apart.

Concrete sequence (32 legacy generations, so current_delegate_source_rank() == 32):

1. Generations are probed together. LEGACY_DELEGATES is oldest-first age order (chat_delegate.rs:747) and the oldest copy usually answers first (:269). Gen 2 answers with room R carrying identity A; the slot is empty, so this is a StoreFresh and the slot becomes Present(A).
2. Gen 20 answers with identity B. The ranked merge correctly prefers B in memory and records identity[R] = 20.
3. Gen 20's save runs: identity_rank_for_save(R) == 20 < 32 and local.self_sk (B) != remote.self_sk (A) → RefusedIdentityOverwrite. Nothing is written.
4. request_legacy_seal_on_quiescence() fires and no later session re-probes gen 20.

Net: memory holds B, the delegate keeps A, and B becomes unreachable for good.

On origin/main step 3 writes B and the rollback does not occur, so this is a regression in the migration path — the scenario this workstream exists to fix. It is also the same hazard response_handler.rs:1913-1921 already describes in its own words as what the #527 seal-delay was written to prevent, reintroduced from the save side.

Second finding, same root

The deferred signing-key migration (response_handler.rs:1640-1710) is unconditional and knows nothing about the refusal. So a refused pass can leave room:<vk> → Present(self_sk = A) while __signing_key:<room> → B — the delegate signing with an identity the stored room does not carry, which is the split #527 fixed, re-created from the other direction. The refusal cannot be evaluated in isolation from the rest of the migration.

Lower-severity (real, but holes in a new guard rather than regressions)

• TOCTOU — rooms is snapshotted once at chat_delegate.rs:5079 but identity_rank is read per-room later at :5149, after awaited round-trips during which deferred merges can mutate both. The guard can then pass on a rank that does not describe the snapshot it is about to write.
• Source pin satisfiable by an unrelated match — chat_delegate.rs:1765 searches the whole extracted body for SavedSlot::Tombstone, which also occurs in the tombstone loop (:5225, :5231), so deleting the bookkeeping the assertion protects leaves the pin green.
• chat_delegate.rs:1739 — measured body length 9,215 against a 10,000-char guard; the next ~15 lines trip it, with a message that misdirects.

Note the adversarial reviewer did not run mutation testing (the brief forbade modifying the worktree), so its non-vacuity claims are reasoning rather than measurement.

Status

Parking this as a draft. It fixes a bug that is real in code but has no established field trigger, while currently introducing one that fires in ordinary migration — net-negative as written, and there is no incident forcing a rushed reshape.

The available direction: ROOM_SLOT_STATE already records which rooms this session wrote, which is exactly the missing provenance — if this session wrote the slot, overwriting it is safe. That is a reshape of the guard rather than a tweak, and would need a fresh review round.

[AI-assisted - Claude]