Repository navigation
test(acp): cover delayed cancel follow-up settlement - #6322
Conversation
Remove the injected cancelled terminal so the delayed background cancel regression covers the adapter's own settlement path.
A terminal stopped frame racing an acknowledged client cancellation must settle the owned prompt as cancelled, regardless of the stale terminal reason.
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
This PR adds an ACP cancellation/follow-up regression test without changing production code. The exact-head CI evidence is successful, and the review found no verified merge-blocking defect.
Findings / Required Changes
No blocking or actionable findings.
Non-blocking Observations
- The added test injects an
agent_endalready markedcancelled(packages/coding-agent/test/acp/acp-cancel-settlement.test.ts:3307). It verifies cancelled settlement and a successful follow-up, but does not exercise the liveend_turn-to-cancellednormalization path after cancellation. Passingend_turnthere would strengthen coverage of that path; this is an optional test improvement, not evidence of a product defect.
CI / Verification
- Dev CI run
37197714951completed successfully on reviewed head96a313077f7635815840647a46157ad416a1648e. - The changed-file test shard and finalized affected-evidence/aggregate validation succeeded. The exact-head check runs had 15 successes, 8 skips, and no failures; the risk-selected virtual-integration lane was skipped.
- Historical Main CI failures referenced in the PR body were on other SHAs and do not establish a failure on this head. No local tests were run as part of this review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
Additive test-only change; no production API or persisted contract change. The asserted outcomes match the stated cancel/follow-up behavior. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Reviewed cancel acknowledgement, terminal settlement, correlation, and follow-up paths; no changed production lifecycle or failure path. |
| A3 — Security / Privacy / Trust | APPROVED |
No new production trust boundary, authority, input handling, or data exposure. |
| A4 — Verification / Tests / CI | APPROVED |
Changed-file test shard and aggregate evidence checks passed at the exact head; optional normalization-path coverage is noted above. |
| A5 — Context / Compatibility / Platform | APPROVED |
No production interface, configuration, generated surface, or platform implementation changed; existing fixture abstractions are reused. |
Limitations
The review did not execute tests locally. CI conclusions are based on exact-head GitHub Actions job results; test stdout was not independently inspected. Historical Main CI artifact reports were unavailable for root-cause attribution.
15809d2 to
83950d1
Compare
Verification (coder, t_ec264470)Head is now Measured on linux-x64 (64 cores), run sequentially:
The root cause is not identified, so this PR should not merge as a fix yet (draft is blocked by the draft-guard; PM decides). The Refs #6315 |
|
@snowykr your review findings are addressed (see the reply above). Current head: |
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
This PR changes ACP prompt settlement so a stopped terminal received after an acknowledged cancellation reports cancelled, and adds delayed-cancel/follow-up coverage. The public ACP response shape is unchanged. No verified merge-blocking defect was found.
Findings / Required Changes
No blocking or actionable findings.
Non-blocking Observations
- Cancellation-versus-terminal ordering remains worth clarifying (
packages/coding-agent/src/modes/acp/acp-agent.ts:3025-3028, 4869-4888, 5279). If a correlatedrefusalormax_tokensterminal arrives after cancel acknowledgement, settlement replaces it withcancelled, despite the terminal-ingress policy preserving those reasons. If it arrives before acknowledgement, settlement can return the host reason immediately and a later acknowledgement cannot revise the response. This is a narrow timing case outside the demonstrated conformance scenario and is non-blocking; confirm the intended precedence and preserve it consistently. - The new test (
packages/coding-agent/test/acp/acp-cancel-settlement.test.ts:3300-3312) sends no terminal for the cancelled prompt, so the 25 ms grace path settles it; it does not distinguish the changed terminal-settlement behavior from the base implementation. A correlated staleend_turnterminal after cancellation acknowledgement would make this a regression test for the production branch. This coverage improvement is non-blocking.
CI / Verification
GitHub Dev CI run 37201682730 completed successfully for reviewed head 83950d189eb0be3c8ef8b10fb244df139035c74c. The affected ACP test shard, coding-agent package check, and coding-agent TypeScript build succeeded. The check-run set had 17 successes and 8 skipped checks, including optional/path-irrelevant jobs. No tests were executed as part of this read-only review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
The stated cancellation outcome and unchanged ACP response shape align with the diff; terminal-reason precedence caveat is noted above. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Reviewed cancellation acknowledgement, terminal correlation, settlement, and follow-up lifecycle; no merge-blocking failure established. |
| A3 — Security / Privacy / Trust | APPROVED |
No new authentication, authorization, secret, or trust-boundary behavior identified. |
| A4 — Verification / Tests / CI | APPROVED |
Exact-head affected test/package checks passed; competing-terminal coverage gap is non-blocking. |
| A5 — Context / Compatibility / Platform | APPROVED |
Change stays within the existing ACP response contract and per-waiter cancellation state; no materially preferable shared abstraction was found. |
Limitations
This was a static, read-only review; CI evidence is reported from GitHub and was not independently reproduced. Historical run/artifact endpoints were unavailable to one reviewer and were not used to assess the exact-head CI result.
|
Merged into dev. snowykr approved the exact head — |
|
Dev CI regression from this merge: run https://github.com/Yeachan-Heo/gajae-code/actions/runs/37219259406 (df6c3260), coding-agent shard 7 fails Cause: The fix is #6336 (test aligned with the acknowledged-cancel contract). No other fix is planned from this side. |
…settlement Yeachan-Heo#6322 made a stopped terminal that races in after an acknowledged client cancel settle the prompt as cancelled (ACP: a cancelled prompt responds with the cancelled stop reason). The older test still pinned the racing terminal's own reason (refusal), so dev shard 7 fails deterministically on df6c326.
Agent
codex/gpt-6.1-sol via paseo on studio (worktree gjc-6315b-acp-cancel-r3)
Task
gjc #6315b: ACP cancel -> follow-up must end background prompt with stopReason "cancelled"
Repo: Yeachan-Heo/gajae-code, base dev. Refs #6315 (#6315, triage comment 5978281976).
Goal
In the acpx conformance case
acp.v1.session.cancel.followup_prompt(prompt_background "sleep 5000" -> wait 100ms -> session/cancel -> await), the background prompt must ALWAYS end withstopReason: "cancelled", and the follow-up prompt must end withend_turn. main CI failed 3 of 4 runs (runs 37183597362, 37085451518, 36852829158; 36996268047 passed) — suspected timing race (unconfirmed).Where to change
packages/coding-agent/src/modes/acp/acp-agent.ts:cancel()(~line 2966), admission/settlement branches ~2374–2393, ~2571–2627, ~2946–2955, and therecord.cancelRequested = falsereset points (~2470, 3041, 3085, 3107, 3137). FIRST capture the actual stopReason value on failure (log/repro loop) and identify the exact cause line before fixing.packages/coding-agent/test/a case "cancel 100ms after background prompt start -> cancelled, then follow-up end_turn".Forbidden
scripts/run-acp-conformance.ts(ACPX_VERSION / GIT_HEAD / case list) or exclude cases.scripts/acp-conformance-agent.tsto make it pass (no fake fixes).Local 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)"
Agent log (tail)
Shell] rg -n "background|prompt_background|followup_prompt|session/cancel" packages/coding-agent/test | head -60; sed -n '430,530p' packages/coding-agent/test/acp/acp-cancel-settlement.test.ts
[Shell] sed -n '3140,3225p' packages/coding-agent/src/modes/acp/acp-agent.ts; sed -n '2550,2640p' packages/coding-agent/src/modes/acp/acp-agent.ts; sed -n '3970,4030p' packages/coding-agent/src/modes/acp/a...
[Shell] rg -n "invocationKind|backgroundCorrelations|backgroundBusy|prompt_background" packages/coding-agent/src/modes/acp/acp-agent.ts | head -80; sed -n '2380,2490p' packages/coding-agent/src/modes/acp...
[Shell] sed -n '5110,5185p' packages/coding-agent/src/modes/acp/acp-agent.ts; sed -n '5260,5295p' packages/coding-agent/src/modes/acp/acp-agent.ts
[Shell] rg -n "prompt_background|background prompt|abortScope|invocationKind" packages/coding-agent/scripts packages/coding-agent/src/modes/acp | head -80
[Shell] bun test packages/coding-agent/test/acp/acp-cancel-settlement.test.ts
The first test run is blocked by the fresh worktree having no dependencies, so I’m running the repository’s worktree-safe setup before measuring behavior. No source changes have been made yet.
[Shell] bun run setup:worktree
[Shell] bun test packages/coding-agent/test/acp/acp-cancel-settlement.test.ts
[Shell] ls packages/natives/native; bun --cwd=packages/natives run build
[Shell] ps aux | rg 'cargo|build-native|bun run setup' | head
[Shell] for i in {1..30}; do test -f packages/natives/native/pi_natives.darwin-arm64.node && echo ready && break; sleep 2; done
[Shell] 'ps -o etime,pcpu,command -p $(pgrep -f '"'cargo build|rustc --crate-name pi_natives' | head -1)"
[Shell] cat >> packages/coding-agent/test/acp/acp-cancel-settlement.test.ts <<'EOF' test("cancel 100ms after background prompt start settles cancelled and follow-up end_turn", async () => { const fixture...
[Shell] test -f packages/natives/native/pi_natives.darwin-arm64.node && echo ready || echo building; git status --short
[Shell] 'ps -o etime,pcpu,command -p $(pgrep -f '"'cargo build --target aarch64-apple-darwin' | head -1)"
[Shell] pgrep -af rustc | head -3
The regression test is committed as the first checkpoint. Native compilation is still running in this fresh checkout; once it finishes I’ll run the test repeatedly to determine whether the current code returns
cancelledor leaksend_turnbefore changing production logic.[System Error] Selected model is at capacity. Please try a different model.