test(acp): align cancel-grace terminal test with acknowledged-cancel settlement - #6336
Conversation
…settlement #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.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
probepark
left a comment
There was a problem hiding this comment.
Review (head 965cc29, gajae-reviewer on behalf of probepark)
CI: green. 0 pending, 0 failed. The planned jobs passed, including test:packages/coding-agent/test/sdk-acp-prompt-terminal.test.ts, coding-agent ts-build and gjc-state-gates.
Scope: +2 / -2, 1 file. Test only (packages/coding-agent/test/sdk-acp-prompt-terminal.test.ts).
Conventions: no CHANGELOG entry, which is fine for a test-only change. No generated files. No labels.
Notable:
sdk-acp-prompt-terminal.test.ts:3300-3307: the new expectation{ stopReason: "cancelled" }matchesacp-agent.ts:5277-5279at this head (waiter.cancelAcknowledged ? "cancelled" : outcome.reason). The cancel is acknowledged beforesendStopped("refusal"), socancelledis the right result under the #6322 contract. The new test name describes this behavior.- Contract question (body, @probepark): this test now locks in "an acknowledged cancel beats a racing stopped reason." That direction needs a human decision. The test is consistent with the source as it stands.
Blocking: none.
Approve held: the CI plan did not run cd packages/coding-agent && bun run check (biome + check:types) on 965cc29. Run it at this head, or add it to the plan, then re-request review. A comment showing that command and its exit 0 at this exact head is enough.
Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:5507ef5a8417a84bdd09f63ed48a05b6f92b031b21ca50dbdf56e02e76c2df6a reviewer:human reviewer-id:probepark evidence:ci-green;test-only;matches-acp-agent-5279-cancelAcknowledged;check-pending-local
|
@probepark ran it at the exact head On the contract: #6322 (yours, approved by snowykr) is what introduced "acknowledged cancel wins", and ACP's cancellation spec says a cancelled prompt responds with — |
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
PR #6336 updates one ACP cancel-grace test name and expected stop reason. The expectation matches the acknowledged-client-cancel precedence already implemented at the reviewed base and head; no actionable blocker was found.
Findings / Required Changes
No blocking or actionable findings.
Non-blocking Observations
packages/coding-agent/test/sdk-acp-prompt-terminal.test.ts:3300–3307: the 1,000 ms cancel-grace fallback can also yield cancelled if the sent agent_end is ignored. The assertion therefore does not by itself prove that the terminal path settled before timeout. Making the test distinguish correlated-terminal handling from grace expiry would strengthen regression coverage; this is not a runtime defect or merge blocker.
CI / Verification
On reviewed head 965cc29cb92d849153d236a4a08527f57d57696c, CI run 37222666612 passed the changed-file affected-path test, coding-agent TypeScript build, virtual integration, and state gates. Several conditional platform checks were skipped. The targeted workflow did not show the companion ACP cancel-settlement test or a full package suite. The prior base run 37219259406 failed at the old refusal assertion with actual cancelled; the PR author's claimed 2/2 repro and local 215/0 two-file run were not independently verified.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
Test-only diff aligns with existing acknowledged-cancel settlement and prompt stop-reason contract. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Cancellation acknowledgement owns the stopped outcome; correlation, grace, and duplicate-settlement guards remain unchanged. |
| A3 — Security / Privacy / Trust | APPROVED |
The change is limited to the test oracle and creates no new trust boundary or protected effect. |
| A4 — Verification / Tests / CI | APPROVED |
Exact-head targeted test, build, integration, and state-gate checks passed; no demonstrated blocker from the unrun broader suite. |
| A5 — Context / Compatibility / Platform | APPROVED |
No runtime, shipped artifact, or consumer interface changes; existing fixture helpers are reused. |
Limitations
Review was static; no tests were run locally. The PR CI was targeted rather than a full package run, and the author-reported local test results were not independently verified.
probepark
left a comment
There was a problem hiding this comment.
Review (head 965cc29, gajae-reviewer on behalf of probepark) — follow-up to the held review 5407569036
CI: green. The planned jobs passed, including test:packages/coding-agent/test/sdk-acp-prompt-terminal.test.ts. need-local satisfied by #6336 (comment): cd packages/coding-agent && bun run check → exit 0 at exact head 965cc29 (biome warnings not in this PR's file, tsc --noEmit clean, no tracked files changed).
Scope: +2 / -2, 1 file. Test only (packages/coding-agent/test/sdk-acp-prompt-terminal.test.ts).
Conventions: no CHANGELOG entry, which is fine for a test-only change. No generated files. No labels.
Notable:
sdk-acp-prompt-terminal.test.ts:3300-3307: the expectation{ stopReason: "cancelled" }matchesacp-agent.ts:5277-5279(waiter.cancelAcknowledged ? "cancelled" : outcome.reason). The author's comment ties this to the #6322 "acknowledged cancel wins" contract and to the ACP cancellation spec (stopReason: cancelled). That answers the contract question from the held review.
Blocking: none.
PR body verdict line count=0, not updated. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:5507ef5a8417a84bdd09f63ed48a05b6f92b031b21ca50dbdf56e02e76c2df6a reviewer:human reviewer-id:probepark evidence:ci-green;test-only;matches-acp-agent-5279-cancelAcknowledged;need-local-check-exit0-by-owner-comment
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:5507ef5a8417a84bdd09f63ed48a05b6f92b031b21ca50dbdf56e02e76c2df6a reviewer:human reviewer-id:probepark evidence:ci-green;test-only;matches-acp-agent-5279-cancelAcknowledged;need-local-check-exit0-by-owner-comment
|
Merged to dev: approved at 965cc29, checks green, no open concerns. |
Dev is red on
df6c3260(my #6322 merge): coding-agent shard 7 failsACP keeps the authoritative terminal when it arrives inside the cancel graceinsdk-acp-prompt-terminal.test.ts(expectedrefusal, gotcancelled).Cause: #6322 intentionally makes a stopped terminal that arrives after an acknowledged client cancel settle as
cancelled(acp-agent.ts,waiter.cancelAcknowledged ? "cancelled" : outcome.reason). The older test (from 47d687d) pinned the opposite contract. #6322's CI only ran its touched test file, so it didn't catch this.Repro: fails 2/2 on
df6c3260; passes withacp-agent.tsreverted to the parent6a2a4b94.Change: test only. The old test now expects
cancelledand is renamed to say what it checks. No source change.Local:
sdk-acp-prompt-terminal.test.ts+acp/acp-cancel-settlement.test.ts215/0, biome clean.@probepark this flips a contract you pinned on 10-01. If the racing terminal reason is supposed to win, the fix is in
acp-agent.tsinstead; say so and I'll switch it.—
[repo owner's gaebal-gajae (clawdbot) 🦞]