Repository navigation
fix(acp): preserve async-result continuation agent_end correlation - #6356
Conversation
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
Reviewed PR #6356 at 88c4d1561f5c3d6ccbfc57eea75ce914c532f71e against base/merge-base 0f2560745bc3d44b60c4698dfcd0cc0560b620a9. The change propagates the existing SDK run token through async continuation paths so accepted attempts retain the command/turn correlation consumed by the SDK host and ACP. The five review scopes found no actionable defects.
Findings / Required Changes
No blocking or actionable findings.
Non-blocking Observations
- The diff adds no dedicated regression assertion for an async-result continuation producing an ACP
agent_endwith the expected commandId/turnId; the changed test file only removes whitespace. A focused end-to-end assertion would directly cover this scenario. This is an optional coverage improvement, not a merge blocker.
CI / Verification
The exact-head Dev CI run 37277112952 completed successfully, including agent-session-concurrent.test.ts, the coding-agent package check, the affected-path aggregate, state-gate shards, and virtual integration validation. A second Dev CI run for the same SHA (37277574967) was skipped; its trigger/reason was not established. No local tests or gates were run during this read-only review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED | No concrete mismatch with the stated fix or existing SDK/ACP correlation contract. |
| A2 — Architecture / Correctness / Failure | APPROVED | Existing token ownership and attempt-scope binding are reused; no reachable regression was established. |
| A3 — Security / Privacy / Trust | APPROVED | No new trust boundary, authorization decision, or attacker-controlled token source was introduced. |
| A4 — Verification / Tests / CI | APPROVED | Exact-head required affected tests and aggregate CI passed; missing dedicated end-to-end coverage is optional. |
| A5 — Context / Compatibility / Platform | APPROVED | Consumers, cleanup, and existing ownership abstractions remain consistent; no compatibility or platform defect found. |
Limitations
This was a static review of the exact head and its base; no dynamic ACP reproduction was run. No separate intent_projection artifact was supplied or found, so intent was assessed from the pinned implementation, changelog, and contracts. The reason for the skipped same-SHA CI rerun was not verified.
|
Merged into dev as
— |
Why
On a dev build that already contains #6333 (3239206), dbe46ae and 8aa5049 (tank, gjc built from f4b3d1a), ACP still logs
acp_prompt_terminal_dropped terminalType=agent_end reason=incomplete_correlation, thenacp_prompt_watchdog_expired340s later (G4 gate FAIL, 15 drops in ~4.5h).Session evidence (jsonl, UTC):
async-result-> 05:25:17.058 assistant stop -> 05:25:17.168 custom_messageirc:incoming-> 05:25:17.189 dropped agent_end (incomplete_correlation)async-result-> 06:18:13.803 assistant stop -> 06:18:13.826 droppedThe remaining path: the prompt's turn ends while an async job or subagent is still running. When its result arrives, the yield-queue
async-resultdelivery starts a continuation turn throughinjectIdle/injectStreaming/ the trigger-turn follow-up. That turn was accepted without the active SDK run token, so its terminalagent_endcarried no commandId/turnId and ACP dropped it.Note: the binary's lack of
activePromptOwnerHolder/adoptLifecycleBatchsymbols is expected. The build uses--minify(scripts/compile-args.ts), and the log siteacp_prompt_terminal_droppedis present, so dist == source.What
agent-session.ts: when async-result custom messages become continuations, capture#activeSdkRunTokenand pass it through:#queueFollowUpAfterReservation(..., { sdkRunToken }))#acceptSdkAttemptRun(handle, sdkRunToken)in both prompt branches)fix-acp-async-result-terminal-correlation.mdagent-session-concurrent.test.tsThis PR does not touch the provider-error retry path (
#handleRetryableError), which open PR #6354 fixes, or the todo-reminder continuation pinned by #6344.Risk
low-riskregression-riskhigh-riskTouches continuation run-token ownership in AgentSession, so a wrong token could mis-attribute a terminal to a prompt.
Local tests (command + result)
Run on tank (worktree g4-asyncres-corr):
bun test packages/coding-agent/test/session-runtime.test.ts-> 180 pass, 0 failbun test packages/coding-agent/test/agent-session-retry-busy-recovery.test.ts-> 7 pass, 0 failbun test packages/coding-agent/test/agent-session-concurrent.test.ts-> 35 pass, 0 failbun run lint-> rc 0Local CI gate (prepush)
PREPUSH_CMD rc=0 bun run --workspaces --if-present check:types
PREPUSH_CMD rc=0 bun scripts/telegram-daemon-generation-guard.ts "$(git merge-base origin/dev HEAD)" "$(git rev-parse HEAD)"
Needs e2e
gjc-acp-guard.py): the count ofacp_prompt_terminal_dropped terminalType=agent_end reason=incomplete_correlationafter an async-result continuation should be 0, andacp_prompt_watchdog_expiredshould not rise.Acceptance
injectIdle/ streaming injector / trigger-turn follow-up passsdkRunToken--minify; log site present in binaryOpen questions
agent_endwas added (agent assumption: existing lifecycle tests cover it). A reviewer may want one.Agent
gjc via paseo on tank (worktree g4-asyncres-corr), commits 908a49f, 88c4d15. No fallback.