fix(code-review): retire the review pool when the app is disabled

[leonlaiyc]

Problem / Motivation

review_pool.shutdown_pool says what it is for:

async def shutdown_pool() -> None:
    """Tear down the singleton pool (called on app disable / gateway shutdown)."""

It is not called on app disable. grep shutdown_pool across src/ finds exactly one
production caller — _shutdown_pool, registered on app.on_cleanup in
register_routes. aiohttp fires on_cleanup when the gateway stops and at no other
time, so the first half of that docstring describes something nothing does.

What a disable actually does is refuse the routes. The pool's worker sessions stay alive,
holding an agent runtime up and continuing to run review turns — for a permission the
operator has already withdrawn, while every route answers 403 and reports nothing.

This is the same shape as the auto-improvement app's disable gap, and it is the second
half of a closed pair: register_routes there stands its workers down on shutdown and now
on disable too. Here only shutdown was wired.

Why it matters

The seam's own docstring states the harm, as a security property rather than a tidiness
one:

This exists because a periodic sweep is not an off-switch. An app whose workers hold
anything time-bounded — an auto-approval grant, a lease, a lock — can act once more in
the gap between the operator's click and the next poll, which is a whole turn's worth of
authority handed out after permission was withdrawn.

A review worker is exactly that: it holds a live agent runtime and spends budget on turns
that read the operator's code and can post to their pull requests. teardown.py also runs
notify_app_disabled first, ahead of the app's own onDisable script and backend
teardown, "so a worker holding something time-bounded would [not] keep that authority for
the duration" — an app that registers nothing there silently opts out of that ordering
guarantee.

And because the docstring already claims the disable path is covered, a reader auditing
this app's teardown finds a function that says it handles disable, one caller, and no
reason to look further.

What changed (motivation → approach → change)

Symptom: disabling the app leaves the review pool running. Root cause: shutdown_pool is
wired to one of the two ways the app can be switched off. Fix: wire it to the other one
too, through the mechanism that exists for it, with the same body.

register_routes now also registers an app-disable hook:

async def _retire_pool_on_disable(_app: str) -> None:
    try:
        await review_pool.shutdown_pool()
    except Exception:
        logger.warning("failed to retire review pool on disable", exc_info=True)

Same call the on_cleanup hook makes. shutdown_pool is idempotent (a no-op when no pool
has started) and drops the singleton, so a later re-enable builds a fresh one. Nothing new
is introduced: no state, no config key, no constant, no route, no change to the pool.

Registration is feature-detected with getattr(teardown, "register_app_disable_hook", None), copying Issue Radar's pattern verbatim and for its stated reason — a core build
whose teardown module predates the registry keeps loading, and there the cleanup hook is
the only teardown there is, which is the pre-registry behaviour rather than a regression.
It sits in its own try, matching the existing hook registration, so it can never break
gateway startup.

The shutdown_pool docstring is left alone deliberately: this change makes its existing
claim true, which is the better of the two ways to resolve the mismatch.

The remaining siblings, counted and declared. Filtering the builtin apps to those that
register a worker teardown on on_cleanup gives six; two already register a disable hook
(issue_radar, and auto_improvement as of its own disable fix). The four that do not are
code_review_sage (this PR), dev_fleet (dev_fleet_cleanup), md_notebook
(syncer.stop_syncer) and meetings (_on_cleanup). The other three are not folded
in here because they are not mechanical: dev_fleet's teardown drains a prune worker
mid-flight through _run_uninterruptible because its git mutations must not be torn in
half, so what a disable should do to an in-progress destructive mutation is a decision,
not a copy of this hook. Each of the three needs its own answer to "what does this worker
hold, and what does stopping it mean", which is a judgment per app rather than one sweep.

Second half, after review: retiring the pool was necessary but not sufficient

GPT's exact-head review blocked this, correctly. shutdown_pool() drops the
singleton and get_pool() recreates it on the very next call:

def get_pool() -> ReviewPool:
    global _POOL
    if _POOL is None or _POOL._closed:
        _POOL = ReviewPool()
    return _POOL

So a run already accepted and only waiting its turn on _RUN_LOCK behind
another run woke up after the disable and simply built a new pool — a fresh agent
runtime, and a posted review, for a permission the operator had withdrawn while
it waited. Reproduced before changing anything; the trace on the previous head
is literally ['shutdown_pool', 'get_pool'].

I did not take the suggested revert of the disable hook. The original gap was
real too — leaving the pool alive after disable is its own defect — so both
invariants have to hold at once:

• A. disabling retires existing review runtime authority;
• B. a review accepted before the disable cannot recreate a pool and resume
  after it.

Two halves, using mechanisms that already exist:

• the disable hook now withdraws authority from every live run before it
  retires the pool, marking them in _CANCELLED — the app's existing
  cancellation set. The marks are made synchronously, with no await between
  the loop and the marks landing, so a run waiting on _RUN_LOCK cannot slip
  through in between. The predicate is _is_live, already in this file, which
  also covers the posting phase — a run mid-delivery reports a terminal status
  while still writing to the pull request, so status == "running" alone would
  miss it;
• _run_review_bg re-checks _CANCELLED immediately after acquiring
  _RUN_LOCK, before it claims any change and before get_pool().

Why _CANCELLED and not is_app_enabled. The obvious-looking guard is to
re-read the authoritative enabled flag after the wait. It is stale exactly when it
matters. notify_app_disabled fires before disable_app writes the enabled
flag — apps/teardown.py says so in terms, and says a hook "must not wait on that
flag to decide it has been switched off" — and before the app's own third-party
onDisable script, which that same comment notes can take real time. So for the
whole window this bug lives in, is_app_enabled still returns True, while
_CANCELLED is already correct at that instant.

No enable-side hook is added, and none is needed. apps.teardown has no
enable seam, and inventing one would be a new lifecycle framework for a
one-invariant fix. Because the withdrawal is recorded per run, a genuinely new
review submitted after re-enable carries a run id that was never cancelled and
builds its own pool. A test pins exactly that, with nothing un-set by hand.

Closing admission for the whole transition (added after the second block)

The exact-head GPT review found the remaining half, and it is correct:
_CANCELLED is a point-in-time snapshot. The hook marks the runs that are live
when it fires, so it can only speak for runs that already exist. A review posted
after that scan is registered with an uncancelled id, walks past the _RUN_LOCK
recheck and calls get_pool() — authority rebuilt after the operator withdrew it.

Reproduced before designing anything (see Tests): with the disable already under
way and the hook's snapshot already taken, a new POST is admitted 200, its id is
absent from the snapshot, and driving that run through the real _run_review_bg
reaches get_pool().

Two things were open, not one, and neither covers the other:

• _handle_review had no enabled gate at all. _require_enabled wraps the two
  chat handlers; the route that starts a review was never wrapped. A fully
  disabled app accepted reviews.
• The transition window, where the flag still reads enabled — the window the
  section above already explains is_app_enabled cannot speak for.

Both are now checked at the app's ONE admission chokepoint. Both review entry
points (_handle_review, _handle_review_repo) reach the registry through
_record, so the boundary is that call and not each route:

async def _admit(run: dict) -> bool:
    async with app_lifecycle_lock(_APP_NAME):
        if not await asyncio.to_thread(is_app_enabled, _APP_NAME):
            return False
        await _record(run)
    return True

The enabled read and the insert are ONE critical section, taken under a lock
that already exists. handle_disable_app holds app_lifecycle_lock(name)
across both teardown_app_runtime (which fires the disable hook) and
disable_app (which writes the flag). Holding the same lock across the read and
the _RUNS.insert makes the admission decision and the disable transition
mutually exclusive, in either order:

• a disable that gets there first has finished and persisted enabled=False
  before this read happens, so the request is refused;
• a review that gets there first is in _RUNS before the hook's scan runs, so it
  is marked cancelled like any other live run.

Correction to this PR's previous head. _admit used to read the flag and
then consult a separate point-in-time sample of the same lock
(_lifecycle_is_stable, evaluated under _LOCK). GPT's exact-head review blocked
that, correctly: those are two checks with a suspension between them. The read is
off-loop, so a disable can acquire the lock, run the teardown, persist
enabled=False and release it entirely inside that one to_thread
round-trip — after which the sample finds the lock free and admits a run whose
authority has already been withdrawn. The previous body described that as a
residual "strictly narrower than the window this closes". It was not narrower; it
was the window. It is now closed rather than documented, because the interleaving
has to be removed, not detected. _lifecycle_is_stable and the guard= parameter
are deleted, and _record is byte-for-byte its state on main.

No module-global flag, and no new lifecycle state. A DISABLED = True would
need an enable-side seam to reset it, and there is none — an interrupted disable
would leave the app permanently refusing reviews with nothing to clear it. Here
the disable request owns the lock, so there is no shadow state, nothing latched,
and no way to brick a re-enabled app. app_lifecycle_lock is also held during
install/update/enable of this app; a review submitted then waits for that
transition and is decided on the flag the transition left behind, which is the
conservative answer and is deliberate.

Lock order, inspected rather than assumed. The order taken here is
app_lifecycle_lock → _LOCK, matching the outermost-lock convention already
stated in apps/routes.py. Nothing takes them the other way round: _LOCK is
local to this module, no path holds it while acquiring a lifecycle lock, and the
disable hook — which runs while handle_disable_app holds the lifecycle lock —
scans _RUNS without _LOCK and awaits only review_pool.shutdown_pool, which
never reaches this module. Holding the lifecycle lock across admission also makes
that scan and the insert mutually exclusive, which is what the deleted
under-_LOCK guard existed to approximate.

The blocking read stays off the event loop. It is still
asyncio.to_thread(is_app_enabled, ...), now inside the lock — the same
enablement-then-mutate shape dashboard/handlers/notifications_push.py already
uses, where the lock is held across a to_thread manifest read and the channel
registration it guards.

Not adjusted, and recorded instead: reviewers also noted that a posting-phase
run can be added to _CANCELLED without ever consuming the marker. Nothing in this
change depends on that, and no executable evidence made it necessary for the
admission invariant, so it stays a separate residual rather than growing this PR
into a cleanup.

Tests

New src/kiro_crew/apps/builtins/code_review_sage/tests/test_disable_retires_pool.py,
4 tests, each driving the real register_routes on a bare aiohttp application.

• Disabling retires the pool — fires teardown.notify_app_disabled("code-review-sage"),
  which is what the disable request calls and where it calls it, and asserts
  shutdown_pool ran.
• A failing retire never escapes — shutdown_pool is made to raise and the hook is
  invoked directly, not through notify_app_disabled. That matters: the notifier
  swallows hook exceptions itself, so calling through it would pass whether or not the
  hook contained its own failure. The disable must not depend on the caller's leniency.
• Retiring with no pool started is a no-op, fired twice — an operator may disable an
  app that has never run a review, or disable it twice. This is the over-fix guard, and
  it is the one test that also passes on main.
• register_routes wires a disable hook — a structural guard, so an edit that drops
  the registration fails here instead of silently letting a disabled app keep reviewing.

An autouse fixture unregisters the hook before and after each test: the registry is
process memory shared by every test in the worker, so a hook left behind would leak into
unrelated tests.

Red-before, with routes.py restored to current origin/main (72e429791) and
nothing else changed:

FAILED test_disabling_the_app_retires_the_review_pool
  AssertionError: disabling the app left the review pool running
FAILED test_a_failing_retire_never_escapes
  KeyError: 'code-review-sage'
FAILED test_register_routes_wires_an_app_disable_hook
  AssertionError: assert None is not None
3 failed, 1 passed

Green on the branch: 4 passed. Whole app suite: 754 passed, 32 skipped, with
test_adapters.py::TestEnterpriseHosts::test_malformed_links_are_rejected_not_crashed_on
deselected — that one fails identically on pristine origin/main with this branch's files
removed (UnsupportedPlatform not raised), so it is inherited on this host and not
attributable to this diff.

Gates: check_black_formatting.py pass, flake8 src/kiro_crew/apps/builtins/code_review_sage/
clean, isort --check-only src/kiro_crew test clean, mypy src/kiro_crew — "Success: no
issues found in 1170 source files".

routes.py is in .github/black-baseline.txt and stays there: the diff is 30 added lines
and 0 deletions, with no reformatting riding along.

The queued-review race (added after the block)

Four more tests in the same file, driving the exact interleaving on the real
_RUN_LOCK with asyncio.Events rather than sleeps, so the ordering is
deterministic: B is only released once the disable has actually completed.

Test	Locks in
..._a_review_queued_before_disable_cannot_rebuild_the_pool_after_it	the blocker. A holds the lock, B is accepted and blocked on it, the operator disables while B waits, A releases. Asserts the pool was retired, get_pool was never called, no review work ran, and B ends cancelled
..._a_queued_review_still_runs_when_nothing_is_disabled	the over-fix guard: same interleaving, no disable → get_pool and run_review both happen. Stops the check being a blanket refusal
..._a_new_review_after_re_enable_gets_a_fresh_pool	disable-then-new-review in one test: the second run builds its own pool with nothing reset by hand
..._the_disable_hook_withdraws_authority_from_every_live_run	a posting run (terminal status, posting=True) is marked; a finished run is not

The assertions are on what B actually did — pool creation and review work —
not on whether shutdown_pool() was called. The blocker is specifically about
what happens after shutdown, so a test that only checked the shutdown would pass
on the broken code.

Red-before, against this PR's own previous head 66906d4e2 (the disable hook
present, the race fix reverted): 3 failed, 5 passed. The failure message is
the mechanism verbatim — "a run queued before the disable rebuilt the pool after
it". The 5 that pass include the no-disable control and the four original disable
tests, which must pass on both sides.

Green after: the file's 8 passed. Directly relevant suites —
test_disable_retires_pool, test_review_pool, test_run_endpoints,
test_backend_routes → 269 passed, 16 skipped. flake8, isort and mypy
clean on the changed files. No repository-wide sweep.

The admission race (added after the second block)

Four more tests in the same file. The disable transition is reproduced the way
production runs it — app_lifecycle_lock held, the app's real hook fired
through teardown.notify_app_disabled, and the persisted enabled flag
deliberately still True, because that is its real value in this window. Ordering
is by lock and by await point; there are no sleeps, and the background work an
admitted review spawns is drained by gathering the module's own task set rather
than by yielding and hoping.

Test	Locks in
..._a_new_review_cannot_be_admitted_once_disable_has_begun	the blocker. After the hook's snapshot and before the flag is written, a new POST must be refused — asserted on the boundary (HTTP 403, nothing registered, no work started), not on an internal flag. A test that only checked _CANCELLED would pass on a fix that admitted the run and cancelled it afterwards, which is a weaker promise
..._the_admitted_run_would_otherwise_reach_the_pool_seam	why the refusal matters, driven rather than asserted: takes the run the window admits, checks its id is absent from the hook's snapshot, and runs it through the real _run_review_bg to get_pool(). Skips once admission is closed, since there is then no such run
..._a_new_review_is_admitted_when_no_disable_is_in_flight	the over-fix guard: same path, same enabled flag, no transition → admitted, registered, work started
..._admission_reopens_after_the_transition_completes	invariant D, executable. Nothing is un-set by hand; the second call simply runs after the async with block exits

A fifth test lands in test_backend_routes.py for the steady state: a disabled app
refuses _handle_review with 403 app_disabled and registers nothing.

Red-before, against this PR's own previous head 70f7cfa41 with the new tests
present and routes.py restored from it: 2 failed, 10 passed. The failure
message is the mechanism verbatim — "a new review was admitted after the
operator's disable had already begun (HTTP 200, …)" — and the pool-seam test
passes there, which is the executable proof of step 7: the admitted run really
does reach get_pool().

Green after: the file's 11 passed, 1 skipped (the skip is the pool-seam
drive, by design). Directly relevant suites — test_disable_retires_pool,
test_backend_routes, test_run_endpoints, test_activity_progress,
test_record_adoption, test_post_comments, test_review_driver,
test_store_selfheal, test_followup → 412 passed, 26 skipped. All four
original queued-race tests still pass, so invariant B is not regressed.

Two test fixtures gain one line each, is_app_enabled = lambda name: True, copied
from the followup-route class's own setUp: TestHandlers.setUp here, and
_SageRoutesBase.setUp in test/test_sage_backend_routes_coverage.py. Those cases
are about what an enabled app does; the route simply has a gate now that it never
had, and the precondition is stated rather than depending on a real
installed.json. Without it, four pre-existing request-guard cases in that file
(TestReviewKickoffGuards, TestReviewRepoHandler) answered 403 on the
enablement check instead of on the guard each one is written to exercise. That base
loads a fresh routes module per test, so the line is local to it, and the app's own
disabled-path coverage is untouched — the file has no case that asserts the disabled
response.

The new test file's stub request and response body are typed the way the sibling
builtin's tests already do (cast(web.Request, ...) at construction, an isinstance
narrowing before json.loads). Its handlers are reached through a real import rather
than a dynamically loaded module, so unlike the existing _Req call sites they are
not Any and mypy checks them.

Gates, re-run on the current head against all four changed files:
scripts/check_black_formatting.py passed in scope (4 files in scope, nothing
unformatted outside the baseline); flake8 and isort clean; mypy — "Success: no
issues found in 1 source file". routes.py stays in the black baseline and is not
reformatted. Touched-surface suites only — test_sage_backend_routes_coverage.py
111 passed, and test_disable_retires_pool.py + test_backend_routes.py
214 passed, 17 skipped. No repository-wide sweep, and no local reviewer agents.

Correction to an earlier claim in this section. The mypy line above previously
described a single-file local run; Backend Lint & Type Check runs mypy src/kiro_crew/, which found three errors in the new test file (two arg-type on the
stub request, one union-attr on response.body) that the narrower local invocation
did not surface. Those are the typing changes described above, and the gate is now run
the way CI runs it.

The completed-disable race (added after the third block)

One more test in the same file, plus a restructure of the four admission tests
above so that they model the enabled flag the way production writes it.

..._a_completed_disable_cannot_be_overtaken_by_an_in_flight_enabled_read drives
the exact interleaving the exact-head review named:

1. a review request reads the enabled flag and gets True;
2. while that off-loop read is still in flight, the disable takes
   app_lifecycle_lock, runs the teardown, persists enabled=False, releases;
3. the review request resumes and registers its run.

The property asserted is an order, recorded as each step happens: no run may
enter _RUNS after enabled=False has been persisted. That keeps the test blind
to how the code holds the line — it fails whenever the two events interleave,
and passes whenever they are serialized, in either direction. Ordering is driven
by events and barriers only: the flag read is parked in its own worker thread on a
threading.Event and released by the disable, and the timeouts are watchdogs so a
regression fails the suite instead of hanging it.

Red-before, against this PR's own previous head 54f4389e8, with the new test
present and routes.py restored from it. The event log is the mechanism verbatim:

AssertionError: a review was registered after the operator's disable had completed
and persisted enabled=False:
['review:read_enabled=True', 'disable:enabled_written_false', 'review:run_admitted']

3 failed, 10 passed

Green after, same test, same harness — the two events are serialized:

['review:read_enabled=True', 'disable:serialized_after_admission',
 'review:run_admitted', 'disable:enabled_written_false']

The 200 branch additionally asserts the admitted run is in _CANCELLED, because
ordering alone is only half the invariant: a run admitted ahead of the disable
must still be caught by the hook's scan.

The other two reds on that head are the two admission tests whose helper now
models the flag write. _post_review_mid_disable previously pinned
is_app_enabled to a constant True and called the handler inline inside the
disable's async with — with admission taking that same lock, an inline call
could only self-deadlock, and a constant flag is not what a completed disable
leaves behind. It now serves the request on its own task, the way the gateway
does; signals "the request has reached the admission boundary" from
change_id_for, the last call before _admit, so there is no sleep and no
polling; and then writes enabled=False inside the lock, the order
handle_disable_app uses. The refusal assertions are unchanged — HTTP 403,
nothing registered, no work started — and the request now arrives during the
transition and waits for it, rather than sampling it.

Green on the current head. test_disable_retires_pool.py 12 passed, 1
skipped (the skip is the pool-seam drive, by design). With its directly relevant
siblings test_backend_routes.py and test/test_sage_backend_routes_coverage.py:
326 passed, 17 skipped. Touched-file gates: flake8, isort and
git diff --check clean; mypy on both changed files — "Success: no issues found
in 2 source files" — and over the whole app package, "Success: no issues found in
46 source files"; scripts/check_black_formatting.py passed in scope (4 files
in scope, nothing unformatted outside the baseline). No repository-wide sweep and
no local reviewer agents; server CI owns the broad validation.

The branch is now one commit. PR Hygiene was red for "PR must contain 1 or 2
commits (has 3)" — a required readiness item, unrelated to any reviewer finding.
The commits are squashed into one; git patch-id --stable is identical before and
after the squash, so no content changed with it.

Manual verification

N/A — unit coverage sufficient: the tests exercise the real registration and fire the
disable notification through the same function the disable request calls, so the whole
path this change adds is covered mechanically.

Related Issues

None. Found by auditing which gateway-mounted apps stand a worker down on shutdown but not
on disable, after the same gap was fixed in the auto-improvement app.

Pattern harvest

Rule candidate: review-prompt
Pattern: teardown drops a lazily-recreated singleton, and a consumer queued
before the teardown re-creates it afterwards — authority is withdrawn, then
rebuilt by work that was already in flight.

Generalizes beyond this app. The shape is any get_x() that reads
if _X is None: _X = X() paired with a shutdown_x() that sets _X = None:
retiring the singleton is not withdrawal while any caller can still reach the
getter. A grep cannot see it — the defect is the ordering between a lock wait
and a lifecycle event — which is why this is a review-prompt candidate rather
than a semgrep rule. The generalizable question a reviewer can ask is: "after
this teardown runs, what is still queued that will call the getter?"

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed 9da06b61b241fe42aa6b13edca2c6d651c7fb4bc via the fork AI-review pipeline; updated in place on each push.

1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/apps/builtins/code_review_sage/backend/routes.py:2569 -- disable does not close posting admission
if _is_live(_run):
disabled app -> terminal run’s /post endpoint remains admitted -> _post_comments_bg recreates the pool -> comments are posted after authority withdrawal
Anchor: backend-security-controls
Fix: serialize post admission with the lifecycle lock and recheck cancellation before get_pool().
[BLOCK-MERGE] 9da06b6
[GPT-REVIEWED] 9da06b6

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

F1 concerns the /post endpoint, which the PR's fix left ungated. Confirmed against the base tree:

• _handle_run_post (routes.py:1149) admits solely on run state (found / not running / not already posting / has pending comments, routes.py:1195-1226) with no enablement or _CANCELLED check.
• _retire_pool_on_disable's snapshot only marks runs where _is_live(_run) is true (routes.py diff line, base-mapped ~2569). A finished review awaiting a manual post is status == "done", posting unset → _is_live is False (routes.py:228), so it is never marked.
• _post_comments_bg (routes.py:1034) calls review_pool.get_pool() unconditionally at routes.py:1047, rebuilding the torn-down singleton and posting to the PR.

Harm rung: UNBOUNDED — writing to an external pull request after the operator withdrew authority; externally visible and not self-correcting. Recovery path: none (comments on the PR would need manual deletion). The required combination — a finished, unposted review + disable + a /post request arriving from a stale tab or retry — is a plausible race, not a contradictory or writer-impossible one, so no rarity argument closes it. FLAG record cannot be completed → UPHOLD-FENCED.

[ADJUDICATION] 9da06b6 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 9da06b6
[ADJUDICATION-FENCED] 9da06b6 fenced=1 flagged=0
UPHOLD-FENCED F1 src/kiro_crew/apps/builtins/code_review_sage/backend/routes.py:2569 -- _handle_run_post (routes.py:1149) has no enablement/_CANCELLED check and _post_comments_bg rebuilds the pool via get_pool() (routes.py:1047), so a terminal run's /post posts to the PR after the operator disabled the app, with no recovery.
[GPT-ADJUDICATED-FENCED] 9da06b6

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 9da06b61b241fe42aa6b13edca2c6d651c7fb4bc via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound, evidence-driven layering on existing seams — but invariant B as tested is narrower than as stated: one interleaving still rebuilds the pool post-disable.

Watch

The _CANCELLED recheck runs once, immediately after _RUN_LOCK is acquired — but two awaits sit between it and get_pool() (to_thread(_claim_changes_under_lock) and to_thread(review_pool.runtime_preflight)). A disable landing in those awaits marks the run (it is live), yet the run resumes past the already-passed recheck, calls get_pool() — which rebuilds because _POOL._closed is true — and begin_batch() eagerly spawns a runtime via _ensure_runtime_locked(). The driver's per-change cancelled poll then cancels every change and end_batch kills the runtime, so no review turn runs and nothing posts — but the PR's stated invariant ("a review accepted before the disable cannot recreate a pool… after it") and its test assertion ("get_pool was never called") do not hold for this interleaving; the queued-race test only exercises a disable that lands before the recheck.
Clears when: a second _CANCELLED check lands after the claim/preflight awaits, immediately before get_pool(), with a test driving the disable during the claim to_thread — or the PR's invariant-B wording and test comment are narrowed to "no review turn runs after disable," recording the transient spawn-and-kill as the accepted residual.

[DESIGN-REVIEWED] 9da06b6

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 9da06b61b241fe42aa6b13edca2c6d651c7fb4bc via the fork AI-review pipeline — 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 claims verified against the base tree. I have what I need to write the review.

Verification summary before the verdict:

• The defect is real on base: shutdown_pool's only production caller is the on_cleanup hook (code_review_sage/backend/routes.py:2456-2466), and get_pool() at line 452 runs before any cancellation check, so both races the PR describes reproduce by reading.
• The cited mechanisms all exist as claimed: register_app_disable_hook (apps/teardown.py:395), app_lifecycle_lock held across teardown + flag write in handle_disable_app (apps/routes.py:1651-1674), and notifications_push.py:97-100 uses the same to-thread-inside-lock shape.
• Sibling count checks out: six builtins register on_cleanup teardowns; only issue_radar has a disable hook on this base.
• Two things don't fully earn their place: the getattr feature-detection (its "older core" premise is contradicted by the same diff's unconditional app_lifecycle_lock import), and the fix leaves the identical racy is_app_enabled read in _require_enabled guarding the chat/followup handlers in the same file.

First-Principles-Verdict: CONCERNS

The admission fix condemns the unserialized is_app_enabled read, then leaves that exact read guarding the chat and followup handlers in the same file.

Not justified as shipped

5. getattr feature-detection of the disable registry — rides along, inherited: copied "verbatim" from Issue Radar for symmetry, and its "core build predates the registry" premise is contradicted by this same diff's unconditional from kiro_crew.apps.manager import app_lifecycle_lock, which such a core would fail on first.

What this change ships

Intent: make disabling the code-review app actually stop its review runtime, not just its routes — a FIX.

1. Disabling stands the review workers down at the click, not only at gateway shutdown — justified
2. A review queued behind another run is cancelled by the disable instead of rebuilding the pool — justified
3. Starting a review while disabled is refused with the 403 the chat surface already gives — justified
4. Starting a review now waits behind an in-flight disable instead of racing it — justified
5. Disable hook registered via getattr feature-detection — rides along; inherited premise
6. _APP_NAME constant, one spelling for both registries — rides along
7. Existing route tests now stub the app enabled — rides along, required by item 3

Watch

_require_enabled (code_review_sage/backend/routes.py:2118) keeps the off-loop is_app_enabled read that _admit's own docstring says "cannot close this" — 2 consumers (grep @_require_enabled): _handle_chat_get:2143 and _handle_followup_start:2259, the second of which arms a session resume on the reviewer's context during the same transition window. Point patch; siblings counted.
Clears when: _handle_followup_start admits under app_lifecycle_lock too, or a recorded reason it need not.

Subtractions

Drop the getattr(teardown, "register_app_disable_hook", None) guard and its if — call teardown.register_app_disable_hook(_APP_NAME, _retire_pool_on_disable) directly inside the existing try: this builtin ships in the same package as the registry, and the diff already imports app_lifecycle_lock unconditionally, so the degraded-core branch is unreachable (only other user of the getattr form: issue_radar's per-cycle watchdog, 1 site).

[FIRST-PRINCIPLES-REVIEWED] 9da06b6

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 9da06b61b241fe42aa6b13edca2c6d651c7fb4bc via the fork AI-review pipeline; updated in place on each push.

Review details

Advisory only: the disable hook marks posting runs cancelled but no path ever clears them.

FINDING — src/kiro_crew/apps/builtins/code_review_sage/backend/routes.py:2562 — _retire_pool_on_disable runs _CANCELLED.add(str(_run.get("run_id") or "")) for every _is_live run, which includes a posting run (bool(run.get("posting"))), but no path discards a posting run's id: the only _CANCELLED.discard (routes.py:549) is in _run_review_bg's finally, which has already completed before posting begins, _post_comments_bg's finally releases claims only, and _handle_run_delete refuses to remove a posting run — so _CANCELLED retains a dead id on every operator-disable-while-posting (no functional misfire, since run ids are uuid4().hex[:12], but the set only ever grows) → Fix: discard run_id in _post_comments_bg's finally, or do not mark posting runs in the hook.

[OPUS-REVIEWED] 9da06b6

[leonlaiyc]

Accepted, reproduced, and fixed on e9fa9571 — but not by the suggested revert.

I reproduced your race before changing anything, driving the exact interleaving on the real _RUN_LOCK: A holds the lock, B is already accepted and blocked on it, the operator disables while B waits, A releases. On the previous head the trace is literally ['shutdown_pool', 'get_pool'] — the pool was retired and then rebuilt by the queued run, which then ran review work. Your mechanism is correct in every step.

Why not the revert. The original gap was real too: shutdown_pool's own docstring says it is called "on app disable / gateway shutdown", and only the second half was true, so a disable left worker sessions alive holding an agent runtime for a withdrawn permission. Reverting trades your defect for that one. Both invariants have to hold:

• A. disabling retires existing review runtime authority;
• B. a review accepted before the disable cannot recreate a pool and resume after it.

The fix, in two halves, using seams that already exist:

• the disable hook now withdraws authority from every live run before retiring the pool, marking them in _CANCELLED — this app's existing cancellation set. The marks are made synchronously: there is no await between the loop and the marks landing, so a run waiting on _RUN_LOCK cannot slip through in between. The predicate is _is_live, already in the file, which also covers the posting phase — a run mid-delivery reports a terminal status while still writing to the PR, so status == "running" alone would miss it;
• _run_review_bg re-checks _CANCELLED immediately after acquiring _RUN_LOCK, before it claims any change and before get_pool().

On the guard I deliberately did not use. The obvious version is to re-read the authoritative enabled flag after the wait. It is stale exactly when it matters: notify_app_disabled fires before disable_app writes the enabled flag — apps/teardown.py says so in terms, and warns that a hook "must not wait on that flag to decide it has been switched off" — and before the app's own third-party onDisable script, which the same comment notes can take real time. For the entire window this bug lives in, is_app_enabled still returns True. _CANCELLED is already correct at that instant.

No enable-side hook was added. apps.teardown has no enable seam and I did not invent one for a single invariant. Because the withdrawal is per run, a genuinely new review after re-enable carries a run id that was never cancelled and builds its own pool — pinned by a test that resets nothing by hand.

Evidence. Against this PR's own previous head 66906d4e2, with the race fix reverted and the disable hook still present: 3 failed, 5 passed; the failure message is the mechanism verbatim. Green after: 8 passed in the file, and 269 passed / 16 skipped across test_disable_retires_pool, test_review_pool, test_run_endpoints, test_backend_routes. The assertions are on what the queued run actually did — pool creation and review work — not on whether shutdown_pool() was called, since the blocker is precisely about what happens after that.

The PR description is re-synced to what now ships.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #5274 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5274: REBASE. Different goals on the same admission surface. If PR #6802 lands, PR #5274's local-review and local-fix entry points need the same _admit treatment, otherwise the app-disable guarantee has a new bypass; if PR #5274 lands first, PR #6802 must widen its gate to cover the new endpoints. Files: src/kiro_crew/apps/builtins/code_review_sage/backend/routes.py.
• This PR is OVERLAPPING with PR #6797. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6802: CONTINUE_DEVELOPMENT. Complementary halves of one audit, no shared code and no conflict; each app's worker teardown needs its own answer, so neither subsumes the other. Files: src/kiro_crew/apps/builtins/auto_improvement/backend/routes.py.
• This PR is OVERLAPPING with PR #8143. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6802: CONTINUE_DEVELOPMENT. Same file and same lock region, different goals, verified textual conflict. Both fixes are wanted; whichever lands second must re-place the _CANCELLED recheck against the other's lock scope rather than reapply its own hunk. Files: src/kiro_crew/apps/builtins/code_review_sage/backend/routes.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[bolichen97]

@leonlaiyc thanks for this. A repo-wide audit of open PRs found that it overlaps #8143 ("feat(sage): safely parallelize run-scoped reviews", @Premshay).

Shared files: src/kiro_crew/apps/builtins/code_review_sage/backend/routes.py and src/kiro_crew/apps/builtins/code_review_sage/tests/test_backend_routes.py.

The collision is in one block, _run_review_bg. Your PR adds if run_id in _CANCELLED: ... return as the first statement inside async with _RUN_LOCK:. #8143 narrows that same async with _RUN_LOCK: down to the single _claim_changes_under_lock call and dedents everything after it, including review_pool.get_pool(), out of the lock. The lines your recheck is anchored between are deleted there, so whoever lands second cannot reapply its hunk.

There is a semantic point too. Your guard exists because a run can wait on the whole-run lock for as long as the previous review takes. #8143 removes that wait, so after it the recheck belongs just before pool = review_pool.get_pool() rather than inside the now claim-only lock body. Both fixes are wanted and neither implements the other.

Suggested order: land yours first. It is 4 files, while #8143 is 12 files and still owes a reconciliation with #9077 and #9087 before review; #8143 then re-places the recheck against its new lock scope. Two things to do first: rebase, because your branch is far behind main and main rewrote this block in #7240 and #8186, so the recheck must be re-placed rather than reapplied; and drop the getattr(teardown, "register_app_disable_hook", None) feature detection, since main now provides that symbol.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

[bolichen97]

Rebased onto main 53987e75 by a maintainer as part of the 2026-09-08 open-PR audit. The branch was ~1000 commits behind.

Conflicts: none, clean rebase. I did verify the one production hunk still lands correctly after main rewrote this block twice: _RUN_LOCK still wraps the whole run and review_pool.get_pool() is still inside it after your _CANCELLED recheck, and app_lifecycle_lock / teardown.register_app_disable_hook both exist on main now.

Gates run locally (changed files only): black, isort, flake8 clean on the new test file; the three pre-existing files stay on .github/black-baseline.txt as before. pytest on the three touched test files: 354 passed, 1 skipped.

Two notes for you: the getattr(teardown, "register_app_disable_hook", None) feature detection is now dead weight since main provides the symbol, and #8143 moves get_pool() out of _RUN_LOCK, so whichever lands second must re-anchor the recheck.

Please review the rebase. A maintainer push makes the maintainer the last pusher, so a second approver is needed under the repo's last-push rule. Reply if anything looks wrong.