chore: remove already-removable deprecated leftovers (marketplace comment, skills tombstone, display_dict)#3734
Conversation
…ment, skills tombstone, display_dict)
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: Remove Deprecated Leftovers
🟢 Good taste — Clean removal of deprecated code with no behavioral changes.
Summary
This PR removes three categories of deprecated code that are no longer needed:
plugin/types.py: Removes a dangling comment block with no code beneath it (the re-exported marketplace classes already raisedImportError)context/skills/__init__.py: Removes a docstring-only tombstone module (shims deprecated in 1.16.0, removed in 1.21.0)utils/visualize.py+tool/schema.py: Removesdisplay_dict()(pure alias fordisplay_json()) and repoints the single caller to usedisplay_json()directly
Analysis
- Breaking Change Risk: 🟢 Negligible.
display_dictwas a deprecated pure-forwarder with one in-tree caller. The PR correctly identifies and updates that caller. - Complexity: The changes are straightforward deletions and import updates — exactly what deprecation cleanup should look like.
- Test Coverage: The test file cleanup is appropriate.
display_dicttests were redundant with the existingdisplay_jsontest suite.
Minor Note
The change to tests/sdk/mcp/test_mcp_observation.py is a comment-only update (stale reference to display_dict()). Technically correct, but worth noting it's not a code behavior change.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Rationale: This is a pure cleanup PR removing deprecated code that is no longer used. display_dict was a one-line forwarder to display_json — removing it and updating the single caller has zero functional impact. The other changes are dead comment blocks and unused tombstone modules. No behavioral changes, no new dependencies, no API modifications.
VERDICT:
✅ Worth merging — Well-scoped cleanup that makes the codebase cleaner without any risk.
KEY INSIGHT:
The deprecation CI gate only scans @deprecated decorators and specific function calls — this PR correctly identifies the blind spot and cleans up prose deprecations that the gate misses.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
The deprecated SDK leftovers were removed while the real visualization path and JSON rendering still work with representative user inputs.
Does this PR achieve its stated goal?
Yes. On origin/main, display_dict imported successfully, matched display_json, and openhands.sdk.context.skills imported as the tombstone module; on the PR commit, display_dict raises ImportError and a clean checkout raises ModuleNotFoundError for openhands.sdk.context.skills. The affected user-facing render path (Action.visualize) still returns Rich Text and renders the same Action: Demo / Arguments output using display_json, including nested data.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully and installed project packages into .venv |
| CI Status | 🟡 24 successful, 2 skipped, 7 pending, 0 failing at review time |
| Functional Verification | ✅ SDK imports/removals and real visualization/rendering behavior verified manually |
Functional Verification
Test 1: Deprecated alias/tombstone removal with unchanged Action visualization
Step 1 — Establish baseline on origin/main:
Ran git switch --detach origin/main && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_sdk_visualize.py base where the script imported the old symbols and rendered a real Action subclass via Demo().visualize:
display_dict import: OK
context.skills import: openhands.sdk.context.skills
display_dict equals display_json: True
visualize returns: Text
visualize contains Arguments: True
visualize contains x/y/nested: True
Action: Demo
Arguments:
x: 1
y: "hi"
nested: {values: [1, 2, None]}
kind: "Demo"
This confirms the old state: display_dict was a pure alias for display_json, the tombstone module was importable, and the Action visualization path rendered arguments correctly.
Step 2 — Apply the PR changes:
Ran git switch vasco/chores && git reset --hard 6a59af0f9b224eeb44e2ec57b2a48bf0ff1822ec, then git clean -fdx openhands-sdk/openhands/sdk/context/skills to remove ignored cache artifacts left by switching from the base branch and simulate a clean checkout.
Step 3 — Re-run with the PR in place:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_sdk_visualize.py pr plus the same Action rendering script:
display_dict import: ImportError as expected
display_dict error: cannot import name display_dict from openhands.sdk.utils.visualize (.../openhands-sdk/openhands/sdk/utils/visualize.py)
context.skills import: ModuleNotFoundError as expected
context.skills error: No module named openhands.sdk.context.skills
display_json returns: Text
visualize returns: Text
visualize contains Arguments: True
visualize contains x/y/nested: True
Action: Demo
Arguments:
x: 1
y: "hi"
nested: {values: [1, 2, None]}
kind: "Demo"
This shows the removable deprecated runtime symbols are gone while the changed Action.visualize path still renders the expected arguments through display_json.
Test 2: display_json behavior for non-dict inputs
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python - <<PY ... to call display_json as a library user with a list, string, number, boolean, and None:
list: contains expected marker -> True
[List with 5 items]
[0]: "item1"
[1]: "item2"
[2]: 42
[3]: True
[4]: None
string: contains expected marker -> True
"just a string"
number: contains expected marker -> True
42
boolean: contains expected marker -> True
True
none: contains expected marker -> True
null
This confirms the remaining renderer still handles the representative inputs that users would previously have reached through the deprecated alias.
Issues Found
None.
This QA review was created by an AI agent (OpenHands) on behalf of the user.
3 participants
HUMAN:
I just saw the leftovers. So I decided to delete them.
AGENT:
Why
The repo has a deprecation CI gate (
.github/scripts/check_deprecations.py) that fails a release oncecurrent_version >= removed_in. But it only scans@deprecated/warn_deprecated/warn_cleanupcalls and REST-route decorators. Anything deprecated in prose — a docstring note, a dangling comment, a tombstone module — is invisible to it and rots silently.A sweep at the current version (1.28.0) turned up three such items that are already safe to remove today (none are tracked by the gate, so this touches no deprecation metadata):
plugin/types.pyends with a "Deprecated marketplace classes … re-exported here for backward compatibility" comment that has no code beneath it. The classes moved toopenhands.sdk.marketplacein Move marketplace definitions to openhands.sdk.marketplace module #2786, so the comment isactively misleading —
from openhands.sdk.plugin.types import <Marketplace…>already raisesImportError.context/skills/__init__.pyis a docstring-only tombstone for skill re-export shims that were deprecated in 1.16.0 and removed in 1.21.0 (seven minors ago). Nothing imports the module.utils/visualize.py::display_dict()is a one-line forwarder marked "deprecated, usedisplay_json" in its docstring — yet it's still imported and called in production attool/schema.py:263.Summary
plugin/types.py: delete the dangling "Deprecated marketplace classes" comment block (no code, misleading).context/skills/: delete the docstring-only tombstone__init__.py(canonical location isopenhands.sdk.skills).utils/visualize.py: removedisplay_dict()and repoint its only caller (tool/schema.py) todisplay_json(). Drop the now-redundantdisplay_dicttests (the existingdisplay_jsonsuite is a superset) and fix a staledisplay_dict()mention in atest_mcp_observation.pycomment.Issue Number
N/A — opportunistic cleanup found while auditing un-deleted deprecations.
How to Test
Library-only cleanup with no behavior change (
display_dictwas a pure alias fordisplay_json), so the end-to-end evidence is the real render path plus the gate staying green:Type
4. Deprecation CI gate unchanged
Type
Notes
display_dictwas a public symbol inopenhands.sdk.utils.visualize, but it was a deprecated pure-forwarder with one in-tree caller; removing it leavesdisplay_json(identical output). Worth a glance from the SDK-API-breakagecheck in CI.
acp_env/return_metricsbatch due in 1.29.0) are intentionally left alone until the version bump.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:6a59af0-pythonRun
All tags pushed for this build
About Multi-Architecture Support
6a59af0-python) is a multi-arch manifest supporting both amd64 and arm646a59af0-python-amd64) are also available if needed