fix(sdk): let a late agent_failed supply the reason a settled prompt lacks - #4105
Conversation
d68c339 to
b5e5cc8
Compare
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Red-team review — exact head b5e5cc8 vs dev d26ceda
Verdict: REQUEST_CHANGES — the fix is internally correct but lands in the wrong reconciler; the production runtime path it claims to fix is untouched and the bug persists.
Evidence (decisive): the modified reconciler is not on the production path
sdk/bus/index.ts instantiates two reconcilers. The PR modifies the one that production never transitions through:
index.ts:4122—kindReconciliation = createKindAwareReconciliation({ store: durableStore })← productionindex.ts:4131-4133—createPromptReconciliation()with the explicit comment:// Backward-compatible process-local prompt reconciler kept for unit-test isolation; production path uses kindReconciliation for prompt+skill.
Wiring that confirms production routes exclusively through kindReconciliation:
- transitions:
index.ts:4971-4972—notePromptReconciliation: (correlation, frame) => { void kindReconciliation.noteTransition("prompt", correlation, frame); } - lookups:
index.ts:4141-4142—lookupPromptStatus = ... kindReconciliation.lookup("prompt", selector) createPromptReconciliationis referenced insrc/exactly twice (index.ts:132import,4133instantiation), and the only method ever invoked on that instance in production is.cleanup()(index.ts:4177). ItsnoteTransition/lookup/noteAccepted/admithave zero production callers.
The real bug survives unchanged in kind-aware-reconciliation.ts
kind-aware-reconciliation.ts:196-198:
const record = candidate.get(keyOf(kind, correlation));
if (!record || record.terminalAt !== undefined) return { value: undefined, changed: false };A late agent_failed arriving after an already-terminal record is still dropped outright — no enrichment, no reason attached. This is the exact behavior the PR title ("let a late agent_failed supply the reason a settled prompt lacks") claims to fix, and it is the exact code path that actually serves Q26 turn.prompt_status. A third copy of the same pattern exists in sdk/host/session-runtime.ts:411-414 (createInvocationReconciliation.noteTransition) and is also untouched.
Type divergence — the new error? field will never cross the wire
The PR adds error?: { code; message } to terminal_ok on the shadow TurnPromptReconciliation in prompt-reconciliation.ts:64-74. The canonical DTO in sdk/prompt-status.ts:76-82 (TurnPromptReconciliationTerminalOk) — the type that kindReconciliation.lookup returns and that the Q26 query handler serializes (host/query/handlers.ts:636-639) — has outcome? on terminal_ok but no error?. So even after this PR, a client calling turn.prompt_status on a settled prompt that received a late failure still gets terminal_ok with no error detail.
Tests assert against the unreachable unit
All three new/expanded cases in sdk-q26-prompt-status.test.ts call createPromptReconciliation(...) directly — the shadow reconciler. None exercise kindReconciliation or any integration path (sdk-prompt-terminal-arbiter, sdk-host-wiring). I confirmed the 25 unit tests pass against this head, but they cannot detect that the production fix is missing.
What is correct (in isolation)
- Enrichment guard
frame.type === "agent_failed" && record.error === undefinedcorrectly implements first-reason-wins. status/terminalAt/ retention /clientRefIndexare untouched — no resurrection.sanitizePromptFailureis reused; no new code/message surface and no unbounded text leak.- No regression to the unit's prior behavior; CI green at head.
Requested changes
- Apply the same late-
agent_failedenrichment tokind-aware-reconciliation.tsnoteTransition(and decide onsession-runtime.ts:411createInvocationReconciliation). - Add the
error?field toTurnPromptReconciliationTerminalOkinsdk/prompt-status.ts(the canonical DTO), and surfacerecord.errorfromkindReconciliation.lookupforterminal_ok, so the reason actually reaches the wire. - Add at least one integration test through
kindReconciliation(or the Q26 query handler) proving the late reason surfaces end-to-end; otherwise the regression is silently re-introduced.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
CI note: the two shard failures are pre-existing on
Both appear in Its own suite is green: 25 pass, 0 fail, and 22 pass / 3 fail when the source change is reverted. |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Repair review — production-path fix landed at bba0d04
Verdict: MERGE_READY — the prior REQUEST_CHANGES is resolved. The late agent_failed enrichment now applies to the production reconcilers, the canonical DTO carries error? on terminal_ok, and the reason reaches the Q26 wire. No new CI failures; only the documented #4078 baseline (ultragoal-redteam-resident-cache, title-source-persistence) remains, present on dev itself.
What changed (4 files, +101/−3 vs b5e5cc8; PR head now bba0d04)
-
kind-aware-reconciliation.ts(production bus path) —noteTransitionno longer drops a lateagent_failedon an already-terminal record. Enrichment is guarded byframe.type === "agent_failed" && record.error === undefined(first-reason-wins, exactly-once), returnschanged: trueso it persists throughstore.transact, and does not re-runcleanupRecords— status,terminalAt, retention order, and the clientRef index are untouched.lookupnow surfacesrecord.erroronterminal_okalongsideoutcome. -
session-runtime.ts(SDK-only host path) —createInvocationReconciliation.noteTransitiongets the equivalent enrichment with the same first-reason-wins guard, plus the local diagnostic log (SDK invocation failed (late)) for parity with the first-failure path. Itslookupalready surfacederroron terminal records, so no lookup change was needed. Documented as serving the same Q26 contract viacreateSdkSessionRuntimeExtension. -
prompt-status.ts(canonical DTO) —TurnPromptReconciliationTerminalOknow haserror?: { code; message }with the same docstring as the shadow unit, and the contract comment documents the settle-once / late-enrichment / first-reason-wins semantics. This is the type that the Q26 query handler serializes, so the reason now crosses the wire.
Verification (run at exact head bba0d04)
| Suite | Result |
|---|---|
sdk-prompt-terminal-arbiter (kind-aware production, incl. 3 new e2e tests) |
9 pass |
sdk-q26-prompt-status (shadow unit) |
25 pass |
sdk-reconciliation-store + sdk-reconciliation-recovery |
16 pass |
session-runtime.test.ts |
6 pass |
sdk-host-wiring (93, full integration) |
93 pass |
sdk-surface-parity |
5 pass |
sdk-broker-lifecycle-e2e + sdk-downgrade-rollback |
63 pass |
check (biome + check:types tsc) |
clean |
The 3 new end-to-end tests in sdk-prompt-terminal-arbiter exercise the production kindReconciliation path: (a) late agent_failed enriches a terminal_ok record and the reason surfaces through lookup and persists to the durable store snapshot; (b) a second disagreeing late frame does not overwrite the first (first-reason-wins); (c) late agent_start/agent_end do not enrich. CI independently confirms sdk-q26-prompt-status, sdk-prompt-terminal-arbiter, session-runtime.test.ts, and ts-build all pass on bba0d04.
CI at bba0d04
The only failures are shard-1-of-8, the aggregate Affected path validation, and the downstream evidence producer — all caused by the two #4078 baseline tests (ultragoal-redteam-resident-cache.test.ts:563, title-source-persistence.test.ts:299), which fail identically on base dev (d26ceda). On dev, 7 of 8 shards fail from the same baseline; on this PR head, only 1 shard fails, with exactly those two tests. No reconciliation/prompt-status/session-runtime failure is present. My change strictly improved CI coverage.
Adversarial notes
- Privacy/sanitization intact. All three sites reuse
sanitizePromptFailure(code safe-token ≤64, fixed redacted message). No unbounded provider text is retained on the record or wire; the session-runtime late path also logs viaformatPromptFailureForLocalLog(local-only). - No resurrection. In both reconcilers the enrichment branch returns before any status/terminal/retention mutation;
cleanupRecordsis deliberately not re-run in the kind-aware path, so retention order and capacity eviction are stable. - Durability coherence. The enriched
terminal_okrecord witherrorset remains valid underisValidRecord(erroris validated independently atreconciliation-store.ts:111-114;outcomevalidation is unaffected). On restart, terminal records pass throughsettleProcessRestartunchanged. - Shadow unit retained. The original
createPromptReconciliationfix (b5e5cc8) is preserved as the reference implementation; both now agree.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
Integration is bounded-held behind active #4108 and #4098 SDK/session-runtime owners, not for a late-failure-reason defect. Exact head The PR changes — |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
BOUNDED_OWNER_HOLD — exact head bba0d04ab681c51e8b0a741b3c94e5edc84cbe54
The #4088 implementation is LGTM in isolation: production kindReconciliation, the SDK host reconciler, and the canonical Q26 DTO all apply sanitized, first-reason-wins late-failure enrichment without changing terminal status, terminalAt, capacity/retention, or client-ref ownership. The complete six-file diff against PR base d26ceda4229e456d526bf454ed2ef59bcf772686 has no whitespace errors and no generated artifact changes. origin/dev was refreshed to 797da02670fd5c32fc28b1a9f95d46d4d82be485; it is an ancestor-compatible update with no touched-file overlap.
Targeted mutation teeth at this exact head: sdk-prompt-terminal-arbiter passes 9/9 with the implementation; reversing the production enrichment hunk yields 7 pass / 2 fail, proving the new durable lookup/first-reason assertions fail without it. Exact-head CI passes the affected reconciliation, Q26, host-runtime, typecheck, production-host-isolated, native-build, smoke, and state-gate checks. Its aggregate check remains red only through shard-1/evidence-producer; those failures are documented baseline failures, not a changed-path failure.
Contributor tier: probepark is a write collaborator and the fork allows maintainer modification. Automated/Codex review has no comments on this PR. I am not modifying the branch because active internal owners #4108 and #4098 control the same session-runtime terminal contract: #4108 currently asserts the opposite late-failure result, while #4098 is a conflicting lifecycle-authority integration. Merging or independently repairing this branch before those owners reconcile the single settle-once contract would create a contradictory test/behavior pair.
Signed verdict: bounded owner hold, not a merge authorization. Preserve this exact reviewed implementation and integrate it only when the #4098/#4108 owner resolves the shared terminal contract and rebases/revalidates against current dev.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Yeachan-Heo
left a comment
There was a problem hiding this comment.
BOUNDED_OWNER_HOLD — exact head bba0d04ab681c51e8b0a741b3c94e5edc84cbe54
LGTM in isolation. I independently reviewed the complete six-file diff against its declared base d26ceda4229e456d526bf454ed2ef59bcf772686: production kindReconciliation, the host reconciler, and the canonical Q26 terminal_ok DTO consistently permit only sanitized, first-reason-wins late agent_failed enrichment. They preserve terminal status, terminalAt, retention/capacity behavior, and client-ref ownership. No inline automated/Codex comments are present; prior maintainer REQUEST_CHANGES is resolved by the exact-head approval. The diff has no whitespace errors and no generated artifacts to update.
Exact-head verification passed: bun test over sdk-prompt-terminal-arbiter, sdk-q26-prompt-status, sdk-kind-aware-reconciliation, and host session-runtime — 42 pass / 0 fail / 125 expectations; after generating the ignored docs index required by the checkout, bun run check:types passed.
origin/dev was refreshed to 797da02670fd5c32fc28b1a9f95d46d4d82be485; this head is not merged and its Dev CI is UNSTABLE (Affected path validation / test:@gajae-code/coding-agent:shard-1-of-8 failed, with dependent evidence/aggregate failures). More importantly, active PR #4108 changes the same packages/coding-agent/src/sdk/host/session-runtime.ts; #4098 remains the active lifecycle-authority integration. This external contributor branch is maintainer-editable, but parallel repair would create conflicting terminalization ownership. Hold integration for those owners; no contributor mutation is requested from this review lane.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
bba0d04 to
138c776
Compare
The test asserted `error` stays undefined after a late provider failure on a terminal record. Yeachan-Heo#4105 makes exactly that case populate the reason, so the two branches asserted opposite contracts on the same path and each would have broken the other on merge. Yeachan-Heo#4105 was reviewed and approved on that design, so this conforms to it. What survives is pinned instead: a late failure must not flip status, must not re-stamp terminalAt, must not re-open the turn, and must not overwrite a reason already present. A mutation matrix confirms the rewrite still catches resurrection and last-reason-wins, and stays forward-compatible with Yeachan-Heo#4105. Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns the suite from 12 pass to 10 pass / 2 fail, so the core fix stays load-bearing Not-tested: absence of enrichment is not caught here, because undefined is still an allowed value until Yeachan-Heo#4105 lands
|
A gap in this PR, found while reconciling #4108 against it. Reporting against my own approved branch because an approval is not evidence. This PR changes That is the same shape as the blocker the first review caught here: a proof that is internally consistent while pointing at the wrong object. It caught the production-path miss; this is the residue. #4108 now carries a comment at the exact assertion that will become load-bearing for this once both land, so the follow-up is anchored in-file rather than in a review thread: Until then |
|
CI note: Clean $ bun test packages/coding-agent/test/agent-session-fallback-upstream-count.e2e.test.ts
10 pass
10 failHalf that file fails on the branch's own tip. This PR touches This is the third distinct set of pre-existing failures I have hit on this branch as |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Exact-head review — 138c776 vs base e8a600c
Terminal verdict: REQUEST_CHANGES (bounded, integration-scoped). The late agent_failed enrichment contract is correct and internally non-contradictory; the exact implementation must stay intact. Two PR-specific blockers remain, both test/integration-shaped, plus one external base CI blocker. No merge.
1. Contract validation — sound (verified at exact head)
- First-reason-wins is enforced by gating on
record.error === undefinedin all three reconcilers (kind-aware-reconciliation.ts,prompt-reconciliation.ts, hostcreateInvocationReconciliation); late generic frames never overwrite a specific reason. - No resurrection: the late branch mutates only
error; status,terminalAt, retention order, and the clientRef index are untouched (cleanupRecordsintentionally not re-run). Tests assertterminalAtunchanged andactiveCountstays 0. terminal_okgains optionalerroron the canonical DTO;failedalways carrieserror(record.error ?? sanitizePromptFailure(undefined)→internal). The Q26 handler returns the lookup verbatim; no consumer rejects the new optional field.- Persistence is real: kind-aware
queueMutationpersists viastore.transact;reconciliation-store.isValidRecordacceptsterminal_ok+errorand survives reload; the host reconciler persists and re-sanitizes on hydrate. - Doc comments in
prompt-reconciliation.tsandprompt-status.tsare consistent with the implemented settle-once exception. - Local run at exact head: 136 pass / 0 fail across
sdk-prompt-terminal-diagnostics,sdk-host-wiring,sdk-prompt-terminal-arbiter,sdk-q26-prompt-status,session-runtime; biome + tsc clean.
2. PR-specific blocker A — host-side enrichment ships untested (confirmed)
createInvocationReconciliation.noteTransition in session-runtime.ts is enriched, but every new test lives in the bus-level suites. I independently reproduced probepark's self-report (2026-08-10T00:02:45Z): reverting only the host hunk leaves the entire 5-file suite green — 136 pass / 0 fail. The host path has no load-bearing proof.
3. PR-specific blocker B — proven semantic contradiction with #4108
Merged-tree evidence (scratch worktree, git merge 138c776ec into #4108 head df081e1507):
- #4108 alone:
session-runtime.test.ts→ 11 pass / 0 fail. - #4108 + #4105 merged: → 10 pass / 1 fail —
post-acceptance invocation terminalization > a later provider error never overwrites an already terminal prompt, atexpect(settled.result?.error).toBeUndefined()(merged-file lines 487–488).
That scenario is exactly issue #4088's: the terminal is claimed by agent_end on one path while the failure reason arrives on another. #4105's enrichment sets error there; #4108's assertion demands it stay undefined. #4108's call-site comment ("noteTransition ignores an already-terminal record, so this cannot overwrite an existing terminal") is stale once #4105 lands — true for status/terminalAt, false for error. Merge order matters: #4108 must land with its terminalize-once assertion aligned to the enrichment contract (status/terminalAt unchanged, sanitized error present), or #4105's host proof must land alongside #4108's harness.
4. Overlap ruling — #4098 cleared, #4108 entangled
- #4098 (
43ea4d6649, session-lifecycle refactor): zero file overlap with #4105's six-file touch set; no reconciliation/prompt-status/session-runtime changes. The prior hold rationale for #4098 is resolved on file and contract grounds — not a blocker. - #4108 (
df081e1507): true commit set is exactlysession-runtime.ts(+7/−2) andsession-runtime.test.ts(+233, incl. theinvocationHarnessthe missing host proof needs). Direct file + semantic entanglement with #4105's host change; resolution is owned by the #4108 lane (same author), which already carries the harness and the load-bearing assertions.
5. External base CI blocker — #4124 (verified, not repaired here)
CI run 31342546050 (exact head, PR event): 3 failed jobs = test:@gajae-code/coding-agent:shard-1-of-8 + downstream evidence producer + fail-closed aggregator (both fail closed off the shard per #4124's mechanism). Local repro of the shard suite agent-session-fallback-upstream-count.e2e.test.ts: 10 fail / 10 pass at exact head and an identical 10 fail / 10 pass at pure base e8a600c — same two ~5s queued steer/followUp successor timeouts. Issue #4124 ("regression: #4120 breaks AgentSession continuation across all CI shards") documents the identical shard-1 signature. All red on this head maps 1:1 to the #4124 dev-base regression; no PR-specific CI failure exists. Shared repair belongs to the #4124 lane; none was attempted here.
6. Action requested (no branch mutation made)
No edits were made to this branch — the exact implementation is kept intact. The author (T0) owns both #4105 and #4108, and #4108 already carries the host harness; mutating #4105's branch to add a host test that #4108 will also add would create a duplicate mutation owner and merge conflicts.
Requested, in order:
- Land #4108 with
terminalize-oncealigned to the enrichment contract, or rebase #4105 onto #4108 and add the smallest host-side exact-contract test via #4108'sinvocationHarness(lateagent_failedafterterminal_ok→ status/terminalAtunchanged, sanitizederrorpresent, no new active slot). - Re-run exact-head CI after #4124 lands (external blocker).
- Re-request review on the rebased head for a signed merge decision.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
Taking option 1. Rebased onto #4108; it applied clean. Blocker B is already resolved — the review read #4108 at a stale head. At Blocker A is real and is being fixed here, not on #4108 — you are right that the host path has no load-bearing proof, and #4108's assertion could not supply one on its own base, because In flight on this branch: collapse the Agreed on #4124: shard-1 is 10 pass / 10 fail at pure base, so nothing on this head is PR-specific. Not touching it from here. |
138c776 to
d163d76
Compare
|
Blocker A closed. Pushed You were right that reverting the host hunk alone left all 136 green. It no longer does:
Two edits, both in
Both mutations were applied one at a time against a pre-mutation copy and the source restored after each; On merge order — this branch now sits on top of #4108, so landing #4108 first is not just preferred, it is what the assertion depends on. |
d163d76 to
787354f
Compare
|
Correction to my previous comment. When I wrote that this branch sits on top of #4108, it did — on #4108 at Rebased onto the current #4108 head and re-verified from scratch rather than trusting the earlier run:
New head |
The test asserted `error` stays undefined after a late provider failure on a terminal record. Yeachan-Heo#4105 makes exactly that case populate the reason, so the two branches asserted opposite contracts on the same path and each would have broken the other on merge. Yeachan-Heo#4105 was reviewed and approved on that design, so this conforms to it. What survives is pinned instead: a late failure must not flip status, must not re-stamp terminalAt, must not re-open the turn, and must not overwrite a reason already present. A mutation matrix confirms the rewrite still catches resurrection and last-reason-wins, and stays forward-compatible with Yeachan-Heo#4105. Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns the suite from 12 pass to 10 pass / 2 fail, so the core fix stays load-bearing Not-tested: absence of enrichment is not caught here, because undefined is still an allowed value until Yeachan-Heo#4105 lands
787354f to
3f1bc4a
Compare
|
CI update: shard-1-of-8 no longer fails on #4124's signature — that regression is fixed on # clean origin/dev @ 10144dc8d
$ bun test packages/coding-agent/test/agent-session-fallback-upstream-count.e2e.test.ts
20 pass 0 fail # was 10 pass / 10 fail — #4124 is goneThe two tests failing on this head now are different ones, and both are still # clean origin/dev @ 10144dc8d, nothing applied
$ bun test packages/coding-agent/test/task-fork-context.test.ts
23 pass 1 fail # fork context policy surface > passes a sanitized fork seed…
$ bun test packages/coding-agent/test/model-selector-action-menu-role-binding.test.ts
14 pass 1 fail # ModelSelector assignment menu role bindings > reports the effective default…Neither file is in this PR's touch set, and neither has a path to it. Worth stating plainly that I checked instead of assuming: when #4124 got fixed the old blanket explanation stopped being true, and a stale "it's dev's fault" would have hidden a real failure. |
The test asserted `error` stays undefined after a late provider failure on a terminal record. Yeachan-Heo#4105 makes exactly that case populate the reason, so the two branches asserted opposite contracts on the same path and each would have broken the other on merge. Yeachan-Heo#4105 was reviewed and approved on that design, so this conforms to it. What survives is pinned instead: a late failure must not flip status, must not re-stamp terminalAt, must not re-open the turn, and must not overwrite a reason already present. A mutation matrix confirms the rewrite still catches resurrection and last-reason-wins, and stays forward-compatible with Yeachan-Heo#4105. Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns the suite from 12 pass to 10 pass / 2 fail, so the core fix stays load-bearing Not-tested: absence of enrichment is not caught here, because undefined is still an allowed value until Yeachan-Heo#4105 lands
The test asserted `error` stays undefined after a late provider failure on a terminal record. Yeachan-Heo#4105 makes exactly that case populate the reason, so the two branches asserted opposite contracts on the same path and each would have broken the other on merge. Yeachan-Heo#4105 was reviewed and approved on that design, so this conforms to it. What survives is pinned instead: a late failure must not flip status, must not re-stamp terminalAt, must not re-open the turn, and must not overwrite a reason already present. A mutation matrix confirms the rewrite still catches resurrection and last-reason-wins, and stays forward-compatible with Yeachan-Heo#4105. Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns the suite from 12 pass to 10 pass / 2 fail, so the core fix stays load-bearing Not-tested: absence of enrichment is not caught here, because undefined is still an allowed value until Yeachan-Heo#4105 lands
3f1bc4a to
9db6b00
Compare
|
Re-requesting a verdict. Both PR-specific blockers from the exact-head review are closed, and the head has moved twice since it was written. Blocker A (host enrichment untested) — closed. Reverting the host hunk alone no longer leaves the suite green:
Blocker B (contradiction with #4108) — closed. You read #4108 at Blocker C (#4124 base CI) — that regression is fixed on Current head is rebased onto |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Terminal verdict — APPROVE at exact head 9db6b00fc11e43faeaae8254820268f165b31085
Head-movement note (pinned vs current). The brief pinned exact head 3f1bc4ac00f25dc440ca51ce261e2b5e6511d41b vs base dev 10144dc8d483f7a26df245cbb1ca9bd1b20eb5fd. Since then the branch was rebased onto the newer base 515ef1aa63 (dev advanced with #4130). I verified PR content is byte-identical between the two heads (git diff 3f1bc4ac00 9db6b00fc1 over all PR files = 0 lines); only the base moved. All evidence below was re-executed fresh at the current exact head 9db6b00fc1.
The prior REQUEST_CHANGES blocker is resolved. The host-side createInvocationReconciliation.noteTransition late-enrichment mutation now has a load-bearing host test (test(sdk): make the host reconciler's late-failure enrichment load-bearing — session-runtime.test.ts, assertion tightened from the [undefined, {code:"upstream_error",…}] allow-list to the strict expect(settled.result?.error).toEqual({ code: "upstream_error", message: "Prompt submission failed." })).
Independently executed mutations (host path, current head 9db6b00fc1)
| Check | Result |
|---|---|
Baseline session-runtime.test.ts |
13 pass / 0 fail |
Mutation A — revert host enrichment hunk in session-runtime.ts |
11 pass / 2 fail — both fail via createInvocationReconciliation.noteTransition after a terminal claim, with settled.result?.error = undefined |
Mutation B — remove record.error === undefined guard |
11 pass / 2 fail — first-reason-wins broken: a later transport_reset overwrote the recorded upstream_error |
Mutation A failures (both host-path):
a reason attached after a prompt settled is never replaced by a later failurepost-acceptance invocation terminalization > a later provider error enriches but never re-opens an already terminal prompt
Mutation B failures (both host-path):
a late agent failure never overwrites the reason an already terminal record carriesa reason attached after a prompt settled is never replaced by a later failure
Invariants pinned by the tests: status/terminalAt/identity stay exactly as claimed (settledTerminal ≡ claimedTerminal); a second late failure changes nothing (first reason wins, sleep-stamped terminalAt immutability is observable); the recorded reason is the sanitized late failure — never fabricated, never raw (transport reset is redacted to upstream_error/Prompt submission failed.).
CI at current exact head
Dev CI run 31347946435 at 9db6b00fc1: every affected-path job green — test:packages/coding-agent/src/sdk/host/session-runtime.test.ts: success, sdk-prompt-terminal-arbiter: success, sdk-q26-prompt-status: success, notifications-live-stream: success, session-manager-resident-cache: success, sdk-production-host-isolated: success, ts-build: success, gjc-state-gates: success, native-build: success. The only red is shard-1-of-8, whose failures are exactly the two current-base tests already failing on clean dev (ModelSelector assignment menu role bindings > reports the effective default when an active profile supplies it; fork context policy surface > passes a sanitized fork seed and cache identity without sharing provider state) — the #4125 base regression, reproduced identically at the clean dev base. Zero #4105-local CI failure; the shard redness is not a reason to request changes.
Coexistence / interaction
- #4108 — its commits are embedded in this PR (old-head commit #2
d1aaa17d10= pinned #4108 head; rebased equivalent46195437at current head). The merged tree is the current head; combined contract suite (session-runtime.test.ts+sdk-prompt-terminal-arbiter.test.ts+sdk-q26-prompt-status.test.ts) runs 47 pass / 0 fail at9db6b00fc1. #4108 can close as superseded after this PR merges. - #4098 — zero file overlap with this PR (full-diff scan); single
export * from "./prompt-status"no-op re-export only. No interaction.
Verdict: APPROVE (MERGE_READY). The host-path enrichment is load-bearing, first-reason-wins and terminal immutability are load-bearing, and no local CI or cross-PR blocker remains. Per review protocol I do not merge; the maintainer may.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
…ptance A post-acceptance submission error was recorded as agent_failed only when the kind was "skill". For an ordinary prompt the error was dropped, so nothing terminalized the correlation: the session host stayed alive reporting live=true while the run selector returned unknown, the result query returned an empty assistant item, and a queued steer was never consumed. An SDK consumer waiting on a terminal event waited forever. The submission promise rejects after preflight acceptance only when the work is over, so every kind must terminalize there. noteTransition ignores an already-terminal record, so this cannot overwrite an existing terminal. Lore-id: 4056-terminalize-interrupted-prompt Confidence: high Scope-risk: narrow Reversibility: easy Tested: mutation -- reverting the source turns the suite from 11 pass to 9 pass / 2 fail Not-tested: whether the session stops reporting live=true and drains the queued steer; this proves the record terminalizes, not that every consumer of that state reacts
The test asserted `error` stays undefined after a late provider failure on a terminal record. Yeachan-Heo#4105 makes exactly that case populate the reason, so the two branches asserted opposite contracts on the same path and each would have broken the other on merge. Yeachan-Heo#4105 was reviewed and approved on that design, so this conforms to it. What survives is pinned instead: a late failure must not flip status, must not re-stamp terminalAt, must not re-open the turn, and must not overwrite a reason already present. A mutation matrix confirms the rewrite still catches resurrection and last-reason-wins, and stays forward-compatible with Yeachan-Heo#4105. Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns the suite from 12 pass to 10 pass / 2 fail, so the core fix stays load-bearing Not-tested: absence of enrichment is not caught here, because undefined is still an allowed value until Yeachan-Heo#4105 lands
…eral The pinned Biome 2.5.2 flags assignment-in-expression inside the new invocation harness (`noAssignInExpressions`, severity error), which fails the package check gate on this PR head. Splitting the increment keeps the harness semantics identical and unblocks `bun --cwd=packages/coding-agent run check`. Lore-id: 4056-terminalize-interrupted-prompt Confidence: high Scope-risk: narrow Reversibility: easy Tested: package check gate (biome + tsc) passes; session-runtime + sdk host/reconciliation suites 173 pass / 0 fail Not-tested: CI shard behavior, which remains blocked on the red dev base (Yeachan-Heo#4124, Yeachan-Heo#4006 follow-ups)
…lacks The terminal is claimed on one path while the failure reason arrives on another, so ordering is not the caller's to control. `noteTransition` returned as soon as `terminalAt` was set, which discarded the only description of why the prompt failed and left consumers with a settled record carrying no reason at all. A late `agent_failed` now enriches the settled record instead of being dropped. It cannot resurrect it: status, terminalAt, retention order and the clientRef index are untouched, and the first reason wins so a late generic frame never overwrites a specific one. `agent_start` and `agent_end` still return early. Lore-id: 4088-late-failure-reason Confidence: high Scope-risk: narrow Reversibility: easy Tested: mutation -- reverting the source turns the suite from 25 pass to 22 pass / 3 fail; adjacent sdk-prompt-terminal-diagnostics and sdk-host-wiring stay at 93 pass Not-tested: whether a consumer that already observed the terminal event re-reads the record, so the enriched reason may not reach a client that stopped polling at terminal
The late agent_failed enrichment only landed in the process-local createPromptReconciliation shadow unit; production prompt/skill transitions and Q26 lookup route through kindReconciliation, which still dropped a late agent_failed after terminal. The SDK-only host reconciliation (createInvocationReconciliation) had the same gap, and the canonical TurnPromptReconciliationTerminalOk DTO lacked error?, so the reason never reached the wire. Apply first-reason-wins enrichment to both production reconcilers without resurrecting terminal state or changing retention/status, add error? to the canonical terminal_ok DTO and surface it from the production lookup, and cover the behavior end-to-end through kindReconciliation including durability. Lore-id: b5e5cc8-ka-sr-dto Constraint: must not resurrect terminal status, terminalAt, retention order, or clientRef index Constraint: enrichment must persist across reconnect/restart reconciliation Rejected: drop late agent_failed after terminal | loses the only failure reason on the wire Rejected: only fix the shadow unit | production path (kindReconciliation) still drops it Confidence: high Scope-risk: moderate Reversibility: trivial Tested: kind-aware terminal arbiter (late reason, first-wins, no-enrich-on-start/end), Q26, reconciliation store/recovery, session-runtime, host-wiring (93), surface-parity, broker lifecycle Supersedes: b5e5cc8
…aring The enrichment was added to createInvocationReconciliation.noteTransition but every test for it lived in the bus-level suites, so reverting the host hunk alone left the whole suite green. A fix whose removal nothing notices is not covered. Now rebased on Yeachan-Heo#4108, undefined is no longer a legal reason on a settled record, so the terminalize-once allow-list collapses to the exact sanitized shape, and a new scenario pins first-reason-wins on the host path specifically. Confidence: high Scope-risk: narrow Reversibility: easy Tested: 13 pass; removing the host enrichment gives 11 pass / 2 fail, and removing only the record.error === undefined guard gives 11 pass / 2 fail Not-tested: real ACP transport delivering the late frame; the harness drives the reconciler directly
fea8aea to
e3da5ec
Compare
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
The submission success callback narrowed agent_end to kind === "skill", so a non-skill invocation that accepted preflight and then resolved never emitted a terminal transition. Its reconciliation record stayed non-terminal forever -- a dead turn in a live session. The error callback already terminalized unconditionally, so the two halves of the same settled branch disagreed. Lore-id: 4108a1de Constraint: must not assert error absence -- Yeachan-Heo#4105 retired that contract Rejected: cherry-pick the original Yeachan-Heo#4108 commits | their test halves conflict semantically with Yeachan-Heo#4105 Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the kind narrowing turns 14 pass / 0 fail into 13 pass / 1 fail Not-tested: whether every downstream consumer reacts to the new agent_end for non-skill kinds
Fixes #4088.
noteTransitionreturned as soon asterminalAtwas set:The terminal is claimed on one path while the failure reason arrives on another, so the ordering is not the caller's to control. When the reason lost that race it was discarded, and consumers were left with a settled record carrying no reason at all — which is what #4068 asked for and what #4077 fixed everywhere except here.
The change
A late
agent_failednow enriches the settled record;agent_startandagent_endstill return early.It cannot resurrect the record:
status,terminalAt, retention order and theclientRefindex are all untouched, soactiveCount()does not move. First reason wins, so a late generic frame never overwrites a specific one. The reason is sanitized through the samesanitizePromptFailurethe non-terminal path already uses — no second shape — andlookupnow surfaces it onterminal_okas well, since that is the case where a failure reason arriving late is the only signal a consumer will ever get.Evidence
Mutation, both directions:
expect()calls)origin/devThe three new assertions fail without the change, so they are load-bearing rather than describing whatever the code already did. Adjacent suites are unaffected:
sdk-prompt-terminal-diagnosticsandsdk-host-wiringstay at 93 pass, 0 fail.Diff is two files — the reconciliation source and its existing test file.
Known and not fixed
A consumer that stops polling once it sees the terminal event will not observe the enriched reason; this makes the reason available on the record, it does not re-publish a terminal that already went out. If clients need a push, that is a separate change to the publication path and I did not make it here.
I also did not widen this to
agent_start/agent_endarriving late. Those carry no information the settled record lacks, and accepting them would be the resurrection this fix specifically avoids.