fix(connections): rearm premint when a provider grant is invalidated

[pepmach]

Problem / Motivation

G2 pod bug-bash finding 3: when a provider's stored grant is invalidated -- a user
Disconnect, or the pod smoke-test grants destroyed after a run -- the warm premint pool never
re-arms for that provider. The user's next Authorize is cold, measured at ~23s, against a
sub-second warm one.

The cause is a gap in who tells whom. The pool premints approval URLs ahead of need and
deliberately skips any provider that already holds a grant, which is correct -- warming a
connected provider would initiate consent nobody asked for. But nothing ever told the pool
when a grant went away, so a provider that held one at the last scan stayed skipped
indefinitely, and the cold path silently became the normal path for exactly the providers a
user had just disconnected and was most likely to reconnect.

Why it matters

It hits the reconnect flow, which is the one flow where the user has already told us what they
want next. A 23-second wait on a button that is usually instant reads as a broken page rather
than a slow one, and it is worst in the pod bug-bash and smoke-test loops where grants are
destroyed by design -- so the tooling we use to validate Connections is the tooling most
reliably served the slow path.

What changed (motivation → approach → change)

The seam. remove_provider_entry is the only production caller of revoke_local_grant,
which is itself the only thing that unlinks a stored grant. That makes it the single
chokepoint every local invalidation passes through, so the pool learns about invalidation in
one place instead of one place per caller.

Scheduled, never awaited. An activation spawns a helper process and negotiates OAuth. That
must neither hold the config lock the invalidation transaction just released nor delay the
answer to a user who asked for a connection to be removed, so the re-arm is fire-and-forget
and the invalidation path never waits on a mint.

Eligibility is not re-derived. The ordinary tri-state candidate scan stays the authority,
so the re-arm inherits every veto arming already has: a disabled entry, a provider with no
usable auth configuration, a configured entry asking for something other than the registry, a
grant still present because the revoke was refused or shared, and a grant cache that could
not be read at all -- an absence nobody confirmed must never initiate consent. A re-arm that
would itself fail cold is noise, so a provider the scan does not return is simply not warmed.
This also means a Disconnect that deliberately kept the grant re-arms nothing, without the
seam having to reason about that case.

Guards against a storm. At most one re-arm per provider is in flight, so repeated
invalidations stack no duplicate premints. A provider already minting or waiting is left
alone -- re-arming would displace a URL the card is showing and the user may be part-way
through redeeming. The scan is SEL-audited on the same read id as premint, because it is an
acted-on observation of the credential store.

Folded-in advisory (see Related Issues). A spawn that never produced a process left its
provisional generation directory behind for good: the abandon path and startup scavenging both
read a runtime_pid: 0 marker as unproved, and unproved must keep. So repeated spawn failures
accumulated directories that nothing would ever reclaim. A runtime that never reached a PID has
no child that could still be reading that tree -- the same evidence the existing no-runtime
branch already removes on -- so that tree is now released. A marker that names a PID still
needs the full identity proof, because a spawn that failed after its child started may have
left it running.

Tests

12 new tests, all verified red on the parent commit (9 failed, 3 errored) and green here.

Re-arm behavior (test/test_connections_warm.py):

• the re-arm is provider-scoped, not a whole-pool warm, and does not arm its own supervision
• the scheduler hands back before the activation it scheduled has even begun -- pinned on the
  task's own state, not wall-clock timing
• two further invalidations while one re-arm is in flight add no second activation
• a provider the candidate scan vetoes is never warmed
• a live row (minting and waiting, parametrized) keeps its URL and its token
• the grant scan it acts on emits the premint SEL read id
• end to end through the real warm_mint_all: the card ends up waiting on the new generation
• off the event loop the scheduler is a no-op, leaving the provider to the next scan

The seam (test/test_connections_disconnect.py):

• a Disconnect asks the pool to re-arm, scoped to that provider
• through the real scheduler: the response lands while the re-arm is still in flight

Folded advisory (test/test_connections_warm.py):

• a never-bound provisional generation directory is released on a confirmed kill
• a bound generation is untouched until its recorded identity proves dead

Also hardened the disconnect suite's _wire harness to record the scheduled re-arm rather than
run it -- the real scheduler starts an activation that spawns kiro-cli, so leaving it live would
have made all 53 tests in that file launch a helper process.

Suite counts, all green on the rebased head 85960b5f6:

suite	tests
test_connections_warm.py	130 passed
test_connections_disconnect.py	53 passed
test_connections_premint.py	11 passed
test_connections_handoff.py	35 passed
total	229 passed, 0 failed

Gates: mypy --platform linux (1293 source files, no issues), black, isort, flake8 7.1.0 all
clean on the four touched files. No docs touched, so docs-lint does not apply.

Manual verification

N/A -- unit coverage sufficient. The two properties that actually needed proving are structural
rather than observational: the invalidation path does not await the mint (pinned on the task's
own unstarted state through the real scheduler), and the candidate scan remains the sole
eligibility authority (pinned by driving a vetoed provider through it). A live pod run would
re-measure the ~23s finding, which is what motivated the change rather than what validates it.

Related Issues

Folds in the Opus advisory dispositioned as owner-accepted follow-up on
PR #8325 (span=0e5c8da823a6) -- that
disposition froze an otherwise-green head and named the warm-rearm slice as the follow-up, so
this PR is it.

Pattern harvest

Rule candidate: review-prompt
Pattern: a pool that skips work based on cached external state has no way to learn that state
changed -- check every invalidation path for a corresponding re-arm, and check that the re-arm
re-uses the arming path's own eligibility scan rather than re-deriving it.

Why no screenshot: backend-only. The change is a scheduling seam between the grant-ownership
transaction and the warm-mint pool; no panel, component, layout or copy is added or altered. The
user-visible effect is latency on an existing button, which a still frame cannot show.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) -- N/A, no doc surface
• No secrets, credentials, or internal references in the diff

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 05c111bab1262cbe67bea401da0dcdad2084cc44 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 05c111b

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

The claim that remove_provider_entry is the sole production chokepoint for local grant unlink checks out (connections.py:680 is the only caller; revoke_local_grant has no other production caller). The eligibility reuse, per-slug dedup, strong-ref task registry with shutdown settlement, and the residue gate against partial unlinks are all coherent; the folded generation-dir leak fix is disclosed and traced to its dispositioned follow-up. The one hazard the residue gate patches (a partial pair reading as definitive absence in grant_presence) also faces the ordinary scan, but that predates this PR and fixing it there is a separate follow-up, not a flaw in this design.

Design-Verdict: PASS

Right seam (verified sole chokepoint), eligibility delegated to the existing scan, fire-and-forget with settlement — proportionate fix for a real reconnect-latency harm.

[DESIGN-REVIEWED] 05c111b

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 05c111bab1262cbe67bea401da0dcdad2084cc44 — 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 evidence gathered. Final verification summary: the chokepoint claim holds (one production caller of revoke_local_grant), the premint scan only runs on page mount or process death so the gap is real, the residue gate's premise (grant_presence reads a half-removed pair as definitive absence) is confirmed in mcp_grant.py:135-140, and the one counted sibling of the teardown-settlement pattern is the unsettled _premint_tasks set.

First-Principles-Verdict: CONCERNS

The fix earns its place, but the caller-side residue gate ships undeclared, and the teardown settlement leaves its one sibling — _premint_tasks — unsettled.

What this change ships

Intent: after a grant invalidation, the next Authorize is warm instead of a ~23s cold mint — a FIX.

1. Disconnect now schedules a provider-scoped premint re-arm — justified (the fix; chokepoint claim verified: 1 production caller of revoke_local_grant)
2. Disconnect answers without waiting on the re-arm — justified
3. A half-removed grant pair suppresses the re-arm (in-lock re-stat, residue gate) — undeclared
4. Repeated Disconnects stack no duplicate premint (per-slug registry) — justified
5. A card mid-consent keeps its URL (live-row guard) — justified
6. Gateway shutdown settles in-flight re-arms before retiring processes — justified, symptom-level (1 unfixed sibling)
7. The re-arm scan emits the premint SEL read id — justified (documented SEL invariant)
8. Failed spawns no longer accumulate abandoned generation dirs — rides along, declared (owner-accepted follow-up on PR fix(connections): isolate warm mint agent specs per process #8325)
9. Off-loop scheduling is a silent no-op — justified

Watch

• Item 3 is undeclared and the description contradicts it: "Eligibility is not re-derived" and "a grant still present because the revoke was refused" is credited to the scan — but grant_presence (mcp_grant.py:136) reads a partial pair as definitive absence, so the diff adds exactly the caller-side eligibility judgment the description says the seam doesn't make. The gate is derived (consent-over-residue) and correct; the framing isn't.
• Item 6 patches "fire-and-forget warm task unsettled at loop close" only for the new registry. Grepped _premint_tasks: 3 hits, all dashboard/handlers/connections.py:285,793-794 — the same pattern, only strong reference in a set, no shutdown settlement, same post-teardown-spawn exposure _settle_invalidation_rearms's own docstring names. One counted unfixed sibling; accepted-and-deferred is fine, but it should be a recorded decision.

[FIRST-PRINCIPLES-REVIEWED] 05c111b

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 05c111bab1262cbe67bea401da0dcdad2084cc44 — this comment is updated in place on each push.

Review details

The discovery pass recorded no candidates. I independently examined both changed production files (ownership.py re-arm scheduling and residue gating; warm.py _spawn_left_no_process, the _release_runtime_generation change, the _invalidation_rearms registry, _rearm_invalidated_provider/rearm_invalidated_provider, and shutdown_warm_mint's settle step).

Verified: slug is a real parameter of remove_provider_entry (used throughout _judge and the purge). The residue gate is set on every branch that keeps or fails to fully remove a grant, so if not residue schedules a re-arm only on a clean invalidation; _rearm_invalidated_provider re-derives eligibility, skips live rows and grants still present, so an over/under-fire is latency-only. The _invalidation_rearms de-dupe (running.done() check plus the is task guard in _forget_invalidation_rearm) closes the done-callback race cleanly. _spawn_left_no_process requires both pid <= 0 and marker runtime_pid <= 0, and spawn() sets _pid with no intervening await, so a live child always yields pid > 0 and its tree is kept. Nothing grounds a concrete input → call path → wrong outcome at the 80 bar.

No findings.

[OPUS-REVIEWED] 05c111b

Verdict parsed from the review's SHA-scoped output markers for commit 05c111bab1262cbe67bea401da0dcdad2084cc44.

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

[pepmach]

• Fixed — a partial revoke no longer schedules a re-arm (span=3ccb28ec1669)

a PARTIAL revoke still reaches rearm_invalidated_provider(slug); the surviving artifact half makes grant_present read absent-ish and warming starts OAuth against a provider that still has residue

Correct, and the mechanism is sharper than "absent-ish": grant_presence returns False — a DEFINITIVE absence — as soon as either artifact is definitively absent (if False in verdicts: return False). A definitive absence is the one verdict _warm_activation_candidates initiates consent on, so a half-unlinked pair was not merely ambiguous to the scan, it was the strongest possible signal to warm. The round-1 comment at that call site claimed a "refused revoke ... simply is not a candidate", which holds only when the refusal is total (both artifacts present → True → skipped); it was wrong for the partial case, and that wrong sentence is what the fix replaces.

The completeness signal is residue: whether any artifact this transaction OWNS is still on disk when it ends, derived per branch from what that branch actually did rather than from one sweep at the end:

• census gap — every owned pair is deliberately kept, so residue = bool(owned_urls) with no stat at all;
• shared key (continue) — the pair was kept for a named sharer, so residue = True, again statless;
• attempted — after revoke_local_grant returns, the pair is re-read once with surviving_grant_artifacts, and any survivor sets residue.

revoke_local_grant's return value is deliberately not the signal: it reports removed LABELS, so len(removed) < 2 is also the legitimate shape of a provider that only ever had a token. Survivor presence is the only test that distinguishes those.

That re-read is inside the lock, unlike the handler's own post-lock survivor read. Both readings exist for different consumers and the difference is load-bearing: the handler's answer feeds a wire field and explicitly documents that reading it outside the lock "changes nothing", while this one decides whether to start an authorization, so it must not be able to disagree with the unlink it is judging. The handler's read and the grantSurviving contract are untouched.

The never-connected case stays warmable: no owned URLs means no residue, so a Disconnect on a provider with no stored grant re-arms exactly as before.

Pinned red-first; all three fail on 5dc84638a and pass here:

test_a_partial_revoke_does_not_rearm (removed token, surviving registration) failed asserting scheduled == [] — it re-armed over an artifact still on disk. test_a_grant_kept_for_a_sharer_does_not_rearm and test_an_unreadable_census_does_not_rearm cover the two statless keep branches and failed the same way.

Verified on f30dcd929: 234 tests across the warm, disconnect, premint and handoff suites (132 / 56 / 11 / 35), mypy --platform linux (1293 source files, no issues), black, isort, flake8 7.1.0. Single commit on cured main.

[pepmach]

• Fixed — teardown now settles every in-flight invalidation re-arm (span=0e5c8da823a6)

tasks in _invalidation_rearms are never settled at shutdown — shutdown_warm_mint -> _WarmMintRuntime.shutdown only cancels the death-driven self._rearm_task via _disarm_supervision, so a Disconnect firing rearm_invalidated_provider moments before teardown leaks an in-flight task

This is a NEW finding at a reused span id — same file and reviewer lane as the #8325 provisional-marker advisory this PR folded in, but a different defect in different code. That fold-in concerned _release_runtime_generation and generation directories; this one concerns task settlement at teardown and is answered on its own terms below.

Accepted as reported. _disarm_supervision returns only self._rearm_task, so the module-level _invalidation_rearms registry was outside every settlement path. Nothing awaits those tasks in the ordinary course — that is the point of the fire-and-forget scheduler — which makes teardown the only place that can settle them, and left the reported window real.

The settlement shape is the runtime's own, extended to the registry: _settle_invalidation_rearms snapshots the registry, filters to tasks that are neither asyncio.current_task() nor already done(), cancels each, then asyncio.gather(*pending, return_exceptions=True) so the children's CancelledError is consumed there rather than surfacing out of teardown — the same three moves _WarmMintRuntime.shutdown already makes for its supervision re-arm, including the self-task guard.

ORDER is load-bearing and is why the settlement sits in shutdown_warm_mint rather than inside _WarmMintRuntime.shutdown. Two reasons:

1. Re-arms are settled BEFORE processes are retired. A re-arm cancelled first rolls its claims back through warm_mint_all's own BaseException region; retiring first would leave an activation already past that point free to spawn a replacement process after teardown had finished — the same leak one layer down.
2. _WarmMintRuntime.shutdown is not only a teardown path. The reaper calls it to retire an idle process, where cancelling a freshly scheduled re-arm would be wrong: that re-arm is entitled to spawn a new process. shutdown_warm_mint is the gateway cleanup hook, so it is the only caller for which "no re-arm may survive" is true.

The retire is in a finally, so a failure while settling still tears the processes down.

Pinned red-first: test_shutdown_settles_an_inflight_invalidation_rearm fires a re-arm that blocks on an Event, waits for it to be running, then calls shutdown_warm_mint and asserts the task observed its own cancellation and the registry is empty. On 5dc84638a it failed — teardown returned with the task still pending. test_shutdown_leaves_a_completed_rearms_bookkeeping_alone is its companion guard rather than a second regression: it pins that settlement skips finished tasks (task.result() == ["linear"], not cancelled), and it passes on both heads by design, because its purpose is to stop the fix from over-reaching.

Verified on f30dcd929: 234 tests across the warm, disconnect, premint and handoff suites (132 / 56 / 11 / 35), mypy --platform linux (1293 source files, no issues), black, isort, flake8 7.1.0. Single commit on cured main.

[pepmach]

• Fixed — the re-arm registry fixtures now await the cancellation they request (span=5f6cc79d1309)

fixtures do task.cancel() then synchronously warm._invalidation_rearms.clear() WITHOUT awaiting the cancellation; a blocked re-arm at test exit leaves the loop closing on a pending task, cleanup never runs

Upheld and taken. The harness reproduced the exact defect this PR's production change closes: _settle_invalidation_rearms exists because cancelling a task and dropping the last reference to it never delivers the cancellation, and both new fixtures did precisely that.

One correction to the severity, stated because it changes what the fix is worth rather than whether to make it. The leak is LATENT on this head, not firing: every current consumer sets its release event and awaits its own task to completion before teardown, so by the time the fixture ran, the registry's task was already done() and cancel() was a no-op. A probe of the pre-fix fixtures on f30dcd929 produced zero Task was destroyed but it is pending lines, so I am not claiming a reproduction I did not get. What the finding correctly identifies is a TRAP: the first test that fails before its release, or is written without one, silently leaks — and the harness for a task-settlement fix must not be the one place that skips settlement.

SHAPE — an async with helper, not an async fixture. GPT's prescribed remedy ("make both fixtures async") is not available in this repo: async generator fixtures are not collected by the pinned pytest-asyncio (its wrapper reads fixturedef.unittest, removed in pytest 8.1), so @pytest.fixture on an async generator silently fails and @pytest_asyncio.fixture is avoided by convention — stated in test_denied_commands_api.py, test_mcp_apps_call_endpoint.py, test_dashboard_server_startup_coverage.py, test_md_notebook.py and test_mochi_activity.py. The sanctioned shape in those suites is a @contextlib.asynccontextmanager helper entered with async with, so that is what both files now use:

@contextlib.asynccontextmanager
async def _rearm_registry():
    warm._invalidation_rearms.clear()
    try:
        yield warm._invalidation_rearms
    finally:
        await warm._settle_invalidation_rearms()

finally, so a failing test still settles — which is the case the finding is actually about. The teardown DELEGATES to the production settle function rather than re-implementing snapshot → cancel → gather(return_exceptions=True), so the idiom exists in exactly one place in the codebase and the two files cannot drift from it. That is not circular: _settle_invalidation_rearms's own behaviour is pinned independently by test_shutdown_settles_an_inflight_invalidation_rearm and test_shutdown_leaves_a_completed_rearms_bookkeeping_alone, so the fixture is not the only thing asserting it works.

Five consumers moved from a fixture parameter to async with _rearm_registry() as registry. One did NOT: test_the_scheduler_is_a_noop_without_a_running_loop must run with no event loop — that is the property under test — so it cannot enter an async helper. It clears inline, and its own assertion (warm._invalidation_rearms == {}) proves no task was created, so there is nothing to settle. Recorded in its docstring so the exception reads as deliberate.

GREP VERIFIED, as asked: _invalidation_rearms now appears in test/ only inside the two helpers (each clear followed by an awaited settle in finally) and at that one off-loop test's inline clear. git diff origin/main over both files shows this PR adds ZERO .cancel() call sites. The other .cancel() occurrences in test_connections_warm.py are base-owned and belong to the warm runtime's own _reaper / _rearm_task and drain tasks — a different registry, outside this span, and untouched.

Verified on 05c111bab: 188 tests across the two touched suites (test_connections_warm.py 132, test_connections_disconnect.py 56), run additionally under -W error::RuntimeWarning, with zero Task was destroyed but it is pending lines. black, isort, flake8 7.1.0 clean on both files. src/ is unchanged this round and CI's mypy scope is src/kiro_crew/, so the 1293-file clean run from the previous head still stands. Single commit on cured main.

[bolichen97]

Tech Lead review — approved.

Verified the security-critical property independently against the source at 05c111bab, not from the description: the re-arm never reads or reuses a grant. It only re-enters the existing premint path, which mints a fresh approval URL; revoke_local_grant and surviving_grant_artifacts stat and never open artifacts, so the module's presence-only credential boundary is preserved.

The residue gate is load-bearing, not defensive padding. grant_presence returns a definitive False as soon as either paired artifact is definitively absent (if False in verdicts: return False), and _warm_activation_candidates initiates consent on exactly that definitive False — so a half-unlinked pair was the strongest possible signal to warm, not merely an ambiguous one. Gating the schedule on residue is the correct fix at the correct layer, and it fails safe in the right direction: surviving_grant_artifacts counts an UNREADABLE artifact as surviving, so a stalled mount or permission error suppresses the re-arm rather than warming over live credentials. Residue is set on every branch that keeps or fails to fully remove a grant (census gap with owned pairs, shared-key continue, and the post-unlink in-lock re-stat), and the re-stat correctly sits inside the lock because it decides whether to start an authorization.

Eligibility genuinely is delegated, not re-derived: _rearm_invalidated_provider takes the candidate list from _audited_mintable_providers and hands it to warm_mint_all, where _warm_activation_candidates applies the is False (never falsy) presence test — so an unreadable grant cache skips rather than warms, and a still-present grant skips.

On the folded-in advisory: _spawn_left_no_process cannot rmtree a live process's tree. All three _release_runtime_generation call sites are gated on _kill_quietly returning True (a kill that times out returns False and the tree stays attached), and self._pid = self._process.pid is assigned immediately after subprocess creation with no intervening await — so a child that exists always yields pid > 0 and the pid <= 0 + runtime_pid <= 0 conjunction is only reachable when no process was ever created.

No AGENTS.md security invariant is touched: no security.py matchers, no hooks PreToolUse gate, no governance scopes, no denied-command rules, no computer-use surface. The SEL audit emits the premint read id and warns-then-proceeds on an unrecordable event, matching grant_observed's documented best-effort (not fail-closed) policy for presence-only stats.

Scope is proportionate: 197 production lines across two files, 517 test lines, 17 new tests covering precisely the branches that could go wrong (partial revoke, shared-grant keep, unreadable census, live-row non-displacement, dedup under repeated invalidation, off-loop no-op, teardown settlement, bound-generation-untouched).

One residual I am accepting rather than blocking on: a Connect landing between the _mints_lock live-row check and _claim_shared_mints could still have its waiting URL displaced. That is latency/UX only, carries no credential consequence, and is strictly more careful than the existing sibling _rearm_dead_warm_mint, which has no live-row check at all. The First Principles CONCERNS items are advisory and correct as characterized — the description under-declares the residue gate (the disposition comment corrects it openly), and the unsettled _premint_tasks sibling predates this PR.