Skip to content

Handle late ACP session updates - #2262

Merged
mikasenghaas merged 1 commit into
mainfrom
codex/fix-acp-notification-race
Aug 6, 2026
Merged

Handle late ACP session updates#2262
mikasenghaas merged 1 commit into
mainfrom
codex/fix-acp-notification-race

Conversation

@hallerite

@hallerite hallerite commented Aug 5, 2026

Copy link
Copy Markdown
Member

Summary

  • wait briefly for a late ACP session update when session/prompt returns without visible output
  • notify the prompt waiter when message or tool-call output changes
  • preserve the existing empty-tool-turn handling behind allow_empty_tool_reply

Root cause

agent-client-protocol 0.11 dispatches notifications through background tasks but resolves request responses directly in the receive loop. If an agent sends its final session/update immediately before its session/prompt response, the prompt coroutine can resume before the update handler records the visible reply.

This adds a narrowly scoped one-second compatibility grace period only when the prompt would otherwise fail for having no visible output. It can be removed after the upstream SDK provides ordered notification handling or a completion barrier.

Impact

ACP agents such as RLM no longer fail spuriously with ACP agent produced no visible reply when their final update races the prompt response.

Validation

  • uv run pytest tests/v1/ -m 'not e2e' — 69 passed, 70 deselected
  • full non-e2e suite — 913 passed, 70 deselected
  • uv run pre-commit run --all-files — passed
  • synthetic ACP 0.11 notification/response race reproduction — passed

Live Docker/Prime ACP end-to-end tests were unavailable locally because Docker and PRIME_API_KEY were not configured.


Note

Low Risk
Narrow compatibility wait in the ACP runner with a 1s cap; no auth or data-path changes, and existing empty-tool-turn handling is preserved.

Overview
Fixes spurious ACP agent produced no visible reply failures when session/prompt completes before the final session/update is applied (ACP 0.11 dispatches notifications in background tasks).

VerifiersACPClient now signals an asyncio.Condition on message and tool-call updates. After connection.prompt returns, if there is still no visible text, the runner waits up to 1 second for a late text chunk (tool-only updates do not count as a reply). allow_empty_tool_reply behavior is unchanged.

The ACP resume e2e test no longer requires trace.tools; it documents that native MCP tools may not appear on intercepted model requests and that the ACP transcript is authoritative.

Reviewed by Cursor Bugbot for commit b31c0e1. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Handle late ACP session updates by waiting up to 1 second for visible text

  • Adds a LATE_UPDATE_GRACE_SECONDS (1.0s) bounded wait in the prompt function in runner.py after receiving the initial response, allowing late-arriving visible text dispatched in background tasks to be collected before deciding the agent produced no reply.
  • Introduces an asyncio.Condition (output_changed) on VerifiersACPClient so session_update can notify waiters when new visible text or tool calls arrive.
  • Relaxes the test_acp_resume_with_tool assertion in test_e2e.py to no longer require trace.tools to be non-empty, relying on the ACP transcript as the source of truth instead.
  • Behavioral Change: prompt now waits up to 1 second longer per call when no visible reply has been received yet, reducing premature RuntimeError for valid late replies.
📊 Macroscope summarized b31c0e1. 1 file reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted

🗂️ Filtered Issues

No issues evaluated.

@hallerite
hallerite marked this pull request as ready for review August 5, 2026 22:48
mikasenghaas
mikasenghaas previously approved these changes Aug 5, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 93d26d7. Configure here.

Comment thread verifiers/v1/acp/runner.py
Comment thread verifiers/v1/acp/runner.py
Comment thread verifiers/v1/acp/runner.py
Comment thread verifiers/v1/acp/runner.py
@macroscopeapp

macroscopeapp Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

2 blocking correctness issues found. This PR introduces new async synchronization logic and timing behavior. Unresolved review comments identify a high-severity potential race condition that could cause cross-turn data leakage, plus an unnecessary 1-second delay for tool-only turns. These substantive issues warrant human review.

You can customize Macroscope's approvability policy. Learn more.

@mikasenghaas
mikasenghaas merged commit f39c6ad into main Aug 6, 2026
15 of 17 checks passed
@mikasenghaas
mikasenghaas deleted the codex/fix-acp-notification-race branch August 6, 2026 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants