Repository navigation
fix(sdk): keep delayed reconciliation proof terminal_uncertain after admission budget (#6143) - #6145
Conversation
Broker-derived startup extends its proof window after pre-spawn preparation, but a cleanup proof completed after the original admission cutoff cannot be reported as a trustworthy spawn failure.
280d53e to
b11dcc0
Compare
Review fixes (coder)Trigger:
Local verification (macOS arm64, home2, after
New exact head: |
|
I verified this at b11dcc0 by running |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The change preserves the original admission cleanup cutoff while extending the lifecycle proof window, and conservatively returns terminal_uncertain for a late spawn_failed outcome. The source review found no confirmed runtime, contract, security, or compatibility defect. One explicit Linux CI verification requirement in the PR description remains unmet on this exact head.
Findings / Required Changes
- [P2] Run the required Linux lifecycle regression on this head —
packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts:9076-9077- The PR’s “Open questions” explicitly says Linux CI must validate the platform-gated regression. This test returns without exercising its assertions off Linux, and the exact-head run for
b11dcc099a394f506adb5d00692332c7b23513bacontains no job forsdk-broker-lifecycle-e2e.test.ts. The changedlifecycle.tsdoes not select this differently named test through the affected-test mapping (scripts/ci-dev-affected.ts:1361-1375), so successful affected-path checks do not establish that this regression ran. - This is a verification requirement gap, not evidence that the implementation is incorrect. Please run the Linux regression and confirm the required affected shards on this exact head before merge.
- The PR’s “Open questions” explicitly says Linux CI must validate the platform-gated regression. This test returns without exercising its assertions off Linux, and the exact-head run for
CI / Verification
The exact-head affected-path validation, package check, TypeScript build, selected Linux test jobs, virtual integration validation, and state-gate checks passed. The Linux lifecycle regression above was not among the selected jobs. “Merge approval bootstrap” failed because an authorized merge verdict was still awaited; this is the expected approval-policy gate, not a product-test failure. No tests were executed locally for this review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
The persisted admission cutoff and terminal-uncertain classification match the stated broker-derived lifecycle contract; no intent projection was available. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Persistence precedes effects; the extended proof deadline remains separate from the original admission cutoff, with durable uncertainty/replay handling. |
| A3 — Security / Privacy / Trust | APPROVED |
The change alters certainty classification only; it adds no authority, spawn, or authorization path. |
| A4 — Verification / Tests / CI | CHANGES_REQUESTED (Finding 1) |
The PR explicitly requires Linux validation, but the exact-head CI did not run the platform-gated lifecycle regression. |
| A5 — Context / Compatibility / Platform | APPROVED |
Consumers persist/replay the uncertain result; the timing classification is shared across platforms, and no materially preferable existing abstraction was found. |
|
@snowykr Here is Linux evidence for your P2, run on this exact head b11dcc0 (Linux x86_64,
You're right that the affected-path mapping doesn't select this test for |
Review fixes (coder)@snowykr, re Finding 1 [P2]: "Run the required Linux lifecycle regression on this head".
Exact-head Linux CI evidence (head
Local (macOS arm64, Bun) on
The remaining red check is New head: |
Review fixes (coder)Review: snowykr CHANGES_REQUESTED @
Local checks (macOS arm64, home2, worktree
Notes:
New exact head: |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The PR persists the original admission cleanup deadline and adds late-result classification, plus routes lifecycle changes to the broker lifecycle e2e suite. Two P2 correctness gaps remain: persisted cleanup replays bypass the new cutoff classification, while the final time-based wrapper can also overwrite a proven pre-ownership spawn failure.
Findings / Required Changes
-
[P2] Apply the cutoff classification to cleanup replays —
packages/coding-agent/src/sdk/broker/lifecycle.ts:8197-8212- The cleanup-replay branch returns
reconcileLifecycleCleanupbefore reaching the new admission-cutoff conversion at lines 8391-8405. A reachable case is an earlier attempt persistingcleanup_pending, followed by a replay at/afteradmissionCleanupDeadlineAtbut before the extended lifecycle cleanup deadline where exact cleanup now succeeds. The reconciler's default completion remainsspawn_failed, and the broker persists/returns it; the PR's stated terminal-uncertain policy is therefore not applied to this replay. - The base had the same replay result, but this PR adds the cutoff policy and leaves this reachable path outside it. Apply the same persisted-cutoff classification to the cleanup replay result while preserving
cleanup_pendingwhen proof remains incomplete.
- The cleanup-replay branch returns
-
[P2] Preserve proven pre-ownership spawn failures —
packages/coding-agent/src/sdk/broker/lifecycle.ts:8394-8405- For broker-derived launches, the stored admission cutoff comes from the initial readiness deadline, while pre-spawn preparation may consume the remaining receipt-based admission window before resetting the child readiness deadline. A pre-PID spawn error can consequently occur after the stored cutoff but within the effective extended readiness window. The existing
childOwnershipEstablished === falsebranch preserves the directspawn_failedresponse, but this final wrapper then replaces it withterminal_uncertainbased only on reconciliation time. - At base the definite no-child result remained
spawn_failed; at head it becomesterminal_uncertain, whose session fence has no uncertain-create retirement path requiringchildOwnershipEstablished === true. Preserve the proven pre-ownership result, or otherwise classify using the failure/ownership evidence rather than a later time check.
- For broker-derived launches, the stored admission cutoff comes from the initial readiness deadline, while pre-spawn preparation may consume the remaining receipt-based admission window before resetting the child readiness deadline. A pre-PID spawn error can consequently occur after the stored cutoff but within the effective extended readiness window. The existing
CI / Verification
The exact reviewed head is 808f07845c477c5a8a6b7c7b8d418dc872253616. The lifecycle e2e task selected by affected-path validation and its aggregate completed successfully. The workflow run's only failed job was the host-policy “Merge approval bootstrap” gate; its failure is not product-test evidence and is excluded from this review disposition. No local tests or code execution were performed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | CHANGES_REQUESTED |
Finding 1: cleanup replay does not honor the PR's late-cutoff policy. |
| A2 — Architecture / Correctness / Failure | CHANGES_REQUESTED |
Finding 2: reconciliation time overrides a known pre-ownership spawn failure. |
| A3 — Security / Privacy / Trust | APPROVED |
No concrete trust-boundary or authority change was identified. |
| A4 — Verification / Tests / CI | APPROVED |
Pinned lifecycle e2e task and affected aggregate succeeded; merge-approval bootstrap failure is host policy, not product CI. |
| A5 — Context / Compatibility / Platform | APPROVED |
Persisted metadata is scoped to broker admissions; existing affected-path mapping selects the lifecycle e2e suite. |
|
@probepark snowykr's two new P2s at 808f078 both concern the new admission-cutoff classification:
Could you take both? One way to address the second is to gate the conversion on |
…n cutoff A cleanup replay bypassed the admission cutoff conversion and persisted a late spawn_failed result. Reuse the ownership-aware classifier after exact reconciliation, leaving incomplete proof untouched. Confidence: high Scope-risk: narrow Reversibility: easy Tested: focused replay regression and coding-agent lifecycle e2e
The final response wrapper used time alone and replaced a direct pre-PID failure with terminal uncertainty. Route it through the ownership-aware classifier to retain proven failures. Confidence: high Scope-risk: narrow Reversibility: easy Tested: preownership classification regression and coding-agent lifecycle e2e
Review fixes (coder)Review: snowykr CHANGES_REQUESTED @ Both findings go through one shared classifier,
Local verification (macOS arm64, home2)
Diff digest for |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The PR records the original broker-admission cleanup cutoff separately from the extended proof deadline and conservatively reports terminal_uncertain when an owned child’s spawn_failed proof arrives at or after that cutoff. The guard is applied to both live completion and persisted cleanup replay, while preserving proven pre-ownership failures. No blocking or actionable findings were identified.
Findings / Required Changes
No blocking or actionable findings.
Non-blocking Observations
- Durable cleanup rows written before
admissionCleanupDeadlineAtwas added remain readable and replayable but lack that cutoff. If such an already-owned row later reaches the fallbackspawn_failedpath, the new classifier leaves that result unchanged (packages/coding-agent/src/sdk/broker/lifecycle.ts:8147-8154, 8224-8240). This matches base behavior and is not an explicit violation of the stated new-admission behavior; consider whether cross-version in-flight rows need separate handling.
CI / Verification
The exact reviewed head 745ae9d0b0a7617f8401b235029a5bef910747d9 has passing state-gate checks and affected-path validation, including the broker lifecycle E2E test task and its CI planner/self-test checks. The darwin-arm64 tab-worker smoke and several opt-in/manual checks were skipped; no local tests were run as part of this review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
The persisted deadline and classification match the bounded broker-derived admission behavior; no applicable contract violation found. |
| A2 — Architecture / Correctness / Failure | APPROVED |
The owned-child, post-cutoff classification is applied in live completion and durable cleanup replay; proof-budget guards remain separate. |
| A3 — Security / Privacy / Trust | APPROVED |
No new caller-controlled authority, identity bypass, or external-effect path was found. |
| A4 — Verification / Tests / CI | APPROVED |
Regression tests cover delayed cleanup and pre-ownership failure; exact-head affected lifecycle E2E task and state gates passed. |
| A5 — Context / Compatibility / Platform | APPROVED |
Consumers preserve uncertain-outcome semantics; the legacy-row edge is noted as non-blocking and does not contradict the bounded behavior. |
3 participants
Refs #6143
Why
#6135 gives broker-derived non-worktree launches a fresh child readiness window after pre-spawn work. Its persisted proof deadline is extended, but a delayed cleanup proof at the original admission cleanup cutoff was being classified as a proven
spawn_failed. The existing Linux regression test exposes that unsafe classification. #6144 separately reverts #6126; this fix does not modify the #6126 cutoff-reap behavior.What
Persist the original admission cleanup deadline alongside the extended lifecycle proof deadline for broker-derived launches. When a
spawn_failedclassification is reached at or after that original deadline, returnterminal_uncertainwhile retaining durable evidence. Caller-supplied deadlines and worktree launches do not receive the new field. This change is independent of #6144 and cherry-picks onto it without conflicts (280d53ea4->7bbd1c007). No timeout, expectation, or skip was relaxed; the existing regression test remains unchanged.Local tests (command + result)
All commands used
PORT_BASE=35240 COMPOSE_PROJECT_NAME=fix-6143-lifecycle-cutoff; macOS arm64, Bun 1.4.2. The Linux-only regression was temporarily ungated locally to exercise its actual assertions on macOS; the gate was restored and is not in the commit.008f7b14, before fixbun test packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts -t "delayed lifecycle reconciliation proof cannot return spawn_failed cleanup after its deadline"(temporary local platform-gate removal)Expected terminal_uncertain; Received spawn_failed—0 pass, 1 fail; also failed with #6135 lifecycle patch temporarily reversed, so that alone is not a causal isolation claim.280d53ea4git apply -R /tmp/gjc-6143-final.patch; bun test packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts -t "delayed lifecycle reconciliation proof cannot return spawn_failed cleanup after its deadline"; git apply /tmp/gjc-6143-final.patch(temporary local platform-gate removal)Expected terminal_uncertain; Received spawn_failed,0 pass, 1 fail; fix restored and worktree clean.bun test packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts packages/coding-agent/test/sdk-lifecycle-terminal-evidence.test.ts packages/coding-agent/test/sdk-broker-restart.test.ts162 pass, 1 skip, 1 fail, 164 tests; remaining terminal-evidence failure belongs to #6126 and is handled by #6144. Before fix, lifecycle alone153 pass, 1 skip, 0 fail; terminal-evidence4 pass, 1 fail; restart5 pass, 0 fail.bun test packages/coding-agent/test/sdk-broker-prespawn-readiness-budget.test.ts5 pass, 0 fail.7bbd1c007bun test packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts packages/coding-agent/test/sdk-lifecycle-terminal-evidence.test.ts packages/coding-agent/test/sdk-broker-restart.test.ts159 pass, 1 skip, 0 fail, 160 tests; individually terminal-evidence5 pass, 0 fail, restart5 pass, 0 fail.cd packages/coding-agent && bun run check:types$ tsc -p tsconfig.json --noEmit; exit 0.bunx biome check packages/coding-agent/src/sdk/broker/lifecycle.tsChecked 1 file in 81ms. No fixes applied.The requested shard-6/8 and shard-8/8 baseline commands were attempted before dependencies existed and failed during preload. After
bun run setup:worktree, shard-6/8 completed with 9 unrelated failures (tmux/harness/perf/transport/worktree files); shard-8/8 was cancelled when the scope changed to the #6135 regression. These are not counted as evidence of a full CI shard PASS. No e2e/external-service/cluster/browser runs were performed.Needs e2e
No external-service or cluster e2e run in this scope. Linux CI must execute the existing platform-gated delayed-proof case and the affected shard before merge.
Acceptance
lifecycle.ts0/1, after1/0; mutation also0/1.1 pass, 0 failon dev+fix and on #6144+fix.5/0, typecheck and biome pass.7bbd1c007159 pass, 1 skip, 0 fail.Risk classification
low-risk— ordinary fix/maintenance.regression-risk— lifecycle terminal classification changes; independent review required.high-risk— broad/destructive lifecycle change.GJC verdict
Agent PR: the
Merge approvalcheck stays red until a maintainer approves the exact head; that is expected.Agent
GJC coding agent; commit
280d53ea4. UsedPORT_BASE=35240andCOMPOSE_PROJECT_NAME=fix-6143-lifecycle-cutoff; no containers or long-running services were started.Open questions
Linux CI must validate the platform-gated regression and complete affected shards. The two #6126 failures are outside this PR and depend on #6144 landing.
devbun checkpasses (not run; focusedcheck:typesand biome passed)packages/<pkg>/changelog.d/(internal correctness fix; none added)