fix(ui): structurally prevent re-accept orphaning in the invitation GET handler (#367)

[sanity]

Problem

Follow-up to #365 / PR #366. PR #366 fixed the user-visible #365 bug (accepting an invite to a room you're already in joins you twice and orphans the original membership) with an entry-level guard in receive_invitation_modal::accept_invitation. That guard closes every realistic path to the symptom, but it is best-effort: it falls through if ROOMS.try_read() returns Err at accept time.

This PR closes the defense-in-depth gap one layer deeper, in the invitation-accept GET handler (get_response.rs). If the entry-level guard is ever skipped — unreadable ROOMS, or some future caller populating PENDING_INVITES for a room the user is already in — the GET handler still ran the full new-member accept for an already-member room:

• build_state_for_put PUTs a duplicate member for the invitation's fresh key.
• The deferred ROOMS mutation overwrites RoomData.self_sk with the fresh key, orphaning the original membership (the original Accepting an invite to a room you're already in joins twice and orphans the original membership #365 mechanism).
• record_invite_credentials records the fresh key's self_authorized_member / self_member_info.

A first attempt in PR #366 guarded only the self_sk overwrite. External (Codex) review correctly flagged that as worse: it preserved the old self_sk while the rest of the handler still recorded the new key's credentials and PUT the new member, leaving self_sk and the recorded credentials pointing at different keys — a split, internally inconsistent local state. That partial guard was reverted.

Approach

Detect "already a member under the held self_sk" early — before build_state_for_put — and short-circuit the entire accept:

• New pure predicate held_key_is_member(owner_vk, members, self_vk) mirrors vk_is_room_member (the accept-time guard's predicate) so both layers agree on what "already a member" means. It checks the held self_sk, never the invitation's fresh invitee_signing_key.
• At the top of the is_pending_invite branch, synchronously read ROOMS; if the held key is already a member, do a no-op refresh: merge the retrieved state, capture_self_membership_data, repopulate secrets, mark the pending invite Subscribed (so the modal's terminal-success path closes it), and return. No duplicate PUT, no new credentials, no self_sk change.
• Best-effort in the same sense as the entry-level guard: only fires when ROOMS is readable; otherwise falls through to the normal accept (prior behavior for that rare case). A genuine rejoin after leaving is unaffected — leave_room drops the room from ROOMS, so the held key is no longer a member and the guard does not fire.

This is the structural backstop that makes #365 impossible regardless of how the GET was triggered.

Testing

cargo test -p river-ui --bins — full suite passes (366 tests). New / updated tests:

• held_key_is_member_matches_owner_and_members — pure predicate unit test (owner, member, and non-member/genuine-new-join cases).
• reaccept_get_short_circuits_before_build_state_for_put — source-grep pin that the short-circuit is evaluated before build_state_for_put (the full handler reads the ROOMS/PENDING_INVITES signals and PUTs over the WebApi, so it can't be exercised by a host unit test — same approach as the existing Identity/room import can't recover a room several contract-WASM generations behind (restores stale "old IDs") #292 / Accepting an invite to a room you're already in joins twice and orphans the original membership #365 pins in this file).
• repopulate_secrets_call_sites_pinned count updated 5 → 6 for the new refresh path's repopulate_secrets_from_state call (the pin's doc-comment explicitly requires this when a new state-ingestion path is added).

cargo fmt + cargo clippy -p river-ui --bins clean (no new warnings). UI-only change — no delegate/contract WASM touched, so no migration required.

Review

Risk tier: Full (touches invitation/membership state-authorization logic, the surface of a prior bug). Multi-model review run serially (this PR was produced by a dispatched agent that cannot spawn parallel subagents): external non-Claude model pass (codex review / gemini) plus the four review lenses (code-first, testing, skeptical, big-picture). Findings and outcome posted as a follow-up comment.

Closes #367

[AI-assisted - Claude]

🤖 Generated with Claude Code

[sanity]

Review summary (Full tier — serial path)

This PR touches invitation/membership state-authorization, a high-risk surface and the site of a prior bug (#365), so it is Full tier. It was produced by a dispatched agent that cannot spawn parallel subagents, so reviews were run serially by the author per ~/.claude/rules/multi-model-review.md: an external non-Claude model pass (Codex) plus the four review lenses (code-first, testing, skeptical, big-picture) applied to the checked-out code.

External model pass

• Codex (codex review --base origin/main, gpt-5.x): ran successfully across 4 rounds (re-run after each fix, since a review is per-code-content). Final round: clean — "I did not find a discrete correctness issue introduced by the diff."
• Gemini (gemini-cli): unavailable — IneligibleTierError: This client is no longer supported for Gemini Code Assist for individuals (free-tier client deprecated). Not a quota/outage I can wait out; Codex provided the independent non-Claude signal, so no Claude-lens fallback was needed (fallback is only when ALL external models are down).

Codex findings — all addressed

1. [P2] Judge membership against canonical fetched state, not the local snapshot. The first cut gated the short-circuit on local ROOMS, which can be stale (member pruned/banned remotely). Fixed: the short-circuit now reads only which key the user holds from local ROOMS and decides membership against the authoritative retrieved_state.members. A held key that is no longer a member canonically falls through to a genuine rejoin. (commit 27de9ec5)
2. [P2] Genuine-rejoin fall-through could split local identity. The downstream existing-room self_sk overwrite was gated on the pre-merge (stale) local members, so a removed held key could leave self_sk at the old key while the PUT published the fresh member and credentials pointed at the fresh key. Fixed: the merge now runs before the membership check, so the self_sk decision sees canonical merged members — removed key → adopt fresh key (consistent rejoin); held membership → preserved (no orphan). (commit 78fe5abf)
3. [P2] Refresh path sync-status bookkeeping. Round-2 set SYNC_INFO Subscribed, which would mask a not-actually-subscribed room (stale invite after reload, imported room) and stop it receiving updates. Fixed: reset to Disconnected — the status rooms_awaiting_subscription keys on — so process_rooms establishes a real PUT+subscribe if needed (idempotent/self-healing if already subscribed), while still clearing the Subscribing state the pending-invite GET left behind. (commit c3c2c566)

Four-lens read (author)

• Code-first: short-circuit reads held self_vk, checks held_key_is_member against canonical state, refresh-only merge + capture_self_membership_data + repopulate_secrets_from_state (identical to every other refresh/merge path in the file — rebuild_actions_state is correctly NOT called, matching those paths). Logic coheres with stated intent.
• Testing: held_key_is_member pure unit test (owner/member/stranger); reaccept_get_short_circuits_before_build_state_for_put source-grep pin (ordering + canonical-state predicate) — same approach as the existing Identity/room import can't recover a room several contract-WASM generations behind (restores stale "old IDs") #292/Accepting an invite to a room you're already in joins twice and orphans the original membership #365 pins, used because the full handler needs signals/WebApi; repopulate_secrets_call_sites_pinned count bumped 5→6 (the pin's doc requires this for a new state-ingestion path).
• Skeptical: empty canonical state → fall through to normal join (correct for a real first join); no borrow held across defer; owner case short-circuits safely.
• Big-picture: matches the issue's goal (structural backstop, no-op refresh, no split state) and goes slightly further by handling the stale-snapshot rejoin. No tests removed; the call-site-count pin doc updated. The entry-level modal guard retains its own (pre-existing, separately-routed) local-only staleness behavior — out of scope for River UI: structurally prevent re-accept orphaning in the invitation GET handler (follow-up to #365) #367.

Local verification

cargo fmt clean; cargo clippy -p river-ui --bins introduces no new warnings (13 pre-existing, none in get_response.rs); cargo test -p river-ui --bins 366 passed (matching CI's gate). UI-only — no delegate/contract WASM touched, no migration.

[AI-assisted - Claude]