fix: audit safety-override renew fail-closed before commit (#2453)

[bolichen97]

Summary

SafetyOverride.renew() extended a YOLO grant's deadline first and wrote its safety_override:renew SEL event afterwards, non-critically — a failed audit write was swallowed and auto-approval stayed extended with no record. Every other path in the file that creates auto-approval authority (_commit_activation, activate_scoped) audits fail-closed BEFORE committing.

This brings renew() onto the same pattern:

• Eligibility decision stays under _lock (no mutation): permanent-grant and denied early paths are byte-for-byte behavior-preserved.
• Audit before commit, outside the lock: the renewed SEL event is written with critical=True; on failure renew() logs at error level and returns RenewResult(renewed=False, ttl=0, reason="audit_failed") with the deadline unmoved. (No call site or frontend matches reason strings — both Slack call sites branch only on .renewed.)
• Post-audit re-verify: the SEL write runs with the lock released, so renew() re-acquires _lock and re-derives the full live-or-in-grace predicate before committing. A concurrent deactivate() (which zeroes _expires_at) during the audit window refuses the commit — no resurrection. A refused commit emits a corrective denied / reason:not_active_at_commit event so an auditor never reads a renewal that didn't take effect.
• Grace-window semantics, the TTL clamp, and the pre-lock current_adhoc_duration() resolution are unchanged.

Tests (mutation-verified)

• test_sel_crash_refuses_renew_and_leaves_deadline_unmoved — patches the SINK (log_api_access raises, not _log_sel, which would pass vacuously regardless of the critical flag): asserts not-renewed AND deadline unmoved. Fails on unfixed code.
• test_renew_does_not_resurrect_grant_deactivated_during_audit — sink side effect calls deactivate() during the audit window: asserts no resurrection. Fails on unfixed code.
• test_renew_extends_deadline_with_exactly_one_renewed_event — happy-path pin.
• Existing permanent-grant, grace-window, and denied-path tests pass unchanged.

Verification

• isort / flake8 / mypy clean on changed files (the token_auth.py isort failure is a base-branch break, fix already open as style: restore import order in dashboard token_auth (unblocks all PRs) #2478 — will rebase once it lands).
• Full pytest: 40201 passed; the 77 failures are environment-baseline and reproduce identically with the fix reverted (same 20/1549 subset result with and without the change).
• Docs updated in the same commit (security-deep-dive.md, modules/security.md).

Residual notes for reviewers

• The critical SEL write is synchronous by design — it mirrors the post-pentest activate() pattern exactly (same sink, same flag); renewals are rare, human-triggered events.
• On audit_failed, the Slack handlers' failure text still says "not active" (they branch only on .renewed). Fail-closed and out of this issue's scope; happy to file a follow-up for distinguished messaging.

Coordination: scoped strictly to renew() — does not touch deactivate() (#2475) nor pre-empt renew_lease() (#2443).

Closes #2453

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 6995500be09a8d3a762968330614f1895e1ed6ec — updated in place on each push; does not block merge.

Design-Verdict: PASS

Real audit gap, closed with the same fail-closed write-ahead pattern the file already uses for activation; the lock-release re-verify races are reasoned through and mutation-tested.

[DESIGN-REVIEWED] 6995500

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @bolichen97 overrides the GPT 5.6 finding for 6995500be09a8d3a762968330614f1895e1ed6ec; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 6995500be09a8d3a762968330614f1895e1ed6ec — this comment is updated in place on each push.

Review details

I've reviewed the full renew() logic, both Slack call sites, the SEL helper, and the AUTOSDE rules. Let me verify one concurrency concern about the refused-commit corrective SEL write not being a new blocking call on the loop, and confirm the happy path.

The logic is sound: audit-before-commit with a lock-released window handled by re-verification (count snapshot + arm-held check), fail-closed on audit failure, and both Slack sites correctly offload the now-synchronous critical SEL write via asyncio.to_thread (both files import asyncio). The normal path (active grant → audit → still-active → commit) works; refusals only occur under genuine concurrent mutation or audit failure, all in the safe direction. No non-test caller runs renew() unwrapped on the loop.

No findings.

[OPUS-REVIEWED] 6995500

Verdict parsed from the review's SHA-scoped output markers for commit 6995500be09a8d3a762968330614f1895e1ed6ec.

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

[bolichen97]

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97]

Both GPT 5.6 findings addressed in 7390c61:

Blocker 2 (stale renewal overwrites newer activation) — fixed as suggested. renew() now snapshots _activation_count under the first lock; every activation bumps the count, so an unchanged count proves no new grant was installed while the audit ran unlocked. On mismatch the commit is refused (reason:superseded_by_activation in the corrective SEL event) and the fresh activation keeps its own deadline. New mutation-verified test test_renew_does_not_overwrite_activation_landed_during_audit fails with the guard removed.

Blocker 1 (sync SEL I/O on the event loop) — fixed by offloading, not by reverting. Reverting the critical audit would reintroduce the exact vulnerability this PR closes (#2453). Instead the two async call sites (slack/handler.py, slack/events.py) now call renew() via asyncio.to_thread, the same pattern dashboard/chat_handlers.py already uses for the equally-synchronous activate() critical audit. The pre-existing synchronous activate() calls in the Slack handlers predate this PR and are unchanged.

Backend lint failure is the base-branch token_auth.py isort break (fix open at #2478); will rebase when it lands.

[bolichen97]

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97]

GPT round-2 blocker (false renewal audit on concurrent permanent activation) fixed in a15db9a, as suggested — and one better: the early permanent return in the re-verify block is deleted outright rather than rerouted. _permanent can only become true via _commit_activation, which always bumps _activation_count, so the activation-count snapshot guard already subsumes that case: the commit is refused through the corrective-event path (reason:superseded_by_activation), the permanent grant is left untouched, and the SEL reads renewed → denied instead of a false completed renewal. New test test_renew_refuses_when_permanent_activation_lands_during_audit pins the exact event sequence.

Also rebased onto main now that the base isort break is fixed (#2479).

[bolichen97]

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97]

GPT round-3 blocker (grace recheck can undo an explicit off) fixed in 6995500, exactly as proposed: the re-verify now pins the commit to the ARM the renewal began on. A renewal that began ACTIVE requires still_active at commit — it may not slide into the grace arm, because a lapsed grant's intact deadline cannot distinguish natural expiry from an operator's explicit yolo off (deactivate() early-returns on a lapsed grant, the #2475 interaction called out in the issue). A renewal that began IN GRACE may still commit from grace, preserving main's semantics for that path. New mutation-verified test test_renew_begun_active_refuses_grace_commit_after_midaudit_off reproduces the exact scenario (mid-audit lapse + explicit off) and fails against the previous or-grace predicate.

[bolichen97]

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97]

GPT round-4 finding is rebutted as a pre-existing-behavior false positive, with evidence:

1. The scenario is main's existing semantics, not a window this PR opened. On main, renew() holds one lock across eligibility+commit — and the exact same sequence (expired grant → operator off → renew within grace) commits there too, because deactivate() on a lapsed grant early-returns without touching any state renew reads (_active already False, _expires_at left intact). The revocation is a no-op at every interleaving; single-locking the audit cannot change the outcome.

2. The root cause is deactivate()'s early return on lapsed grants — sibling issue SafetyOverride.deactivate() skips its audit event when the grant already lapsed #2475, being fixed in parallel in this same file, and explicitly out of this PR's scope per the issue's coordination note ("do NOT also fix deactivate() (SafetyOverride.deactivate() skips its audit event when the grant already lapsed #2475)"). Once SafetyOverride.deactivate() skips its audit event when the grant already lapsed #2475 makes deactivate zero _expires_at on a lapsed grant, the grace arm here auto-refuses — the two fixes compose with no further change.

3. The proposed fix (grace eligibility + critical audit + commit under one lock) puts synchronous SEL filesystem I/O under _lock, which the issue reporter explicitly ruled out ("the SEL write must NOT happen while holding _lock") and which would stall every concurrent is_active() gate check behind disk writes — the very reason the resolver is also read pre-lock.

Rounds 1-3 findings were all real and are fixed; this one asks this PR to fix #2475's bug in a way that regresses the lock discipline.

/ai-review override gpt 6995500: Scenario is pre-existing main behavior rooted in deactivate()'s lapsed-grant early return (sibling issue #2475, explicitly out of scope here), and the proposed single-lock fix would put SEL filesystem I/O under _lock against the issue's stated lock discipline.

[bolichen97]

/ai-review override gpt 6995500: Scenario is pre-existing main behavior rooted in deactivate()'s lapsed-grant early return (sibling issue #2475, out of scope per the issue's coordination note), and the proposed single-lock fix would put SEL filesystem I/O under _lock against the issue's stated lock discipline.

[github-actions]

Human judgment recorded

@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 6995500be09a8d3a762968330614f1895e1ed6ec.

Scenario is pre-existing main behavior rooted in deactivate()'s lapsed-grant early return (sibling issue #2475, out of scope per the issue's coordination note), and the proposed single-lock fix would put SEL filesystem I/O under _lock against the issue's stated lock discipline.

This decision applies only to this commit. A new push requires a new judgment.