Skip to content

feat: add typed run coordinator contract - #5277

Open
kyleseaman wants to merge 1 commit into
mainfrom
feat/run-coordinator-types
Open

feat: add typed run coordinator contract#5277
kyleseaman wants to merge 1 commit into
mainfrom
feat/run-coordinator-types

Conversation

@kyleseaman

@kyleseaman kyleseaman commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Stacked change: PR 1 of 7

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

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)

  • Adds frozen coordinator records, string enums, typed decisions, fences, commands, completions, and outbox claims.
  • Adds a deterministic in-memory coordinator that defines transition, idempotency, lease, and delivery semantics.
  • Injects the coordinator into SubagentManager without changing runtime authority.
  • Adds a reusable coordinator contract suite and wiring tests.

Tests

  • test/test_run_coordinator_contract.py
  • test/test_run_coordinator_wiring.py
  • Current stack-tip focused coordinator suite: 507 passed, 1 valid platform skip

Manual 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

  • 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

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 43be00a

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

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

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

  • The RFC/README now say "PR 2 is prepared on local branch codex/run-coordinator-types" and audited-at: PR 1 commit c8eda3c6f plus the PR 2 working tree — once merged, main's RFC tracking table cites a branch and working tree nobody else can inspect, making the audit row (whose purpose is verifiability) unverifiable and stale on arrival. Anchor status text and audited-at to merged commits/PR numbers only.
  • The test_resource_limits_schema.py hunk mocking trusted_system_bin/usr/bin/systemd-run has no connection to the coordinator and is unmentioned in the description — an undocumented environment-hermeticity fix smuggled into a contract PR. Move it to its own commit/PR or state it.
  • The in-memory oracle pins semantics designed in the RFC, not extracted from the live manager; any mismatch with actual spawn/terminal ordering stays invisible until authority moves in PR 3+, so if the stack stalls, main carries a ~1,500-line shadow model with zero runtime callers. Acceptable for a tracked 7-PR stack, but the stack landing is what retires this risk.

[DESIGN-REVIEWED] 43be00a

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 43be00a

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

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

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 43be00ad7203073b77566dcbd27be685205006e8 — 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 counts verified. Writing the review.

First-Principles-Verdict: CONCERNS

A typed contract with zero runtime callers ships wired into SubagentManager as a field nothing reads, carrying enum variants nothing constructs.

What this change ships

Intent: 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.

  1. New internal run_coordinator package exporting 21 symbols — zero consumers outside tests (grep run_coordinator in src/: 1 hit, the import below)
  2. Deterministic in-memory coordinator defining transition/lease/delivery semantics — exercised only by its own contract suite
  3. SubagentManager gains a coordinator constructor param, defaulted — stored, never read (_coordinator in subagent.py: 1 assignment, 0 reads)
  4. Steer/cancel/release command operations defined but rejected as unsupported — zero constructors
  5. RFC flipped draftin-progress, README row updated — justified (same-commit doc rule)
  6. subagent.md gains a "Run coordinator seam" section — justified
  7. Reusable 717-line contract suite — justified as the executable oracle for later adapters
  8. test_resource_limits_schema.py now mocks trusted_system_bin — undeclared, rides along

Watch

  • Item 3 is speculative wiring. Zero option: delete the param, the default instantiation, and the three wiring tests; nothing observable changes ("without changing runtime authority" — the description's own words). The PR that first reads self._coordinator can add it.
  • Item 8 is unrelated to this feature and unmentioned. Move it to its own commit or PR so its cause is stated.
  • The RFC now claims audit "against PR 1 commit c8eda3c6f plus the PR 2 working tree" — a local sha and an unmerged tree; the row goes stale the moment this merges.

Subtractions

  • Drop SubagentManager.coordinator param + MemoryRunCoordinator() default (subagent.py:1595) and test_run_coordinator_wiring.py + test_manager_preserves_an_explicit_falsy_coordinator — defer to the first PR that calls the coordinator.
  • Drop the desired_state field and DesiredState enum — one variant ever constructed (memory.py:112), the field is never read; reintroduce with cancel semantics.
  • Drop ObservedState.QUEUED (0 constructions; only appears in the two allow-sets) and the three unsupported CommandOperation variants — UNSUPPORTED_OPERATION handling exists solely to reject surface this same PR added.

[FIRST-PRINCIPLES-REVIEWED] 43be00a

Base automatically changed from feat/run-coordinator-plan to main August 23, 2026 17:33
@iamwhatever
iamwhatever force-pushed the feat/run-coordinator-types branch from 3d01079 to 7eb6e08 Compare August 23, 2026 17:33
@kyleseaman
kyleseaman force-pushed the feat/run-coordinator-types branch from 7eb6e08 to 3d01079 Compare August 23, 2026 17:36
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 23, 2026
@kyleseaman
kyleseaman force-pushed the feat/run-coordinator-types branch from 3d01079 to 8097b0b Compare August 23, 2026 17:47
@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 23, 2026
@kyleseaman
kyleseaman force-pushed the feat/run-coordinator-types branch 7 times, most recently from dfded83 to ca49794 Compare August 24, 2026 19:52
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 25, 2026
@kyleseaman
kyleseaman marked this pull request as ready for review August 25, 2026 13:50
@kyleseaman
kyleseaman requested a review from a team as a code owner August 25, 2026 13:50
@kyleseaman
kyleseaman requested a review from Zedmor August 25, 2026 13:50
@github-actions github-actions Bot removed the readiness: checking Automated validation is still running label Aug 25, 2026
@kyleseaman
kyleseaman force-pushed the feat/run-coordinator-types branch from 628aaa7 to 3c2d04f Compare August 28, 2026 14:29
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 28, 2026
@kyleseaman
kyleseaman force-pushed the feat/run-coordinator-types branch from 3c2d04f to 5c0c65c Compare August 28, 2026 16:19
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Aug 28, 2026
@kyleseaman
kyleseaman force-pushed the feat/run-coordinator-types branch from 5c0c65c to 70de043 Compare August 28, 2026 17:00
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Aug 28, 2026
@kyleseaman
kyleseaman force-pushed the feat/run-coordinator-types branch from 70de043 to fa791cb Compare August 29, 2026 15:23
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 29, 2026
bolichen97
bolichen97 previously approved these changes Aug 30, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kyleseaman
kyleseaman force-pushed the feat/run-coordinator-types branch from fa791cb to 8de2034 Compare August 31, 2026 02:32

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:268complete() 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:306complete() 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:345renew() 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_runningstale_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.py16 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:274is 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:1593self._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:231mark_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:176claim_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:409release_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:361claim_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:92submit() 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:25RunOutcome.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 -tfatal) 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.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Bolin review disposition for the current stack

All numbered findings from the September 2 review were rechecked against the submitted stack.

  1. Fixed: completion replay now validates fence, epoch, expiry, and version before returning the durable event.
  2. Fixed: run, command, and outbox mutation is atomic and rolls back if event construction fails.
  3. Fixed: renewals are monotonic and cannot shorten a live lease.
  4. Fixed: the contract suite now exercises concurrent calls, mutation-sensitive transition reasons, replay, expiry, delivery, and adapter parity.
  5. Fixed: hydrated string-enum values use value equality consistently.
  6. Fixed: coordinator injection uses an explicit None check and the protocol is runtime-checkable.
  7. Fixed: replay identity is bound to run ID, command ID, operation, and payload.
  8. Fixed: each transition binds one clock reading.
  9. Fixed: command claims stage atomically and reject orphan rows without partial leasing.
  10. Fixed: retry timestamps must be finite and valid.
  11. Fixed in the outbox layer: retry delay and attempt handling are bounded and persisted.
  12. Fixed: terminal run and delivered-event claims are cleared, and terminal runs cannot renew.
  13. Rebutted with tests and documentation: CONTINUE intentionally creates a new run while conversation_key preserves the continued session identity.
  14. Fixed across the later command and recovery layers: desired states, queued state, attempts, claim reasons, and run-version delivery fencing are exercised.
  15. Rebutted at the boundary: coordinator lifecycle uses completed, while completion-card metadata deliberately maps success to ok. These are separate typed domains and both are tested.

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 iamwhatever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bolichen97

Copy link
Copy Markdown
Collaborator

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

  • This PR is OVERLAPPING with PR #5278. 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.
  • This PR is OVERLAPPING with PR #5279. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5277: MERGE_DISCUSSION. Extends this PR's port with a second implementation; strictly additive and stacked behind it. Files: src/kiro_crew/run_coordinator/models.py.
  • This PR is OVERLAPPING with PR #5280. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5277: MERGE_DISCUSSION. Later phase of the same stack; unblocks the operations this PR intentionally rejects. Files: src/kiro_crew/run_coordinator/memory.py.
  • This PR is OVERLAPPING with PR #5281. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5277: MERGE_DISCUSSION. Consumes this PR's delivery contract; complementary, not duplicative. Files: src/kiro_crew/run_coordinator/models.py.
  • This PR is OVERLAPPING with PR #5282. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5277: MERGE_DISCUSSION. Adds the recovery capability this PR's adapter deliberately omits. Files: src/kiro_crew/run_coordinator/memory.py.
  • This PR is OVERLAPPING with PR #5283. 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.
  • This PR is OVERLAPPING with PR #7195. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5277: MERGE_DISCUSSION. Independent goals in the same constructor; sequence them rather than choose between them. Files: src/kiro_crew/subagent.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
  • This PR is OVERLAPPING with PR #8003. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5277: MERGE_DISCUSSION. Same files, unrelated behavior; only a rebase-level interaction. Files: src/kiro_crew/subagent.py.

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

@bolichen97

Copy link
Copy Markdown
Collaborator

@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: src/kiro_crew/subagent.py and docs/system-specs/modules/subagent.md. Both change the SubagentManager.__init__ parameter list and body. This PR adds coordinator: RunCoordinator | None = None plus self._coordinator; #7195 adds adaptive_timeout / max_timeout plus an AdaptiveTimeoutPolicy construction. There is no shared symbol and no behavioural interaction, so the only cost is a textual conflict in one parameter block.

On which side is further along: this PR is purely additive against main, which has no run_coordinator package and no coordinator kwarg. #7195 needs rework before it can land, because merged #8891 raised the static subagent timeout default to 10800s, above that PR's 7200s learning ceiling.

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 dirty mergeable state and well behind main, and it is the root of the open stack #5278 through #5283, so a rebase is needed regardless. That rebase is also the moment to drop the undeclared test/test_resource_limits_schema.py hunk, which duplicates a trusted_system_bin patch already present on main.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: passed Eligible automated validation passed for the current revision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants