Repository navigation
fix(agent-server): reject a create whose agent tool module cannot be imported - #5958
Conversation
…imported A conversation whose agent names a tool from a tool_module_qualnames module the server cannot import was created with only a warning, then every message failed with an unhandled 500 (KeyError from resolve_tool). The create now answers 422 naming the tool and the module. A module whose tool the agent does not use stays a warning. The F33 map bullet drops its known-bug marker. Fixes #5940 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
Coverage Report •
|
|||||||||||||||||||||||||
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
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).
Review — PR #5958
Scope check. The change belongs in this repository: it touches the agent-server create path and the verify-agent-server feature map, both owned here, and it implements the API-contract choice issue #5940 asks for (reject the create with a 4xx). Issue #5940 is open with ready-for-dev, bug, priority:low. No product/architecture escalation is needed.
Verification performed on head 5f29be7b (all CI check runs green, including agent-server-tests, cross-tests, security).
- Focused tests:
tests/agent_server/test_conversation_service.py+tests/agent_server/test_conversation_router.py-> 233 passed. - Live repro against a real agent server started from this head: a create whose agent names
qa_ghost_toolwithtool_module_qualnames={"qa_ghost_tool": "qa_ghost"}(module not on the serversys.path) returns422with adetailnaming the tool and the module (qa_ghost: No module named 'qa_ghost'), and no conversation is persisted. - Control case: the same unimportable module whose tool the agent does not use still returns
201and starts the conversation. uv run pre-commit run --files <changed files>passes.
The fix is correct for its scope. The check runs after the agent is finalized but before any conversation state is written, check_usable=False correctly avoids rejecting a registered-but-unusable tool, and the router maps the new UnavailableAgentTool to 422 alongside the existing client-tool registration error on the same route. Both acceptance criteria for the unimportable-module case hold, and the feature-map update matches the observed behavior.
Non-blocking follow-up (outside this PR's scope; not a blocker). The rejection is keyed on tools present in request.tool_module_qualnames, so the same 500-on-every-message remains reachable through two sibling triggers, both reproduced on this head:
- an agent that names a tool registered nowhere and with no
tool_module_qualnamesentry -> create201, then500KeyError: "ToolDefinition 'qa_nonexistent_tool' is not registered"; and - a
tool_module_qualnamesentry whose module imports but does not register the named tool -> create201, then the same500.
The issue's acceptance criteria are scoped to the unimportable-module case, so this PR fully satisfies them. Widening the check to any agent tool that is_tool_available(..., check_usable=False) cannot resolve would close the remaining path and is worth a separate issue.
✅ APPROVED
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
I'm an AI agent (Claude Code) helping Engel Nyst (@enyst) with project maintenance.
Requested by Engel · project thread
Decision needed
Where the failure surfaces is an API-contract choice. This PR rejects the create with
422(detail names the tool and the module), and creates no conversation. Reason: the agent can never be built on this server, the caller is still at the point where it can fix its deployment or agent, and it matches the existing422for client-tool registration errors on the same route. It is also consistent with #5771, which keeps a fresh conversation strict about unregistered tools and only relaxes resume.Alternatives a maintainer may prefer: (a) start without the tool and log a warning (what the old "fail gracefully" comment suggested; the conversation works but silently lacks a tool the caller asked for), or (b) keep
201and record a conversation error / answer 4xx on each message (the caller learns later and is left with an unusable conversation).Why
A client that registers a custom tool the agent server cannot import (its module is not on the server's
sys.path) and puts it in the agent got201on create, then an unhandled500(KeyError: ToolDefinition '<name>' is not registered) on the first and every later message.Summary
_create_conversationremembers whichtool_module_qualnamesmodules failed to import; once the agent is launched, if the agent uses one of those tools and it is not otherwise resolvable, it raisesUnavailableAgentTool, which the router maps to422. A module whose tool the agent does not use is still only a warning.F33.custom-tool-module-unimportabledrops its known-bug marker and now asserts the422; the F33 gotcha and the F27custom-tool-modulesprose are updated.Issue Number
Fixes #5940
How to Test
uv run pytest tests/agent_server/test_conversation_service.py tests/agent_server/test_conversation_router.py: 233 passed. New:test_agent_tool_from_unimportable_module_rejects_create(service: raises, no conversation persisted; an unused unimportable module still starts) andtest_start_conversation_unavailable_agent_tool_returns_422(router).uv run pytest -n 8 tests/agent_server: 2596 passed, 1 failed (canvas_extensions/test_canvas_extension_backend.py::test_exited_leader_descendant_does_not_block_recovery[status], which fails the same way on main in this environment).AGENT_SERVER_VERIFY_HOME):control-agent-server map run --file F33 --only F33.custom-tool-module-unimportable --fresh --repeat 2 --checkout <main at 6e7a853>:xfailtwice ({"create": 201, "message": 500}).xpasstwice ({"create": 422}).map checkok;map run --file F33 --only F33.custom-tool-module-unimportable --fresh:pass(3/3, includingF33.custom-tool-module);map run --file F27 --only F27.custom-tool-modules --fresh:pass.uv run pre-commit run --files <changed files>: all hooks pass.Video/Screenshots
n/a
Type
Notes
Related: #5771 (a module that disappears before resume; different path, no overlap in code). Tracked in the umbrella #5653.
🤖 Generated with Claude Code
https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
Generated by Claude Code
🐳 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:5f29be7-pythonRun
All tags pushed for this build
About Multi-Architecture Support
5f29be7-python) is a multi-arch manifest supporting both amd64 and arm645f29be7-python-amd64) are also available if needed