feat: persist subagent completion delivery - #5281
Conversation
github-actions
Bot
added
the
readiness: checking
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: |
bbb8bd0 to
d54aad2
Compare
github-actions
Bot
added
readiness: action required
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS Three undeclared changes ride along — two shipped frontend behavior fixes and a macOS spawn test — in a backend PR whose 19-bullet description never mentions the website. What this change shipsIntent: make a subagent's completion survive crashes and retries between "result recorded" and "parent notified" — an ADDITION, implementing PR 6 of the in-repo durable-run-coordinator RFC.
New public surface all has real consumers: Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 162bdff |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound outbox design at the coordinator layer, but unrelated frontend fixes are smuggled in and retry state is smeared through the gateway's untyped wave dicts. Watch
[DESIGN-REVIEWED] 162bdff |
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: |
d54aad2 to
e96f0f4
Compare
github-actions
Bot
added
readiness: checking
e96f0f4 to
dd370f8
Compare
github-actions
Bot
added
readiness: checking
dd370f8 to
ba831f9
Compare
|
/ai-review override fable 14636a5: Opus discovery exhausted its 120-turn ceiling without producing a SHA-scoped verdict; this exact head passed independent local review, GPT review, first-principles review, and the complete CI matrix. |
Human judgment recorded@kyleseaman marked the fable AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
|
|
|
|
Disposition: fixed Change: fence-less queued batch cancellation now constructs the neutral stopped member and routes it through the existing synthetic legacy announcement path; non-batch legacy cancellation remains unannounced. Evidence:
|
|
Full-diff overlap audit after rereading updated head #6272 makes queue-full refusal at the same admission boundary and changes typed rejection/announcement ownership. Rebase it only after #5280/#5281's command authority is established: fenced commands must record queue-full as a replayable terminal outbox result; identity-free batch-wave refusals/cancellations must follow the legacy announcement/digest path and keep the wave exemption. That ordering avoids a delivered tombstone hiding a live child, a stranded pending claim, duplicate completion, or an unclosed batch digest. |
|
|
|
Shutdown cancellation now keeps retrying until the synthetic terminal outcome is durably committed; focused shutdown and delivery regressions pass on fdc1adb. |
Durable terminal payloads and replayed completion events now preserve conversation identity, resolved model, and redacted requested-model metadata; replay regressions pass on fdc1adb. |
Synthetic delivery now completes only after a delivered attempt, while callback-started failures return to the adapter's bounded durable retry schedule; the complete delivery suite passes on fdc1adb. |
Both legacy orphan-process reap call sites now run through |
Durable retries now retain the orchestration tracker that owned the first delivery attempt, honor its stopped state even after the slot arms a new plan, and recheck cancellation after awaiting a busy slot before any queue or chat handoff. The regressions fail on the reviewed head and pass on |
|
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Review: persist subagent completion delivery
Reviewed origin/feat/run-coordinator-commands...origin/feat/run-coordinator-outbox (32 files, +5231/−367) at 5df30d604. Eleven angles plus a gap sweep; findings verified by executing the real OutboxDeliveryAdapter, SubagentManager, and the gateway's wave/digest paths. The full test suite passes, so every defect is a coverage gap rather than a broken test. I skipped the extra sweep pass: the confirmed pool was ~32 non-refuted candidates, more than double the reporting cap, so more candidates could only land below the cut.
The theme: this PR's purpose is exactly-once completion delivery, and the exactly-once machinery is breached in both directions — completions that are tombstoned as delivered but never delivered, and completions delivered twice.
Blocking
1. run_coordinator/delivery.py:196 — acknowledge() marks an event DELIVERED using the fence of a still-in-flight drain_once attempt, so a completion is tombstoned as delivered and then its only destination call fails, and drain_once reports DELIVERED for the failure. Event E is claimed by drain_once (fence epoch 1, in self._inflight) while await self._destination(E) is running. The consumption hook fires concurrently (waves.py::_ack_delivery_for_run → acknowledge(E)), reads that live fence outside self._lock, and marks E DELIVERED. The destination then raises _OutboxDeliveryRetry (injection timeout / _delivery_failed); drain_once's except Exception calls release_outbox(fence, …), _validate_delivery sees DELIVERED with a matching fence and returns UNCHANGED/ALREADY_DELIVERED, so attempts.append(DeliveryAttempt(E, DELIVERED)) — and both retry loops (terminal.py:500, admission.py:192, each if any(attempt.status is DeliveryState.DELIVERED)) return. Executed: acknowledge() returned: DELIVERED / drain_once attempts: [DeliveryAttempt(status=DELIVERED)] / destination calls: ['event-1'] / later drain attempts: [] after advancing the clock 10,000 s. The completion never reaches the parent and the event can never be re-claimed.
2. subagent_manager/waves.py:60 — _ack_delivery_for_run_impl retries when acknowledge() raises but treats a None return (acknowledgement NOT applied) as final success, so the event is never marked DELIVERED and the completion is replayed after restart as a duplicate injection. acknowledge() returns None whenever its re-claim yields nothing — and claim_outbox(..., acknowledgement=True) skips a row that is CLAIMED with claim_expires_at > now, which is exactly the state while the 60 s reaper's _drain_pending_outbox holds that event's 22-minute claim. The function then falls through the same unconditional return as success, without retrying and without popping _outbox_contexts. Executed with a real SubagentManager: concurrent drainer claimed: True -> CLAIMED, then after _ack_delivery_for_run: status = CLAIMED, delivered_at = None / context still registered = True / EXACTLY-ONCE VIOLATED. On the next gateway start _drain_pending_outbox re-delivers it via _info_from_outbox and the parent is injected a second copy of a completion it already consumed. The same silent no-op occurs one line earlier when _delivery_event_for_run returns "".
3. subagent_manager/terminal.py:270 — info._digest_held and info._delivery_queued are set once and never cleared anywhere in the tree, so _deliver_outbox_event_impl early-returns False on every later claim and the outbox retry path can never re-route a queued or held completion. grep finds exactly one write for each flag, both = True (gateway.py:6041, gateway.py:6904), and no reset. Once the gateway queues a completion into a busy dashboard slot or holds it for a digest chunk, drain_once gets False and releases with available_at = now + 22 min; every subsequent claim hits if info._digest_held or info._delivery_queued: return False before any routing, so _on_done is never invoked again and the only exit is acknowledge(). But chat_runner.py::_arm_queued_delivery_settlement._on_turn_done starts with if not consumed[0]: return, so a turn that fails before the model consumes the prompt (signed-out CLI, dead provider, purged or closed slot) settles nothing. Executed: round 0..3: status=pending attempts_col=1..4 on_done_calls=1 / FINAL status: PENDING / run id hidden from _reconcile_orphans? True — the event churns PENDING→CLAIMED→PENDING every 22 minutes for the process lifetime with attempts growing unbounded, the parent never gets the result, and because monitoring.py:140-148 builds durable_delivery_run_ids from _outbox_contexts (pruned only on the success path) that run id is also permanently excluded from _reconcile_orphans.
4. slack/gateway.py:6833 — deleting the batch_members_pending() fallback makes _last purely count-driven, but the digest-hold deadline sweep's guard is exactly inverted, so a wave whose done can never reach total is stranded by both mechanisms and every held sibling result is lost. The diff removes if not _last: _last = bool(self.subagent_mgr and not self.subagent_mgr.batch_members_pending(_batch_id)) together with its comment — "a wave member that failed AT SPAWN never reaches this consumer, so done can never hit total". Take a 5-member wave where member 5 never reaches _subagent_done (its durable commit hits _TerminalCommitRejected, which _report_terminal_impl handles with clear_tombstone + return and no _on_done; or its synthetic-batch report is stuck in the record_terminal retry loop). bp['done'] = 4 < bp['total'] = 5, so _last is False and members 1–4 stay _digest_held. The surviving backstop at waves.py:227 is if not self._manager.batch_members_pending(batch_id): continue — so once _batch_submitted is satisfied with no undone member in _agents and nothing queued, it declines to force a flush ("the real wave-close flush is already in flight"). Neither path closes the wave: four finished results are never injected, no delivered tombstone is written, _batch_progress leaks, and the parent waits forever. batch_members_pending now has that one inverted caller as its only production reader.
5. slack/gateway.py:7812 — the new per-batch retry-owner gate returns before the wave counting block and also swallows the two synthetic liveness backstops, so one failed non-final chunk freezes bp['done'] and permanently wedges the wave with no escape hatch. Wave of 12, chunk size 10: member w9 flushes chunk 1, its route raises, _retain_failed_position sets _batch_delivery_retry_owner['wv'] = 'w9'. Every later member hits if retry_owner and retry_owner != info.id: info._delivery_failed = True; return — which is before bp['done'] += 1 at :6777 — so done freezes at 10/12 and _last can never fire. Both unwedging backstops are swallowed too: force_digest_flush mints a fresh uuid.uuid4().hex[:8] on every sweep (waves.py:267-275) so it can never match retry_owner, and _announce_digest_flush_impl then hits the new if info._delivery_failed: return (waves.py:309-311) and settles nothing; record_lost_submission → _spawn_synthetic_batch_terminal_report hits the same gate. Nothing ages the owner out — it is popped only by that member's own success (:7837) — and the finally at :7843-7845 keeps _batch_delivery_locks[batch_id] whenever an owner is registered, so both dicts leak per wedged batch for the gateway's lifetime. Executed: after 10 members: done = 10 chunks = 1 / member w10 _delivery_failed: True | w11: True / closure dicts: [{}, {'wv': <Lock>}, {'wv': 'w9'}].
6. subagent_command_authority.py:956 — reject_waiting_execution now RAISES when no waiting claim exists (it used to be a silent no-op), and the only wrapping caller retries that permanently-unclearable condition forever at 1 Hz, so cancel() and queue settlement never return. The base had finish_error = "" and fell through to stop_execution_heartbeat. Now any second settlement of the same run raises: either the waiting entry is gone ("waiting execution claim not found") or it is present and finish_command returns outcome_conflict because the rejection_reason differs. terminal.py:357-373 catches exactly that exception inside while True with await asyncio.sleep(_TERMINAL_RETRY_SECONDS), no cap and no raise path, so _finalize_queued_cancel_impl/_finalize_queued_rejection_impl never reach _run_terminal_report: the user's Stop hangs permanently, the run gets no terminal record or outbox event, the queue entry is retained forever, stop_execution_heartbeat is never reached so the lease-renew task renews forever and blocks recovery takeover, and the batch member never reaches _subagent_done — compounding finding 4. Executed three ways: 1st reject ok; waiting entry retained: True / 2nd reject RAISES … outcome_conflict / 3rd reject (no waiting entry) RAISES … waiting execution claim not found, and _reject_waiting_before_terminal_impl was still spinning when an external 3.2 s timeout fired.
7. subagent_manager/cancellation.py:309 — cancel_impl still calls reject_waiting_execution with no try/except, but that call is no longer a safe no-op, so a user Stop landing in a narrow race window fails with an authority error and the subagent keeps running. A coordinator-admitted run starts; run.py awaits execution_started(info.id), whose last statement is await self.stop_execution_heartbeat(run_id) — that pops _waiting_executions[run_id] and then awaits asyncio.gather(lease_task, …), while info._coordinator_waiting = False is only executed after that await returns (run.py:354). A Stop arriving inside that window sees info.done False and _coordinator_waiting True, so cancel_impl calls reject_waiting_execution with the claim already popped → AuthorityOutcomeUncertain propagates out of cancel(). _force_reap at :326 is never reached: the subagent keeps running, no stopped card and no subagent_done event are emitted, and the HTTP stop returns 500 (messaging.py:1064 does not wrap it) or an authority-failure response. Executed: the same call is a no-op on the base branch; on HEAD it raises AuthorityOutcomeUncertain: … waiting execution claim not found.
8. subagent_manager/terminal.py:557 — the gateway's new batch gate sets info._delivery_failed and returns NORMALLY, but only the durable branch reads that flag; the legacy branch treats the swallowed callback as successful delivery and writes the delivered tombstone, permanently losing the completion. messaging.py:341 spawns non-keyed batches whenever command_identity is None, so those members have _coordinator_fence is None and take the legacy branch. One member of such a wave is refused at spawn (so it does get a durable outbox event via _spawn_synthetic_batch_terminal_report), its non-final chunk fails to route, and _batch_delivery_retry_owner[batch_id] is set. Legacy member B then finishes successfully; _subagent_done hits the retry-owner gate, sets _delivery_failed, and returns normally. Back in the legacy branch _on_done did not raise, so info._reported_to_parent = True is set and the mark_delivered_on_success and not info.error and not _digest_held and not _delivery_queued condition here is satisfied → await asyncio.to_thread(mark_delivered, B.id). B's folder is tombstoned, excluded from orphan reconciliation, and pruned after subagent_result_ttl_secs. The parent never receives B's result and nothing retries it — B has no outbox event. grep confirms _delivery_failed is read only at terminal.py:320 (durable path), waves.py:309, and gateway.py:7834 — never in the legacy branch.
Should fix
9. subagent_manager/admission.py:157 — _report_synthetic_batch_terminal_impl swallows asyncio.CancelledError and continues its unbounded commit loop when _shutting_down, so cancel_all's bounded straggler gather can never complete and gateway shutdown wedges permanently. cancel_all_impl sets _shutting_down = True, deliberately skips report tasks in the _tasks cancel loop (the new if task in report_tasks: continue at cancellation.py:386), waits _REPORT_DRAIN_TIMEOUT (30 s), times out, then does report_task.cancel() followed by await asyncio.gather(*stragglers, return_exceptions=True). The CancelledError lands inside await record_terminal(request); because _shutting_down is True the handler logs "retrying" and continues. Only one cancellation is ever delivered, and the retry hits a coordinator being torn down, so the loop takes except Exception: await asyncio.sleep(1.0); continue forever. gather() awaits a task that can never finish, so the lease-task cleanup (cancellation.py:465-471) and command_authority.close() never run and the process hangs — defeating the bounded drain whose own comment says stragglers must "gather to completion". Note only the inner delivery loop is guarded by while not self._manager._shutting_down (:188); the commit loop is not. Executed: RESULT: HANG — task still running after cancel(); record_terminal calls = 4; task.cancelling() = 2.
10. subagent_manager/terminal.py:323 — _delivery_batch_progress/_delivery_batch_final are cleared and _reported_to_parent set BEFORE the deferred check three lines later, so a merely-queued digest chunk loses its replay snapshot and a redrive double-counts the member, closing the wave early with wrong totals. Dashboard parent, wave of 12, chunk size 10: w9 flushes chunk 1; the dashboard route always calls _defer_queued_delivery, setting _delivery_queued = True. Back in _deliver_outbox_event_impl, :320 sees no failure, so :322-325 wipe the snapshot and set _reported_to_parent = True; only then does :326 return False and drain_once release the claim, leaving the event PENDING. On any later redrive the snapshot is gone, so _batch_retry is False and the member is counted a second time. Executed: chunk1 fired: 0 | done: 10 | flushed: 10 | _delivery_queued: True, snapshot after terminal.py:323 = None, after redrive: done = 11, then after w10: wave popped (_last fired) = True with batch_finished {'total': 12, 'ok': 12, 'err': 0, 'stopped': 0} broadcast while w11 had never completed — the wave closed early and over-reported successes.
11. subagent_manager/terminal.py:157 — _completion_payload_impl truncates result_summary BEFORE redacting it, so a credential straddling the 4000-char boundary leaks its raw prefix into the durable outbox payload_json. summary, _ = redact_exfiltration_urls(_done_result(info.result)[:_OUTBOX_RESULT_SUMMARY_LEN]) slices first, so a secret spanning that boundary is cut in half, the prefix no longer matches the credential regex, and the raw fragment is persisted into the SQLite outbox row and replayed to the parent on every redelivery and restart. task[:1000] and error[:2000] on the surrounding lines correctly redact first, so only result_summary is wrong. subagent.py:407 already provides _redact_and_truncate, whose docstring says verbatim: "Redact over the FULL text, then truncate (never _redact(x[:n])). Truncating first can cut a credential in half at the boundary, leaving a fragment the redaction regexes no longer match" — and it is already reachable from component modules (monitoring.py:29 imports and uses it). Executed with a 40-char cap: this order yields 'xxx…AKIAIOSFOD' (raw key prefix) while redact_and_truncate yields 'xxx…[REDACTED:'.
12. run_coordinator/delivery.py:27 — the 22-minute outbox claim is never renewed while the destination runs, and the destination's own budget is within 60 s of it, so a lease expiry mid-delivery lets a second drainer re-invoke the destination and inject the completion twice. drain_once claims E with claim_expires_at = t0 + 1320 s and there is no heartbeat for outbox claims (unlike run leases, which got _start_coordinator_heartbeat in this same PR). The destination path budget is _ON_DONE_TIMEOUT (1200 s) + teardown_done.wait(_RESET_TIMEOUT + 30) (60 s) = 1260 s, leaving 60 s for _fire_event, the to_thread mark_delivered calls, and — for a wave flusher — one _settle_digest_holds iteration per held member, each a to_thread plus a full acknowledge() round trip (and SQLiteRunCoordinator rewrites every row per call). Because the lease is wall-clock, a host suspend or NTP step of >60 s during delivery also expires it. Past expiry another drainer claims E and re-invokes the destination → the parent gets the completion twice; owner A then gets mark_delivered → STALE_FENCE, its claim_outbox(..., acknowledgement=True) returns [] because B holds a live claim, so _acknowledge_accepted returns REJECTED, settled stays False, and E leaks in both self._accepted and self._inflight forever. Executed: owner B destination calls while A in flight: ['event-1'] / owner A destination calls: ['event-1'] / => destination invoked 2 times for one completion. Nothing in code or CI pins _DELIVERY_LEASE_SECONDS > _ON_DONE_TIMEOUT + _RESET_TIMEOUT + 30; the derivation lives only in a comment.
13. subagent_manager/monitoring.py:455 — now = time.time() is sampled BEFORE the newly inserted and unbounded await _drain_pending_outbox(), so every deadline in the reaper sweep is evaluated against a clock that can be many minutes stale — and the sweep does not run at all during the drain. _drain_pending_outbox_impl loops while True over drain_once(limit=16), and each of those 16 destination calls is asyncio.wait_for(self._manager._on_done(info), timeout=_ON_DONE_TIMEOUT) with _ON_DONE_TIMEOUT = 1200.0 plus teardown_done.wait(_RESET_TIMEOUT + 30). One wedged parent slot therefore blocks the 60-second reaper iteration for up to 16 × ~21 min per pass while now still holds the pre-drain timestamp. Everything downstream under-reports by the drain duration: elapsed = now - info.started (so a run past _default_timeout is not force-reaped), _is_startup_stalled, _maybe_flag_stall, _sweep_stuck_waves, _sweep_conversations, and _sweep_digest_holds — the only latency trigger that force-flushes held wave results at DIGEST_HOLD_SECS=120. This is self-reinforcing: the deadline flush that would unblock held members is queued behind the blocked delivery. Executed with _REAPER_INTERVAL=0 and a 3 s stand-in drain: staleness: 3.7 s / agent real age: 6.7 s vs _default_timeout 5.0 / force_reap calls: [] — the overdue agent was skipped. The same inline await also sits at :125 ahead of tombstone writing.
14. subagent_manager/monitoring.py:63 — the new reap-before-tombstone pre-pass is a no-op on macOS AND Windows, so the PR's headline ordering invariant does not hold on either non-Linux platform. subagent.py:1935 does proc_stat = os.stat(f"/proc/{pid}") inside except (FileNotFoundError, OSError): return False. On macOS/Windows that always raises, so _is_orphan_process returns False, this guard returns False, and _reap_orphan_process never reaches _kill_orphan_pid or the orphan_reconcile_kill SEL audit — even though platform_compat.pid_exists and kill_pid both have real cross-platform implementations, so only the identity check is Linux-only. Concretely: the gateway crashes on macOS between the durable terminal commit and child teardown. On restart the pre-delivery snapshot loop visits the orphan, kills nothing, reaped_orphan_ids stays empty, then _drain_pending_outbox() delivers the event and _deliver_outbox_event_impl writes mark_delivered, removing the folder from the second list_orphans() — so the surviving child is never killed and is now invisible to every future reconcile. The second loop repeats the same no-op, and the whole pre-pass costs an extra full list_orphans() directory scan plus one thread hop per orphan for nothing. Executed on this host: platform: Darwin / own pid alive: True / _is_orphan_process(own pid, now): False / /proc exists: False. The repo ships install.ps1 and make.ps1, so Windows is supported.
15. slack/gateway.py:6960 — _delivery_progress = dict(bp) is a shallow copy that shares the held_infos LIST with the live wave dict, so a durable retry of the flushing member zeroes _digest_held_at for members of the NEXT chunk and permanently disables the reaper's hold-deadline sweep for them. Wave of 25, chunk size 10, notification-only parent: w9 flushes chunk 1; :6939 rebinds bp['held_infos'] = [] to a new list, and this line's dict(bp) captures that same list into w9._delivery_batch_progress. Members w10–w14 are then held and appended into the shared list. w9's injection exhausts its retries → notify_injection_failed sets _delivery_failed → terminal.py:321 raises _OutboxDeliveryRetry → the outbox redrives w9 → bp = dict(_retry_progress) and :6937 loops over the leaked held_infos setting _held._digest_held_at = 0.0 for w10–w14. waves.py:216 skips any info with _digest_held_at <= 0.0, so oldest gets no entry for the wave and force_digest_flush never fires — with a hanging straggler, five finished results are withheld for the entire _TIMEOUT_SECS window, regressing the #2215 latency trigger. Lines 7055-7059 deliberately rebind fail_lines/ok_lines/guard_msgs/held_delivery_ids/delivery_event_ids (safe) but nothing rebinds held_infos. Executed: ALIAS held_infos snapshot IS live: True / snapshot leaked next-chunk members: ['w10'..'w14'] / after retry, held clocks: [0.0, 0.0, 0.0, 0.0, 0.0] / => sweep guard skips them.
Cut by the reporting cap (all real, lower severity)
ShadowRunCoordinator outbox parity is 100% broken (shadow.py:46): event_id is in _GENERATED_IDENTIFIER_FIELDS so each store mints its own, but the adapter then replays release_outbox/mark_delivered with the primary's id — executed, MISMATCH boundary=mark_delivered on every completion, shadow outbox stuck at claimed, and no shadow test exercises a fenced outbox boundary · context.callback_started = True is set even when _on_done did nothing (terminal.py:294), so the first real _subagent_done_impl run is mislabelled _delivery_retry=True, permanently skipping the done WS broadcast and all orchestration-tracker accounting for that member (executed) · _settle_digest_holds_impl awaits an unbounded _ack_delivery_for_run per id (waves.py:388) after detaching the id list, so one bad id strands ids 3..9 — violating that function's own unchanged docstring — blocks inside drain_once holding a 22-min fence, and a shutdown cancellation loses the rest of the chunk's tombstones and acks · notify_injection_failed now sets _delivery_failed for every caller including legacy notification-only paths, and waves.py:309 newly honours it, so a legacy wave's deadline flush skips _settle_digest_holds and released members are never tombstoned and are re-announced after restart · _report_terminal's drain loop lacks the sibling's if context.callback_started: return and while not _shutting_down (terminal.py:502) → 1 Hz polling where each poll is a full SQLite DELETE+re-INSERT of all three tables plus PRAGMA quick_check · failed/stopped runs now enter owed/held_delivery_ids and the protecting guard if context is None or outcome == "completed" fails open (and scans only _outbox_contexts), so a failed run gets a delivered tombstone, collapsing its 7-day post-mortem window to delivered_ttl_secs (1 h) — the removed comment stated that invariant explicitly · unguarded json.loads in _info_from_outbox_impl (a poison-pill row is retried forever, and DeliveryState has no terminal-failure member) · except BaseException without re-raise in _drain_queue_impl, swallowing Ctrl-C into a fabricated spawn rejection · an approval-rejection lease leak from stop_heartbeat=False plus an early return · the dropped reject_waiting_execution in _finalize_queued_cancel_impl's fence-None branch, whose covering test test_queued_cancel_stops_the_authority_lease was deleted · _arm_queued_delivery_settlement registered twice on the same task · a second lease-renewal loop at terminal.py:101 duplicating SubagentCommandAuthority._start_execution_heartbeat line-for-line, so one run lease now has two owners.
Conventions: subagent.py:60 adds a module-level from kiro_crew.dashboard.chat_utils import dashboard_slot_key to a core module, contradicting two still-standing comments in terminal.py that lazily import the same symbol "because the dashboard layer must not be imported by a core module at import time" — measured: 19 dashboard modules now on the import path, was 3. And cancellation.py:344 introduces the comment marker "(GPT review)", which AGENTS.md forbids verbatim ("no PR/CR numbers, review-round markers…") and which regresses a previously clean comment.
Execution-verified AI-assisted review (Claude Code) across 11 angles plus a gap sweep, run against a local checkout at 5df30d604; nothing modified, nothing posted elsewhere. Findings name the input that reproduces them — please push back where one misreads intent.
|
Bolin review disposition for the current stack All fifteen numbered items from the September 2 review were rechecked.
The resulting outbox keeps terminal events durable through callback, digest, cancellation, shutdown, and retry races; acknowledgements remain fence-checked and delivery metadata round-trips conversation and model identity. Current submitted head: 162bdff. |
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. |
2 participants
Problem / Motivation
Recording a terminal subagent result and delivering its completion to the parent are separate operations, so a crash or retry between them can lose or duplicate the completion event.
Why it matters
Parent synthesis and batch accounting depend on completion events remaining durable until the parent actually consumes them, including failed and stopped runs that must retain their longer post-mortem window.
What changed (motivation → approach → change)
deliveredtombstone.Tests
test/test_run_coordinator_delivery.pycovers fenced claims, retry/defer behavior, acknowledgement races, failed/stopped delivery consumption, durable keyed-rejection routing, startup process-reap ordering, shutdown settlement, replay metadata, and callback retry ownership: 49 passed.test/test_subagent.pycovers keyed and identity-free queued batch cancellation, including the synthetic stopped completion that closes a legacy wave.Manual verification
N/A — retry, queue, digest, crash-recovery, cancellation, callback-ordering, and keyed-rejection behavior is deterministically covered by automated tests.
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: ...)