Skip to content

refactor(acp): extract session_update dispatch into typed per-update handlers#3801

Open
tksrmz wants to merge 2 commits into
OpenHands:mainfrom
tksrmz:refactor-session-update
Open

refactor(acp): extract session_update dispatch into typed per-update handlers#3801
tksrmz wants to merge 2 commits into
OpenHands:mainfrom
tksrmz:refactor-session-update

Conversation

@tksrmz

@tksrmz tksrmz commented Jun 18, 2026

Copy link
Copy Markdown

HUMAN:

Splitting up the giant session-update dispatcher so it's easier to read and extend. Pure cleanup, no behavior change — please sanity-check that I moved every branch faithfully.


AGENT:

Why

The dispatch block in _OpenHandsACPBridge._handle_session_update inlined the logic for every update kind (AgentMessageChunk / AgentThoughtChunk / UsageUpdate / ToolCallStart / ToolCallProgress, plus the fork-session branch) directly inside a single if/elif chain. The method had grown large, hurting readability and testability. Extracting each branch into its own handler method — and annotating the arguments with concrete update types instead of Any — clarifies responsibilities and improves type safety.

This is a pure refactor with no behavioral change.

Summary

  • Extracted the dispatch branches of _handle_session_update into dedicated methods: _handle_fork_update / _handle_message_chunk / _handle_thought_chunk / _handle_usage_update / _handle_tool_call_start / _handle_tool_call_progress (masking logic and the terminal-event emission conditions are unchanged).
  • Annotated each extracted handler's update argument with its concrete type (AgentMessageChunk / AgentThoughtChunk / UsageUpdate / ToolCallStart / ToolCallProgress) instead of Any.

Issue Number

No related issue.

How to Test

The change touches a single file: openhands-sdk/openhands/sdk/agent/acp_agent.py.

  1. Run the related unit tests:

    uv run python -m pytest tests/sdk/agent/test_acp_agent.py tests/sdk/agent/test_acp_dedup_and_truncation.py -q
    

    397 passed. These cover every branch touched by this change: ACP message accumulation, thought/usage handling, tool-call start/progress, secret masking, and terminal-event dedup/truncation.

  2. Type-check (to validate the concrete type annotations):

    uv run pyright openhands-sdk/openhands/sdk/agent/acp_agent.py
    

    0 errors, 0 warnings, 0 informations.

Video/Screenshots

No GUI change is involved (internal refactor), so screenshots are not applicable.
The execution logs of the commands above serve as evidence instead:

$ uv run python -m pytest tests/sdk/agent/test_acp_agent.py tests/sdk/agent/test_acp_dedup_and_truncation.py -q
collected 397 items
tests/sdk/agent/test_acp_agent.py ...................................... [  9%]
(...)
tests/sdk/agent/test_acp_dedup_and_truncation.py .............           [100%]
======================= 397 passed, 9 warnings in 11.99s =======================

$ uv run pyright openhands-sdk/openhands/sdk/agent/acp_agent.py
0 errors, 0 warnings, 0 informations

Type

  • Bug fix
  • Feature
  • Refactor
  • Breaking change
  • Docs / chore

Notes

  • Pure refactor: no change to externally observable behavior, public API, or serialization format.
  • Existing comments (the masking rationale, the explanation of emitting exactly one terminal event to avoid O(n^2) storage/relay, etc.) are moved verbatim into the extracted methods — no information is lost.
  • Two commits: (1) extract the handlers → (2) annotate the extracted handlers with concrete types.

@tksrmz tksrmz marked this pull request as ready for review June 18, 2026 22:00
@all-hands-bot all-hands-bot requested a review from neubig June 20, 2026 13:05
@all-hands-bot

Copy link
Copy Markdown
Collaborator

[Automatic Post]: I have assigned @neubig as a reviewer based on git blame information. Thanks in advance for the help!

This comment was created by an AI agent (OpenHands) on behalf of the user.

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