Repository navigation
fix: make session/delete idempotent and fix one session/load update-ordering straggler - #1199
Open
agourakis82 wants to merge 7 commits into
Open
agourakis82 wants to merge 7 commits into
agourakis82 wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The deletion error filter suppresses messages beyond the intended cases and lacks regression tests.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Makes session deletion idempotent and ensures load notifications precede the response.
Changes:
- Handles missing-session deletion errors.
- Awaits command updates during session loading.
- Updates ordering snapshots.
| File | Description |
|---|---|
src/acp-agent.ts |
Adjusts delete and load behavior. |
src/tests/acp-scenarios/__snapshots__/air/session-load-replay.jsonl |
Updates AIR ordering. |
src/tests/acp-scenarios/origin-main/plain/session-load-replay.jsonl |
Updates plain-client ordering. |
src/tests/acp-scenarios/origin-main/zed/session-load-replay.jsonl |
Updates Zed ordering. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
agourakis82
added a commit
to agourakis82/claude-agent-acp
that referenced
this pull request
Oct 1, 2026
…y path Addresses review feedback on agentclientprotocol#1199: the new idempotency behavior in deleteSession wasn't covered by dedicated tests, and only relied on the existing "propagates errors from the SDK delete call" test for the not-swallowed case. Adds two cases covering the exact error shapes the SDK's deleteSession throws for ACP-DELETE-001 ("Session ... not found ...") and ACP-DELETE-002 ("Invalid sessionId: ..."), confirming both resolve to {} instead of rejecting. The existing "propagates errors" test already exercises a message that does not match the regex ("Session not found on disk"), confirming an unrelated error still throws -- verified this stays true with the new regex.
This was referenced Oct 1, 2026
agourakis82
added a commit
to agourakis82/claude-agent-acp
that referenced
this pull request
Oct 2, 2026
…y path Addresses review feedback on agentclientprotocol#1199: the new idempotency behavior in deleteSession wasn't covered by dedicated tests, and only relied on the existing "propagates errors from the SDK delete call" test for the not-swallowed case. Adds two cases covering the exact error shapes the SDK's deleteSession throws for ACP-DELETE-001 ("Session ... not found ...") and ACP-DELETE-002 ("Invalid sessionId: ..."), confirming both resolve to {} instead of rejecting. The existing "propagates errors" test already exercises a message that does not match the regex ("Session not found on disk"), confirming an unrelated error still throws -- verified this stays true with the new regex.
agourakis82
force-pushed
the
fix/tck-load-delete-ordering
branch
from
October 2, 2026 14:27
e924830 to
1f8ad62
Compare
…rdering straggler
Found via the official ACP Test Compatibility Kit (acp-tck) run against
0.84.0, independent of any existing issue report.
session/delete (ACP-DELETE-001, ACP-DELETE-002): the SDK's `deleteSession`
throws when there is no persisted, non-empty transcript to remove -- a
session that was created but never prompted, or a sessionId that was never
valid or never created at all. The spec treats deleting such an id as a
no-op success (docs/protocol/v1/session-delete.mdx D2), so the adapter now
swallows exactly those two known error shapes ("Invalid sessionId: ...",
"Session ... not found ...") and rethrows anything else. Both requirements
now PASS.
session/load (ACP-LOAD-002): `loadSession` scheduled its post-replay
`sendAvailableCommandsUpdate` in a `setTimeout(0)`, which under event-loop
load can fire after `return result` -- after the JSON-RPC response for
`session/load` has already gone out, which the spec forbids (no replay
update may follow the response). The comment on the removed setTimeout
("so it doesn't interleave with history") was already satisfied by the
preceding `await this.replaySessionHistory(...)`; the timer added nothing
but the race. Awaiting the send directly before `return result` fixes it.
`startMcpAuthentication` stays fire-and-forget, unchanged.
Updated the AIR golden snapshot and the two origin-main comparison
fixtures for `session-load-replay` to match the new (correct) message
order -- same two messages, swapped position, confirmed by diff.
Does not fully resolve ACP-LOAD-002: a second, unrelated straggler source
(the async `generate_session_title` control request that runs on turn-idle,
real LLM latency) can still land a `session_info_update` after a later
request's response. Filing that separately together with ACP-CANCEL-002,
which shares the same shape (a different setTimeout(0)-deferred
`sendAvailableCommandsUpdate`, from `session/new`'s own creation, straggling
past an unrelated `session/cancel` response) -- both need a maintainer call
on the right systemic fix rather than another scattered patch.
Verified against the TCK, not just the unit suite: before this change,
`claude-agent-acp` failed ACP-DELETE-001, ACP-DELETE-002, and ACP-LOAD-002.
After: ACP-DELETE-001 and ACP-DELETE-002 pass; ACP-LOAD-002's
`available_commands_update` straggler is gone, though the requirement
still fails on the separate title-generation straggler noted above.
…y path Addresses review feedback on agentclientprotocol#1199: the new idempotency behavior in deleteSession wasn't covered by dedicated tests, and only relied on the existing "propagates errors from the SDK delete call" test for the not-swallowed case. Adds two cases covering the exact error shapes the SDK's deleteSession throws for ACP-DELETE-001 ("Session ... not found ...") and ACP-DELETE-002 ("Invalid sessionId: ..."), confirming both resolve to {} instead of rejecting. The existing "propagates errors" test already exercises a message that does not match the regex ("Session not found on disk"), confirming an unrelated error still throws -- verified this stays true with the new regex.
The v2 session/resume replays through loadSession, which now waits for the available_commands_update instead of deferring it with setTimeout(0), so the update arrives before the response (ACP-LOAD-002) rather than after it. The v2 resume test only passed before because the deferred update landed outside the window it measures. It now asserts the commands update is the last update before the response and keeps the strict check on the replayed history. The v2 session-load-replay golden moves the same update ahead of the response; its content is unchanged.
agourakis82
force-pushed
the
fix/tck-load-delete-ordering
branch
from
October 2, 2026 18:46
1f8ad62 to
0a91cff
Compare
Contributor
Author
|
CI and "Conventional PRs" on this PR (head Could one of you approve the workflow runs when you have a moment? The same applies to my other open PRs; there is a short consolidated note in #1220 listing all five. I'll fix whatever CI turns up, and I'm happy to rebase or hold this one back if it would collide with the ACP v2 work. Nothing else is changing on this branch. |
Signed-off-by: agourakis82 <agourakis82@users.noreply.github.com>
Signed-off-by: agourakis82 <agourakis82@users.noreply.github.com>
Preserve post-response subagent configuration and MCP authentication while sending legacy commands before the load response. Update replay-end expectations and ensure post-response work does not resend commands. Validation: build, lint and formatting pass; replay/options tests pass (584 tests). Full suite: 3367 passed, 30 skipped, one provider-model test failure reproduced on unmodified main 782e463. Assisted-by: OpenAI Codex
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make session deletion idempotent for missing transcripts and deliver legacy session/load command updates before the response.
Missing-transcript handling matches the SDK's known errors for the requested session ID and propagates unrelated SDK errors. Invalid non-session IDs succeed without calling SDK deletion. Regression tests cover the accepted missing-transcript forms and similar messages that must still reject.
The branch incorporates main at 782e463 without rewriting published history. SDK fallback handling remains in SessionIndexConnection, preserving the session-index and AIR archive paths. ACP v2 keeps commands in its setup response. Legacy clients receive their command update after history replay and before the load response; upstream's current subagent configuration/state publication and MCP authentication remain scheduled after the response. The replay tests also check that this later work does not resend commands.
Validation at e7519ba:
AI assistance: OpenAI Codex assisted with the merge integration, regression checks and validation. A local model review was consulted and its suggestions were checked against source and executable results.