feat: add typed run coordinator contract - #5277
Conversation
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound contract-first seam, deliberately inert; the concerns are hygiene — docs anchored to unmerged local state and an unrelated test fix riding along. Watch
[DESIGN-REVIEWED] 43be00a |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All counts verified. Writing the review. First-Principles-Verdict: CONCERNS A typed contract with zero runtime callers ships wired into What this change shipsIntent: give the subagent system a typed run-lifecycle boundary so later PRs can move authority onto a durable store — an ADDITION, PR 1 of a declared 7-PR stack.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 43be00a |
3d01079 to
7eb6e08
Compare
7eb6e08 to
3d01079
Compare
3d01079 to
8097b0b
Compare
dfded83 to
ca49794
Compare
628aaa7 to
3c2d04f
Compare
3c2d04f to
5c0c65c
Compare
5c0c65c to
70de043
Compare
70de043 to
fa791cb
Compare
bolichen97
left a comment
There was a problem hiding this comment.
Approved after a full-diff review (scope match, no out-of-scope files, security surface checked, tests verified non-vacuous). Review notes available on request.
fa791cb to
8de2034
Compare
bolichen97
left a comment
There was a problem hiding this comment.
Review: typed run coordinator contract
Reviewed origin/main...origin/feat/run-coordinator-types (10 files, +1298/−7) at 2c7e842c0. Findings verified by executing MemoryRunCoordinator and the contract suite, plus mutation testing. All 16 tests pass, so every item is uncovered behaviour.
The headline: this PR's job is to define semantics that PRs 3–7 must implement, and in several places the in-memory oracle encodes semantics no durable store can honour — or is weaker than one. A SQLite port that behaves correctly will therefore disagree with the oracle, and the contract suite cannot tell which is right.
Blocking
1. monitoring/memory.py:268 — complete() consults the outbox replay key BEFORE _validate_transition, so the terminal transition is the one boundary that is never fence-, epoch-, expiry- or version-checked. Executed: after gw1 legitimately completes run-1, advance the clock 999s (lease long dead), then call complete() with RunFence('run-1','ATTACKER',4242) and expected_version=-1 → returns unchanged/completion_replay and hands back the live OutboxEvent, from which a DeliveryFence is derived. The same forged fence with outcome=FAILED returns rejected/outcome_conflict, so STALE_FENCE is never reported and a fenced-out writer cannot tell it lost the lease. This contradicts the RFC added in this same PR (rfc-durable-run-coordinator.md:385: the completion transaction "verifies the fence and legal transition"; :379 promises "a typed stale_fence result"). Proof the suite can't catch it: a subclass that validates the fence first — returning stale_fence for the forged call — passes all 14 contract tests unchanged, so PRs 3–7 can "conform" while disagreeing on whether fencing is total.
2. memory.py:306 — complete() is non-atomic: it commits the run to TERMINAL and the command to APPLIED before constructing the outbox event, so a failure at _id_factory() loses the completion permanently with no retry path. Executed with an id_factory that raises once: the run is left observed_state=terminal, outcome=completed, version=4 and the command applied, outbox events=0, then the exception propagates. Retrying with the corrected version and the same still-valid fence returns rejected/invalid_transition, because TERMINAL is excluded from _COMPLETABLE_STATES (:35-42). The outbox stays at 0 forever — the run is recorded COMPLETED and the parent session can never be notified. Reachable today via the suite's own 3-id fixture (test_run_coordinator_contract.py:43) exhausting on a 4th event. Any SQL port would roll all three writes back in one transaction, so the oracle is strictly weaker than the implementations it certifies.
3. memory.py:345 — renew() writes until unconditionally instead of max(current, until), so an older or reordered renewal SHRINKS a live lease and lets a second gateway steal a run the first owner still believes it holds. Executed: claim with OwnerLease('gw1', now+60) → lease_expires_at=160.0; renew(until=now+60) → True, 160.0; a second, older renew(until=now+1) → returns True and the lease shrinks to 101.0. Advance 2s: gw1's own fence is now invalid (renew → False, mark_running → stale_fence, so its terminal outcome is unrecordable) while claim_commands(OwnerLease('gw2', …)) succeeds and bumps lease_epoch to 2 — two workers on one subagent run. UPDATE runs SET lease_expires_at = ? reproduces this exactly in SQLite. Two overlapping heartbeat tasks, or one retried renew carrying a stale deadline, is enough. The only guard is until <= now; mutation-removing even that leaves all 16 tests green.
4. test/test_run_coordinator_contract.py:44 — the suite billed as the executable contract oracle has a ~45% mutation score: 11 of 20 injected invariant breaks stay green. Measured. Deleting all ten async with self._lock: guards from memory.py → 16 passed; grep -c 'gather|create_task|ensure_future' over both new test files returns 0, so no two coordinator calls are ever in flight and the transaction property a SQLite port must reproduce is entirely uncertified. Also green: removing the lease-expiry term from _validate_transition (an owner whose lease died at t=105 completes at t=199); dropping the expiry term from claim_commands so a live lease can be stolen (gw2 claims at t=100 while gw1's lease runs to t=130, fencing gw1 out mid-run); adding TERMINAL to _COMPLETABLE_STATES (one run emits both a success and an error event, stored outcome flips COMPLETED→FAILED); inserting a duplicate outbox row on replay; dropping outcome from the replay-equality check; deleting the delivery-claim expiry check; ignoring available_at in claim_outbox; entering RUNNING directly from ACCEPTED; bumping version on the idempotent UNCHANGED branch; never marking a completed run's commands APPLIED. Separately, INVALID_TRANSITION (3 production sites), NOT_FOUND (2), TRANSITIONED (4), COMPLETED, COMPLETION_REPLAY, DELIVERED, ALREADY_DELIVERED and DELIVERY_RELEASED have zero reason assertions, so RFC §7.1's "legal and illegal state transitions" is half unwritten. And the fixture hardcodes MemoryRunCoordinator with no params=/indirect hook (the repo already uses params=[...] in test_deploy_script_validation.py), so a durable implementation must fork all 520 lines.
5. memory.py:274 — is identity comparisons on str-backed enums break for any durable implementation that hydrates a TEXT column into a bare str, and the frozenset membership tests silently do not — the two styles disagree exactly at the persistence boundary this RFC introduces. Executed: with outcome supplied as the bare string 'completed' (what SELECT outcome FROM runs yields, and what json.dumps/loads round-trips a str-Enum into), an identical completion replay returns rejected/outcome_conflict instead of unchanged/completion_replay — at-least-once delivery breaks and a retry is misreported as a conflicting second outcome. Same class at :249: with observed_state hydrated as 'starting', mark_running() returns rejected/invalid_transition on a legal transition. RunOutcome.COMPLETED == 'completed' is True while is is False, yet 'accepted' in _STARTABLE_STATES is True because a str-Enum hashes as its value — so a SQLite port passes every membership guard and fails every identity guard, with no test able to see it because the memory oracle never serialises. Affected: :162-163, 219, 245, 249, 274, 291, 308, 361-363, 386, 395.
6. subagent.py:1593 — self._coordinator = coordinator or MemoryRunCoordinator() uses truthiness, so any Protocol-conformant coordinator that is falsy is silently replaced by a throwaway in-memory one — and RunCoordinator is not @runtime_checkable, so nothing can detect the swap. Executed: a MemoryRunCoordinator subclass adding __len__ returning len(self._runs) — a natural backlog/queue-depth accessor for a durable implementation — is falsy while empty. SubagentManager(…, coordinator=inj) then yields manager._coordinator is inj False and type(...).__name__ == 'MemoryRunCoordinator', with no error. Once the seam becomes authoritative in PRs 3–7, every run the caller submits lands in a store no durable reader ever queries — silent total data loss. isinstance(MemoryRunCoordinator(), RunCoordinator) also raises TypeError, so conformance cannot be asserted at the boundary. The one test guarding this (test_run_coordinator_wiring.py:23) injects a plain MemoryRunCoordinator, which is always truthy, so it structurally cannot catch it. Fix: coordinator if coordinator is not None else MemoryRunCoordinator().
7. memory.py:78 — the idempotency-replay branch never checks that the request's run_id/command_id match the stored command's, so submit() returns a receipt describing a different run than the caller asked for. Executed: submit(run_id='run-A', command_id='cmd-A', idempotency_key='session42:summarise') then submit(run_id='run-B', command_id='cmd-B', same key, same payload_hash) returns unchanged/idempotent_replay with receipt.run.run_id == 'run-A' and receipt.command.command_id == 'cmd-A', while get_run('run-B') is None. The caller now tracks run-B — a run the coordinator has never heard of — and will fence, renew and complete against an id that does not exist (every call → NOT_FOUND). Only payload_hash is compared (:81), so any caller that derives the key from something other than the run_id (e.g. session+task, the natural choice for spawn dedupe) silently orphans runs. A durable UPSERT ON CONFLICT(idempotency_key) reproduces this exactly.
Should fix
8. memory.py:231 — mark_starting, mark_running and complete read self._clock() twice inside one locked step, validating the lease at t1 and stamping the write at t2, so a transition can commit under a lease that had already expired. Executed with a clock returning 104.0 then 106.0 against lease_expires_at=105.0: _validate_transition passes, then replace(..., updated_at=self._clock()) stamps 106.0 → applied/transitioned with the write landing after the lease authorising it was dead, while a takeover owner may already hold epoch 2. Same double read at 242+257 and 205+295. renew() and both claim_* methods correctly bind now once, and a SQLite port using one transaction timestamp cannot reproduce this window — so the oracle and any correct durable implementation disagree on whether the transition is legal.
9. memory.py:176 — claim_commands commits each run/command mutation inside the scan loop and indexes self._runs[current.run_id] unguarded, so one bad row raises KeyError after earlier runs are already leased. Executed with three healthy runs plus one orphan command row (run_id='GONE'): claim_commands(...) raised KeyError('GONE') at :160, and all three earlier runs were already mutated (owner='gw1' epoch=1 lease_until=130.0, command claimed attempt=1) while the caller received zero CommandClaim objects. Since mark_starting/mark_running/complete all require a fence, those runs are unstartable until the 30s lease expires with their attempt counters already burned, and no other command in the batch is claimed (dispatch starves). An orphan command row is representable in the RFC's own schema — §5.3 declares run_id TEXT NOT NULL with no FOREIGN KEY — and reachable via PR 7's legacy-folder import.
10. memory.py:409 — release_outbox() persists the caller's available_at with no validation; a NaN wedges the event PENDING forever because available_at <= now is False at every instant, and DeliveryState offers no dead-letter state to surface it. Executed: release_outbox(fence, available_at=float('nan')) → applied/delivery_released, stored pending/nan; claim_outbox() polled at now=100, 1.0001e6 and 1e12 returns [] every time. The subagent completion is never claimable, never delivered, never retried, and invisible to monitoring. NaN is realistic, not hostile — any exponential-backoff expression that overflows to inf and then subtracts yields NaN. -inf and -1e9 were also accepted and produce the opposite failure: immediate re-claim with attempts never reset.
11. memory.py:361 — claim_outbox's expired-claim branch ignores available_at and never advances it, attempts is incremented but never read, and DeliveryState has no failed/dead-letter member — so an undeliverable event is redelivered in a tight unbounded loop. Executed with a worker that claims and dies without releasing: attempts climbs 1,2,3,4,5,6… and claim_epoch in lockstep, with available_at pinned at 100.0 every round. The expired predicate (:362-364) tests only claim_expires_at <= now, so unlike the pending arm it ignores available_at, and :367-374 never pushes it forward. Nothing caps attempts, and CommandStatus.REJECTED is written at exactly one site, so there is no backoff and no terminal failure state. Same shape on the command side: five crash cycles gave attempt=5, lease_epoch=5, still claimed. Any durable implementation matching this contract hot-loops on a poison event at claim-TTL rate for the process lifetime; RFC:404 promises "backoff is bounded and persisted" and "permanent routing failure" handling the port cannot express.
12. memory.py:332 — terminal records never release their claims: complete() leaves owner_id/lease_expires_at/lease_epoch intact and renew() has no terminal guard, and mark_delivered() leaves claim_owner/claim_expires_at/claim_epoch populated — so both fences stay valid forever. Executed: after complete(), renew('run-1', fence, until=now+300) returns True and extends the dead run's lease to 400.0, with no state check anywhere in renew (:336-343). For PR 7's "acquire expired leases and reconcile", a completed run can never be seen as reclaimable, so terminal rows hold ownership indefinitely; and a stale executor heartbeat gets a bare True back and cannot detect that its run was finalised underneath it. On the delivery side, after mark_delivered the event still reads claim_owner='gw2' claim_expires_at=150.0 claim_epoch=2 and the ALREADY_DELIVERED branch applies no expiry check, so a crashed-and-restarted gw2 replaying an old fence cannot distinguish "I delivered this" from "this is still mine." A durable store also cannot use claim_owner IS NULL or claim_expires_at to separate in-flight from settled rows.
13. memory.py:92 — submit() rejects any request whose run_id already exists, which makes CommandOperation.CONTINUE — declared executable and documented as accepted by this same PR — impossible to submit for the run it is meant to continue. Executed: submit(SPAWN, run_id='run-1') → applied/created; submit(CONTINUE, run_id='run-1', fresh command_id, fresh key) → rejected/identity_conflict. A continuation by definition targets an existing run, so the only accepted form uses a brand-new run_id — and that record is indistinguishable from a spawn, losing the parent run's identity; command.operation is never read by any transition method. docs/system-specs/modules/subagent.md:23, added in this commit, asserts the phase "accepts executable spawn and continue submissions", and the RFC's commands table has no UNIQUE on run_id. So a PR-3 author will build the continuation path on a submission the coordinator cannot accept.
14. models.py:10 — the declared state machine is not total: the entire desired-state axis is write-only, ObservedState.QUEUED is unreachable while still gating two transition guards, and CoordinatorReason.CLAIMED, RunRecord.attempt and OutboxEvent.run_version are dead. Verified by grep plus a full lifecycle drive. desired_state occurs exactly twice — the field declaration and desired_state=DesiredState.RUN (:106) — and is never read or branched on; DesiredState.CANCEL and RELEASE have 0 references anywhere and the Protocol has no cancel/release method, yet RFC §5.2 declares desired state "records operator intent". QUEUED has no producer but is a member of both _STARTABLE_STATES and _COMPLETABLE_STATES, so those guards advertise admission for a state the contract cannot reach — while RFC §2.2 lists "was a request accepted, rejected, queued, or dispatched?" as a question the record must answer. RunRecord.attempt is set to 1 and never incremented even across a full takeover (verified: gw2 re-claims at epoch 2 with command.attempt=2 while run.attempt stays 1). A durable layer reading these enums as the specification will add CHECK constraints and indexes on columns that can only ever hold one value.
15. models.py:25 — RunOutcome.COMPLETED = "completed" contradicts the repo's self-declared single source of truth OUTCOME_OK = "ok"; the other three members are byte-identical, so the mismatch hides on the most common path. subagent_completion_meta.py:31-34 declares the four values under the comment "this is the single source of truth for a completion's outcome", and website/src/pages/chat/subagentCompletion.ts:74 pins the same union, runtime-validated at :262 via new Set(['ok','failed','stopped','interrupted']). FAILED/STOPPED/INTERRUPTED match exactly; success is 'completed' vs 'ok'. A migration mapping written by pattern-match therefore passes for every failure/stop/interrupt case and breaks only on success: 'completed' is rejected by the frontend set, so the completion card silently falls back to glyph regexes — the exact regression the meta key was introduced to eliminate. test_subagent_completion_meta.py pins the existing 4-tuple, so a future edit to RunOutcome will not go red there. Spell COMPLETED = 'ok' or import OUTCOME_* directly (that module is a deliberate leaf with no cycle risk).
Below the cap (all verified)
submit(accepted=False) caches a transient admission rejection permanently under the idempotency key and burns the run_id, so a retry after capacity frees returns a replay of the dead terminal record · mark_starting's command argument is never validated — a fabricated REJECTED command with lease_epoch=999, owner_id='attacker' returns applied/transitioned, and it is asymmetric with mark_running(run_id: str) · claim_commands' PENDING arm has no live-lease check and its non-execution arm mints a fence at an un-acquired epoch, both unreachable only by coincidence today (PR 5's steer/cancel/release commands are by definition a new PENDING row against a leased run) · completion.terminal_at is stored unvalidated (accepts 0.0, negative, inf, NaN, producing terminal_at < created_at) · no eviction or TTL on any of the five dicts: measured 1019 B/run retained for the process lifetime (4.86 MiB at 5000 runs), claim_commands(limit=1) costs 1.19 ms at 10k rows vs 0.0005 ms at 1, and complete() scans + tuple()-copies every command (2.38 ms at 20k) — the port has no prune operation · claim order is lexicographic by command_id on a created_at tie, not FIFO, so with uuid4().hex ids in one clock tick dispatch order is effectively random · _outbox_by_run_type keyed by (run_id, event_type) is dead generality, and a genuinely conflicting completion carrying a different event_type is misreported as invalid_transition rather than outcome_conflict · the RFC front matter regresses a resolvable anchor: audited-at: c8eda3c6f is not a valid object (git cat-file -t → fatal) whereas the removed c4f253891 resolves, and last-audited/implementation-prs: [] were left untouched against docs/request-for-change/README.md:160-162; the RFC also names branch codex/run-coordinator-types, which exists on no remote · test/test_resource_limits_schema.py:276 is unrelated scope creep and a duplicate (sb.platform_compat is kiro_crew.platform_compat, so it patches the identical attribute as the pre-existing patch twelve lines below; 57/57 pass with the new block removed) · cleanup: _validate_transition returns a CoordinatorResult | RunRecord union requiring isinstance at five call sites; the fencing rule is spelled three times; mark_starting/mark_running are 25-line near-clones; the legal-transition tables are private to memory.py rather than exported from models.py.
On the injection seam specifically (a stated focus): it is otherwise authority-neutral — self._coordinator has exactly one write and zero reads in src/, the run_coordinator import graph is stdlib-only (no cycle), the kwarg is appended last so no positional call site shifts, construction costs 0.24 µs, and the sole production construction site (slack/gateway.py:7875) runs on-loop. The only defect at the seam is the or-based discard in finding 6.
Execution-verified AI-assisted review (Claude Code) run against a local checkout at 2c7e842c0; nothing modified, nothing posted elsewhere. Findings name the input that reproduces them — please push back where one misreads intent. ARCC was not queried (search_arcc unavailable in the session), so standard practice plus the repo's own documented invariants were applied instead.
|
Bolin review disposition for the current stack All numbered findings from the September 2 review were rechecked against the submitted stack.
The below-cap executable correctness observations were also covered by the later command, outbox, and recovery layers; durability remains bounded by the serialized SQLite adapter rather than unbounded in-memory authority. Current submitted head: 43be00a. |
iamwhatever
left a comment
There was a problem hiding this comment.
Let us hold on for pushing this series of changes out, as it is having some overlaps with multiple features under development. Let us chat more about how to fit all of them together first.
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
|
@kyleseaman Thanks for this. An audit of all open PRs on 2026-09-08 found one other open PR touching the same code, and it is worth coordinating before either lands. #7195 (@rubencu, "feat: adapt subagent timeouts to observed duration") edits the same two files as this PR: On which side is further along: this PR is purely additive against main, which has no Suggestion: keep both PRs separate and sequence them instead of folding one into the other. Whichever rebases onto main first keeps the parameter block as written, and the second adds its kwargs on top. This PR is currently in a @rubencu for visibility on the shared constructor. Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
Problem / Motivation
The subagent system has no typed boundary for durable run ownership, lifecycle transitions, command claims, or completion delivery.
Why it matters
A narrow contract lets the current implementation evolve additively while preserving the proven legacy execution path until durable authority is ready.
What changed (motivation → approach → change)
SubagentManagerwithout changing runtime authority.Tests
test/test_run_coordinator_contract.pytest/test_run_coordinator_wiring.pyManual verification
Not applicable; this is a backend contract and compatibility seam.
Related Issues
no linked issue: this stack implements the locally reviewed durable run coordinator RFC.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)