fix: harden run coordinator lifecycle races

[kyleseaman]

Stacked change: PR 7 of 7

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

Problem / Motivation

Adversarial review of the assembled stack exposed narrow races around pre-start leases, terminal result filling, batch delivery, stale recovery fences, PID reuse, and uncertain transport responses.

Why it matters

These windows occur at crash and concurrency boundaries, where a false success, duplicate child, lost completion, stale acknowledgement, or leaked capacity slot would undermine the coordinator's core guarantees.

What changed (motivation → approach → change)

• Heartbeats admitted work before queue and approval waits can expire its execution lease.
• Makes stored command results authoritative on exact replay while reconstructing terminal recovery only when the durable result is empty.
• Registers live batch identity before a rejection event becomes claimable, preserving silent delivery and accounting across drains, retries, restart, and digest settlement.
• Preserves a registered non-terminal manager lifecycle when post-registration work raises, reporting typed outcome uncertainty instead of cancelling the child.
• Tightens terminal/outbox fencing, protected PID/start-identity checks, bounded shutdown settlement, and canonical rejection replay metadata.
• Keeps protected terminal events withheld without letting one failed cleanup stall unrelated completions.
• Propagates cancellation and process-control BaseException signals instead of converting them into durable spawn failures.
• Preserves a manager-recorded terminal rejection when post-registration work raises; only an unregistered failure synthesizes an exception-derived outcome.
• Retains an unsettled terminal manager rejection through orderly shutdown until its fenced completion is durable or a newer durable owner supersedes it.
• Closes late coordinator-claim and deferred-delivery acknowledgement races without allowing local and recovery paths to report the same completion.
• Retains durable delivery debt for failed or stopped events until the idle turn consumes the event.
• Adds deterministic regressions for each reviewed crash window and race.

Tests

• Expanded authority, MCP uncertainty, coordinator contract, delivery, recovery, security, error-contract, and protected-process teardown regression suites.
• The current stack-tip focused coordinator suite passes: 507 passed, 1 valid platform skip.
• Black, subprocess encoding, isort, flake8, Linux-parity mypy, docs, brand, harness-parity, and public-tree scrub gates pass at the stack head.

Manual verification

N/A — this PR hardens backend crash and concurrency paths with deterministic regression tests.

Related Issues

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

Pattern harvest

Rule candidate: review-prompt
Pattern: every crash or cancellation path must preserve its durable fence and keep terminal and delivery settlement monotonic and single-consumer.

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 — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging c2524d909f1bfe8560821d529c692f6c43ee80a4.

This comment is updated in place on each push.

BLOCKING -- src/kiro_crew/subagent_command_authority.py:829 -- Batch metadata is registered after the event becomes claimable

completed = await self._coordinator.complete(...) precedes self._manager.prepare_coordinator_rejection(...)
Batch rejection -> periodic drain claims during this await -> completion loses batch identity, so wave accounting never reaches its total and the digest remains stranded.
Anchor: residual/crash-data-loss-corruption
Fix: Move prepare_coordinator_rejection(...) before complete(...).
[BLOCK-MERGE] c2524d9
[GPT-REVIEWED] c2524d9
False positive or not applicable? A repository writer can comment:
/ai-review override gpt c2524d909f1bfe8560821d529c692f6c43ee80a4: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound hardening of an RFC'd coordinator — but the cancel path trades heartbeat renewal for a fixed 22-minute lease, restating a remote timeout as a magic number.

Watch

• _CANCEL_CONTROL_LEASE_SECS = 22.0 * 60.0 hard-codes "the manager's bounded 20-minute parent-delivery wait" (_ON_DONE_TIMEOUT = 1200.0 in subagent.py) plus margin, in a different module, as prose-comment coupling — if _ON_DONE_TIMEOUT grows, the lease silently under-covers and a live cancel gets torn from its owner mid-teardown; if it shrinks, a gateway crash mid-cancel leaves that command claimed and unreplayable for up to 22 minutes. The class already owns lease-heartbeat machinery (_start_execution_heartbeat renews a 90s execution lease); a renewed short lease dies with its owner instead of wedging for the worst-case duration.

Suggestions

• Derive the cancel lease from _ON_DONE_TIMEOUT + a named margin (or reuse the existing heartbeat) so the two limits cannot drift apart silently.

[DESIGN-REVIEWED] c2524d9

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of c2524d909f1bfe8560821d529c692f6c43ee80a4 — 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 in. The backend hardening matches the description and the spec/RFC were updated in the same commit; the notable gaps are undeclared frontend riders and one newly-orphaned public method.

First-Principles-Verdict: CONCERNS

A backend race-hardening fix ships two unrelated, undeclared frontend fixes and orphans announce_durable_rejection, which now has zero production callers.

What this change ships

Intent: make subagent run admission, rejection, delivery, and teardown survive crashes and concurrency without false success or duplicate delivery — a FIX.

1. Queued/approval-gated runs keep a lease heartbeat before execution — justified
2. Manager rejections commit a durable failed run + outbox event; non-batch ones marked already-delivered — justified
3. Exact replay returns the stored/terminal result instead of re-spawning — justified
4. Command result fill is fenced by run owner/epoch against recovery takeover — justified
5. Shutdown settlement bounded to 3 attempts, then logs debt and releases the lease — justified
6. Cancel commands hold a 22-minute claim covering the parent-delivery wait — justified
7. BaseException propagates instead of becoming a durable spawn failure — justified
8. Reaper proves pinned PID/start-identity death (and awaits SIGKILL reap) before delivery — justified
9. Legacy import can no longer mutate an existing coordinator row — justified
10. Theme overlays/audio torn down on remote-Crew switch; artifact height reporter re-posts after load; a macOS pass_fds test — rides along, undeclared

Watch

• Item 10: three real fixes (each with a nameable harm, so not deletable) whose defects the description never mentions — "this PR hardens backend crash and concurrency paths" — hidden inside a 4,000-line backend PR where no frontend reviewer will look for them. They belong in their own PRs.
• The bulk black-reformat of test_subagent.py/test_process_tree_kill.py plus baseline pruning rides along; AGENTS.md asks for that in its own commit.

Subtractions

• Delete SubagentManager.announce_durable_rejection (src/kiro_crew/subagent.py:2374) and announce_durable_rejection_impl (src/kiro_crew/subagent_manager/admission.py:45): this PR replaced every production caller with prepare_coordinator_rejection/deliver_coordinator_event — grep announce_durable_rejection in src/ now finds only the definition and its delegate (0 consumers; remaining hits are tests).

[FIRST-PRINCIPLES-REVIEWED] c2524d9

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

The discovery pass reported no candidates, so Step 1 has nothing to falsify. I independently reviewed every production hunk in the diff (memory.py result-fill fence, models.py additive field, slack/gateway.py removal, subagent.py coordinator guard, subagent_command_authority.py failure-persistence path, and subagent_manager/terminal.py reap logic). The result_fill path in memory.py is correctly fenced by owner/lease-epoch for execution commands; the delivery-state wiring round-trips consistently; the SIGKILL reap wait is non-blocking on the loop; redaction is strengthened, not weakened; and no harness-parity or security invariant is crossed. Nothing rose to a grounded, reachable defect at the required bar, and I found no new material meeting the Step 2 threshold.

No findings.

[OPUS-REVIEWED] c2524d9

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

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

[kyleseaman]

Resolved the GPT 5.6 blocking finding from c715bc970a014c9cd04d74055093cdfe91d2d2de in a6c254ea5c263fe0025667154263d0e60b7002a7. A rejected durable completion now raises AuthorityOutcomeUncertain, so the HTTP response retains transport_error: true and counted: true; the regression also proves the command resumes without invoking the manager twice.

[kyleseaman]

Resolved the GPT 5.6 finding on merge head 7c9a6238e1d238d56af9027c9cf1245daac4f2c7 in branch head 43565884f36d4e66f84cdc75a59f3246491f39df. Exceptions from the durable completion call now surface as AuthorityOutcomeUncertain; the new commit-then-raise regression proves the counted batch member is replayed from durable state without re-running the manager.

[kyleseaman]

Addressed the blocking post-registration capacity finding in 18b6117d8. If the synchronous facade has already registered non-terminal work when a later step raises, the authority now returns typed outcome uncertainty and leaves the manager lifecycle intact; it does not enter rejection cleanup or cancel the scheduled child. Added a deterministic regression and revalidated the fully rebased focused suite (640 tests).

[kyleseaman]

Final stack review disposition: READY at e7bee8b. The fully restacked coordinator, authority, outbox, recovery, messaging, and MCP suite is 649 passed, 1 skipped; black, isort, flake8, changed-source mypy, docs, brand, harness-parity, scrub, and diff gates all pass. No remaining P0/P1 or blocking AUTOSDE finding was identified.

[github-actions]

UX Review (Fable 5) — ✅ PASS

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

No screenshots in the diff; the backend strings are internal logs/exception text on the subagent coordinator path, not dashboard copy. The frontend changes remove two visible defects — theme overlays/audio bleeding over a remote Crew's dashboard, and healthy artifacts falsely flagged as "no longer showing" — and add no new strings, controls, or states.

UX-Verdict: PASS

Pure defect-removal PR: theme overlays and ambient audio now yield to a remote Crew, and healthy artifacts stop being falsely flagged — no new surfaces or copy to judge.

[UX-REVIEWED] c2524d9

[kyleseaman]

/ai-review override gpt 4988e54: Neither website/src/pages/ChatPage.tsx nor any website/electron file is present in this PR diff against feat/run-coordinator-recovery; the finding reviews unrelated repository state outside the 12-file coordinator hardening delta.

[github-actions]

Human judgment recorded

@kyleseaman marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 4988e5469c5f56e9850328c8b891818f1db85e7f.

Neither website/src/pages/ChatPage.tsx nor any website/electron file is present in this PR diff against feat/run-coordinator-recovery; the finding reviews unrelated repository state outside the 12-file coordinator hardening delta.

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

[bolichen97]

LGTM

[kyleseaman]

span=26970c492280 — Fixed in fcc84bbc46efc3c840c01670d8c000ffbca75d36. Orderly authority shutdown now retains and retries a known terminal manager rejection until its fenced completion is durable, while a durable terminal record or newer command claim releases the local debt. Regressions cover both a transient completion failure and a lost response after the completion committed.

[bolichen97]

Review: harden run coordinator lifecycle races

Reviewed origin/feat/run-coordinator-recovery...origin/feat/run-coordinator-hardening (17 files, +1607/−240) at 17d4954c8. Because this PR is itself the product of an adversarial review, the brief was to audit the fix rather than rediscover the bugs: for each claimed fix, does it close the window it names, does its regression genuinely pin it (mutation-tested by reverting the fix and checking the test goes red), and did it move the race rather than remove it. Findings are marked EXECUTED or READ.

Two things retracted up front, because they were my own earlier suspicions and they were wrong:

• The deleted _arm_queued_delivery_settlement(...) call at slack/gateway.py:7375 is correct. The base branch armed it twice — unconditionally at 7378 and conditionally at 7410. The PR removed the unconditional duplicate; the if _owes_delivery: call survives at 7403. When _owes_delivery is False, _on_consumed is never wired so consumed[0] stays False and the removed call could never have settled anything. [EXECUTED]
• The "weakened" assertion in test_keyed_queued_spawn_renews_lease_before_manager_registration (changed from lease_expires_at > 200.0 to claim_recovery(...) == []) still pins its fix: with _start_execution_heartbeat stubbed to a no-op, both that test and test_keyed_approval_wait_renews_lease_until_manager_takes_over go RED. [EXECUTED]

The central claim does not hold

The PR presents "deterministic regressions for each reviewed crash window." Three of them pin behaviour that already exists on the base branch, and the genuinely new fence has no test defending it.

A. subagent_manager/admission.py:723 — admission.py is comments-only in this diff, so its two new tests pin pre-existing behaviour. [EXECUTED] diff base_admission.py HEAD | grep '^[<>]' | grep -v '^#' | wc -l → 0. Both test_pre_task_policy_failure_rolls_back_registration_and_capacity and test_keyed_pre_task_policy_failure_is_durably_rejected run green against the base admission.py: 2 passed. They add coverage, not pinning.

B. subagent_command_authority.py:618 — the BaseException/cancellation propagation guard the spec presents as new already exists verbatim on the base branch. [EXECUTED] git show origin/feat/run-coordinator-recovery:…subagent_command_authority.py | grep -n BaseException → the guard is present at base lines 564-566 and 779-781. test_manager_base_exception_is_never_converted_to_spawn_failure against the base authority: 1 passed. Yet docs/system-specs/modules/subagent.md adds "process-level BaseException signals such as cancellation, KeyboardInterrupt, and SystemExit always propagate instead of being converted into a spawn result" as a new rule. This was one of the three decision-order changes flagged for special attention; it is not a change at all.

C. subagent_manager/terminal.py:359 — test_deferred_manager_event_does_not_repeat_parent_callbacks pins the pre-existing _digest_held or _delivery_queued guard, not the new line. [EXECUTED] The test passes against the base terminal.py: 1 passed. The only production change in that hunk is self._manager._outbox_live_run_batches.pop(event.run_id, None) in the else: branch, which the test never exercises; the guard it depends on appears as unchanged context (only its comment was reworded).

D. subagent_command_authority.py:1080 — none of the three conditions in the new _release_superseded_execution_failure is pinned by any test. [EXECUTED] terminal_run forced False → 64 passed. terminal_command forced False → 64 passed. newer_claim forced False → 64 passed. Only stubbing the whole helper to return False fails anything (1 failed), and deleting the whole close() pending loop fails 2. newer_claim is the recovery-takeover fence and terminal_run the durable-outcome check; either can be deleted or inverted without a single red test, so a future edit that drops the takeover fence ships silently.

Blocking

1. subagent_manager/terminal.py:949 — the process_stopped fix covers only the clean-reset path: the except asyncio.TimeoutError (SIGKILL) and except Exception arms never set the Event, so the protected terminal event is never drained and the parent is never notified. [EXECUTED] Probe over three reap paths:

[clean reset]        process_owned=False  on_done=1  outbox=DELIVERED
[reset-hung/SIGKILL] sigkill called: 1    process_owned=True   on_done=0  outbox=PENDING
[reset-raised]                            process_owned=True   on_done=0  outbox=PENDING

A protected run force-reaped while _sessions.reset() hangs takes the TimeoutError arm, SIGKILLs, and skips the PID check entirely. process_stopped stays unset, so the terminal reporter hits if not process_stopped.is_set(): return (terminal.py:595) before drain_once, and claim_outbox then permanently skips the event (memory.py:1093-1103) because run.process_owned is still True. A hung reset is the reaper's documented raison d'être, and it is exactly the hang this commit claims to fix. The new test test_force_reap_checks_protected_process_before_outbox_delivery sets mgr._sessions.reset = AsyncMock(return_value=True), so it exercises neither arm.

2. subagent_command_authority.py:1015 — close()'s new pending-failure loop busy-spins without ever yielding when _release_superseded_execution_failure returns True without popping, starving the entire event loop. [EXECUTED]

close() spun 5001 iterations in 0.002s without ever yielding
iterations/sec: 2298913
pending debt still held: 1

The helper pops only if self._pending_execution_failures.get(command_id) is pending, but returns True unconditionally. If a concurrent spawn replaces the entry for that command_id during the helper's get_command_by_key await, the snapshot object is no longer the live one: the helper returns True, the for body continues, settlement_failed stays False so no asyncio.sleep runs, and the while re-tests a still-non-empty dict. Gateway shutdown hangs and nothing else on the loop can run. An earlier probe variant confirmed the starvation directly: its own asyncio.wait_for(close(), timeout=2.0) never fired and thread-based pytest-timeout had to kill it at 120 s.

3. subagent_manager/terminal.py:933 — the new liveness probe uses platform_compat.pid_exists alone and ignores run.process_start_id, so a just-SIGKILLed zombie, a recycled PID, and an unsignalable PID all read "alive" and stall delivery on the nominal path too. [EXECUTED] after SIGKILL, before wait(): pid_exists = True / after wait(): pid_exists = False. session_lifecycle.py:403-415 force-kills a surviving child with kill_process_tree_async(pid, SIGKILL) and never wait()s it, and on POSIX pid_exists is os.kill(pid, 0), which returns True for an unreaped zombie child and True on PermissionError/OSError ("exists but we can't signal it"). So immediately after a successful reset the check reads alive, process_stopped is never set, and the event stays PENDING. PID reuse behaves identically — the very window the PR claims to close. The correct primitives already exist and are unused here: recovery.py:129-150 _verified_live_process compares process_start_time(pid) against run.process_start_id; terminal.py:116 already calls process_start_time; and platform_compat.pid_liveness() exists precisely to separate ALIVE from UNSIGNALABLE.

4. subagent_command_authority.py:1012 — close() never returns while the coordinator is down, and _pending_execution_failures is the only settlement cache missing from the final cleanup block. [EXECUTED] A) close() TIMED OUT after 3s -> UNBOUNDED; completion attempts: 4 / A) pending debt still held: 1. With complete() permanently rejecting, close() retries once per second forever and orderly gateway shutdown never completes — the same unbounded-shutdown shape confirmed in #5280 and #5281, but here placed before heartbeat cancellation, so leases keep renewing while shutdown hangs. The trailing cleanup clears _lease_tasks, _execution_results, _waiting_executions and _waiting_execution_keys but not _pending_execution_failures (:1066-1069), so any entry the loop fails to drain is retained for the object's lifetime.

5. subagent_command_authority.py:775 — _complete_execution_failure hand-builds the completion payload and omits three keys the canonical builder writes, so a batch rejection reconstructed from the outbox loses its conversation and model identity. [EXECUTED] Roundtrip: MISSING from authority payload: ['conversation_key', 'requested_model', 'resolved_model']; reconstructed conversation_key: ''. The canonical builder is terminal.py:237-270 _completion_payload_impl (17 keys); the new payload has 14, and _info_from_outbox_impl (terminal.py:326-328) reads all three. For a keyed CONTINUE batch member the conversation identity for a continuation is gone, and any field added to the canonical builder will silently be missing from authority-built rejections.

6. run_coordinator/memory.py:257 — moving the two import_legacy validations ahead of the idempotent-replay lookup converts a previously harmless replay of an already-accepted import into a hard REJECTED/INVALID_TRANSITION. [EXECUTED]

exact replay:                                 unchanged  idempotent_replay
replay w/ terminal-but-no-outcome:            rejected   invalid_transition
replay w/ event_type but no delivery_state:   rejected   invalid_transition
run still intact after rejected replays:      terminal

A legacy state.json row imported successfully and then re-read on a later sweep with a dropped outcome (or event_type without delivery_state) used to return idempotent_replay; it now returns a rejection for a run the importer already owns. test_terminal_legacy_import_cannot_override_claimed_authoritative_run exercises a CLAIMED authoritative run, not a malformed replay, so nothing covers this.

Should fix

7. run_coordinator/memory.py:675 — the new result_fill stale-fence guard is gated on command.operation in _EXECUTION_COMMANDS, so a CONTROL command's empty APPLIED result can be filled with no run-owner or lease-epoch check. [READ] if result_fill and command.operation in _EXECUTION_COMMANDS: guards the STALE_FENCE return, _EXECUTION_COMMANDS = frozenset({SPAWN, CONTINUE}) (:42), and the unconditional if result_fill: write follows at :686. A CANCEL/STEER/RELEASE command finished APPLIED with an empty result_json can therefore have that result filled by any holder of a matching command fence, including one whose run lease recovery has since taken over. The run-fencing this PR adds covers only SPAWN/CONTINUE.

8. subagent.py:2354 — prepare_coordinator_rejection pops and cancels self._tasks[run_id] — the live _run(info) task — and runs BEFORE the terminal complete() commits, so a failed commit leaves a non-terminal run whose reporter is already cancelled. [READ] _tasks[agent_id] = asyncio.create_task(self._manager._run(info)) at admission.py:740, 743, 753, 764, 775, 799; prepare(...) at authority :766-772 precedes the complete() call at :796; pending.run is receipt.run, captured before the manager call. So if the manager registered the run and _run advanced state via mark_starting/mark_running (bumping run.version), a late audit raising with registered.done True and a non-empty error runs _persist_execution_failure: prepare cancels the live run task, then complete() is called with the stale claim-time version → VERSION_CONFLICT → AuthorityOutcomeUncertain. The run is left non-terminal with no reporter and no outbox event, orphaned until periodic recovery (60 s sweep plus lease expiry).

9. subagent_command_authority.py:628 — a post-registration manager exception is silently swallowed and reported as SUCCESS whenever the registered child already finished cleanly. [READ] _execution_succeeded (:1252-1255) is return result is not None and not (bool(getattr(result,'done',False)) and bool(getattr(result,'error',''))), and the except BaseException block assigns local_result = registered at :632 without re-raising. So if spawn() registers the run, the child completes successfully (done=True, error=''), and a later callback in spawn raises, registered.done is True so the AuthorityOutcomeUncertain raise is skipped, control falls through, _execution_succeeded returns True, and the command is finished APPLIED as a normal success — the exception never surfaces.

10. subagent_command_authority.py:761 — a non-batch rejection commits an already-DELIVERED (unclaimable) event AND cancels the manager's announcement task, so a lost HTTP response means the parent is never told — and the replay path returns without delivering either. [READ, with one EXECUTED leg] delivery_state=(PENDING if batch_id else DELIVERED) at :761 makes the event permanently unclaimable by claim_outbox (which claims only PENDING or expired-CLAIMED, memory.py:1104-1111), and prepare_coordinator_rejection cancels _tasks[agent_id] — which for the "no approval mechanism configured" rejection is _spawn_announcement(info) (admission.py:816). If the synchronous HTTP response is lost in transit, the exact replay takes the TERMINAL early-return, which never calls _deliver_execution_failure (gated on if pending.batch_id: at :731). No path notifies the parent. Verified separately by execution that drain_once(event_id=...) on a DELIVERED event is a silent no-op (claim_outbox returns [] → break). This is documented as intentional in docs/system-specs/modules/subagent.md, but the lost-response hole is not addressed.

11. subagent_command_authority.py:530 — the new admit_reserved TERMINAL early-return drops batch_id, batch_total and silent, and returns the run task unredacted. [READ] Lines 530-535 construct AdmittedExecution(run_id, task, done=True, error=…) with four arguments only, and the dataclass defaults batch_id='', batch_total=0, silent=False. So a batch member whose first POST commits a terminal FAILED run but whose response is lost gets an exact replay that returns all wave fields at defaults — a caller closing the wave from batch_id/batch_total sees ''/0 and the wave never settles from this response. The same path passes receipt.run.task unredacted while the sibling _persist_execution_failure uses _redact(pending.run.task).

Below the cap

docs/system-specs/modules/subagent.md asserts at least three pre-existing behaviours as new fixes — the BaseException rule (B above), the deferred-callback guard (C above), and "the adapter never holds its local claim lock while invoking routing", where run_coordinator/delivery.py is not in this diff at all and the lock is already released before self._destination(event) at delivery.py:152 [EXECUTED] · _CANCEL_CONTROL_LEASE_SECS = 22.0 * 60.0 is a third uncoupled literal alongside _DELIVERY_LEASE_SECONDS = 22 * 60.0 and _ON_DONE_TIMEOUT = 1200.0, so changing the 20-minute cap silently drifts both [READ] · safe_result in _persist_execution_failure rebuilds AdmittedExecution copying eight fields via getattr but never counted, which defaults to True — latent today because both counted=False construction sites (:486, :584) return before reaching this path, but the first future caller that routes an uncounted result through here double-counts a wave member [READ] · the test double's delivered_batches is populated from kwargs the production caller never passes and is never asserted — dead scaffolding [EXECUTED].

Refutations and clean results (please don't re-spend here)

Beyond the two retractions above: the 22-minute cancel claim breaks no sweep — claim_controls has no production caller outside the sqlite/shadow delegation wrappers, so nothing recovers expired control claims either way. renew() does not bump lease_epoch (memory.py:1042-1064), so the result_fill owner/epoch guard correctly survives the 30 s heartbeat and the headline "fill an empty result exactly once under the same fence" rule is sound for execution commands. No memory-vs-durable split-brain: sqlite.py::_invoke delegates every method to a freshly loaded MemoryRunCoordinator and snapshots the full tables back, including command.result_json and outbox status/delivered_at, so the delivery_state and result_fill changes propagate to the durable backend. No existing test broken: 339 passed / 1 skipped across the eight adjacent files, 231 passed across the five changed test files. Conventions are clean for this PR: all 14 changed .py files pass the pinned black==26.3.1 --target-version py310, test/test_subagent.py is black-clean so its removal from .github/black-baseline.txt is correct, and there is no CHANGELOG or commit-count workflow gate. Note the repo's own scripts/check_black_formatting.py does fail — on test/test_run_coordinator_sqlite.py, which is not among this PR's 17 files; its scope is origin/main...HEAD (83 files), so that is a stack-level red gate inherited from #5279.

One item I could not verify: the POST half of the lookup_response-vs-admit_reserved precedence divergence. The GET half is confirmed — a terminal COMPLETED run whose command is APPLIED with an empty result_json reports {'found': True, 'id': 'run-c', 'task': 't', 'status': 'spawned'}, i.e. the completion is invisible to lookup. My POST probe raised AuthorityConflict: idempotency_conflict from a hand-written payload_json whose hash did not match _payload(), so it did not reproduce a true exact replay. Both enclosing branches are unchanged context in this diff, so any divergence here is pre-existing — merely made more reachable by the switch to always-APPLIED rejections.

---

_{Execution-verified AI-assisted review (Claude Code) run against a local checkout at 17d4954c8; all probes, mutated source copies and the base worktree lived under /tmp, the worktree is unmodified, and nothing was posted elsewhere. Every finding is labelled EXECUTED or READ so you can weight them; the four mutation results in section D are the ones I would act on first, since they mean the new hardening is not defended by the suite. Findings name the input that reproduces them — please push back where one misreads intent.}

[kyleseaman]

@bolichen97 I worked through the review against the integrated stack and pushed 08fea9cd7.

Fixed:

1. Force-reap now checks the protected process after clean reset, reset timeout/SIGKILL, and reset exceptions.
2. Shutdown settlement is bounded, yields on replacement races, clears every local settlement cache, and leaves unresolved durable claims for lease takeover.
3. Protected-process release uses tri-state liveness plus the persisted start identity; owned subprocesses are also bounded-waited after SIGKILL so zombies do not pin delivery.
4. Rejection completions now carry conversation, requested-model, and resolved-model identity.
5. Existing legacy imports are recognized before validating agent-writable replay data and cannot mutate an authoritative row.
6. Coordinator rejection preparation/cancellation happens only after the durable terminal completion commits, using the latest non-terminal run version.
7. Post-registration exceptions with a clean terminal child are reported as outcome uncertainty instead of success.
8. Every rejection event starts PENDING and gets an immediate delivery attempt, leaving recoverable debt if delivery fails.
9. Terminal replay preserves batch id, batch total, and silent, while returning the redacted task.
10. The completion replay contract now checks the exact preceding run version; id allocation precedes lifecycle mutation; renewals are monotonic; non-finite outbox releases are rejected; submit replay checks both identities and the operation.
11. The superseded-failure regressions independently pin terminal-run, terminal-command, and newer-claim predicates.

I did not add a run-lease fence to CONTROL result-fill. Control commands have their own command fence and deliberately do not own or renew the execution run lease; requiring the execution owner/epoch there would make a valid cancel/steer/release result unfillable after normal execution takeover.

Fresh verification on the submitted tree: 389 passed, 1 skipped across the coordinator contract/SQLite/shadow/wiring/recovery/delivery, authority, reap-race, and process-tree suites. Full repository isort, flake8, and Linux-parity mypy pass, along with the deterministic formatting, encoding, SDK-boundary, async-I/O, lockdown, brand, harness, lock, testpath, changelog, focus, docs, per-file coverage, vendored-tree, and scrub gates.

[kyleseaman]

@bolichen97 Follow-up on the review response: the integrated stack is now at 4253122ee. Subsequent automated passes found two residuals in lower layers and both are fixed in their owning PRs: rejected KIROCREW_HOME values retain the safe default-home fallback while live retargets still fail closed, and definite coordinator submit failures now use the tombstone-free legacy settlement path so restart recovery cannot lose the parent notification. The restacked top passes 524 focused tests with 1 valid platform skip; current-head CI and reviewer passes are running.

[kyleseaman]

Bolin review disposition for the current stack

The complete September 2 hardening review was rechecked.

A. Addressed: the admission cases are described as added coverage, not a newly introduced production branch.
B. Addressed: the BaseException rule is documented as a preserved invariant rather than a new behavior.
C. Addressed: the deferred-callback regression now targets the changed live-batch cleanup behavior.
D. Fixed: separate mutation-sensitive tests cover terminal run state, terminal command state, and newer-claim takeover before pending failure debt is released.

1. Fixed: clean reset, timeout and exception reap paths all resolve protected process identity before making delivery claimable.
2. Fixed: pending shutdown settlement always yields and cannot spin when an entry is concurrently replaced.
3. Fixed: protected-process checks use PID liveness plus durable start identity, covering zombies, unsignalable processes, and PID reuse.
4. Fixed: close is bounded, preserves durable debt for recovery, and clears local pending state.
5. Fixed: failure completion uses the canonical payload builder and preserves conversation and model metadata.
6. Fixed: exact legacy replay is recognized before validating mutable replay input.
7. Rebutted: control result fill is fenced by command authority. Requiring execution run ownership would invalidate legitimate control completion after ownership transitions.
8. Fixed: manager teardown is deferred until durable terminal settlement succeeds or a newer owner is proven.
9. Fixed: post-registration exceptions surface as outcome uncertainty even when the child has already completed.
10. Rebutted: non-batch synchronous rejection is the delivery boundary, so DELIVERED is intentional; exact replay returns the same response without duplicating parent delivery.
11. Fixed: terminal replay preserves batch identity, silent state, redacted task, and counted state.

The below-cap counted-state observation is also fixed. Current submitted head: c2524d9.

[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. Stack tip revising this PR's own contract suite; depends on it landing first. Files: test/test_run_coordinator_contract.py.

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