Repository navigation
Conversation
After a restart without the module that defines a custom tool, the conversation's stored events name classes that are no longer loaded, and one unknown kind failed the whole load (500 on every message and event search on the agent server). - EventLog.path_to_root (and the incremental view walk) skip an event whose JSON no longer validates, logging it and following its stored parent_id; unparseable JSON still raises. - SystemPromptEvent leaves out stored tools of an unknown kind. - On resume (a system prompt already stored), the agent skips tool specs that are not registered, with a warning; a fresh conversation still raises. - Drop the known-bug marker from F27.custom-tool-missing-on-resume. Fixes #4080 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
I re-fetched the current head (4a364c72), the PR body, the discussion, the linked issues, and the head's check runs. The head still matches, there are no existing reviews, and every check run on this head passes. I ran the new focused test file (tests/sdk/conversation/test_resume_without_custom_tool.py, 5 passed) and the surrounding suites (tests/sdk/conversation, tests/sdk/event, tests/sdk/agent, tests/sdk/tool — all pass).
The core change is sound. EventLog._read_branch_node/path_to_root and the incremental walk in ConversationState.view now skip an event whose JSON parses but whose model no longer validates; SystemPromptEvent._drop_unknown_tools tolerates a vanished tool class; and resume drops an unregistered tool with a warning. Unparseable JSON still raises, and a fresh conversation still raises KeyError, both preserved as intended.
One remaining load path is still strict, so the linked issue's degrade-per-event goal is not fully met for legacy conversations:
ConversationState._resolve_active_leaf still reads the leaf event strictly (openhands-sdk/openhands/sdk/conversation/state.py:291, not a changed line). When a conversation has no persisted leaf_event_id (pre-tree/legacy logs — leaf_event_id was added in #3922, while custom tool modules shipped earlier in #1383), the active leaf is resolved by scanning backwards with self._events[i]. If the last real event is one whose kind is no longer loaded (exactly the custom-tool case this PR targets), that read raises ValidationError and the whole conversation still fails to load — the same symptom as #4080. path_to_root was made tolerant, but this pre-pass runs before it and was not, so the tolerant walk is never reached.
Reproduction on the current head (persist a pre-tree log by appending events directly to EventLog with leaf_event_id=None, then rename the last event's kind to an unknown one and resume with agent=None):
ConversationState.create -> state.rebuild_view() -> _resolve_active_leaf()
-> EventLog.__getitem__ -> Event.model_validate_json
pydantic_core.ValidationError: Value error, Unknown kind 'GoneEvent' for openhands.sdk.event.base.Event
The post-tree case (persisted leaf_event_id) is handled correctly: _resolve_active_leaf returns the stored id without reading, and the tolerant path_to_root skips the bad event. Only the legacy shape still strands the conversation.
This is narrower than the primary scenario, but it is a supported path (the tree feature explicitly keeps legacy history loadable) and it reproduces the linked issue's "entire conversation fails to load" outcome. Suggested scope: resolve the legacy leaf tolerantly as well (e.g. skip an unreadable tail event the way _read_branch_node does, or have the reverse scan use the tolerant read), or state explicitly that pre-tree conversations are out of scope for #4080.
🔄 CHANGES REQUESTED
Co-authored-by: openhands <openhands@all-hands.dev>
|
📁 PR Artifacts Notice This PR contains a |
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
Why
A persisted conversation became unreadable after restart if the module defining one of its custom tools was no longer registered.
Summary
Tolerate unavailable custom-tool models only when recovering persisted history. Both full and incremental history traversal continue past an unknown event, and agent initialization uses the readable prefix to recognize a resumed conversation.
Source: event_store.py · base.py · system.py.
Issue Number
Fixes #4080
How to Test
Previously recorded verification
uv run pytest tests/sdk/conversation/test_resume_without_custom_tool.py: 5 passed (3 fail onmain: the two branch-walk tests and the end-to-end resume; the corrupt-JSON and fresh-conversation tests pin unchanged behaviour). The end-to-end test persists a conversation with a registered custom tool, unregisters it and renames the stored kinds, resumes withagent=Noneas the server does, and sends a message.uv run pytest -n 8 tests/sdk/agent tests/sdk/conversation tests/sdk/event tests/sdk/tool: all pass except 2 ACP timing tests that pass when re-run alone (flaky under parallel load).AGENT_SERVER_VERIFY_HOMEset to a private directory:control-agent-server map run --file F27 --only F27.custom-tool-missing-on-resume --fresh --repeat 2 --checkout <main worktree at dfd8fe8>:xfailtwice.xpasstwice (messagePOST200,events/search200, server log showsSkipping unreadable event,Leaving out tool of unknown kind 'QaF27EchoTool'andTool 'qa_f27_echo' is not registered; resuming conversation ...).control-agent-server map checkok, andmap run --file F27 --only F27.custom-tool-missing-on-resume --fresh:pass.uv run pre-commit run --files <changed files>: all hooks pass.Video/Screenshots
n/a
Type
Notes
Skipped events are logged; this adds no public opaque-event placeholder or UI count. Malformed JSON and non-object records still fail. The alternative of requiring every historical tool to be re-registered remains stricter; broader tool-set compatibility is tracked in #2667, with verification under #5653.
Visual explanation by Codex using HumanLayer show-me.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:02d0a23-pythonRun
All tags pushed for this build
About Multi-Architecture Support
02d0a23-python) is a multi-arch manifest supporting both amd64 and arm6402d0a23-python-amd64) are also available if needed