refactor: extract subagent lifecycle boundaries

[kyleseaman]

Stacked change: PR 2 of 7

Stack: #5277 → #5278 → #5279 → #5280 → #5281 → #5282 → #5283
Base: #5277
Next: #5279

Problem / Motivation

SubagentManager owns scheduling, lifecycle arbitration, slot accounting, terminal delivery, and session cleanup in one large state machine.

Why it matters

The existing race discipline is strong, but its boundaries need to be explicit before durable coordination can safely take authority.

What changed (motivation → approach → change)

• Extracts SubagentScheduler for admission queue and slot accounting.
• Extracts SubagentLifecycle around the existing exactly-once terminal and release tokens.
• Rewires the manager through those boundaries while preserving public behavior and callback ordering.
• Adds focused lifecycle, scheduler, and manager wiring coverage.
• Keeps the Windows zombie-watchdog unit test independent of shared executor saturation.

Tests

• test/test_subagent_scheduler.py
• test/test_subagent_lifecycle.py
• test/test_subagent_boundary_wiring.py
• Existing stop, cancellation, and context-group tests
• test/test_mcp_gatewayd_coverage.py

Manual verification

Not applicable; behavior is covered through deterministic backend lifecycle tests.

Related Issues

no linked issue: this stack implements the locally reviewed durable run coordinator RFC.

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] b14126d

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of b14126d3c35de48593c9122b0922ed3f563ab7e8 — 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. Emitting the review.

First-Principles-Verdict: CONCERNS

A backend "refactor preserving public behavior" ships an undeclared recovery-shutdown behavior change plus three frontend/test riders, two byte-matching commits already on main.

What this change ships

Intent: split SubagentManager's scheduling and terminal-lifecycle state into explicit boundaries ahead of the durable-coordinator RFC — a restructuring ADDITION.

1. Admission, queue, stagger, slot accounting move to new SubagentScheduler — justified (RFC PR 3 plan).
2. Terminal claims, report-task ownership, teardown gates move to new SubagentLifecycle — justified.
3. Old private manager fields become delegating views — justified compat; 7 setters have test-only writers.
4. Gateway shutdown during cancel-recovery now ends the run as "cancelled (recovery failed)" and notifies the parent instead of silently returning — undeclared behavior change.
5. Recovery slot-wait rechecks done/reap each poll — rides along.
6. _is_our_child resolved via the session helper loader — undeclared.
7. Theme overlays/audio tear down when a remote Crew activates — rides along; duplicate of main 680baf944.
8. Artifact widgets re-report height after iframe load — undeclared frontend fix riding along.
9. acp spawn pass_fds assertion made platform-aware + darwin test — rides along; duplicate of main b8209cabb.
10. Three test files reformatted and pruned from black-baseline.txt — rides along (AGENTS.md: own commit).

Watch

• Description says "preserving public behavior and callback ordering", but cancellation.py replaces if …_shutting_down: return with raise RuntimeError("gateway shut down before recovery respawn"), and test_recovery_observing_shutdown_finalizes_instead_of_returning pins the NEW outcome. Likely an improvement — declare it or split it out.
• Items 7 and 9 match merged main commits 680baf944 and b8209cabb by subject and content; if they are real branch commits rather than stacked-diff artifacts, rebase them away. Item 8 has no such commit and is a frontend behavior change no sentence of the description mentions.
• Description says "PR 2 of 7" while the RFC edit in this same diff says "PR 3 is prepared" and audits "PR 2 commit 53f365a17" — the framing and the shipped doc disagree.

Subtractions

• Delete SubagentScheduler.release() (subagent_scheduler.py:82) — grep scheduler.release(: 0 production callers; all 5 release sites (admission.py:519,545,766; run.py:454; terminal.py:438) deliberately route through the _release_slot facade seam and re-spell the decrement.
• Drop the manager setters nothing in src/ writes (_running_count, _last_spawn_ts, _queue, _report_tasks, _report_owners, _teardown_gates, _max_concurrent — grep \._<name>\s*= in src: 0 hits each outside the defining file); keep only the getters production coordinators read.
• Move the pure-reformat hunks of test_mcp_gatewayd_coverage.py, test_stop_kill_cancel.py, test_subagent_context_group_plumbing.py and their baseline pruning to their own commit, per AGENTS.md's black-baseline rule.

[FIRST-PRINCIPLES-REVIEWED] b14126d

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've verified the key seams myself: admission/drain/continuation equivalence, the rejection-path _release_slot+decrement (guaranteed True because occupy() always precedes it and sets _slot_released=False), the atomic try_reoccupy recovery path preserving the past-max_concurrent invariant, the _load_child_process_helpers() 4-tuple ordering matching every unpack site, and the bare-name resolution of _load_child_process_helpers in terminal.py via bind_component_globals (present in _COMPONENT_GLOBAL_BINDINGS and imported into subagent.py). The shutdown-raise recovery change is deliberate and pinned by a dedicated test asserting a single terminal report. All extracted scheduler/lifecycle methods are behavior-preserving against their originals, and are covered by characterization tests.

Nothing survives falsification at 80+, and I found no new grounded defect in Step 2.

No findings.

[OPUS-REVIEWED] b14126d

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

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

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound strangler-fig extraction, but the slot-release boundary is only half-moved and one shutdown-race behavior change is undeclared in a "behavior-preserving" refactor.

Watch

• Slot release is claimed at the boundary but decremented at call sites. SubagentScheduler.release() composes claim + decrement, yet only test_subagent_scheduler.py calls it; all three production sites keep if self._manager._release_slot(info): self._manager._scheduler.running_count = max(0, ...) (admission.py ×2, terminal.py). The PR exists to make boundaries explicit before the durable coordinator takes authority, and subagent.md now states the scheduler "owns … one-shot slot release" — but the claim/decrement pairing stays discipline-enforced across three duplicated sites, so PR 3+ inherits three migration points where a missed one silently skews capacity. Route production through release() (delegating from _release_slot_impl to keep the monkeypatch seam).
• Undeclared behavior change in the recovery-shutdown race. The description says "preserving public behavior and callback ordering," but cancellation.py replaces the old silent return on _shutting_down with raise RuntimeError("gateway shut down before recovery respawn"), which flows into the existing handler and now finalizes the run as "cancelled (recovery failed)" with a terminal report. Likely an improvement (no stranded running card), but it changes what a user sees when shutdown races a recovery — name it in the PR description or split it out so it isn't invisible inside a refactor.

Note: the website/theme and test_acp_client.py hunks in the given diff range are merge-base drift from main commit 680baf944, not part of this PR's commit; they were excluded from review.

[DESIGN-REVIEWED] b14126d

[github-actions]

Human judgment recorded

@kyleseaman marked the fable AI finding as false positive, not applicable, or explicitly accepted for f1d3a834966369e8fde4c958118042be96c7fd67.

Exact parent-to-head diff is 13 files (668 additions, 162 deletions) limited to the documented subagent extraction; the cited auth, registry, Electron, ACP, and workflow files are inherited base content and absent from this PR diff.

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

[kyleseaman]

/ai-review override design f1d3a83: Exact parent-to-head diff is 13 files (668 additions, 162 deletions) limited to the documented subagent extraction; the cited auth, registry, Electron, ACP, and workflow files are inherited base content and absent from this PR diff.

[github-actions]

Human judgment recorded

@kyleseaman marked the design AI finding as false positive, not applicable, or explicitly accepted for f1d3a834966369e8fde4c958118042be96c7fd67.

Exact parent-to-head diff is 13 files (668 additions, 162 deletions) limited to the documented subagent extraction; the cited auth, registry, Electron, ACP, and workflow files are inherited base content and absent from this PR diff.

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

The merge-base changed after approval.

[bolichen97]

LGTM

The merge-base changed after approval.

[bolichen97]

Review: extract subagent lifecycle boundaries

Reviewed origin/feat/run-coordinator-types...origin/feat/run-coordinator-boundaries (22 files, +813/−369) at 006d948d6. Because the PR's claim is behaviour preservation, the review was run as a differential: four BEFORE/AFTER harnesses against both checkouts, six mutation tests, and the repo's own gates. 5006 tests pass across all 67 files that touch SubagentManager.

Credit where due, verified by execution rather than assumed: callback ordering and the exactly-once tokens really are byte-identical. Identical output on both trees for plain _force_reap event order (tombstone → subagent_done → on_done), a reap racing _run's finally, a reap after _run already released, queue admission/drain, _unqueue queue-depth emits, cancel-recovery on both terminal and healthy paths, call_later delays across all 9 admission/drain state combinations, queued-conversation lookup, wave batch_members_pending, and cancel_all report-owner bookkeeping. A 12-run × 3-slot seeded contention harness with interleaved reaps produced identical running_count, exactly-once reporting and token state across 12 seeds. admission()/take_ready()/continuation_delay() are arithmetically identical to the deleted gates at every boundary (elapsed == stagger, stagger == 0, last_start in the future, max_concurrent == 0), and the moved guards are still armed (removing the slot-token re-arm from reoccupy fails 3 tests; removing the drain stagger gate fails 2; removing the recovery capacity guard fails 3).

One thing is not preserved, and it is the important one.

Blocking

1. subagent_manager/cancellation.py:91 — folding the terminal/shutdown check into the TOP of the new while True: capacity-wait loop puts _shutting_down ahead of raise RuntimeError("no free slot for recovery respawn"), converting "raise and fully finalize+report" into a silent return. The run loses its terminal record, its cancelled tombstone, its failure stat, and its terminal report to the parent.

Reachable because cancel_all_impl sets _shutting_down = True (:263) and then awaits real work — asyncio.gather(*followup_watchers) (:286) and await _announce_followup_failure(...) (:295) — before it reaches the _tasks cancellation loop (:307). A cancel-recovery parked in await asyncio.sleep(0.25) wakes inside that window. Executed with the real cancel_all(), max_concurrent=1, one slot-holding agent, and one agent carrying pending_followups so the slow branch is taken:

AFTER  (this PR):  info.done = False   info.error = ''                          _finalized = False  tombstone = 0  terminal_report = 0
BEFORE (base):     info.done = True    info.error = 'cancelled (recovery failed)'  _finalized = True   tombstone = 1  terminal_report = 1

Recovery's finally pops <id>:recovery from _tasks and the interrupted run's own _run finally already popped _tasks[info.id], so cancel_all()'s cancellation loop finds nothing to finalize either, and _resume_guarded's except CancelledError never runs. The run is left done=False forever: the parent card stays "running", cost is never recorded, and with no tombstone written the only recovery is restart orphan reconciliation. Static corroboration: the or self._manager._shutting_down still sitting in the raise guard at :103 is now provably dead code, since :94 returns first. The only test on this path patches _RECOVERY_SLOT_WAIT_SECS to 0.4s and therefore covers only the deadline branch, which is why 5006 tests stay green — and test_subagent_turn_resilience.py:635-641 asserts info.done is True after cancel_all(), the invariant this path now violates.

2. subagent_manager/admission.py:296 — the spawn admission gate no longer routes through _should_stagger_queue, so that documented override/monkeypatch seam is silently dead — directly contradicting the comment this PR adds at subagent.py:1676 ("Cross-boundary calls still route through this facade, preserving overrides and monkeypatch seams"). Executed on both trees with a forcing override installed:

BEFORE:  seam consulted 1×   →  info.queued = True   queue_len = 1
AFTER:   seam consulted 0×   →  info.queued = False  queue_len = 0

SubagentManager._should_stagger_queue (subagent.py:2117) and _should_stagger_queue_impl (admission.py:599) now have zero production callers. A subclass or integration that overrides admission policy is bypassed with no error — the spawn starts immediately and exceeds the caller's intended concurrency/stagger policy. The PR concedes it by rewriting five test call sites (test_spawn_reasoning_effort.py:378/386 → mgr._running_count = mgr.max_concurrent; test_subagent_context_group_plumbing.py:96/108/116 → patching mgr._scheduler.admission). Out-of-tree overrides get no such rewrite. The docstring that is the module's only prose spec for the gate (including the dynamic-subagent-sizing.md §5.3 citation) now sits on dead code, while the live policy at subagent_scheduler.py:38-43 has none.

3. subagent_manager/run.py:453 — the same bypass for slot release, and here the retained wrapper is a booby trap. All five production release sites were rewritten to _scheduler.release(info) (run.py:453, terminal.py:418, admission.py:519/542/734), leaving _release_slot with zero callers — but it still consumes the one-shot _slot_released token without decrementing anything. Executed at max_concurrent=1:

_release_slot(info)        -> True   running_count = 1   (token spent)
_scheduler.release(info)   -> False  running_count = 1   (slot unrecoverable)
admission().slot_free      -> False  (spawn queue starved forever)

That is exactly the permanent _running_count inflation the token was introduced to prevent, and the docs still point at the footgun: subagent.py:1419 says _slot_released is "claimed via SubagentManager._release_slot" and terminal.py:66 says the slot "has its own one-shot token (_release_slot)" — both now false. _release_slot_impl's own docstring still asserts "that caller decrements _running_count once and drains the queue." Worse, the new test codifies the divergence as intended (test_subagent_boundary_wiring.py:38-42 asserts _release_slot(info) is True and running_count == 1), so a future maintainer reading either the docstring or the test will pair the wrapper with a manual decrement — or call it alone. Three test files still hand-pair _release_slot with a manual decrement, encoding a production idiom this PR deleted. Fix: delete both, or make the wrapper return self._scheduler.release(info) so there is exactly one behaviour.

4. subagent.py:1731 — three of the eight compatibility views became getter-only, and none work on a __new__-built manager, contradicting the spec text this PR adds (docs/system-specs/modules/subagent.md: "Private manager views for the old counter, queue, report-task, and teardown-gate fields remain as compatibility adapters"). Executed on both trees, all eight assignments:

AFTER:   _report_tasks / _report_owners / _teardown_gates  -> AttributeError: property has no setter
         _queue / _running_count / _max_concurrent / …     -> AttributeError: _scheduler   (on a __new__ instance)
BEFORE:  all eight -> OK

_scheduler/_lifecycle were not added to _COMPONENT_TYPES (:1526), so the class's own documented lazy-composition escape hatch (__getattr__, :1537) does not cover them. Reads misreport too: because the property getter itself raises AttributeError, Python re-dispatches to __getattr__ and names the wrapper (AttributeError: _queue) instead of the real missing _scheduler — and getattr(mgr, '_running_count', default) silently returns the default rather than raising, so a getattr-with-default call site reads a fabricated value. The PR had to delete exactly these assignments from test_stop_kill_cancel.py:670/737; out-of-tree callers (this repo is a fork; the deleted lines carry # Fork adaptation comments) get no such fix. The read-only/read-write split is not a rule — it is a census of what happened to break: the 5 writable names are exactly the names something still assigns, and the 3 read-only names are exactly the ones whose last assignments this PR removed.

Should fix

5. subagent_scheduler.py:94 — remove() selects survivors with entry not in removed, i.e. full dict.__eq__ over the whole spawn kwarg set, where the base filtered on _preassigned_id. "task" is the first key inserted by enqueue({...}) and _preassigned_id the last, so CPython's dict_equal starts each comparison by memcmp'ing multi-KB prompt strings — cost now scales with prompt size, which the previous code was immune to. Measured through the unchanged public API on both checkouts (500-deep queue, 13.5 KB tasks, one distinct str per entry):

_unqueue x1:                    BEFORE 0.049 ms   AFTER 0.293 ms   (6.0x)
crew_chat.purge_slot loop x500: BEFORE 7.74 ms    AFTER 78.08 ms   (10.1x)
same at 64 KB tasks:            BEFORE 8.39 ms    AFTER 474.0 ms   (56.5x)

That is synchronous event-loop blocking in a function whose own comments say building a store inline "froze chats and heartbeats." It is also the wrong identity primitive: two value-equal entries are dropped together even though only one matches run_id (executed with {id:True} / {id:1} / {id:'z'} and remove('True') — BEFORE kept ['1','z'], AFTER kept ['z'], evicting an entry with no id match). Latent today because _preassigned_id is a unique uuid. A single-pass partition on the id is faster than BEFORE (measured 0.0019–0.028 ms) and obviously correct.

6. test/test_mcp_gatewayd_coverage.py:1897 — monkeypatch.setattr(gw.asyncio, "to_thread", …) mutates the stdlib asyncio module process-wide (verified: gw.asyncio is sys.modules['asyncio']), and running the probe inline removes the exact guarantee the production comment says the offload exists to protect. gatewayd.py:3937-3941 states "asyncio.all_tasks() must be read ON the loop… calling it inside the worker thread raises RuntimeError and would kill this watchdog on its first probe." Executed: real to_thread → RuntimeError: no running event loop; inline → returns 1. So a refactor that folds task_count into _snapshot_state — the single named regression — leaves both patched tests green, and the await asyncio.to_thread(_write_diagnostic, …) blocking file append now runs on the loop in a repo that maintains scripts/check_sync_io_in_async.py for exactly that rule. Blast radius: pytestmark = xdist_group('mcp_gateway') pins the module into one worker process, so any leaked background task (this file creates and cancels ~20) that calls asyncio.to_thread during the window silently runs blocking IO on the loop — surfacing as a timeout in an unrelated test. And it is inconsistent: test_healthy_server_only_writes_probe_baselines polls 300× against the real executor without the patch. The seam convention already exists (loop.run_in_executor(subprocess_executor(), …) from kiro_crew.executors, used for this exact kind of probe at cron.py:1526); give _zombie_diagnostic an injectable offload and patch that, scoped to gw exactly like this file's own monkeypatch.setattr(gw, "_write_diagnostic", …) two lines earlier.

7. Scope: 310 of the 355 changed test lines are pure black reformatting, and the one genuinely risky change is buried in it. Isolated by black-normalising each base file and diffing: test_mcp_gatewayd_coverage.py 196 changed / 9 semantic; test_stop_kill_cancel.py 118 / 22; test_subagent_context_group_plumbing.py 41 / 14 — ~26% of the whole 1182-line diff. AGENTS.md:358-361 is explicit: "If a file you touched is listed in the baseline, formatting it is welcome but optional — do it in its own commit", and :242 "One logical change per commit." This PR has one commit with no body. The pruning itself is correct and mandatory (I installed the pinned black==26.3.1 — local was 24.10.0, and setup.cfg:161-163 warns they disagree by ~680 files — and confirmed all three files are clean and the gate exits 0), so the defect is the bundling, which is how the process-wide to_thread patch ships unreviewed at 2% of its hunk's diff. It also makes the PR non-revertible in halves.

8. test/test_subagent_context_group_plumbing.py:131 — _max_concurrent = 4 / _running_count = 0 / _spawn_stagger_secs = 0.0 are now INERT, because the admission MagicMock installed at :121 also hijacks take_ready on the drain path. Executed on both trees with hostile values (_max_concurrent=0, _running_count=99, _spawn_stagger_secs=1e9): AFTER the drain still fires; BEFORE drain fired: False. So these tests can no longer catch a capacity or stagger regression, and the mid-test admission.return_value = … flip at :134 exists only to undo the mock the test installed for the spawn half. That is the clearest evidence the extraction made the seam coarser, not finer: one patch point now governs both _spawn_impl and _drain_queue_impl, so every queue-round-trip test must be choreographed in two phases. The same PR shows the better idiom two files over — test_spawn_reasoning_effort.py:378 uses the coupling-free mgr._running_count = mgr.max_concurrent. The fabricated AdmissionDecision(True, False, 0.0) is also an unreachable triple: the real admission() always returns retry_after=None when slot_free is False.

9. subagent_lifecycle.py:105 — close_teardown pops the gate only when it is still the registered one, replacing the base's unconditional pop and weakening the invariant the deleted comment stated ("Set BEFORE the entry is dropped… one arriving after finds no entry, which now means exactly 'nothing left to wait for'"). Executed: open twice for one run id, close with the first gate → g1.set True, g2.set False, left_behind_unset True; the base leaves nothing behind. If two _run(info) cycles for one id ever overlap, waves.py:303 blocks on asyncio.wait_for(gate.wait(), timeout=_RESET_TIMEOUT + 30) on a gate nobody will set, stalling the wave digest. I could not construct a production interleaving (the recovery handshake serialises on asyncio.wait({orig_task}, …)), so this is ranked low — but the guarantee is now strictly weaker than the comment the PR kept, and _teardown_gates has no other pruner, so the dict grows unboundedly over a long-lived gateway. The identity check is arguably the better semantics; it is the undeclared, untested change that is the problem.

Notes

admission.py:381 — delay = decision.retry_after or 0.0 coalesces on falsiness for an Optional[float] whose None and 0.0 cases are both real; dead today only via an unenforced cross-field invariant, and the same file gets it right 240 lines later (:623, :692 both use is not None). If admission() is ever mocked (the PR's own new test does exactly that) or returns slot_free=True, retry_after=None, this silently becomes call_later(0.0, …) — an immediate burst defeating the stagger gate on the very path that enforces it. · subagent.py:1719 — the _spawn_stagger_secs setter silently clamps with max(0.0, value) where the base stored verbatim, so the view does not round-trip (-1.0 → 0.0, and max(0.0, nan) → 0.0); the clamp is redundant with __init__ and produces the only decision divergence in the whole admission machine (54/2520 cases, all requiring now - last_start < 0). · subagent.py:1598 — self._coordinator = coordinator or MemoryRunCoordinator() is written and never read anywhere in src/; the base commit landed a typed coordinator that already defines admission (ObservedState.QUEUED + SubmitRun.accepted), fenced one-shot claims (RunFence/OwnerLease), and idempotent terminal completion (COMPLETION_REPLAY vs OUTCOME_CONFLICT) — and this PR re-implements all three as raw mutable bools (info.done, _finalized, _slot_released) with no fence, version or epoch. Two vocabularies for one lifecycle, added one commit apart, so PR 4's migration must reconcile a bool-flag model against a versioned/fenced one instead of swapping an implementation. · subagent_manager/terminal.py:489 — the new comment says "Resolve through the session facade so this leaf keeps no ACP edge", but all four helpers still resolve to kiro_crew.acp.client at runtime (verified: [f.__module__ for f in h] is ['kiro_crew.acp.client'] × 4). The gate is diff-scoped, so touching this file would have turned it RED; routing through session.py — already baselined for 12 such edges — keeps current == baseline. cron.py:1503-1573 performs the byte-identical import and is baselined. That is a gate defeat, not a boundary fix, and platform_compat.py already owns the receiving API (process_descendants, get_process_start_id, kill_process_tree_pinned). It also makes session.py's untyped positional 4-tuple a public contract for an unrelated subsystem — this PR already appended the 4th element and had to add a throwaway _ at session_lifecycle.py:379, and a swap would type-check (all four are Callable[..., Any]) and pass the suite while the resulting TypeError is swallowed by except Exception: logger.exception("Reaper: SIGKILL failed"). · The eight compat properties have no ratchet, so the spec's "must not regain independent state" is prose only; this repo already ships the mechanism three times (check_agent_sdk_boundary.py + baseline + test, test_workflows_architecture.py's AST layering scan, check_sync_io_in_async.py). · subagent_scheduler.py and subagent_lifecycle.py land at the top level of a 194-module flat namespace while every production importer and consumer lives inside subagent_manager/, which is already a package for exactly this — and inconsistently with run_coordinator/, the sibling this same RFC created one PR earlier as a package. · SlotOwner declares only _slot_released and no id, so the scheduler structurally cannot own the invariant the spec assigns it — it mutates a foreign private attribute and cannot key its own token; the sibling LifecycleInfo in this same PR does carry id, proving the id-keyed shape was available (executed: an external info._slot_released = True desyncs running_count permanently with no way for the scheduler to notice). · SubagentScheduler(stagger_seconds=0.0) at :1572 is a placeholder overwritten 97 lines later through the compat setter, so during __init__ the boundary reports "no stagger" where the base raised AttributeError — loud → silent. · Five mutators for one counter (occupy, reoccupy, try_reoccupy, claim_release, release) with reoccupy public but called only by try_reoccupy three lines below; max_concurrent unclamped while its sibling stagger_seconds is floored, and both sibling pools clamp. · AdmissionDecision is the third class of that name in src/, and two different AdmissionDecision types are consumed ~90 lines apart in one function body; should_queue is fully derivable from the other two fields, which is why the tests can construct a self-inconsistent triple. · Three byte-identical one-shot latch bodies across the two new files, while the repo already has three claim_*() -> bool helpers (slack/gateway.py:981, slack/outbound.py:136, dashboard/crash_dump_store.py:544). · begin_reap/mark_reaped/await_report are one-line statics over state the boundary does not own, and await_report splits an 8-line docstring from the asyncio.shield it explains. · test_subagent_boundary_wiring.py:22 is tautological — assert manager._queue is manager._scheduler.queue can never fail because the property body is that expression; a test that would bite mutates through one side and observes the other. It also breaks on the first queued cancel, since remove() rebinds self.queue to a fresh list. · docs/request-for-change/rfc-durable-run-coordinator.md:23 advances the prose audit basis to "PR 2 commit 53f365a17" — a SHA that resolves to no object in this repo (the real base is 2c7e842c0) — while last-audited: 2026-08-22, audited-at: c8eda3c6f (also unresolvable) and implementation-prs: [] are left stale, against docs/request-for-change/README.md's explicit same-PR rule. docs-lint.sh does not inspect front matter, so CI catches none of it. · terminal.py:489's in-method import dropped the # circular import marker that made it a permitted AUTOSDE.yaml exception — and there is in fact no cycle: verified that importing kiro_crew.session loads neither subagent_manager nor subagent, and both import orders work at module scope.

Gates, for the record: pinned black==26.3.1 clean on all 18 changed files and check_black_formatting.py green; check_agent_sdk_boundary.py, check_sync_io_in_async.py, check_loop_bound_locks.py, check_testpaths_coverage.py, check_lockdown_before_publish.py, check_brand_name.py, check_harness_parity.py, check_changelog_history.py, docs-lint.sh, scrub-lint.sh all pass; pinned flake8 7.1.0 and isort 6.0.0 clean; pinned mypy 1.14.1 --platform linux → "no issues found in 1248 source files"; new modules at 98%/94% coverage; CHANGELOG correctly untouched; the subagent spec updated in the same commit.

---

_{Execution-verified AI-assisted review (Claude Code) across 10 angles plus a gap sweep, run against local checkouts of both this branch and its base at 2c7e842c0; nothing modified in either tree, nothing posted elsewhere. Also cleared so you needn't re-check: no BEFORE usage of any of the 8 moved attributes is orphaned (I enumerated all of them); read-modify-write through every view still works; _load_child_process_helpers's 4-tuple unpack order is correct and no test injects a 3-tuple; __init__ ordering is safe; await_report preserves the cancellation contract exactly; and all *_impl methods have __globals__ rebound by bind_component_globals, so kiro_crew.subagent.<name> patches still reach coordinator internals.}

[kyleseaman]

Bolin review disposition for the current stack

Every numbered item from the September 2 review was rechecked.

1. Fixed: recovery cancellation and shutdown paths finalize and report instead of silently returning.
2. Fixed: admission still routes through the manager facade, preserving overrides and monkeypatch seams.
3. Fixed: every release site delegates through the facade and decrements capacity exactly once.
4. Fixed: compatibility views support assignment and lazy composition on minimal managers.
5. Fixed: queue removal selects by run ID and does not compare full queued payload dictionaries.
6. Fixed: watchdog tests retain the real offload boundary and patch only the project seam.
7. Fixed with deterministic lifecycle coverage.
8. Fixed with deterministic cancellation and slot-accounting coverage.
9. Rebutted: teardown state is removed only after an identity-checked ownership match. The stricter pop is intentional because an unconditional pop can erase a newer run that reused the ID.

The final static-gate regressions found during submission were also fixed in this layer: queue-delay calculation no longer references a removed local, and lazy boundary composition is correctly typed.

Current submitted head: b14126d.

[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 #5277 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 #5277: MERGE_DISCUSSION. Direct stack dependent, not a duplicate: complementary phase built on this PR's branch. Files: src/kiro_crew/subagent.py.

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

[dwu96]

🤖 Kiro Crew [operator: dwu96#8c08bee0]: This PR has been inactive for 7+ days. I reviewed the blockers and they require your input rather than a mechanical fix. CI is not the problem: PR Readiness is green on b14126d3c35de48593c9122b0922ed3f563ab7e8, the readiness: passed label is current, and GPT 5.6, Opus 4.8 and UX all report no blocking findings.

What needs your decision:

1. The stack root, not this PR, carries the conflict. This PR's base is feat/run-coordinator-types (feat: add typed run coordinator contract #5277), and measured against it there is nothing to resolve here: base tip 43be00ad is a direct ancestor of head b14126d3, one commit ahead, a clean fast-forward. The merge conflict label (added 2026-09-05) reflects feat: add typed run coordinator contract #5277 being 751 commits behind main and conflicting there. Re-stacking feat: add typed run coordinator contract #5277 → refactor: extract subagent lifecycle boundaries #5278 → feat: add sqlite run coordinator shadow #5279 → feat: make subagent commands idempotent #5280 → feat: persist subagent completion delivery #5281 → feat: recover subagents from the run coordinator #5282 → fix: harden run coordinator lifecycle races #5283 onto current main is yours to sequence; rebasing this branch onto main on its own would absorb feat: add typed run coordinator contract #5277's content and move the base branch out from under feat: add sqlite run coordinator shadow #5279 and everything below it, so automation will not do that.

2. Two review lanes ask for opposite things about SubagentScheduler.release(). Design wants production routed through it ("Route production through release(), delegating from _release_slot_impl to keep the monkeypatch seam") because the claim/decrement pairing currently stays duplicated across admission.py ×2 and terminal.py. First Principles wants it deleted outright ("0 production callers; all 5 release sites deliberately route through the _release_slot facade seam"). Both cannot be satisfied, and which one is right is a call about where slot authority should sit before the durable coordinator lands.

3. The recovery-shutdown behavior change is undeclared. cancellation.py replaces the silent return on _shutting_down with raise RuntimeError("gateway shut down before recovery respawn"), so the run now finalizes as cancelled (recovery failed) with a terminal report, while the description says the refactor preserves public behavior. Design and First Principles both ask you to name it in the description or split it out. Either choice changes what this PR claims to ship.

4. First Principles reads three hunks as riders that duplicate already-merged main work: the theme overlay and audio teardown (680baf944), the acp spawn pass_fds platform guard (b8209cabb), and the artifact-widget height re-report, which no sentence of the description covers. Design reads the website/theme and test_acp_client.py hunks as merge-base drift instead and excluded them from review. Confirm which reading is correct — it depends on how you staged the branch, which I cannot infer from the diff.

5. First Principles also asks for two subtractions and a commit split: drop the seven manager setters with no src/ writers, and move the pure-reformat hunks in test_mcp_gatewayd_coverage.py, test_stop_kill_cancel.py and test_subagent_context_group_plumbing.py plus their black-baseline.txt pruning into their own commit per AGENTS.md. Both remove or restructure surface you designed, so they are scope calls, not mechanical edits.

Also noted for whoever picks this up later: the Coverage Gate failure on this head is not real — it comes from the superseded CI run 33684965789, which was cancelled wholesale, and the winning run 33684965959 is green.

When you've addressed these, the pipeline will re-assess on its next cycle. Add pr-no-autofix if you'd prefer it left alone entirely.