fix(auto-improvement): stand the app's workers down when it is disabled

[leonlaiyc]

Problem / Motivation

_require_enabled closes the API, not the work.

Every auto-improvement route is wrapped in it, so disabling the app makes them all
answer 403 app_disabled. Both in-process workers are untouched:

• a PR watcher keeps running agent turns inside a per-PR clone on a timer
  (DEFAULT_NUDGE_INTERVAL_S apart), so it goes on acting on the operator's
  repositories;
• the run supervisor's worker thread keeps going, holding the clone lock and
  spending budget, and the agent/measurer subprocess it spawned keeps running.

The operator sees the app switched off and the work carries on.

register_routes already stands both down on gateway shutdown (— _stop_watchers, and
_stop_run added by #6701). aiohttp fires on_cleanup when the gateway stops and at
no other time, so neither reaches a disable. #6701's review said so:

The problem statement says the run is orphaned "when the gateway shuts down or the
app is disabled", but an on_cleanup hook fires only at gateway shutdown; disabling
the app just 403s the routes (_require_enabled, routes.py:131) while the run keeps
spending budget. The disable half has an existing, purpose-built mechanism this change
doesn't use: register_app_disable_hook / notify_app_disabled
(src/kiro_crew/apps/teardown.py:395,419; grep shows 1 registered consumer, issue_radar's
crew_runtime.py:1045). One unfixed sibling path of the same root cause — either wire it
or declare disable out of scope the way the join-window gap already is.

Verified on current main (72e429791): grep register_app_disable_hook across
apps/builtins/auto_improvement/ returns nothing. The registry has exactly one consumer,
Issue Radar.

Why it matters

The seam's own docstring states the harm this closes, and states it 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.

Auto-improvement's run is precisely such a worker: #6701's own hook docstring names what
it holds — the clone lock, and budget. So the disable path today spends money and holds a
lock on behalf of an app the operator has already turned off, and reports nothing, because
the only surface that would say so is returning 403.

teardown.py also orders notify_app_disabled first, ahead of the app's own
onDisable script and backend teardown, for exactly this reason — "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.

What changed (motivation → approach → change)

Symptom: disabling the app leaves both workers going. Root cause: their stops are wired to
one of the two ways the app can be switched off. Fix: wire them to the other one too,
through the mechanism that already exists for it, with the same bodies.

register_routes now also registers an app-disable hook:

async def _stop_on_disable(_app: str) -> None:
    try:
        pr_watchers.get_registry().stop_all()
    except Exception:
        logger.warning(...)
    try:
        await asyncio.to_thread(runner.get_supervisor().stop)
    except Exception:
        logger.warning(...)

Each worker is signalled the way its own on_cleanup hook signals it, and for that hook's
reasons. stop_all only sets flags and joins nothing (— "Nothing is joined — a route must
not block for a turn to finish"), so it runs inline; stop is idempotent (a no-op when
idle) and bounded (it signals the run and joins for at most STOP_JOIN_TIMEOUT_S), so it
goes off the loop because that join blocks.

The two are contained separately, which is the one thing this hook has to do
deliberately that the shutdown path gets for free: on_cleanup holds two independent
hooks, so a raising watcher stop cannot skip the run stop there. Collapsed into one
function, a single try would have made exactly that regression, and a test pins it.

Nothing new is introduced: no state, no config key, no constant, no route, no change to
RunSupervisor or PRWatcherRegistry.

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 shutdown hook
is the only stop there is — the pre-registry behaviour, not a regression. It is wrapped in
its own try, matching the existing lifecycle-hook registration, because a failure here
must never break gateway startup.

Registering in register_routes rather than per-cycle is deliberate. Issue Radar
re-registers from its watchdog because the registry is process memory and a restart empties
it; register_routes runs once per gateway process, which is the same guarantee arrived at
more cheaply.

docs/system-specs/modules/auto-improvement.md gains a paragraph beside the
_require_enabled line it corrects — that sentence, on its own, reads as though 403 is what
disabling does.

One line is pruned from .github/black-baseline.txt. routes.py is listed there but is
already black-clean, and the gate scopes to changed files, so touching the file surfaces the
stale entry as 1 graduated entry to prune and fails until it is removed. No reformatting
rides along: the whole routes.py diff is the 36 added lines.

Tests

New src/kiro_crew/apps/builtins/auto_improvement/tests/test_disable_stops_run.py,
3 tests, mirroring test_shutdown_stops_run.py's harness so the two paths are pinned the
same way. Each drives the real register_routes on a bare aiohttp application and starts a
run that parks in driver.run, so the supervisor genuinely owns a live thread rather than
a mock.

• Disabling stops an active run — fires teardown.notify_app_disabled(APP_NAME),
  exactly what the disable request does and at the point it does it, then asserts the
  driver was asked to stop and the supervisor reached STATUS_DONE.
• Disabling also stops the PR watchers — the second worker, and the one that acts on
  the operator's repositories rather than just spending budget.
• A failing watcher stop still stops the run — stop_all is made to raise, and the
  run must still be signalled. This is the containment guard: it fails if the two calls
  are ever collapsed under one try.
• Disabling while idle is a no-op — fired twice, because an operator may disable an app
  that is doing nothing, repeatedly. This is the over-fix guard, and it is the one test
  that passes on main as well: the fix must not turn a harmless disable into an error.
• register_routes wires a disable hook — a structural guard matching the one
  test_shutdown_stops_run.py carries for on_cleanup, so an edit that drops the
  registration fails here instead of silently letting a disabled app keep running.

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_stops_an_active_run
  AssertionError: disabling the app did not ask the driver to stop — the run kept going
FAILED test_disabling_the_app_also_stops_the_pr_watchers
  AssertionError: disabling the app left the PR watchers running
FAILED test_a_failing_watcher_stop_still_stops_the_run
  AssertionError: a failing watcher stop swallowed the run stop
FAILED test_register_routes_wires_an_app_disable_hook
  AssertionError: assert None is not None
4 failed, 1 passed

The one that passes on main is the idle no-op guard, which is the point of it.

Green on the branch: 5 passed. Whole app suite: 972 passed, 63 skipped. Gates:
check_black_formatting.py pass, flake8 src/kiro_crew/apps/builtins/auto_improvement/
clean, isort --check-only src/kiro_crew test clean, mypy src/kiro_crew — "Success: no
issues found in 1170 source files".

Manual verification

N/A — unit coverage sufficient: the tests exercise the real registration and the real
supervisor over a genuinely running worker thread, 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

Closes the unfixed sibling path named in the review on #6701.

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

[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 #6802 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 #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.

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

[bolichen97]

Rebased onto main 53987e75 by a maintainer as part of the 2026-09-08 open-PR audit.

The commit itself rebased cleanly. One follow-up was required by the rebase: .github/black-baseline.txt was reset to main's version, dropping this PR's prune of the auto_improvement/backend/routes.py entry. Main has since added code to that file (the IsolationProbeError branch, #8151) that is not black-clean, so the file no longer graduates and keeping the prune would fail the black gate as a new offender. No behaviour change: the disable-hook fix, its test, and the spec update are untouched.

Gates run locally on the changed files only: black, isort, flake8, and test_disable_stops_run.py plus test_shutdown_stops_run.py and test_finding_detail.py (38 passed).

Please review the resolution. 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.

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 92be4c8

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

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

Design verification is complete: the teardown seam exists with exactly the quoted semantics, _stop_on_disable mirrors the existing shutdown hook bodies, disable does not clear the hook registry (only uninstall does), and register_routes runs once per gateway process unconditionally — so the once-per-process registration holds. The one real issue is drift between the description and the authoritative patch.

Design-Verdict: CONCERNS

Right seam, root-cause fix, well pinned — but the pushed diff is not the branch the description validates.

Watch

Description↔diff drift: the description says "One line is pruned from .github/black-baseline.txt" — the patch contains no such hunk, and routes.py is still on line 19 of the base's baseline — while it also says "No reformatting rides along: the whole routes.py diff is the 36 added lines," yet the patch reformats _handle_pr_status. By the author's own account of the gate ("fails until it is removed"), touching a now-clean baselined file without the prune fails the black check, so the claimed green gates were run on a different branch state than what was pushed.
Clears when: the baseline prune is in the pushed diff (or the description matches the actual patch) and the black gate is green on this HEAD.

[DESIGN-REVIEWED] 92be4c8

[github-actions]

First Principles Review (Fable 5, fork) — 🔴 BLOCK

Premise-level review of 92be4c84eeb6cd804bfa75ee23bf423cd38e0a6f 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 evidence is gathered. Composing the review.

First-Principles-Verdict: BLOCK

The diff ships a reformat hunk the description explicitly denies, and omits the black-baseline prune it explicitly claims — framing contradicted by the diff.

Not justified as shipped

6. _handle_pr_status reformat — rides along; description says "No reformatting rides along: the whole routes.py diff is the 36 added lines."

What this change ships

Intent: make disabling the auto-improvement app actually stop its two in-process workers, not just 403 its routes — a FIX.

1. Disabling the app now stops an in-flight run at the click — justified
2. Disabling the app now stops the PR watchers at the click — justified
3. A failing watcher stop can no longer skip the run stop — justified
4. Spec paragraph beside the _require_enabled line it corrects — justified
5. Five new tests pinning the disable path, red-before shown — justified
6. _handle_pr_status error response re-wrapped — rides along, denied by the description

(Verified in base: the seam exists at teardown.py:395/419 with exactly 1 consumer, issue_radar crew_runtime.py:1284 — grep register_app_disable_hook, matches the description's count; notify_app_disabled fires first, before the enabled flag, teardown.py:138-148; twin harness test_shutdown_stops_run.py exists.)

Blockers

Rider reformat with contradicted framing. Exception (b), all four parts by reading: the title says fix; the _handle_pr_status hunk (routes.py:475-481) is unrelated to disable wiring; its zero option is free — routes.py is baselined (.github/black-baseline.txt:19) and the gate's own doc says "Touching one is still free" (check_black_formatting.py:29-31); items 1-3 remove the defect without it. Worse, the claimed prune ("One line is pruned from .github/black-baseline.txt") is absent — grep black-baseline in the patch: 0 matches — and the graduated check is unscoped (baseline - unformatted, check_black_formatting.py:215), so if this hunk was the file's only violation the gate goes red exactly as the description predicts, with no prune shipped. Delete the reformat hunk, or ship the prune the description already describes.
Clears when: the routes.py:475-481 hunk is dropped, or the baseline entry src/kiro_crew/apps/builtins/auto_improvement/backend/routes.py is removed in the same diff.

Subtractions

• Drop the _handle_pr_status reformat hunk (routes.py:475-481) — cosmetic, unrelated, and the fix is complete without it.

[FIRST-PRINCIPLES-REVIEWED] 92be4c8

[github-actions]

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

Reviewed 92be4c84eeb6cd804bfa75ee23bf423cd38e0a6f 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/auto_improvement/backend/routes.py:1537 -- disable leaves a worker-restart race (origin: validation)

pr_watchers.get_registry().stop_all()
Concurrent /run after this hook but before disable_app() -> enabled check passes -> new worker survives the completed disable.
Anchor: residual/security
Fix: Prevent new worker admission throughout disable, then stop existing workers.
[BLOCK-MERGE] 92be4c8
[GPT-REVIEWED] 92be4c8

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

The fenced finding F1 targets the new _stop_on_disable hook. I've confirmed the disable ordering and the window it describes.

Conditions confirmed this run:

• Teardown runs notify_app_disabled → _stop_on_disable (worker stop) at teardown.py:148, and the enabled flag is written only later by disable_app at routes.py:1674 — after the entire remainder of teardown_app_runtime (onDisable script up to 30s, stop_app_backend, deregister_app). The TOCTOU window is not a tick; it spans the multi-second tail of teardown.
• The auto-improvement /run route is gated only by _require_enabled reading is_app_enabled (routes.py:1442, :136); it is not taken under app_lifecycle_lock (held only in handle_disable_app, routes.py:1651), so a concurrent /run in that window passes the enabled check and starts a fresh supervisor run.
• The new run is a gateway in-process thread; nothing in the remaining teardown steps (backend-process stop, deregister) joins or stops it, so it survives the completed disable and keeps spending budget / holding the clone lock — the exact withdrawn-permission harm the fence classes unbounded.

FLAG requires a rarity argument that the condition combination is extreme. It is not: the window is a normal multi-second teardown, and the triggering /run is an ordinary unlocked request — a plausible interleaving, not a contradictory-timing artifact. I cannot complete the FLAG evidence record, so the verdict holds as written.

Harm rung: UNBOUNDED (worker runs agent turns on operator repos after permission withdrawn). Conditions: teardown.py:148 (stop before flag) vs routes.py:1674 (flag write); /run gated only at routes.py:136/:1442, outside app_lifecycle_lock. Real fix (an admission gate held closed across the whole disable sequence) is bounded and does not reduce the harm below unbounded.

[ADJUDICATION] 92be4c8 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 92be4c8
[ADJUDICATION-FENCED] 92be4c8 fenced=1 flagged=0
UPHOLD-FENCED F1 src/kiro_crew/apps/builtins/auto_improvement/backend/routes.py:1537 -- A concurrent /run during the multi-second gap between the worker-stop hook and the enabled-flag write passes _require_enabled (not under the lifecycle lock) and starts a supervisor run that survives disable; the interleaving is plausible, not extreme.
[GPT-ADJUDICATED-FENCED] 92be4c8