Skip to content

fix(sdk): wait boundedly for late ACP read_only mode confirmation - #68

Merged
geeen-jp merged 1 commit into
mainfrom
issue-67
Oct 7, 2026
Merged

geeen-jp merged 1 commit into
mainfrom
issue-67

Conversation

@geeen-jp

@geeen-jp geeen-jp commented Oct 7, 2026

Copy link
Copy Markdown
Owner

AGENT:
Implemented through the multi-agent pipeline (Sonnet design/review/triage, gpt-6-luna implementation, gpt-6.1-sol audit). Evidence from the live ai-dev-workflow-go runtime (Cursor ACP, Grok 4.7 medium non-fast): 3 of ~20 read_only ask setups failed with ACPSessionModeError: ... did not confirm read_only session mode 'ask' (advertised current='agent', observed=None) (2026-10-04 15:46 JST wf#319; 2026-10-07 15:34 and 20:55 JST wf#345), each in the same second as Setting ACP session mode: ask. A direct cursor-agent acp probe received current_mode_update(ask) 4/4 times, so the confirmation arrives but can be processed after the session/set_mode response.

Why

_apply_acp_session_mode read get_current_mode_id exactly once, immediately after session/set_mode returned. When the CurrentModeUpdate notification trails the response, the mode is not yet observed and startup fails even though the server does switch to the requested read-only mode.

Summary

  • New named constant _ACP_SESSION_MODE_CONFIRM_TIMEOUT (5s), read at call time so tests can patch it.
  • _OpenHandsACPBridge keeps a per-session asyncio.Event set on every non-empty CurrentModeUpdate; new wait_for_current_mode(session_id, timeout) waits on it (no sleep polling) and returns the confirmed mode or None.
  • _apply_acp_session_mode: the existing acceptance rule is unchanged and returns without waiting. Only when nothing was observed does it wait (bounded) for the confirmation. Fail-closed: a timeout raises ACPSessionModeError (message includes within <timeout>s); a different confirmed mode, before or during the wait, raises immediately.
  • _verify_read_only_mode_after_config, provider qualification, public API and event shapes are unchanged. No CLI/adapter/provider version conditions.
  • AGENTS.md records the behavior.

Issue Number

Fixes the race described in #67 (the issue is closed by the orchestrator after merge).

How to Test

env -u CLAUDE_CODE_OAUTH_TOKEN -u CLAUDE_AUTH_CODE .venv/bin/python -m pytest -q tests/sdk/agent/test_acp_permission_policy.py tests/sdk/agent/test_acp_agent.py (454 passed). The new tests are test_read_only_waits_for_mode_update_after_set_mode_response, test_read_only_mode_confirmation_timeout_reports_wait, test_read_only_wrong_mode_update_fails_without_waiting, test_read_only_wrong_mode_before_response_fails_without_waiting, and test_read_only_confirmed_before_response_does_not_wait. An autouse fixture shortens the timeout so the existing fail-closed tests stay fast. Wider validation (tests/sdk/settings, tests/sdk/agent/test_acp_agent.py, tests/agent_server, changed tests): 2622 passed. pyright and pre-commit pass on the changed files.

(The env -u is needed only on hosts that export Claude OAuth variables: two existing TestACPEnvConflictSuppression tests read them. This is unrelated to this change.)

Type

  • Bug fix
  • Feature
  • Refactor

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

📁 PR Artifacts Notice

This PR contains a .pr/ directory with temporary PR-specific documents. The directory will be automatically removed when the PR is approved.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

Endpoint audit

⚠️ 18 actionable Agent Server contract divergence(s) · report-only

Contract: pinned release artifact

Category Count
Actionable client-only calls 0
Actionable server-only operations 18
Documented non-divergences 13
Agent Server contract operations 129
Audited handwritten client endpoints 119

Actionable client-only calls (0)

none

Actionable server-only operations (18)

  • DELETE /api/canvas-extensions/installed/{}
  • DELETE /api/llm/provider-connections/{}
  • GET /api/canvas-extensions/installed
  • GET /api/canvas-extensions/installed/{}
  • GET /api/canvas-extensions/installed/{}/bundle
  • GET /api/conversations/{}/events
  • GET /api/file/archive
  • GET /api/git/commits
  • GET /api/git/commits/{}/changes
  • GET /api/init
  • GET /api/llm/provider-connections
  • PATCH /api/canvas-extensions/installed/{}
  • PATCH /api/llm/provider-connections/{}
  • POST /api/canvas-extensions/install
  • POST /api/conversations/{}/load_plugin
  • POST /api/file/create_directory
  • POST /api/init
  • POST /api/llm/provider-connections
Documented non-divergences (13)

Client calls intentionally absent from the filtered contract (11)

  • GET /
  • GET /alive
  • GET /health
  • GET /ready
  • GET /server_info

Reason: Operational Agent Server endpoints intentionally excluded from the filtered public release artifact.
Owner: OpenHands runtime maintainers

  • DELETE /api/meta-profiles/{}
  • GET /api/meta-profiles
  • GET /api/meta-profiles/{}
  • POST /api/meta-profiles/{}
  • POST /api/meta-profiles/{}/activate

Reason: Client-ahead API stacked on the pending Agent Server meta-profiles implementation.
Owner: OpenHands SDK maintainers
Tracking: OpenHands#3744

  • POST /api/profiles/{}/validate

Reason: Client-ahead API stacked on the pending Agent Server pre-flight LLM validation endpoint.
Owner: OpenHands TypeScript client maintainers
Tracking: OpenHands#4422

Server operations covered by an exposed browser URL (2)

  • GET /api/conversations/{}/workspace
  • GET /api/conversations/{}/workspace/{}

Reason: RemoteWorkspace.startWorkspaceSession exposes these authenticated URLs for browser iframe and file requests; they are not HttpClient method calls.
Owner: OpenHands TypeScript client maintainers

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

Python API breakage checks — ✅ PASSED

Result: ✅ PASSED

Action log

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

REST API breakage checks (OpenAPI) — ✅ PASSED

Result: ✅ PASSED

Action log

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

Coverage

Coverage Report •
FileStmtsMissCoverMissing
openhands-sdk/openhands/sdk/agent
   acp_agent.py255334087%264, 334–337, 378, 410, 539, 546, 559, 584, 601, 604, 689, 696, 699–700, 798, 805, 825, 867, 870, 948, 1004, 1085–1086, 1090, 1100–1103, 1109–1112, 1115, 1122, 1134–1136, 1142, 1148, 1155, 1157, 1198, 1286, 1400, 1448, 1568, 1572, 1574, 1576, 1605, 1608, 1610, 1614, 1617, 1626, 1709, 1715, 1738, 1796, 1806–1812, 1838–1840, 1851–1852, 1889, 1892, 1909–1913, 2131, 2135, 2171, 2177, 2185–2186, 2189, 2205–2206, 2222, 2225, 2228–2233, 2238, 2248, 2309–2319, 2328, 2368–2369, 2421, 2423, 2427, 2431, 2447, 2589, 2736, 2884, 2887–2888, 2905–2906, 2922, 2939, 2944, 2968, 2999, 3012, 3026, 3047, 3076, 3092, 3105, 3117, 3166, 3174, 3177, 3181–3184, 3195–3197, 3219, 3227–3228, 3231–3234, 3243–3245, 3251–3253, 3261–3262, 3274–3279, 3282–3284, 3291–3294, 3299–3303, 3315, 3341–3342, 3346–3347, 3355–3358, 3365, 3417, 3422, 3515–3516, 3518, 3539–3540, 3640, 3647–3648, 3739, 3995–3996, 3999, 4003–4005, 4008–4012, 4247, 4348, 4462, 4520, 4526, 4541–4542, 4553–4556, 4582, 4590, 4763–4765, 4777–4779, 4806, 4821–4822, 4825–4827, 4834, 4838, 4849–4850, 4858, 4863, 4875–4876, 4885–4890, 4897, 4902–4905, 4907–4910, 4912–4915, 4937, 4949–4951, 4986–4987, 4990, 4998–5000, 5004, 5006, 5010, 5013, 5022–5024, 5028, 5062–5064, 5066, 5071–5073, 5082–5086, 5089, 5094–5096, 5099–5100, 5115–5117, 5172–5174, 5412–5414, 5453, 5457, 5468, 5478, 5486, 5498, 5523, 5544–5546, 5560, 5571–5574, 5653–5654, 5695–5696, 5711–5712, 5720, 5723–5724, 5729, 5731, 5765–5766, 5782–5783
TOTAL428461180872% 

@geeen-jp
geeen-jp merged commit b6afda1 into main Oct 7, 2026
39 checks passed
@geeen-jp
geeen-jp deleted the issue-67 branch October 7, 2026 23:26
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.

1 participant