test: pin that self_sk survives a delegate re-key migration (#612)

[sanity]

Problem

RoomData::self_sk is a room's actual signing/identity private key. It is not
derived from anything and cannot be regenerated — if it is lost, the room is
lost.

Nothing deliberately protects it across a delegate WASM re-key. It survives only
because it happens to sit inside the RoomData blob stored under the generic,
indexed delegate key room:<b58 owner_vk>, and the delegate's key_index is
exactly what the migration probe's ListRequest enumerates. Move the key into
its own bespoke secret the way handle_store_signing_key does (that writer never
touches key_index) and it silently leaves the indexed set and stops migrating.

That refactor is a reasonable-looking hardening move — a raw private key does not
belong in a multi-purpose blob — which is what makes it dangerous: it reads as
pure improvement. The signing_key: secret gets away with the pattern only
because it is regenerable (migrate_signing_key re-derives it from self_sk
after every migration). self_sk has no such fallback.

River re-keys its delegate roughly weekly, so this lands as "every user's
identity in every room is gone" at the next routine release, with no error raised
on any path.

#612 documents the hazard. An issue is a sign, not a fence — this PR
adds the fence.

Approach

Test-only, plus documentation. No production behavior change.

Two paired tests in freenet_api::response_handler::tests, driving the real
code on both sides of the re-key rather than asserting the field exists:

• self_sk_survives_a_legacy_delegate_migration — the predecessor's blob comes
  from reconcile_room_present (the sole production writer of room:<vk>);
  discovery runs through the real plan_load_from_keys; the successor's read is
  the same from_reader::<RoomSlot> + reconstruct_rooms pair that
  migrate_legacy_per_room runs. Then it asserts the key arrived intact.
• a_relocated_self_sk_would_not_survive_a_legacy_delegate_migration — proves the
  negative. Stores a blob shaped the way the relocation would shape it, holds
  everything else identical, runs the same migration reader, and shows the key
  does not come across while the room migrates fine and no error is raised
  anywhere. Without this, the positive test could pass for the wrong reason.

A third test added in review, only_the_indexed_delegate_paths_register_keys_for_migration,
pins the delegate-side mechanism the other two depend on: the generic
handle_store_request / handle_cas_store_request paths must register their keys
in key_index, handle_store_signing_key must not, and handle_list_request must
answer from that index. Source-scraped because the behavioral form cannot fail on
native targets (DelegateCtx::set_secret is a no-op, get_secret always returns
None), so it would pass whether or not the handler indexed anything.

Documentation at the two points of temptation, both pointing at #612 and the
tests: the self_sk field definition (ui/src/room_data.rs) and
reconcile_room_present (ui/src/components/app/chat_delegate.rs).

The only non-test change is widening reconcile_room_present from fn to
pub(crate) fn so the test can use the real writer instead of hand-rolling the
blob. No logic change; nothing outside its module calls it.

The tests live in river-ui, which CI gates via cargo test -p river-ui --bins.

Testing

cargo test -p river-ui --bins — 894 pass. Also green under
--features example-data,no-sync (898). cargo fmt and cargo clippy clean
(remaining warnings are pre-existing on main).

Both tests were mutation-checked, not just observed passing:

1. Writer mutation — merged.self_sk = None before serialization in
   reconcile_room_present (simulating the relocation dropping it from the
   blob). The positive test fails with its intended message. Two pre-existing
   writer tests (a_saved_slot_always_carries_self_vk,
   reconcile_room_present_diverged_identity_keeps_local) also catch this one —
   reported for accuracy; this PR is not the sole guard against that particular
   edit.
2. Reader mutation — dropping self_sk when reconstruct_rooms inserts a
   Present slot, which is Do not move self_sk out of RoomData's generic migration path without a compensating migration mechanism #612's exact failure mode (the migration path stops
   carrying the key). Only the new test catches it: 892 other tests pass.
   That is the gap this PR closes.

The negative test passes under both mutations, as it should — it never touches
the mutated code, which is what keeps the pair independent.

The third test was mutation-checked in three further directions (indexed path stops
indexing / bespoke path starts indexing / list anchor renamed) — all caught. Details,
including one mutation that initially did not fire and why, are in the review comment
below.

All mutations were reverted; the branch is the clean state.

Reviewed by two independent lenses (code-first, testing); no blocking findings. The one
should-fix is the third test above; dispositions for the rest are in the review comment.

Refs #612. Surfaced #614 (chat-delegate tests never run in CI).

[AI-assisted - Claude]

[sanity]

Two notes for reviewers:

• The mutation evidence in the PR body distinguishes two cases deliberately. The writer mutation is also caught by two pre-existing tests, so this PR is not the sole guard there. The reader mutation — reconstruct_rooms dropping self_sk, which is Do not move self_sk out of RoomData's generic migration path without a compensating migration mechanism #612's exact failure mode — is caught by only the new test; the other 892 pass. That second one is the gap being closed.
• While tracing the delegate side of the migration path I found that the chat-delegate crate's ~39 unit tests never run in CI. Out of scope here (it's a CI-config change), filed as chore(ci): chat-delegate unit tests never run in CI #614.

This PR is on hold and must not be merged — see the pinned hold note below.

[AI-assisted - Claude]

[sanity]

Independent review (Light tier) — complete

Two independent lenses, both blind to each other, neither wrote the code:

• code-first (read the code before the description, hunt intent/implementation mismatch)
• testing (would this fence actually fire when the hazardous refactor lands?)

Neither found a blocking issue. The code-first lens independently re-traced every factual claim in the new doc comments against the actual code — the room:<b58 owner_vk> key format, handle_store_signing_key never touching key_index, reconcile_room_present being the sole populated-slot writer, migrate_signing_key being what makes the signing_key: pattern safe — and confirmed all of them, plus verified the tests call the same production functions migrate_legacy_per_room calls. It found no discrepancy between the description and the diff.

Findings and dispositions

1. (should-fix) The delegate-side key_index asymmetry was asserted only in prose. Both lenses converged on this. It is the actual mechanism that makes a secret discoverable to the migration probe, so leaving it unverified meant the doc comments could rot silently.

Fixed in ddf11875: only_the_indexed_delegate_paths_register_keys_for_migration pins all four halves — handle_store_request and handle_cas_store_request must call set_key_index(, handle_store_signing_key must not, and handle_list_request must answer from get_key_index(.

Source-scraped rather than behavioral, deliberately: on native targets DelegateCtx::set_secret is a no-op and get_secret always returns None, so a test asserting "this secret never appears in a ListResponse" would pass whether or not the handler indexed it — a verification that cannot fail. It lives in the UI crate rather than beside the code it scrapes because the chat-delegate crate's tests are not wired into CI (#614); it should move once they are. Scraping another crate's file also can't self-match its own needles.

Mutation-checked in three directions rather than assumed:

Mutation	Result
handle_cas_store_request stops calling set_key_index	caught
handle_store_signing_key genuinely starts indexing itself	caught
handle_list_request anchor renamed	caught

The first attempt at the second mutation inserted a bare let _ = set_key_index; and was not caught — correctly, since the needle is set_key_index( and a bare reference is not a call. Re-run with a real call it fails as intended. Recording the miss because it is the difference between a checked pin and an assumed one. Body slicing was separately verified to extract 49/56/27/18 lines, matching the real function extents.

2. (should-fix, judgment call) No test covers "an upstream construction site quietly stops populating RoomData.self_sk" — both new migration tests inject self_sk directly rather than driving a real room-creation/join/accept path.

Not fixed here, deliberately. Three reasons: (a) it is a different hazard from #612 — that issue is about the storage location leaving the indexed path, whereas this is a plain population bug in room creation, with a different fix and a different owner; (b) the shape is already partially pinned by the pre-existing self_sk/self_vk pairing source-scrape in room_data.rs; (c) driving a real construction path requires the Dioxus global signals and WEB_API, which is precisely why this file's UI-side migration coverage is built from extracted pure functions in the first place. Worth its own issue if anyone disagrees, but folding it in here would widen a test-only PR into the async/Dioxus harness problem.

3. (nit, both lenses) The negative test's hand-rolled RelocatedRoomData is a plausible-but-unverified guess at what a future relocation would emit, and will not automatically break the day a real relocation lands — a human has to follow the doc pointer.

Acknowledged, not changed. That is the intended tradeoff for a tripwire of this kind, and it is why the warning was placed at the field definition rather than only in the issue tracker. The positive test is the load-bearing one and does not depend on the guess. Verified non-vacuous today: ciborium encodes structs as name-keyed maps, so field order cannot drift the test into passing for the wrong reason, and every RoomData field the trimmed struct omits carries #[serde(default)] or #[serde(skip)].

4. (nit) pub(crate) widens who can call reconcile_room_present outside its CAS wrapper. Real but minor; single call site today. Left as-is, noted here so a future reuse gets scrutiny.

Now 894 tests pass locally; cargo fmt and cargo clippy clean.

[AI-assisted - Claude]

[sanity]

Merged

Fable 5 review returned MERGE AS-IS (no blocking findings), all checks passed on ddf11875, and this merged as b3ae3d4b. The earlier DO-NOT-MERGE hold is retracted and no longer applies.

The reviewer re-ran the mutation checks independently rather than trusting the PR body, reproducing the reader-mutation result exactly (893 pass, exactly 1 fail — self_sk_survives_a_legacy_delegate_migration). It also exercised a mutation shape not reported here: #[serde(skip)] on RoomData::self_sk, the realistic "stop persisting it in the blob" form of the relocation, which goes red with 8 failures including the positive test. It verified the CI wiring is real, brace-match-checked that the source-scrape slices cover full function extents so the negative assertion cannot be vacuous, and confirmed the pub(crate) widening is visibility-only. Both dismissals recorded above were independently checked and held.

Two non-blocking nits are recorded on #612 rather than left in this thread.

[AI-assisted - Claude]