feat(buzz-relay): NIP-FI stateless enforcement (S3) — upgrade gate, NIP-42 pairing, session lifetime, JWKS warm

[wpfleger96]

This PR implements stateless NIP-FI enforcement (S3) in buzz-relay: WebSocket upgrade gate, NIP-42 key pairing, three-term session lifetime, and JWKS warm/background refresh — all with no DB reads/writes for identity.

Without this, the relay accepted connections with no assertion verification. This PR closes that gap by wiring NIP-FI enforcement entirely in the relay layer, using the buzz-auth verifier crate added by #7214.

• Add crates/buzz-relay/src/nip_fi_upgrade.rs: check_nip_fi_at_upgrade() validates Nostr-Federated-Identity: Bearer <token> at WebSocket upgrade. Rejects all prohibited transport forms (missing, repeated, comma-combined, empty, non-Bearer, whitespace-in-token). Exact HTTP denial wire contract: 401 + WWW-Authenticate: Nostr for missing evidence, 403 for rejected/denied, 503 for unavailable; all text/plain; charset=utf-8 with no free text. [FI-TRACE-DENIAL-ORACLE]
• Add crates/buzz-relay/src/nip_fi_config.rs: NipFiRelayConfig loads mode, issuer set, maximum_assertion_age, and max_connection_lifetime from env. Enforce mode with missing required config fails the process at startup (fail-closed). Missing JWKS snapshot at startup yields 503 until a snapshot lands — relay availability is never hostage to IdP availability. [FI-TRACE-DEPENDENCY-FAIL-CLOSED]
• Add crates/buzz-relay/src/main.rs JWKS warm + background refresh: ProductionJwksSource::get_snapshot() on startup (triggers initial fetch) and a background loop at min(refresh_interval_seconds). Network error at startup is warn-only (fail-open to allow deployment); background errors are warn + retry.
• Add NIP-42 key pairing in handlers/auth.rs: when conn.nip_fi_assertion is present and carries an asserted_key, the proven NIP-42 pubkey must match exactly. Mismatch → restricted: authorization denied + connection cancel. Unconditional check — no flag reads. [FI-TRACE-ASSERTION-KEY-MISMATCH]
• Add three-term session deadline in connection.rs: session_deadline = min(connection_time + max_connection_lifetime, min(authority_deadlines)) where authority_deadlines includes exp, iat + maximum_assertion_age, and key_snapshot_hard_deadline. Expiry fires the exact Nostr lease-expiry text and cancels the connection. [FI-TRACE-LEASE-BOUND]
• Add NIP-11 federated_identity discovery field in nip11.rs per [FI-TRACE-DISCOVERY-PRIVATE]; never exposes issuer URLs, audiences, or enrollment details.

Scope note — HTTP data-plane enforcement deliberately deferred to S5. Bridge (/bridge), Git LFS (/git), and media (/media) HTTP endpoints are not gated in this PR. Enforcement for those surfaces is scoped to S5 per Will's explicit ruling; the corresponding NIP-FI spec amendment is merged in #7254. WebSocket connections (root relay and audio) are fully gated here.

Targeted test evidence: 34/34 NIP-FI tests pass — denial matrix byte-exact, terminal-denial channel saturated-queue tests (B3), upgrade-gate non-WS passthrough tests (B4), three-term deadline bounds, DenyProtected 503 pin, config fail-closed.

Relates to #7214

[github-actions]

🔐 Codex Security Review

Status: review required for the current range.

The current range is 88687876f7808a2fd742b7eb2e4b9f87d999ad8d...9f0acac193f1561983796c7de350a1b1efe2a8e1.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 9f0acac193f1561983796c7de350a1b1efe2a8e1 to authorize a new review.
Any previous review applies only to its recorded range.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested

Reviewed exact head cbd0ded50af716f6a7c071a940e7394ffc761b77 against base 04babf02655440b4dfd37f2e2df605ead0a030d8. Source/metadata only: no checkout, build, tests, or PR-code execution. Independent configuration/JWKS, session/auth, and audio/discovery lanes are integrated.

The delivery contract is stateless S3 WebSocket enforcement: fail-closed upgrade, matching NIP-42 proof, bounded session authority, and private discovery, while preserving ordinary document serving. HTTP bridge enforcement and admin deny/boot belong to the separately planned S5/S4 work and are not blockers here.

1. P1: Install audio expiry enforcement before authentication and admission

Changed timer placement: audio/handler.rs:824–837

Upgrade with a valid assertion expiring in one second, then send matching-key AUTH after two seconds, still inside the five-second AUTH window. No FI timer is running during AUTH or setup. Pairing checks only the key, and calculating an expired deadline does not reject it (302–326). The connection proceeds through membership auto-add, room admission, roster publication and the awaited participant-joined event before installing the timer. The auto-add and event paths can persist changes (1424–1445, 1523–1526). A slow setup dependency extends this interval.

Enforce the deadline across community bootstrap, AUTH and setup, stopping expired admission and safely cleaning partial setup. Test the real handler with expiry during both AUTH and an admission dependency; assert closure and no new post-expiry membership/room/join side effects. The helper/writer expiry test does not exercise this placement. This violates NIP-FI’s equality-is-expired and maximum-session-bound contract.

2. P2: Fence root frame admission after expiry, not just the socket writer

Expiry integration: connection.rs:373–395; receive/dispatch: 589–610

Expiry cancels the token but leaves AuthState::Authenticated intact. If a buffered EVENT/REQ and cancellation are both ready, the unbiased receive select! can choose the frame and invoke handle_text_message. Neither dispatch, quota admission nor the handlers reject an expired/cancelled session, so it starts a new authorized handler after expiry. This can occur before the writer processes Close; it is not a request to roll back work admitted before the deadline. An assertion that expires during bootstrap likewise has no synchronous deadline guard before AUTH.

Check deadline/cancellation at the actual admission boundary, including AUTH, and prevent late async admission from publishing authenticated state. Add deterministic cancelled-plus-ready-frame and already-expired-bootstrap cases. Keep the fix scoped to stopping new admission, not a general handler-lifecycle rewrite.

3. P2: Preserve the terminal denial when the control queue is full

nip_fi_session.rs:183–195; root pairing: 107–111

Both expiry routes and root pairing discard try_send errors before cancelling. With the capacity-eight control queue full while the writer is temporarily backpressured, the denial is lost. When the writer recovers it drains the old frames and sends Close, but never the required restricted: authorization denied frame. The cancellation drain cannot recover a frame that was never queued.

Preserve the terminal reason independently of ordinary queue capacity, without making expiry depend on an unbounded send. Cover saturated queues for root pairing and both expiry routes, then release the writer and assert fixed denial followed by Close. Current tests only use available capacity.

4. P2: Do not apply the WebSocket gate to ordinary root document requests

router.rs:337–350

With enforce mode, a mapped non-admin host, Git web GUI enabled and a readable bundle, normal browser GET / with Accept: text/html now returns 401 before reaching the existing SPA fallback (404–415). Plain fallback NIP-11 requests also become 401; only explicit application/nostr+json is exempt. This is an unintended document-serving regression, not required WebSocket protection.

Separate non-upgrade document handling from upgrade admission. Test ordinary HTML and fallback JSON requests, while ensuring a genuine upgrade with HTML Accept remains gated. Do not create an Accept-header authentication bypass.

5. P2: Isolate the router fixture’s process-global environment access

router.rs:1437–1444

The new fixture removes NIP-FI variables without the lock used by nip_fi_config tests in the same test binary. One concrete interleaving: enforce_without_issuers_fails_closed sets mode to enforce; this fixture removes it; the config test reads Off and its expect_err fails. The reverse interleaving can make this fixture’s Config::from_env().expect(...) fail. Default parallel test execution therefore has a new order-dependent failure path.

Use a shared lock for the mutation/read window in all affected tests, or construct the fixture without process environment. Preserve the existing parallel suite behavior rather than relying on serial test flags.

Startup validation, per-issuer JWKS refresh/snapshot bounds, unconditional key pairing, and static private discovery otherwise hold in the inspected source. DenyProtected’s 503 implementation disagrees with its 403-oriented function documentation; its startup type describes misconfiguration-repair mode, so I am not treating that ambiguity as an additional blocker. Align and pin that contract separately.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested

Reviewed head 7d87ab3ac0b623522b2532c260c6a08a4945b57e against base 187df22252fa24cce2f3295fb9df9f4dc211b30a, including the changes since the previous reviewed head. The terminal-denial queue saturation and ordinary-document interception findings are addressed. Three prior findings remain, and the router reordering introduces one test-path regression.

P1: Audio still begins room admission after the session deadline

The new pairing-time check covers an assertion already expired when AUTH finishes, but the expiry task is still installed only after admission and lifecycle publication.

Concrete source-derived interleaving, with mesh disabled and an otherwise authorized member: pairing passes just before deadline D; the pre-join channel read returns after D; room.add_peer then begins a new admission, followed by joined/participant publication, without another deadline check. The expired connection enters the room before the timer can close it. This is new work admitted after expiry, not a request to roll back work admitted before expiry.

Enforce the effective deadline throughout AUTH/setup and fence room admission after awaited dependencies. Keep cleanup ownership for any already-acquired room/remote/lease resources rather than simply dropping the whole handler on timeout. Add a regression with a successful setup dependency deliberately spanning D; the new already-expired-at-pairing test does not cover it.

P2: Root dispatch still admits buffered requests after expiry

The added AUTH-finalization cancellation check does not protect an already-authenticated connection. recv_loop still makes an unbiased choice between a ready buffered frame and cancellation. Once the expiry task has cancelled the token, it may select a new EVENT/REQ; dispatch has no deadline/cancellation fence, and the quota gate and handlers still accept the unchanged Authenticated state. A valid event can therefore reach persistence after being newly admitted on an expired session.

Check cancellation and the absolute deadline at the root admission boundary, including AUTH before side-effecting gates; do not rely only on a separately scheduled timer. Cover cancelled-plus-buffered EVENT/REQ and immediately-ready expired AUTH through the production boundary. The current B2 test explicitly fails earlier at its lazy-DB ban lookup, so removing the new guard would not falsify it.

P2: The two ENV_LOCK statics do not serialize the shared environment

router.rs declares a new module-local mutex, distinct from nip_fi_config.rs's mutex. The comment calling them shared does not make them the same lock. For example, enforce_without_issuers_fails_closed can set MODE=enforce, the router fixture can remove it under its different mutex, and the config test then reads Off and fails expect_err. The reverse interleaving can make router configuration fail.

Remove process-environment mutation from these fixtures, or use one shared synchronization mechanism across the affected reads and writes. This remains a parallel-suite race, not a process-metadata concern.

P2: Root router tests now hit the unseeded database before their asserted gate

The gate moved below bind_community, but nip_fi_enforce_state still creates a lazy DB pool without a relay.example community fixture. On the intended no-infrastructure setup, or a database without that host, binding returns the generic 404 before NIP-FI or the document fallback is reached. The existing root missing-assertion/no-verifier tests expect 401/503, while the added plain-GET and HTML-Accept upgrade tests expect 200/401. These fail independently of the environment race and do not exercise their advertised seams.

Keep the genuine-upgrade gate before the tenant lookup where appropriate, or supply a controlled successful host-resolution fixture for tests that need the document/upgrade path. Do not change the assertions to accept the early 404: that would stop testing the fixes.

Scope and validation

Source/metadata-only review; no checkout, build, test, import, or PR-code execution. The interleavings above are source-derived, not executed reproductions. Applied exact-base product, architecture, AGENTS and TESTING guidance. HTTP data-plane enforcement remains S5; administrative disconnect/deny-set remains S4. Neither is a blocker for this S3 review. Dedicated terminal queues address ordinary-control saturation; document requests now bypass the FI gate, subject to the existing tenant boundary. Closeout requires the four concrete issues above, not a general lifecycle rewrite or rollback of previously admitted work.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested

Reviewed exact head 23f43850459156c3e180d79c3235810cf6b3d892 against base 187df22252fa24cce2f3295fb9df9f4dc211b30a, including the delta from previously reviewed 7d87ab3a. The new root cancellation check addresses the previously reported cancelled-plus-buffered-frame case, and audio now arms expiry before post-pairing admission. However, the new header condition introduces an assertion bypass, and the new audio cancellation return skips committed-join cleanup. Pending-auth lifetime and two fixture defects remain.

1. P1: Apply FI to every request the WebSocket extractor can accept

router.rs:381–403

A direct HTTP/1.1 GET / to a mapped, active host with ordinary WebSocket key/version headers, Upgrade: websocket, Connection: xupgrade, and no FI assertion skips this gate: the new token-equality test is false, so nip_fi_assertion becomes None. But pinned Axum 0.8.9 accepts Connection by substring, not token equality. Its extractor accepts this value; pinned Hyper 1.9.0 creates upgrade intent from the Upgrade header, and Axum constructs the WebSocket without another handshake validation.

Thus this reaches handle_connection with no assertion. Pairing treats None as not applicable, and no FI deadline is armed. A key satisfying ordinary NIP-42/local policy can establish a session without federated evidence in both Enforce and DenyProtected modes. This is an enforcement bypass, not merely invalid RFC syntax.

Do not use a narrower predicate to skip authorization than the actual upgrader uses. Gate every successful extraction before returning its upgrade response, or reject malformed candidates rather than passing them to a more permissive extractor. Preserve ordinary HTML/NIP-11 document behavior. Add a real upgrade regression with Connection: xupgrade in both modes; it must never reach 101 without the required FI decision.

2. P2: Finish teardown after a join has already been published

audio/handler.rs:875–900

On the local-owner path, the handler broadcasts joined, then awaits the persisted/fanned-out PARTICIPANT_JOINED event. If expiry fires during that await, the new check_cancel!(cleanup: ...) removes the peer and returns. It bypasses the normal left broadcast, PARTICIPANT_LEFT event, last-peer archive/end event, and owner release at 1066–1169.

Room::remove_peer publishes an internal roster delta, not the JSON left sent to same-pod clients. Existing peers and durable huddle history therefore retain a participant that has disconnected; a sole-peer huddle also skips normal auto-end. Route post-published-join cancellation through the complete teardown, retaining generation/ownership fences. Pause lifecycle emission across expiry in a production-path regression and assert join/leave symmetry and last-peer cleanup. The added mid-admission test pre-cancels before AUTH and does not reach this boundary.

3. P2: Cover pending authentication with the session deadline

audio/handler.rs:320–356

The timer is now before room admission, but still after the challenge write, five-second AUTH wait, and NIP-42 verification (231–290). Upgrade with a valid assertion having one second remaining and withhold AUTH: the socket survives until the independent five-second timeout, then drops without the FI expiry denial because no expiry task was created. The community-active await before this handler has the same unbounded pre-timer gap on both WebSocket routes.

NIP-FI Session policy requires termination at the earliest effective deadline, including a short configured maximum connection lifetime. Enforce that bound from upgrade through bootstrap and pending AUTH, while preserving cleanup ownership. Add a near-expiry/no-AUTH case, not just an already-expired assertion checked after successful pairing. This is the remaining lifetime portion of the earlier finding; the specific post-pairing room-entry race is addressed.

4. P2: Remove ambient NIP-FI reads from the router fixture

router.rs:1468–1477

Removing the fixture's environment mutations does not isolate its reads. Config::from_env().expect(...) still invokes fallible NipFiRelayConfig::from_env() (config.rs:1268) before the explicit override. In parallel, nip_fi_config tests set MODE=enforce without issuers or MODE=permissive under their private mutex (386–389, 418–419). The router reader takes neither lock, so it can panic before constructing the fixture. Use an environment-independent constructor or genuinely shared synchronization covering readers and writers. The current comment claiming these reads are irrelevant is incorrect.

5. P2: Make the root gate tests reach the intended route boundary

router.rs:1484–1514

The fixture still creates a lazy DB pool without seeding relay.example. Root binding at 344–355 precedes FI, so a missing/unavailable database or missing host returns 404 before the gate or document fallback. The tests at 1550, 1574, 1647, and 1674 expect 401/503/200/401 respectively. The new single-header negative tests can instead pass vacuously on that same 404. Explicit NIP-11 Accept negotiation and audio have different ordering and are not affected by this particular failure.

Provide controlled successful host resolution through the production route seam, or a real seeded tenant. Do not weaken expectations to accept 404. Verify both positive denial/document cases and malformed-header cases actually reach their intended decision. This remains independently broken even after fixing the environment race.

6. P2: Use the canonical FI denial for local-policy rejection

Root pairing integration, audio pairing integration

After a valid assertion and matching NIP-42 proof, root still returns ban-specific blocked: you are banned from this community (auth.rs:175–200) or restricted: not a relay member (245–255). Audio likewise distinguishes relay membership from channel membership (handler.rs:406–449). The new FI path therefore exposes which private local policy rejected the same supplied evidence, rather than the fixed restricted: authorization denied required by NIP-FI's rejection table (623–634).

Keep legacy responses when FI is off, but route FI local-policy admission failures through the canonical denial path. Test matching-key policy denials against the same public text/frame contract used for pairing mismatch. This is a concrete gap in the new FI integration, not a request to change off-mode UX.

Scope, evidence, and exit criteria

Source/metadata only: immutable blob verification, exact dependency-source inspection, and three independent lanes integrated. No checkout, build, tests, or PR code executed. Scenarios above are source-derived, not runtime reproductions. JWKS/config/verifier wiring and private NIP-11 capability shape were reviewed; HTTP data-plane enforcement remains S5 and issuer disconnect remains S4.

The exit criteria are the six bounded issues above. Existing detached-handler cleanup debt is not a new authority-bypass finding, and already-admitted work need not be rolled back. The dedicated terminal queue addresses the previously agreed ordinary-queue saturation case. Separately, a stalled socket writer can still delay shutdown; that broader existing lifecycle limitation is noted as follow-up rather than expanding that fix into another blocker here. No general handler-lifecycle rewrite is requested.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested

Reviewed exact head acaa6e9a1395752850638faade23f6bdce0972cf against base c328202cb08cac5b8c1610d2d373e69889772b4e, concentrating on the corrective delta from 23f43850 and the six previously agreed exit criteria. The successful 48101-commit path now reaches normal teardown, and the root missing/unavailable-assertion tests now reach the FI gate before tenant binding. Those fixes are credited below; the remaining defects are not requests for a general lifecycle rewrite.

1. P1: The root WebSocket assertion bypass remains

router.rs:357–384

A direct HTTP/1.1 GET / to a mapped, active host with valid WebSocket key/version headers, Upgrade: websocket, Connection: xupgrade, and no FI assertion still skips this gate. Its exact-token predicate is false, so the callback receives nip_fi_assertion = None. Pinned Axum 0.8.9 accepts Connection by substring; Hyper 1.9.0 creates upgrade intent from the Upgrade header. Successful extraction at router.rs:413 therefore still returns an upgrade. Pairing treats None as not applicable, and no FI lifetime is armed. A key passing ordinary NIP-42/local policy obtains an assertion-free session in Enforce and DenyProtected.

Gate every successfully extracted upgrade, or reject candidates that the narrower predicate declines instead of passing them to the more permissive extractor. Preserve ordinary HTML/NIP-11 serving. Add a real upgrade-path Connection: xupgrade regression in both modes; the existing helper only supplies Connection: Upgrade and does not witness this case.

2. P2: Complete cleanup when the JSON join is published but the transaction is rejected

audio/handler.rs:859–921, Db-error branch:923–944

The successfully committed PARTICIPANT_JOINED case from the last review is fixed: even if expiry occurs during its commit/fan-out, execution proceeds to normal teardown. The remaining case is before commit: local peers receive JSON joined at line 872, then the DB/lock await crosses the FI deadline and acquire_effect() rejects (or the transaction fails). Both error branches remove the peer and return without the matching JSON left or last-peer owner cleanup.

Room::remove_peer emits only the internal roster delta (room.rs:407–425), not PeerCtrl::Json. The same-pod desktop consumer inserts peer identity on joined and removes it on left (playout.rs:542–639); the missing leave leaves stale routing state. The early return also skips normal last-peer archive/end and fenced owner release (handler.rs:1115–1218).

Ensure already-published roster effects have corresponding cleanup, or defer publication until a successful admission can own normal teardown. Keep generation/ownership fences. Do not manufacture a 48102 for an uncommitted 48101, and do not roll back legitimately pre-admitted commits. Add a production-path witness that pauses after JSON publication but before commit across expiry, plus a commit-error case. The audio barriers in nip_fi_test_hooks.rs:77–88 are not called by this handler; the pre-cancelled-before-AUTH test does not exercise the transaction boundary.

3. P2: The FI deadline still does not cover bootstrap and pending audio AUTH

audio/handler.rs:238–354

The new gate is still created only after the challenge write, five-second AUTH wait, verification, and pairing. Upgrade with an otherwise valid assertion having one second remaining and withhold AUTH: the socket survives until the independent five-second AUTH timeout, then exits without the FI expiry denial because no FI timer was created. Before either active handler, the community-active await remains outside the timer as well (root:215–234, audio:190–208).

Enforce the existing earliest-deadline contract from upgrade through bootstrap and pending authentication, not merely after pairing. Add a near-expiry/no-AUTH case and a delayed-bootstrap case; the already-expired-at-successful-pairing test does not cover either. This does not reopen rollback of work admitted before expiry.

4. P2: The router fixture still races ambient NIP-FI configuration

router.rs:1480–1494

Config::from_env().expect(...) still calls fallible NipFiRelayConfig::from_env() at config.rs:1268 before the fixture override. Concurrent tests set BUZZ_NIP_FI_MODE=enforce without required issuers or set the invalid permissive value under their private mutex (nip_fi_config.rs:334, 381–419). This reader shares neither lock and can panic before constructing the fixture. Removing writes from this fixture did not remove its reads.

Use an environment-independent fixture constructor, or shared synchronization covering these readers and writers. The comment saying the FI env read is irrelevant is not true of fallible construction.

5. P2: The plain-GET witness still fails before reaching the document fallback

router.rs:1643–1675

Moving the FI gate before tenant binding fixes the root WS-denial tests. It does not make the plain-GET test DB-free: no Accept header means it bypasses the early NIP-11 response, no upgrade headers means it skips FI, and unconditional bind_community at line 397 still precedes fallback line 455. The fixture supplies an unseeded lazy pool and relay.example; missing host/DB returns 404, while nip_fi_enforce_plain_get_serves_nip11_not_401 requires 200. The single-header negative tests can still pass on this unrelated 404.

Provide controlled successful host resolution or a seeded tenant for these fallback witnesses, retaining the no-Accept plain-GET case. Do not replace it with the already-covered explicit NIP-11 Accept shortcut or weaken the assertions to accept 404. Verify malformed-header/document cases reach their intended boundary.

6. P2: Matching-key FI local-policy denials still reveal private policy distinctions

auth.rs:175–200, auth.rs:245–255, audio/handler.rs:404–447

With valid FI evidence and matching NIP-42 proof, root still sends blocked: you are banned from this community versus restricted: not a relay member; audio distinguishes restricted: not a relay member from not a member. These responses disclose which private local policy rejected the same supplied evidence instead of NIP-FI's fixed restricted: authorization denied.

Keep legacy off-mode UX, but route FI local-policy rejections through the canonical public denial contract. Add matching-key policy-denial witnesses, not only pairing-mismatch tests. The new effect-gate rejections also introduce noncanonical restricted: session expired (event.rs:742–746,789–793, req.rs:280, count.rs:112). A handler that observes the deadline before the timer runs can enqueue that text; reuse the established canonical FI expiry denial rather than adding a fifth public string. This is part of the same denial-contract criterion.

Evidence, scope, and exit criteria

Source/metadata review only, with exact-head blob verification, pinned dependency-source inspection, and independent router, root-handler, and audio evidence integrated. Carl independently checked the root-handler paths, including the bootstrap await omitted from the root lane’s initial lifetime assessment. No checkout, build, tests, or PR code executed. Scenarios above are source-derived, not runtime reproductions; the PR's scoped-test claims are not verification of this exact head.

Reviewed the corrective admission gate and caller integration, membership/event transaction, root/audio startup and terminal paths, policy denial branches, document routing/fixtures, and the existing verifier/config/JWKS/discovery wiring. HTTP data-plane enforcement remains S5; issuer disconnect/deny-set remains S4. Previously addressed ordinary-queue saturation and cancelled-buffered-frame cases are not reopened. Ordinary-disconnect detached-handler debt and a stalled socket writer remain follow-ups, not new blockers here.

The exit criteria remain these six bounded issues, narrowed where the corrective changes worked. No broad handler-lifecycle rewrite or rollback of effects legitimately admitted before the deadline is requested.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested

Reviewed head d49538715672e719796413acf2356eaca91aec26 against base c328202cb08cac5b8c1610d2d373e69889772b4e, reconciling the previous published review at acaa6e9a1395752850638faade23f6bdce0972cf and the source review of 0f17f23095076a17a5d08b54fc4e53df6ef38d70 that was withheld when the head moved. The latest commit adds four audio witnesses and test-only hooks; release-build enforcement is unchanged. Real handler barriers, post-commit direct joined publication, and improved witness coverage are credited below. Six prior contract findings remain; the postcommit remote-return regression and shared DB-witness defect were introduced in the intervening corrective commits.

1. P1: Root upgrade still bypasses NIP-FI with Connection: xupgrade

router.rs:357–384, successful extraction:413–440

This file is byte-identical to the previous reviewed head. On a mapped active host, send a direct HTTP/1.1 GET / with valid WebSocket key/version, Upgrade: websocket, Connection: xupgrade, and no FI assertion. The exact-token precheck is false and leaves nip_fi_assertion=None, but pinned Axum 0.8.9 checks a substring, and Hyper 1.9.0 creates upgrade intent from Upgrade. Successful extraction still upgrades without FI in both Enforce and DenyProtected. Ordinary NIP-42/local policy can then admit an assertion-free session without pairing or an FI lifetime.

Gate every successful upgrade or reject candidates that the narrower precheck declines; preserve ordinary document serving. Add the actual xupgrade upgrade-path witness in both modes.

2. P2: The FI lifetime still excludes bootstrap and pending audio authentication

audio/handler.rs:227–358, root wrapper:221–240, shared bootstrap await:202–211

The audio FI task is still created only after challenge delivery, the independent five-second AUTH wait, verification, and pairing. Upgrade with a valid assertion having one second remaining, then withhold AUTH: no FI timer exists during that wait, so the socket survives its authority deadline and leaves through the independent AUTH timeout without the FI denial. Both root and audio also await is_community_active before entering the handler that creates the FI task. Capturing the timestamp at upgrade does not enforce it while those awaits are pending.

Start enforcing the existing earliest-deadline contract across bootstrap and pending authentication. Retain near-expiry/no-AUTH and delayed-bootstrap regression cases; this does not request rollback of effects legitimately admitted before expiry.

3. P2: Matching-key FI denials still reveal private local policy

auth.rs:175–200, membership:245–255, audio policy:408–451

A valid assertion with matching NIP-42 proof still receives blocked: you are banned from this community versus restricted: not a relay member on root; audio distinguishes relay membership from not a member. The same supplied evidence exposes which private server policy rejected it, contrary to the fixed restricted: authorization denied contract. Changed effect-gate branches also retain the extra restricted: session expired string (EVENT, REQ, COUNT), observable when the handler sees the deadline before the timer queues its canonical denial.

Preserve legacy off-mode UX, but use the established FI public denial for FI local-policy and expiry rejection. Add matching-key policy-denial cases, not only key-mismatch tests.

4. P2: The router fixture still reads the fallible process environment before overriding FI

router.rs:1476–1494, config.rs:1268, NIP-FI env-mutating tests:381–419

Config::from_env().expect(...) still calls NipFiRelayConfig::from_env() before the explicit override. Concurrent config tests set Enforce without required issuers, or the invalid permissive mode, under a mutex this reader does not take. The router fixture can panic before reaching its override. Removing fixture writes did not remove ambient reads.

Use an environment-independent constructor or synchronization shared by the actual readers and writers. The new DB-backed fixture helpers also call this fallible constructor and should not reproduce the same problem.

5. P2: The plain no-Accept GET fixture still fails before the document fallback

router.rs:1643–1675, routing:397–455

The fixture uses an unseeded lazy DB and relay.example. A request with neither Accept nor upgrade headers bypasses the early NIP-11 shortcut and FI gate, then performs host binding before the fallback. With no mapped tenant or an unavailable DB this returns 404, while the test demands 200. Single-upgrade-header negatives can likewise pass on unrelated tenant failure.

Supply controlled successful host resolution or an isolated seeded tenant for these fallback cases. Keep the no-Accept case; substituting the explicit Accept shortcut or accepting 404 would erase the intended coverage.

6. P2: Failed audio admission can still leak pending peer visibility and room ownership

registration:700–755, snapshot and failure cleanup:846–969

Prior finding 2 is partly repaired: this connection's direct joined now follows successful commit. However, pending A is already in room.peers; a concurrently successful B snapshots and broadcasts A before A commits. A's expiry/DB-error branches remove A without a correcting JSON notification. room.rs:407–425 sends an internal roster delta, not the control JSON consumed by Desktop's joined/left handling (desktop/src-tauri/src/huddle/playout.rs:542–639; Desktop also supports full roster replacement). These branches also fail to release the owner renewer attached at handler.rs:802–810 when the room empties; generation-fenced release remains in normal teardown at 1238–1241.

Keep pending peers out of published admission state, or balance exposed control state on failure, and release the emptied room's owned generation. This is not a claim that rolled-back 48101 persists. Cover concurrent pending-A/successful-B followed by A failure.

7. P2: Initial remote notification failure now abandons a committed audio join

handler.rs:942–965, committed teardown:1178–1190

The moved remote ws_send.send(joined) error return runs after 48101 is committed/fanned out, but before reader-task setup and normal teardown. It removes the local peer without the admission-ID-matched 48102. Owner-side stream-drop cleanup (audio/join.rs:1347–1359) removes and broadcasts the peer but writes no durable 48102. A remote connection lost at this initial notification therefore leaves a committed join without its corresponding leave.

Once commit wins, initial notification failure must enter committed-admission teardown. Cover this specific postcommit send failure; it is a regression from the relocated publication, not a request to solve generic stalled-writer debt.

8. P2: New real-DB witnesses bypass the isolated Postgres test lane

AUTH fixture:464–505, audio fixture:3054–3064

W1, W9/W10/reaffirm, and the new CW5/CW5-variant/CW8/CW10 witnesses hard-code the shared localhost development DB, are ordinary non-ignored tests in mod tests, and return success when the DB is unavailable (auth.rs:771–777; audio/handler.rs:3166–3171,3289–3294,3439–3444). Exact-base CONTRIBUTING.md:218–240 requires ignored postgres_tests discovery and runner-provided isolated URLs. The actual runner uses ignored-only execution (scripts/postgres-test-run.sh:81), matching module/binary filters (.config/nextest.toml:18–20, wrapper:36–41), and exports the isolated DB URLs (wrapper:87–89).

These witnesses can silently pass without exercising their boundary, miss the sanctioned lane, or touch the shared developer DB instead of the isolated database. Consolidated once across AUTH/audio: move the tests into the dedicated lane, use existing test_support::database_url(), and fail on infrastructure errors there. No claim is made here about whether an additional discovery audit independently fails CI.

Credit and witness limits

The mandatory Arc<SessionAdmissionGate> conversion preserves effect-permit checks in AUTH, EVENT (ephemeral/persistent), REQ and COUNT. Off-mode has no deadline; cancellation rejects effects only during teardown. No production regression was identified in that type change. W2–W4 now call real handler barriers before their DB seam and fail loudly if the barrier is not reached; this is source inspection, not an executed test result.

The join transaction still composes optional membership and 48101 and retains the permit through commit/fanout (audio/handler.rs:1748–1886). These are coverage dispositions, not additional production defects, and all real-DB credits depend on repairing finding 8:

• CW5 / concurrent-add: CW5 now reaches the real AutoAddRequired path and asserts both membership and 48101 are absent after cancellation at the precommit hook (3578–3715), closing the prior Existing-only limitation. The concurrent-add variant (3746–3898) reaches the correct pre-lock interleaving, but its count-only assertions cannot prove the auto-add was skipped: the helper's ON CONFLICT DO UPDATE preserves row count and can overwrite role (buzz-db/src/store/channel_members.rs:255–263). The fixture uses the same member role as auto-add. Seed a distinguishable external role and assert it survives; different invited_by alone is insufficient because that column is not updated on conflict. Keep mutation claims tied to observable assertions; no mutation execution was independently verified.
• CW8: the real-DB variant awaits the actual post-add-peer hook and cancels the handler's token (4238–4248), earning peer-removal/zero-48101 coverage. It still discards the socket-read result and permits an empty room to remain in the map (4250–4261), so it does not prove handler completion or cleanup_if_empty. Assert completion and absence of the last-peer room entry. The lazy-DB variant neither awaits hook arrival nor cancels its token and does not witness this boundary. Neither variant covers findings 6–7's later failure paths.
• CW10: the post-fanout hook is inside the real permit scope (1870–1875); the witness checks the committed row at the hook, cancellation with expiry incomplete, then both tasks' completion after release (4374–4463). Credit that commit-won/quiescence structure. The held read guard blocks expiry by construction, but ten scheduler yields are not an explicit expiry-start handshake; use bounded synchronization before asserting cancellation. No subscriber delivery or local_event_ids assertion establishes the broader fanout claim. Do not equate the source placement or unverified mutation comments with executed fanout evidence.

Scope and validation

Source/metadata review only. Exact-base product/architecture/testing guidance and NIP-FI contract read; pinned source bytes and dependency versions checked. No checkout, build, tests, imports, or PR-code execution. Reproduction scenarios are source-derived, not runtime runs. Author test or mutation claims were not independently verified.

Root/audio admission, pairing, timer origins, effect-gate caller integration, transaction/notification cleanup, router fallback and fixtures, and new witness wiring are the relevant surfaces. Unchanged verifier/JWKS/discovery wiring is not re-audited as a new feature. No new event kinds, wire fields, or public statuses are introduced by the latest test-only delta. HTTP data-plane S5, issuer disconnect/deny-set S4, generic ordinary-disconnect detached-handler debt, and stalled socket writer remain excluded. Preserve pre-admitted bounded effects; no general lifecycle rewrite is requested.

addressed