River UI: structurally prevent re-accept orphaning in the invitation GET handler (follow-up to #365)

[sanity]

Background

PR #366 fixes 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 (plus the check_membership_status modal routing). That guard closes every realistic path to the symptom.

This issue tracks a defense-in-depth gap one layer deeper, in the invitation-accept GET handler (ui/src/components/app/freenet_api/response_handler/get_response.rs).

The gap

If the entry-level guard is ever skipped — e.g. ROOMS.try_read() returns Err at accept time, or some future caller populates PENDING_INVITES for a room the user is already in — the GET handler still runs the full new-member accept for an already-member room:

• build_state_for_put (~line 249) synthesizes a join for the invitation's fresh key and PUTs a duplicate member to the network.
• The deferred ROOMS mutation overwrites RoomData.self_sk with the fresh key (~line 335), 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 (~line 411) records the fresh key's self_authorized_member/self_member_info.

Why a partial backstop is NOT enough

A first attempt in PR #366 guarded only the self_sk overwrite ("don't clobber self_sk if the held key is already a member"). External (Codex) review correctly flagged this as worse, not better: it preserves the old self_sk while the rest of the handler still records the new key's credentials and PUTs the new member. That leaves self_sk and self_authorized_member/self_member_info pointing at different keys — a split/inconsistent local state that breaks identity export and rejoin/member-info flows. The original (unconditional-clobber) behavior is at least internally consistent. So the partial guard was reverted.

The correct fix

Detect "already a member under the held self_sk" early — before build_state_for_put — and short-circuit the entire accept: do not PUT a duplicate member, do not record new self-credentials, do not change self_sk. Treat it as a no-op / refresh (merge the retrieved state, repopulate secrets, clear the pending invite). This needs a synchronous ROOMS read near the top of the is_pending_invite branch, with a fall-through only when ROOMS is genuinely unreadable.

Add a regression/pin test for the short-circuit. This is the structural backstop that makes #365 impossible regardless of how the GET was triggered.

Refs: get_response.rs (~249 build_state_for_put, ~335 self_sk overwrite, ~411 record_invite_credentials). Follow-up to #365 / PR #366.

[AI-assisted - Claude]

[sanity]

Issue Priority Assessment

Generated: 2026-06-19T03:21:41.166539559+00:00
Issue hash: d1bdaf2d

Value Assessment

Now I have enough context to write a clear assessment.

Relevance: The issue is still valid. The entry-level guard (PR #366) blocks the user from clicking Accept when already a member via the UI modal's check_membership_status → render_already_member branch. However, accept_invitation itself (line 824) has no such guard, and the GET handler's is_pending_invite branch in get_response.rs unconditionally calls build_state_for_put (~line 249) and then sends the PUT to the network — before the deferred already_member check (~line 407) inside the ROOMS mutation closure. The defense-in-depth gap described in the issue exists exactly as written.

Assessment:

This issue is still relevant and accurately describes a real gap in the codebase. PR #366 added an entry-level guard in receive_invitation_modal.rs (check_membership_status + render_already_member) that prevents a user from clicking Accept when their current key is already a member. However, accept_invitation itself has no equivalent guard, and the GET handler's is_pending_invite branch (starting around line 132 of get_response.rs) unconditionally calls build_state_for_put and fires a network PUT before the already_member check runs inside the deferred ROOMS mutation closure at ~line 407. This means any caller that populates PENDING_INVITES for an already-joined room — a reload mid-subscription, a future CLI/DM path, or even a ROOMS.try_read() returning Err at accept time — bypasses the UI guard and hits the orphaning path.

User impact is low-frequency but high-severity when it occurs. The original #365 bug (accepting an existing room's invitation replacing self_sk with a fresh key, losing all message-send capability until a hard reload) is the exact outcome. The entry-level modal guard substantially closes the main realistic path, so ordinary daily users are unlikely to trigger this now. However, the reload-mid-subscription auto-resume path (PendingRoomStatus::PendingSubscription on restart, documented at line 193 of receive_invitation_modal.rs) is a real scenario where ROOMS may not yet be populated when the GET response arrives, meaning the deferred already_member check silently falls through to the orphaning logic on every page reload during subscription.

Strategically, this is a correctness fix for data integrity (membership identity is the foundation of every other operation), not a feature addition. The fix as described in the issue — reading ROOMS synchronously before build_state_for_put and short-circuiting the entire accept for already-member rooms — is straightforward and self-contained. The lack of a regression test (also called out in the issue) is a real gap: PR #366 was explicitly reviewed and a partial guard was reverted because it produced a worse inconsistency, demonstrating the subtlety of the logic.

The issue is not blocking other active work, and there is no hard deadline. Its urgency is moderate: the primary symptom (user loses room access) requires a hard reload to recover, which is disorienting but not catastrophic. The fix is low-risk and well-specified, making it a good candidate to address in the near-term cleanup cycle rather than deferring indefinitely — especially because the test coverage gap leaves the existing PR #366 guard unverified against future regressions.

Cost Assessment

Now I have a good picture of the code. The issue is well-understood and the code location is confirmed. Let me check the record_invite_credentials and the check at line 407 more carefully to understand the current state.

The existing check at line 407-415 (already_member) only gates the "add member to local room state" block — it does NOT prevent build_state_for_put (line 249), self_sk overwrite (line 336), or record_invite_credentials (line 390) from running. The issue's description matches the current code exactly.

This is a well-scoped, low-complexity issue. The fix involves:

1. Adding a synchronous ROOMS.try_read() check near line 132 (inside the is_pending_invite branch, before build_state_for_put) to detect if the invitee's key is already a member of the room.
2. If already a member: skip build_state_for_put, skip self_sk overwrite, skip record_invite_credentials, and instead do a no-op merge + clear the pending invite.
3. Adding a regression test for this short-circuit path.

The scope is a single file (get_response.rs) with one test addition. There are no external dependencies. The risk is low — the guard is additive, and the fall-through behavior (when ROOMS is unreadable) is the same as today. The main tricky edge case is the defer() pattern used throughout this handler: the "already a member" check must happen synchronously before the defer closure is constructed, since signal reads inside defer are in a different execution context. The existing already_member check at line 407 shows exactly this pattern already exists in the defer closure for the local-state mutation, but the issue correctly identifies that build_state_for_put and record_invite_credentials happen outside the defer and need the guard applied before them.

Testing is straightforward: a unit test against build_state_for_put with an already-member key, plus a handler-level test verifying no PUT occurs when the invitee's self_sk is already in the room's member list. The existing test infrastructure in the file (lines 1383+) provides ample scaffolding.

Assessment: This is a low-to-medium cost issue. The change is confined to a single function in one file (get_response.rs), the fix pattern is clearly specified in the issue, and the existing code around line 407 already demonstrates how to do the membership check synchronously before a defer block. The main implementation risk is ensuring the short-circuit path correctly handles the no-op branch — merging state, repopulating secrets, and clearing the pending invite — without accidentally skipping any side effects that a legitimate fresh-accept would need. Writing the regression test is the most time-consuming part, since the test setup for this handler requires building a ChatRoomStateV1 with a member already present and verifying that no PUT is issued. Estimate: 2–4 hours total including test coverage. No external dependencies, no third-party coordination, no bureaucratic overhead.

---

Generated by issue-prioritizer