fix(members): offload deny-path SEL audits off the event loop (#8523)

[bolichen97]

Closes #8523

Problem

_deny_app_caller and the three member-pin denial sites in src/kiro_crew/dashboard/handlers/members.py called _sel().log_api_access(...) synchronously from async handlers. On a fresh gateway the first _sel() touch performs synchronous filesystem initialization — HMAC key load/create and the chain-head read — ON the event loop, stalling every gateway task (AUTOSDE no-blocking-call-on-event-loop).

Fix

Every deny-path audit in the module now offloads the whole call — the _sel() accessor INCLUDED — via await asyncio.to_thread(lambda: ...), the shape _shared.py's non-owner denial and autonudge.py's authorization audit already use, and each site is wrapped in the same except Exception guard those precedents carry ("audit must never change the outcome"): an executor shut down mid-teardown or an audit write failure degrades to a debug log, never to a 500 replacing the 404/409. The helper's fast allow path (no app token) stays on-loop with no thread hop. _deny_app_caller became async; its three call sites await it. No status code, body, or ordering of any response changed: every new await sits immediately before its return, with no state read across the suspension point.

Tests

• TestDenialAuditOffload::test_app_denial_audit_runs_off_the_event_loop — thread-ident assertion on the app-token denial (helper site), plus kwargs contract.
• TestDenialAuditOffload::test_member_pin_denial_audit_runs_off_the_event_loop — same for the pin-mismatch denial (409 path), and asserts no slot was minted.
• TestDenialAuditOffload::test_first_sel_touch_initializes_off_the_event_loop — a real SecurityEventLog first touch (HMAC key create verified on disk) driven through the deny path, recording the thread _init_locked runs on; catches the accessor being hoisted out of the lambda. Displaces then RESTORES the prior process singleton per the sel_private_root convention (the fixture itself would pre-consume the first touch).
• TestDenialAuditOffload::test_every_members_sel_audit_is_offloaded — AST guard: every log_api_access call in the module, regardless of receiver spelling, must sit inside an asyncio.to_thread lambda.

Mutation-verified: reverting each of the four sites to a sync call, and hoisting the accessor out of the lambda, are each caught by a distinct test. Local gates green (isort 6.0.0 / flake8 7.1.0 / mypy 1.14.1 / black 26.3.1 CI pins, black-baseline, brand, subprocess-encoding, harness parity). Full pytest: 86341 passed; the 89 failures reproduce identically on clean main (host-environment: /local/home uid ownership, AF_UNIX path length, userns EPERM) — none touch this diff.

Pre-push review lanes: GPT (gpt-5.6-sol) 1 BLOCKING (singleton restore-to-None in the first-touch test) and Opus (claude-opus-5) 1 BLOCKING + 3 CONCERNS (black gate offender, same singleton restore, missing audit exception guard vs the three in-repo precedents, AST guard pinning the spelling rather than the operation) — all five verified real on-source and fixed in this commit.

Pattern harvest

Class: deny-path audit added before the offload convention landed keeps running on-loop while new success-path audits in sibling files offload.

Rule candidate: any log_api_access reachable from an async handler must go through asyncio.to_thread/run_in_executor with the SEL accessor inside the offloaded callable, wrapped so an audit failure never changes the response.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 61ca6e37baa891f2bdd04ddbc77aa44180ffe8c2 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Copies the repo's established offload-with-guard audit shape onto four missed deny sites; convention-consistent, minimal, and regression-pinned by an AST ratchet.

Suggestions

• Four near-identical 10-line to_thread + except Exception blocks now live in one module; a small module-local async def _audit_denied(**kwargs) helper would dedupe them and make the AST guard's job trivial for future sites.
• The AST guard ratchets only members.py while the PR's own harvest names a repo-wide class ("any log_api_access reachable from an async handler"); promote the guard to a shared/CI-level check in a follow-up so the next pre-convention module doesn't need its own copy.

[DESIGN-REVIEWED] 61ca6e3

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 61ca6e37baa891f2bdd04ddbc77aa44180ffe8c2 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 61ca6e3

Verdict parsed from the review's SHA-scoped output markers for commit 61ca6e37baa891f2bdd04ddbc77aa44180ffe8c2.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 61ca6e37baa891f2bdd04ddbc77aa44180ffe8c2: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 61ca6e37baa891f2bdd04ddbc77aa44180ffe8c2 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 61ca6e3

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 61ca6e37baa891f2bdd04ddbc77aa44180ffe8c2: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 61ca6e37baa891f2bdd04ddbc77aa44180ffe8c2 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All the evidence is in. The fix matches verified in-repo precedents (_shared.py:1921, autonudge.py:133, handlers/session_control.py:56), satisfies the AUTOSDE no-blocking-call-on-event-loop rule, adds no new public surface, and the boot-time prewarm alternative is explicitly forbidden by another recorded rule (no-new-work-on-gateway-boot-path, per dashboard/session_control.py:1653). Final review:

First-Principles-Verdict: PASS

Every item is the fix or its guard, matches a recorded repo convention, and adds zero public surface — the only excess is three byte-identical inline blocks.

What this change ships

Intent: stop deny-path audits in the members endpoints from freezing the gateway on a fresh start — a FIX.

1. App-token denials on member endpoints no longer stall every gateway task — justified (members.py denial audits initialize SEL synchronously on the event loop #8523, AUTOSDE no-blocking-call-on-event-loop)
2. Three pin-mismatch/orphan-history denials likewise audit off-loop — justified
3. An audit-write failure now yields the 404/409 instead of a 500 — declared; derived from the offload's own executor-shutdown failure mode, matching _shared.py:1930 and autonudge.py precedents
4. AST ratchet test pinning every members.py audit as offloaded — guards the fix
5. Thread-ident and real-first-touch tests — the fix's proof

No config key, flag, or exported symbol is added; _deny_app_caller stays module-private (3 callers, all updated).

Watch

The declared "Pattern harvest" understates the scale: grepping log_api_access under dashboard/handlers/ alone returns 250+ sites and only ~13 sit inside a to_thread lambda (members ×4 new, _shared, secrets, session_control, connections ×5, messaging ×1) — hooks.py ~30, files.py ~50, messaging.py ~35, core.py ~25 remain on-loop. Accepted-and-deferred, but the rule candidate is load-bearing, not optional.

Subtractions

Fold the three member-pin blocks in members.py (lines 285–299, 323–339, 391–406) into one module-local async def _audit_pin_denial(request, slug, error) — they are identical except the error= string, and a local _audit helper is already the repo's shape (autonudge.py:125, tailnet_mobile.py:200); ~45 lines become ~20 and the AST guard still passes.

[FIRST-PRINCIPLES-REVIEWED] 61ca6e3

[bolichen97]

Disposition — ACCEPT-AND-DEFER (advisory CONCERNS; the cause-level fix is extracted to issue #8608).

• "Copies 15–18 of a per-site patch whose one-line cause-level fix in sel.py/startup remains unbuilt" — granted on the mechanism, and the named subtraction is now tracked rather than argued away.

The premise holds as stated: _init_locked runs on whichever thread first calls sel() (sel.py:536), and once initialized log_api_access only enqueues onto an unbounded queue so that "callers never block" (sel.py:367). Warming the singleton once, off-loop, at startup therefore subtracts all 18 wrappers instead of adding a 19th, and #8608 proposes exactly that subtraction: await asyncio.to_thread(sel) before state.ready = True (server.py:4113), then delete every to_thread(lambda: … log_api_access …) wrapper including this PR's four, and migrate the #8523 first-touch test to pin the startup warm instead of the per-site spelling.

What this PR does not do, deliberately, is make that startup change here. It fixes the four sites #8523 names, in the shape the module's 14 in-repo precedents already use; moving SEL's first-touch init onto the gateway boot path is precisely what the AUTOSDE no-new-work-on-gateway-boot-path rule makes a decision worth its own review, so it lands as #8608 rather than widening a fix whose scope issue #8523 already fixed. The description's Pattern harvest rule candidate is superseded by #8608's cause-level rule: warm once at startup, enqueue everywhere after.

Defer further per-site hops: move SEL's first-touch init off the caller's thread — an await asyncio.to_thread(sel) before state.ready = True (server.py:4113; fire-and-forget warm precedent at server.py:3133) or _init_locked inside the sel-writer thread — then delete all 18 asyncio.to_thread(lambda: … log_api_access …) wrappers, including these four.