test(cli): close three holes in the #441 membership-guard source pins

[sanity]

Follow-up to #541, addressing findings from independent review. No production behaviour change — the authorization logic from #541 is untouched. This fixes the source pins that were supposed to protect it, and one inaccurate docstring.

Problem

Independent review found the #541 pins did not hold. I confirmed all three by executing the mutation against the full riverctl lib suite; it stayed green (294 passed) in every case.

1. The call-site needle stopped at a comma, before )?;

The needle was authorize_send(&room_state,room_owner_key,&sender_vk,. Two mutations matched it and survived:

• let _ = authorize_send(&room_state, room_owner_key, &sender_vk, members_delta.as_ref()); — the authorization decision is discarded entirely. let _ = also silences unused_must_use, and there is no -D warnings anywhere in CI, so nothing else catches it either.
• authorize_send(&room_state, room_owner_key, &sender_vk, None)?; — compiles via inference, and silently breaks the inactivity-rejoin path: a pruned member holding valid stored credentials can no longer send.

This is precisely the failure mode the pin's own doc comment claimed to prevent.

2. The scan count was method-sensitive, not merely name-sensitive

It matched only .any(|m| m.member.member_vk == ...) and missed .any(|m| m.member.id() == ...), which is equally owner-blind, is the more common idiom in this codebase (dm.rs:270,300,434, debug.rs), and has sender_member_id already in scope at the call site. A guard written that way keeps the count at 2 and passes.

The #541 commit message framed this whole class as closed. It was not — the fix there addressed variable renaming and stopped short of the scan method.

Separately, production was "everything before the test module", which still contained 15 mid-file #[cfg(test)] modules, so an unrelated future test could trip the count spuriously.

3. The docstring named the wrong contract-side backstop

It cited MessagesV1::verify. Verified against contracts/room-contract/src/lib.rs: update_state calls apply_delta (or merge) and never calls verify. What screens a message on the update path is MessagesV1::apply_delta's author retain — a MemberId comparison with no signature check. verify runs from validate_state, i.e. when a peer validates state, not as the gate on an update.

This does not weaken the owner branch, which is self-enforcing (it requires the caller to hold the owner's private key), but the comment overstated the backstop and would mislead the next reader.

Approach

The pins are now pure functions over source text — production_source and membership_guard_violations — rather than inline assertions. That is the load-bearing change: it makes the pins themselves testable, which is exactly what was missing. Six pin_catches_* meta-tests feed deliberately-mutated source through membership_guard_violations and assert it objects.

A pin nobody has watched fail is not a pin. That is how all three of these shipped.

Specifics:

• The call-site needle now runs through members_delta.as_ref() and the ?;, accepting both trailing-comma spellings so a future rustfmt that collapses the call onto one line cannot false-fail it.
• Both scan idioms are counted.
• production_source strips test modules line-wise on column-0 markers. Deliberately not brace-matching: api.rs holds 43 raw strings, and a matcher would have to lex them all correctly to avoid desynchronising. Every test module here starts with #[cfg(test)] at column 0 and ends with } at column 0, so this needs no lexer and cannot be confused by a brace inside a string or comment. Guarded by production_source_strips_test_modules_but_keeps_production, plus an early bail inside membership_guard_violations if the strip ever eats the send path (which would otherwise make every check pass vacuously).

Testing

301 tests pass. Every mutation below was applied to the real file and the suite re-run — not argued from inspection:

Mutation	Before	After
let _ = authorize_send(...) (authz discarded)	survived	caught
authorize_send(..., None) (rejoin path broken)	survived	caught
.any(|m| m.member.id() == sender_member_id) (MemberId idiom)	survived	caught
Call site fully re-inlined (the original #441 shape)	caught	caught
room_owner_key / sender_vk arguments swapped	caught	caught
Owner clause removed from room_has_member_key (reverts #541)	caught	caught

The first three are the review findings; the last three confirm this rewrite did not regress what the old pin did catch.

cargo fmt clean. No new clippy warnings — the six in riverctl are all pre-existing and outside this diff.

On the fourth review finding

The review also flagged cli/src/commands/identity.rs:290-300 as having the same owner-blind scan, telling the owner "Try sending a message first to populate membership data" — advice that can never work.

I could not reproduce this and believe it is a false positive, so I have not filed it. The scan sits in the final else of a three-way chain at identity.rs:264-300:

let is_owner = signing_key.verifying_key() == room_owner_key;   // :258
let (authorized_member, invite_chain) =
    if let Some(am) = room_info.self_authorized_member.clone() { ... }   // :265
    else if is_owner { /* synthesizes the owner's AuthorizedMember */ }  // :267
    else { /* :275 — the scan and that error message */ };

The owner is caught by else if is_owner at :267 and never reaches the scan. is_owner is computed from the resolved signing key, so a --signing-key-file override pointing at the owner's key still lands in the owner branch. The error string occurs exactly once in the file, reachable only when !is_owner, and for its actual audience — a pruned non-owner — "try sending a message first" is accurate advice, since a send triggers the rejoin path.

Happy to be shown wrong if the reviewer has a path I have missed.

Refs #441

[AI-assisted - Claude]

[sanity]

Independent review — pin robustness lens

Reviewed at 7796c015 by a reviewer that did not write the code. Because it could not run cargo, it ported production_source and membership_guard_violations to Python verbatim and validated the port by reproducing the known baseline and all three previously-confirmed results. I then executed its two headline claims directly.

Verdict: this closes all three confirmed mutations. The pin machinery itself is not yet robust — three fixes recommended before merge.

What this PR fixes — confirmed

mutation (each previously left all 294 tests green on #541)	now
let _ = authorize_send(...)	CAUGHT
authorize_send(..., None)?	CAUGHT
.any(|m| m.member.id() == sender_member_id)	CAUGHT (count 3, expected 2)

The needle running through )?; and accepting both trailing-comma spellings is correct, as is counting both scan idioms.

Finding 1 — commenting the call out survives the pin proper

Replacing the call with // was: authorize_send(&room_state, room_owner_key, &sender_vk, members_delta.as_ref())?; leaves check 1 satisfied: the commented text squashes to a string containing both the prefix and the )?; tail.

Executed: the suite does go red — 299 passed, 2 failed — but the failures are pin_catches_discarded_authorization_result and pin_catches_rejoin_delta_replaced_by_none, both panicking must be caught, got: []. Those are the meta-tests failing because their str::replace can no longer find REAL_CALL to mutate. The pin itself is fooled; the red is a staleness side effect that disappears the moment REAL_CALL is updated or the meta-tests are refactored.

Fix: strip lines whose trimmed form starts with // before the substring search.

Finding 2 — the stripper can silently stop covering production

production_source skips from a line exactly #[cfg(test)] to the next column-0 }. Any #[cfg(test)] item that is not brace-terminated — use, const, type, mod foo; — makes the skip run past the item and swallow production code.

Executed, and this is the dangerous direction. Inserting

#[cfg(test)]
use std::fmt::Debug;

before unseal_nickname_display (cli/src/api.rs:715), together with a planted owner-blind helper in the swallowed window:

pub(crate) fn sneaky_owner_blind_guard(state: &ChatRoomStateV1, vk: &VerifyingKey) -> bool {
    state.members.members.iter().any(|m| m.member.member_vk == *vk)
}

leaves the full suite green — 301 passed. The scan-count never saw the new scan. A pin that silently stops covering production reads as protection while providing none.

Two related notes: the doc at :9412 says the file holds 15 #[cfg(test)] modules — it is 12 modules plus 3 #[cfg(test)] fns (:501, :785, :814), so the stripper already depends on non-module items happening to close at column 0. And the stated reason for rejecting brace-matching does not hold: the reviewer wrote a raw-string/comment-aware reference stripper and diffed it against the line-wise one over the real file — byte-identical, zero hunks. The 43 raw strings desynchronise nothing.

Fix (keeping the line-wise approach): after consuming #[cfg(test)], require the next line to open a braced item and push a violation otherwise.

Finding 3 — check 1 is not scoped to send_message_with_key

:9474 searches the whole production text while its failure message at :9483 claims it pins that function. Contrast check 3, which correctly scopes to fnbuild_rejoin_delta(. With eight near-identical send paths in this file, a sibling copy carrying the guard would satisfy a pin about the original.

Finding 4 — the scan count misses live idioms (defence-in-depth only)

Surviving the count: .find(..).is_some(), .position(..), swapped operands, renamed closure binding, and two idioms already in this codebase — HashSet<MemberId>::contains, which build_rejoin_delta itself uses at :3339-3347, and members_by_member_id(), the dominant idiom (api.rs:4949, debug.rs:76, deputies.rs:215, member_info.rs:135/289/324). A re-inline at the send path is still caught by check 1, so this is bounded — but the comment at :9495 should stop describing the count as robust against renaming.

Scope gap outside api.rs

cli/src/private_room.rs:264-268 is owner-safe only via the early return at :259 — the same order-dependent shape that is pinned in build_rejoin_delta, but with no pin. Deleting that early return reintroduces the #441 class in the private-room heal path with nothing to catch it.

Credit

The meta-tests operate on include_str!("api.rs") — the real file — and hard-fail if the target text is absent, so the committed tests carry end-to-end proof rather than only proving the checker objects to synthetic input. That is the strongest part of this PR. (Caveat: the owner-blind-scan meta-tests str::replace on a comment that occurs 12 times, so they insert 12 scans rather than 1 — assertions hold, but they are blunter than they read.)

The reviewer also independently reached the same conclusion as the author on the identity.rs finding from the earlier review: it is a false positive, the owner is caught by else if is_owner and never reaches that scan.

[AI-assisted - Claude]