fix(acp): replay session history before responding to session/load (#12285 follow-up) (#26957)

Switches `_replay_session_history` from `loop.call_soon`-deferred (after the
`LoadSessionResponse` is written) to `await`-inline (before the response is
constructed) for both `session/load` and `session/resume`. Adds defensive
try/except around the awaited call so a replay helper crash still yields a
successful load response — partial transcripts are acceptable, total
load failure is not.

The deferral was added on May 2 in commit 19854c7cd with the rationale "Zed
only attaches streamed transcript/tool updates once the load/resume response
has completed." That justification was incorrect:

- Zed's current ACP integration (zed-industries/zed
  crates/agent_servers/src/acp.rs) explicitly registers the session-update
  routing entry BEFORE awaiting the loadSession RPC, with the comment:
  "so that any session/update notifications that arrive during the call
  (e.g. history replay during session/load) can find the thread."
- Every other reference ACP server (Codex, Claude Code, OpenCode, Pi, agentao)
  replays history BEFORE responding to the load request.
- The ACP spec wording ("Stream the entire conversation history back to the
  client via notifications") and the natural JSON-RPC reading both mean
  "during the request's lifetime", not "after the response resolves".

Empirical reproduction (reported by Biraj on @agentclientprotocol/sdk
v0.21.1): the same custom ACP client works correctly against Codex /
Claude Code / OpenCode / Pi but receives 0 notifications from Hermes
because it measures the per-call notification count at the moment
`loadSession` resolves — which on Hermes was before the `call_soon`-
scheduled replay coroutine had a chance to run.

Changes:
- `acp_adapter/server.py`: remove `_schedule_history_replay`; both
  `load_session` and `resume_session` now `await self._replay_session_history`
  before returning, wrapped in try/except that logs and continues on
  helper exceptions.
- `tests/acp/test_server.py`: replace the single
  `test_load_session_schedules_history_replay_after_response`
  (which encoded the now-incorrect post-response ordering) with two tests
  asserting `events == ["replay", "returned"]` for load and resume.
  Add two regression tests confirming that a replay helper raising still
  yields a `LoadSessionResponse` / `ResumeSessionResponse` rather than
  propagating the exception out as a JSON-RPC error.

Result: 240 ACP tests pass (was 238), ruff clean. Verified end-to-end:
biraj's synchronous notification-counter pattern now sees 6 notifications
during `loadSession` for a 5-message session, matching all other reference
ACP servers.

The `_fenced_text` change in `acp_adapter/tools.py` from the same May 2
commit is orthogonal and intentionally left intact — it's a separate,
still-valid fix for Zed's pipe-as-table rendering.

Refs #12285. Follows up #26943 (which added thought-chunk replay but kept
the deferral).
This commit is contained in:
kshitij 2026-05-16 07:41:34 -07:00 committed by GitHub
parent f3a4af9cf2
commit 3034eee38e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 121 additions and 30 deletions

View file

@ -673,25 +673,91 @@ class TestSessionOps:
]
@pytest.mark.asyncio
async def test_load_session_schedules_history_replay_after_response(self, agent):
"""Zed only attaches replayed updates after session/load has completed."""
async def test_load_session_replays_history_before_returning_response(self, agent):
"""Per ACP spec, replay must complete BEFORE load_session returns.
Spec-compliant ACP clients (Codex, Claude Code, OpenCode, Pi, Zed)
attach their ``session/update`` listeners before awaiting the
``loadSession`` RPC and rely on receiving the full transcript within
the request's lifetime. Deferring replay via ``loop.call_soon`` (the
prior behavior in May 2026) broke clients that read notification
counts synchronously against the load response see #12285 follow-up.
"""
new_resp = await agent.new_session(cwd="/tmp")
state = agent.session_manager.get_session(new_resp.session_id)
state.history = [{"role": "user", "content": "hello from history"}]
events = []
events: list[str] = []
async def replay_after_response(_state):
async def replay_records(_state):
events.append("replay")
with patch.object(agent, "_replay_session_history", side_effect=replay_after_response):
with patch.object(agent, "_replay_session_history", side_effect=replay_records):
resp = await agent.load_session(cwd="/tmp", session_id=new_resp.session_id)
events.append("returned")
assert isinstance(resp, LoadSessionResponse)
assert events == ["returned"]
await asyncio.sleep(0)
await asyncio.sleep(0)
assert events == ["returned", "replay"]
# Replay must have happened BEFORE the response was constructed —
# i.e. before the `events.append("returned")` after the await resolves.
assert events == ["replay", "returned"]
@pytest.mark.asyncio
async def test_resume_session_replays_history_before_returning_response(self, agent):
"""Same spec rationale as ``load_session`` — replay before responding."""
new_resp = await agent.new_session(cwd="/tmp")
state = agent.session_manager.get_session(new_resp.session_id)
state.history = [{"role": "user", "content": "hello from history"}]
events: list[str] = []
async def replay_records(_state):
events.append("replay")
with patch.object(agent, "_replay_session_history", side_effect=replay_records):
resp = await agent.resume_session(cwd="/tmp", session_id=new_resp.session_id)
events.append("returned")
assert isinstance(resp, ResumeSessionResponse)
assert events == ["replay", "returned"]
@pytest.mark.asyncio
async def test_load_session_survives_replay_helper_exception(self, agent, caplog):
"""A replay helper raising must not turn load_session into an error.
With awaited replay, an exception in ``_replay_session_history`` now
propagates into the ``load_session`` handler. The defensive try/except
guard at the call site must catch and log it so the JSON-RPC client
still receives a ``LoadSessionResponse`` partial transcripts are
acceptable, total load failure is not.
"""
new_resp = await agent.new_session(cwd="/tmp")
state = agent.session_manager.get_session(new_resp.session_id)
state.history = [{"role": "user", "content": "hi"}]
async def boom(_state):
raise RuntimeError("simulated replay helper crash")
with caplog.at_level("WARNING", logger="acp_adapter.server"):
with patch.object(agent, "_replay_session_history", side_effect=boom):
resp = await agent.load_session(cwd="/tmp", session_id=new_resp.session_id)
assert isinstance(resp, LoadSessionResponse)
assert "history replay raised during session/load" in caplog.text
@pytest.mark.asyncio
async def test_resume_session_survives_replay_helper_exception(self, agent, caplog):
"""Same guarantee as ``load_session`` for the resume path."""
new_resp = await agent.new_session(cwd="/tmp")
state = agent.session_manager.get_session(new_resp.session_id)
state.history = [{"role": "user", "content": "hi"}]
async def boom(_state):
raise RuntimeError("simulated replay helper crash")
with caplog.at_level("WARNING", logger="acp_adapter.server"):
with patch.object(agent, "_replay_session_history", side_effect=boom):
resp = await agent.resume_session(cwd="/tmp", session_id=new_resp.session_id)
assert isinstance(resp, ResumeSessionResponse)
assert "history replay raised during session/resume" in caplog.text
@pytest.mark.asyncio
async def test_resume_session_creates_new_if_missing(self, agent):