chore: remove verified dead code - #4225
Conversation
Remove six symbols confirmed unreferenced across the whole tree (git grep, absent from every __all__, no star-imports): - _AgentProtocol (openhands-sdk/.../agent/response_dispatch.py) — only mentioned in a docstring, which now points at the mixin's TYPE_CHECKING block instead. - ConversationResponse, GenerateTitleRequest, GenerateTitleResponse (openhands-agent-server/.../models.py) — not wired to any route and absent from the generated OpenAPI schema, so the HTTP API is unchanged; the now unused 'from openhands.sdk import LLM' import is dropped. - get_category() (openhands-sdk/.../critic/impl/api/taxonomy.py) - predict_labels() (openhands-sdk/.../critic/impl/api/client.py) Refs #4224
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
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.
✅ QA Report: PASS
Verified the dead-code cleanup by running the affected SDK and agent-server paths before and after the PR; behavior stayed unchanged while the targeted symbols disappeared.
Does this PR achieve its stated goal?
Yes. The PR set out to remove verified dead code without changing runtime behavior. I confirmed the five targeted symbols were present on origin/main and absent on the PR branch, while a real SDK conversation, response classification, critic categorization, a live agent-server /server_info request, and the generated OpenAPI schema all behaved the same before and after.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully; no tests/linters/typecheckers were run locally. |
| CI Status | 🟡 GitHub shows 23 successful, 0 failing, 7 pending, 2 skipped checks at review time. |
| Functional Verification | ✅ Real SDK and agent-server behavior matched base; OpenAPI schemas were identical. |
Functional Verification
Test 1: Targeted symbols are removed while SDK/critic behavior stays stable
Step 1 — Establish baseline on origin/main:
Ran a Python script that imported the response dispatcher, critic taxonomy/client, and agent-server models, then exercised response classification and critic categorization with concrete inputs.
Output:
{'response_dispatch': ['content', 'empty'], 'critic_categories': {'sentiment': None, 'agent_behavioral_issues': [{'name': 'did_not_follow_instruction', 'display_name': 'Did Not Follow Instruction', 'probability': 2.0}], 'user_followup_patterns': [], 'infrastructure_issues': [], 'other': []}, 'removed_symbols_available': {'get_category': True, 'predict_labels': True, 'ConversationResponse': True, 'GenerateTitleRequest': True, 'GenerateTitleResponse': True}}
This shows the old branch still exposed the targeted dead-code symbols, and the nearby runtime paths produced concrete outputs.
Step 2 — Apply the PR's changes:
Checked out vasco/cleanup-dead-code-4224 at 23828702504b3a74b0410c75018ca3e928dfb02f.
Step 3 — Re-run with the PR in place:
Ran the same script.
Output:
{'response_dispatch': ['content', 'empty'], 'critic_categories': {'sentiment': None, 'agent_behavioral_issues': [{'name': 'did_not_follow_instruction', 'display_name': 'Did Not Follow Instruction', 'probability': 2.0}], 'user_followup_patterns': [], 'infrastructure_issues': [], 'other': []}, 'removed_symbols_available': {'get_category': False, 'predict_labels': False, 'ConversationResponse': False, 'GenerateTitleRequest': False, 'GenerateTitleResponse': False}}
This confirms the symbols were removed, while response classification and critic categorization returned the same results.
Test 2: Real SDK conversation still dispatches assistant content
Step 1 — Establish baseline on origin/main:
Ran a real SDK Agent + Conversation with the configured LLM and prompt Respond with exactly QA_OK and no other text.
Output:
{'agent_reply': 'QA_OK', 'message_events': 2}
This exercises the user-facing SDK conversation path and confirms the base branch dispatches a content response normally.
Step 2 — Apply the PR's changes:
Checked out vasco/cleanup-dead-code-4224.
Step 3 — Re-run with the PR in place:
Ran the same SDK conversation.
Output:
{'agent_reply': 'QA_OK', 'message_events': 2}
This confirms the ResponseDispatchMixin cleanup did not break normal assistant-message dispatch.
Test 3: Live agent-server still serves HTTP and OpenAPI is unchanged
Step 1 — Establish baseline on origin/main:
Started the real agent server with uv run python -m openhands.agent_server --host 127.0.0.1 --port 18764, then requested /server_info and /openapi.json with curl.
Output summary:
/server_info -> 200, title "OpenHands Agent Server", version "1.37.1"
openapi_removed_components -> {'schema_count': 438, 'removed_present': {'ConversationResponse': False, 'GenerateTitleRequest': False, 'GenerateTitleResponse': False}}
This shows the base server starts and the soon-to-be-removed models were not exposed as OpenAPI components.
Step 2 — Apply the PR's changes:
Checked out vasco/cleanup-dead-code-4224.
Step 3 — Re-run with the PR in place:
Started the real agent server with uv run python -m openhands.agent_server --host 127.0.0.1 --port 18765, then requested the same endpoints.
Output summary:
/server_info -> 200, title "OpenHands Agent Server", version "1.37.1"
openapi_removed_components -> {'schema_count': 438, 'removed_present': {'ConversationResponse': False, 'GenerateTitleRequest': False, 'GenerateTitleResponse': False}}
OpenAPI comparison -> {'openapi_equal': True, 'base_schemas': 438, 'pr_schemas': 438}
This confirms the agent-server still runs and the REST/OpenAPI surface is unchanged by the model cleanup.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
neubig
left a comment
neubig
left a comment
There was a problem hiding this comment.
Approved — I found no live org-internal consumers of the removed symbols. The only exact downstream artifact was the stale TypeScript Swagger snapshot, which is being removed in OpenHands/typescript-client#292.
3 participants
HUMAN:
Small PR to delete some dead-code
AGENT:
Why
Several symbols in the SDK and agent-server are defined but never used anywhere in the tree. They add maintenance surface (imports to keep valid, type-checking, docs) for no benefit. This removes the dead-code items from #4224 (the duplication, simplification, and
model_dump_succint()items in that issue are intentionally not part of this PR).Each symbol was verified unreferenced before removal: whole-tree
git grepshows only the definition site, none appear in any__all__, and no module doesfrom ... import *on them.Summary
_AgentProtocolfromopenhands-sdk/openhands/sdk/agent/response_dispatch.py(its only reference was a docstring, which now points at the mixin'sTYPE_CHECKINGblock instead) and drop the now-unusedProtocol/runtime_checkableimports.ConversationResponse,GenerateTitleRequest, andGenerateTitleResponsefromopenhands-agent-server/openhands/agent_server/models.py(none are wired to a route) and drop the now-unusedfrom openhands.sdk import LLMimport.get_category()fromopenhands-sdk/openhands/sdk/critic/impl/api/taxonomy.py(a thin wrapper overFEATURE_CATEGORIES.get(), whichcategorize_features()already calls directly).predict_labels()fromopenhands-sdk/openhands/sdk/critic/impl/api/client.py.Net change: −82 lines across 4 files, no behavior change.
Issue Number
Refs #4224 (dead-code items only; other items in the issue are out of scope for this PR).
How to Test
From the repo root:
Results in this branch:
ruff check/ruff format --check: pass, all files formatted.pyright:0 errors, 0 warnings, 0 informations.pytest: 87 passed.api.openapi()reports 404 components, none of them the removed models), so the HTTP API surface is untouched.Video/Screenshots
N/A — internal dead-code removal with no runtime or UI behavior change; evidence is the command output above.
Type
Notes
predict_labels()andget_category()live under the internalcritic/impl/api/package and are not exported via any__all__, so this is not a public-API change._AgentProtocolremoval is safe becauseResponseDispatchMixinalready re-declares the same members under its ownif TYPE_CHECKING:block, which is what pyright actually uses.Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang: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:2382870-pythonRun
All tags pushed for this build
About Multi-Architecture Support
2382870-python) is a multi-arch manifest supporting both amd64 and arm642382870-python-amd64) are also available if needed