fix(cli): let the room owner send with an explicit signing key

[sanity]

Problem

riverctl message send <room> <msg> --signing-key <owner-key> (or RIVER_SIGNING_KEY set to the owner's key) always failed for the room's own owner:

Error: Signing key is not a current member of this room and no stored membership
credentials were found for automatic rejoin. If you were pruned for inactivity,
ensure you first accepted an invitation via `riverctl invite accept`.

The message also actively misdirects — it tells a room's owner to go accept an invitation to their own room.

Root cause (verified against the contract, not inferred)

The guard in send_message_with_key tested membership by scanning room_state.members.members. The owner is structurally absent from that list: MembersV1::verify (common/src/room_state/member.rs:59-66) rejects a members list containing the owner —

if member.member.id() == owner_id {
    return Err("Owner should not be included in the members list".to_string());
}

— because owner membership rides on ChatRoomParametersV1::owner instead. So is_member was always false for the owner. And build_rejoin_delta early-returns (None, None) for the owner ("Owner doesn't need to re-add"), so members_delta was None too. Both halves of if !is_member && members_delta.is_none() were satisfied on every owner send.

This is specific to the explicit-signing-key path; the storage path (send_message) never ran this guard.

Approach

Count the owner as a member, and make the decision testable.

The one-line fix the issue suggested is correct but would have landed untested: the guard sits inside an async method that needs a live WebSocket node. So the decision is extracted into two pure helpers — room_has_member_key and authorize_send — following the pattern already used in this file for rejoin_preferred_nickname ("lives in rejoin_preferred_nickname so it is unit-testable without a node connection").

CLI-only. No contract, WASM, signed-struct, or verify/apply_delta change — so no delegate-migration ritual is involved. git diff --stat touches cli/src/api.rs alone.

Why this opens no authorization hole

• The owner exemption is sender_vk == room_owner_key, comparing the full 32-byte VerifyingKey (ed25519_dalek's PartialEq compares as_bytes()). sender_vk is derived from the caller's own private key, so the clause is satisfiable only by someone holding the owner's private key. It is not a client-supplied claim.
• Deliberately not a MemberId comparison. MemberId is fast_hash (hash = hash*31 + byte over i64), which is not collision-resistant; using it here would have been the weaker primitive.
• room_owner_key is not a free-floating assertion of role — it selects the room. owner_vk_to_contract_key derives the contract key from ChatRoomParametersV1 { owner } + code hash, so passing a different owner key addresses a different contract. There is no cross-room escalation.
• The client guard is a pre-flight for a good error message, not the authorization boundary. The contract independently verifies every message: MessagesV1::verify validates an owner-authored message's signature against parameters.owner, and apply_delta retains a message only when its author is a listed member or the owner.
• Nothing else was relaxed. The diff adds exactly one disjunct; the members-scan and rejoin-delta branches are behaviourally unchanged, so a banned or pruned member's path through the guard is identical to before. The owner cannot be banned in the first place — is_ban_authorized refuses target == owner_id outright.
• Private rooms are unaffected: build_message_body runs before the guard and still errors when no room secret is available, so the owner cannot send plaintext into a private room.

Sibling explicit-key paths (finish-the-fix audit)

I audited every other site that could carry the same implicit-owner blind spot. send_message_with_key was the only one. Reporting this as an audit result rather than inventing changes:

Site	Status
edit_message, delete_message, add_reaction, remove_reaction, send_reply, set_nickname	No membership guard at all — they call build_rejoin_delta and use its result, so the owner passes through unharmed. Not instances of this bug.
dm.rs send + room_has_member	Already owner-correct (member_id == owner_id || ...).
identity.rs export	Already branches on is_owner.
private_room.rs::build_member_info_heal	Early-returns for the owner before its members scan.
deputies.rs	Already documents and handles "the owner is never in members.members".
build_rejoin_delta	Owner early-return precedes its scan.

Testing

Nine tests in cli/src/api.rs::authorize_send_tests. Every claim below was verified by actually applying the mutation and re-running, not by inspection:

Mutation	Caught by
Remove the owner clause from room_has_member_key (i.e. revert the fix)	owner_may_send_although_members_list_never_lists_them, room_has_member_key_counts_the_owner_and_listed_members_only
Revert the call site to the original inline owner-blind guard	send_paths_delegate_the_membership_guard_to_authorize_send
Remove the contract's owner arm from apply_delta's author retain	owners_message_survives_apply_delta_with_no_membership_delta
Make authorize_send allow everything (authorization removed entirely)	stranger_without_rejoin_credentials_is_rejected, owner_exemption_is_not_reachable_from_attacker_controlled_state
Add a third owner-blind scan to a sibling path under a fresh variable name	send_paths_delegate_the_membership_guard_to_authorize_send
Delete build_rejoin_delta's owner early-return	send_paths_delegate_the_membership_guard_to_authorize_send

Notable coverage choices:

• The reproduction asserts its own premise. owner_may_send_although_members_list_never_lists_them first asserts the owner is absent from the fixture's members list, so fixture drift cannot silently make it vacuous.
• The guard fix alone would not have been enough, so it is not tested alone. owners_message_survives_apply_delta_with_no_membership_delta builds the delta exactly as the owner path does (members: None, member_info: None), applies it, and asserts the message actually lands in recent_messages — passing the guard but having the contract drop the message would have shipped a send that reports success and delivers nothing.
• The security test treats room state as hostile. Room state arrives from the network, so owner_exemption_is_not_reachable_from_attacker_controlled_state stuffs it with a members entry carrying the owner's key and a forged owner member_info, then asserts the attacker and 34 other keys are all still refused.
• The pin counts scans rather than banning spellings. The first version banned two specific expressions; a guard written with a differently-named variable would have walked straight past it. It now pins the count of raw members.members scans (2, both owner-safe) and asserts build_rejoin_delta's owner early-return still precedes its scan. The mutation table above confirms the rename case is now caught.
• The test module sits at the end of the file on purpose. Splitting at a mid-file #[cfg(test)] would have cut the send_message_with_key call site out of the scraped text and left the pin permanently green; the test asserts the split landed correctly in both directions.

Honest coverage gaps

• There is no end-to-end test of the riverctl message send command itself — it needs a live node and there is no harness for it here. Coverage is the guard decision (unit) + call-site delegation (source pin) + contract acceptance (unit), which spans the whole causal chain except the WebSocket plumbing the bug never involved.
• The sibling paths are covered by the scan-count pin only, not by behavioural tests, because they have no membership guard to exercise. The pin makes a future owner-blind guard fail CI.
• contract_rejects_an_owner_entry_in_members pins a river-core invariant this fix depends on, not CLI code. It is deliberate: if the contract ever permitted owner entries, this special case would deserve a rethink.

cargo fmt clean; cargo test -p riverctl -p river-core green (284 riverctl lib tests + the river-core suites); no new clippy warnings (the six in riverctl are all pre-existing and outside this diff).

Note for a follow-up, not fixed here

MemberId is fast_hash — a Java-style hash*31 + byte rolling hash over i64, which is trivially collidable algebraically, despite the comment at common/src/room_state/member.rs:692-694 claiming a collision would take "3 * 10^59 years". Nothing in this PR depends on it (the owner check deliberately compares full verifying keys), and the contract's signature checks mean a collision yields confusion rather than forgery — but the comment is wrong and the primitive is weaker than it looks. Changing it is wire-format/contract territory needing its own review and sign-off, so it is deliberately out of scope here.

Closes #441

[AI-assisted - Claude]

[sanity]

Multi-lens review

Three distinct adversarial lenses, run serially, each reading the checked-out code rather than the description. Per the project's review rule this change is Full tier — it touches an authorization path. Claude lenses only; no external models (opt-in per multi-model-review.md).

Lens 1 — code-first (read the implementation before the issue text)

#	Finding	Resolution
1.1	Issue's stated mechanism could be wrong.	Verified independently, not trusted. MembersV1::verify rejects an owner entry (member.rs:59-66) and build_rejoin_delta early-returns for the owner. Both confirmed by reading. Pinned by contract_rejects_an_owner_entry_in_members.
1.2	The fix moves membership computation to after build_rejoin_delta, changing evaluation order.	Not a behaviour change. build_rejoin_delta takes &ChatRoomStateV1 and only reads; it cannot affect the membership answer.
1.3	Two extracted functions for a one-line issue fix — over-engineering?	Dismissed with reason. The guard is inside an async method requiring a live node, so the one-line fix would have shipped untested. The file already uses this pattern (rejoin_preferred_nickname, extracted verbatim "so it is unit-testable without a node connection").
1.4	authorize_send returns Ok on rejoin_members_delta.is_some(). An empty delta would pass the guard while re-adding nobody — send reports success, contract drops the message.	Not reachable. build_rejoin_delta seeds members_to_add with one element unconditionally before extending, so it is never Some(empty). Pre-existing invariant, unchanged here. Flagged rather than "fixed" because adding an emptiness check would alter behaviour beyond this issue's scope.
1.5	New intra-doc link [ApiClient::owner_vk_to_contract_key] on a pub(crate) item may not resolve.	Fixed/verified. cargo doc --document-private-items reports no unresolved link for any symbol in this diff (the file has pre-existing unrelated link warnings).
1.6	The error text still advises riverctl invite accept, which the issue called misleading.	Deliberately unchanged. After the fix the owner never reaches it; for a genuine non-member the advice is accurate. Churning user-facing copy without evidence is exactly what the prompt/UX-change bar warns against.
1.7	Doc comment referred to parameter key after it was renamed candidate.	Fixed in cbd3431.

Lens 2 — security / authorization (the lens for this change)

Each question answered against the code, not by assertion.

#	Question	Answer
2.1	Can a non-owner now impersonate the owner?	No. The clause is sender_vk == room_owner_key where sender_vk = signing_key.verifying_key(). ed25519_dalek::VerifyingKey's PartialEq compares as_bytes() — exact 32-byte equality (verified in the vendored source). Satisfying it requires holding the owner's private key.
2.2	Is ownership cryptographically verified, or merely asserted?	Verified. A SigningKey cannot be constructed for a public key you do not hold, and the message is signed with that same key, so possession is proven to the contract by signature — not by a client-supplied field.
2.3	Is room_owner_key attacker-controlled?	It selects the room, it is not a role claim. owner_vk_to_contract_key derives the contract key from ChatRoomParametersV1 { owner } + code hash, so a different owner key addresses a different contract. No cross-room escalation.
2.4	Does the contract still reject a forged owner delta?	Yes. MessagesV1::verify validates an owner-authored message against parameters.owner; apply_delta's retain keeps only member-or-owner authors. The client guard is a pre-flight for a good error message, not the boundary.
2.5	Does the relaxed guard let a banned or removed member through?	No. The diff adds exactly one disjunct; the members-scan and rejoin branches are behaviourally identical, so a banned/pruned member's path is unchanged. The owner cannot be banned at all — is_ban_authorized returns false for target == owner_id before any grant is considered. The owner cannot be "removed" either: they are never in the list, and ownership is fixed by the contract parameters.
2.6	Does the owner exemption bypass private-room encryption?	No. build_message_body runs before the guard and still errors when no room secret is available.
2.7	Is the exemption reachable from untrusted room state?	No, and this is now tested. Room state arrives from the network; owner_exemption_is_not_reachable_from_attacker_controlled_state stuffs it with a members entry carrying the owner's key plus a forged owner member_info and asserts the attacker and 34 other keys are still refused.
2.8	Was a weaker comparison primitive used?	No, and this was a live risk. MemberId is fast_hash (hash*31 + byte over i64) — trivially collidable, despite the "3 * 10^59 years" comment at member.rs:692-694. The owner check deliberately compares full verifying keys, so this diff adds no exposure to it. Recorded as an out-of-scope follow-up in the PR description.
2.9	Should the storage path (send_message) get the same guard?	No. It never had one; adding it would be a new restriction that could break working flows, which is scope creep on an authorization path.

Conclusion: no authorization hole. The change strictly widens a client-side pre-flight by one condition that requires the owner's private key, on a path where the contract independently enforces authorization.

Lens 3 — testing (coverage gaps, vacuous tests)

#	Finding	Resolution
3.1	"Would each test fail if the fix were reverted?" must be demonstrated, not assumed.	Demonstrated by applying six mutations and re-running — table in the PR description. Includes the worst case (authorization removed entirely), caught by two tests.
3.2	The pin banned two literal spellings; a guard using a differently-named variable would slip past it — the exact failure mode the pin exists to prevent.	Fixed in eb34bc7: pins the count of raw members.members scans instead. Mutation-confirmed by injecting a third scan under a fresh name (self_vk2), which is now caught.
3.3	The two adjacent &VerifyingKey parameters are a swap footgun the compiler cannot catch, and swapping them silently narrows the guard.	Already pinned. The whitespace-stripped scrape asserts exact argument order; a swap mutation was applied and is caught.
3.4	Passing the guard proves nothing if the contract then drops the message.	Covered by owners_message_survives_apply_delta_with_no_membership_delta, which builds the owner's real delta shape and asserts the message lands. Mutation-confirmed against the contract's owner-retain arm.
3.5	The reproduction test could become vacuous through fixture drift.	Guarded: it asserts the owner is absent from the fixture's members list before asserting the outcome.
3.6	A source pin split at a mid-file #[cfg(test)] silently loses production code (a known trap in this repo).	Avoided: the module is placed at end-of-file deliberately, and the test asserts the split landed correctly in both directions.
3.7	Are the sibling paths actually covered?	Honestly: by the scan-count pin only, not behaviourally — they have no membership guard to exercise. Stated as a gap in the PR rather than implied as coverage.
3.8	No end-to-end test of the CLI command.	Acknowledged gap. Needs a live node; no harness exists. The unit + pin + contract tests span the whole causal chain except the WebSocket plumbing the bug never involved.

Finish-the-fix audit

Every other site that could carry the implicit-owner blind spot was checked; send_message_with_key was the only instance. The sibling explicit-key paths (edit / delete / reaction / unreact / reply / set_nickname) have no membership guard, so the owner passes through unharmed — they are not instances of this bug. dm.rs, identity.rs, private_room.rs and deputies.rs already handle the owner explicitly. Full table in the PR description. Reported as an audit result rather than manufacturing changes to look thorough.

Findings status

All findings are fixed (1.5, 1.7, 3.2) or dismissed with a specific reason (1.3, 1.4, 1.6, 2.9). Nothing outstanding.

[AI-assisted - Claude]