fix(llm): serialize is_subscription so subscription mode survives remote agent-server transport#3633
fix(llm): serialize is_subscription so subscription mode survives remote agent-server transport#3633xingyaoww wants to merge 1 commit into
Conversation
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 WITH ISSUES
Functional QA confirms the PR preserves subscription mode across SDK serialization boundaries; CI is not fully green at the time of review.
Does this PR achieve its stated goal?
Yes. The PR set out to make is_subscription survive serialization so a remote agent-server rebuild keeps subscription-specific behavior. I exercised the SDK as a user would by constructing subscription LLM/Agent objects, crossing a JSON model_dump → model_validate transport boundary, and reusing the restored object; base loses the flag, while this PR preserves it and keeps subscription message formatting active after restore.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully |
| CI Status | Python API, Validate PR description), 8 in progress, 1 skipped |
| Functional Verification | ✅ Base reproduces flag loss; PR preserves is_subscription through LLM and Agent payloads |
Functional Verification
Test 1: LLM remote-style serialization keeps subscription behavior
Step 1 — Reproduce baseline without the fix:
Ran git checkout --quiet origin/main && uv run python /tmp/qa_subscription_transport.py:
payload_has_is_subscription=False
payload_is_subscription=None
restored_is_subscription=False
plain_restored_is_subscription=False
instructions='SYSTEM'
inputs_contain_system_text=False
inputs_contain_user_text=True
This confirms the bug: a subscription-marked LLM loses the flag after JSON serialization/rebuild, so the restored object behaves as non-subscription.
Step 2 — Apply the PR's changes:
Checked out fix/serialize-is-subscription at d672c782056d15576e3636054d31855555dbbd65.
Step 3 — Re-run with the fix in place:
Ran uv run python /tmp/qa_subscription_transport.py:
payload_has_is_subscription=True
payload_is_subscription=True
restored_is_subscription=True
plain_restored_is_subscription=False
instructions='You are OpenHands agent, a helpful AI assistant that can interact with a computer to solve tasks.'
inputs_contain_system_text=True
inputs_contain_user_text=True
This shows the serialized payload now carries is_subscription=True, the rebuilt LLM keeps it, a plain LLM still defaults to False, and subscription-specific message formatting remains active after the transport boundary.
Test 2: Agent payload preserves the nested subscription LLM
Step 1 — Reproduce baseline without the fix:
Ran git checkout --quiet origin/main && uv run python /tmp/qa_agent_transport.py:
agent_payload_llm_has_is_subscription=False
agent_payload_llm_is_subscription=None
restored_agent_llm_is_subscription=False
This confirms the remote-agent payload path loses the nested LLM subscription flag on base.
Step 2 — Apply the PR's changes:
Checked out fix/serialize-is-subscription at d672c782056d15576e3636054d31855555dbbd65.
Step 3 — Re-run with the fix in place:
Ran uv run python /tmp/qa_agent_transport.py:
agent_payload_llm_has_is_subscription=True
agent_payload_llm_is_subscription=True
restored_agent_llm_is_subscription=True
This verifies the Agent serialization boundary used by remoting now preserves the subscription LLM flag.
Unable to Verify
I did not exercise a live ChatGPT subscription/Codex request or an authenticated end-to-end RemoteConversation against a running agent-server because no ChatGPT subscription credentials were available in the QA environment. The core transport behavior was verified locally at the exact SDK serialization/rebuild boundary that remote transport uses.
Issues Found
- 🟠 Issue: CI is not currently green:
Python APIandValidate PR descriptionare failing, with 8 checks still in progress in the latest REST check snapshot. - No functional issues were found in the exercised subscription serialization behavior.
This review was created by an AI agent (OpenHands) on behalf of the user.
Verdict: PASS WITH ISSUES
Backward compatibilityReviewed against the SDK deprecation policy (public API removals need deprecation metadata with a ≥5-minor-release runway):
|
|
✅ 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.
⚠️ QA Report: PASS WITH ISSUES
Functional QA confirms is_subscription now survives SDK serialization and local agent-server transport; the only issue noted is current CI status (API breakage checks are failing).
Does this PR achieve its stated goal?
Yes, functionally. On origin/main, a subscription-marked LLM lost the flag during model_dump() → model_validate() (serialized_has_is_subscription=False, restored_is_subscription=False). With this PR, the same SDK path preserves it (serialized_has_is_subscription=True, restored_is_subscription=True), and a local agent-server created through RemoteConversation returned agent.llm.is_subscription=True from /api/conversations/{id}.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully and installed the editable SDK/server packages. |
| CI Status | Python API, REST API (OpenAPI)), 9 in progress. |
| Functional Verification | ✅ Real SDK calls plus a local agent-server transport path preserved subscription mode. |
Functional Verification
Test 1: Reproduce serialization loss, then verify PR preserves the flag
Step 1 — Reproduce / establish baseline (without the fix):
Ran git checkout --detach origin/main && OPENHANDS_SUPPRESS_BANNER=1 VIRTUAL_ENV= uv run python - <<'PY' ... PY to create a subscription-marked LLM, serialize it, rebuild it, and format response messages:
branch_ref=origin/main
original_is_subscription=True
serialized_has_is_subscription=False
restored_is_subscription=False
instructions_is_none=False
formatted_inputs=[{"type": "message", "role": "user", "content": [{"type": "input_text", "text": "USER"}]}]
This confirms the bug exists on the base branch: the original object was in subscription mode, but the serialized payload did not include the flag and the rebuilt object reverted to non-subscription behavior.
Step 2 — Apply the PR's changes:
Checked out PR commit 3b118a994d72bbc0313a5be031a610d20dfdeb64.
Step 3 — Re-run with the fix in place:
Ran the equivalent SDK script with llm.is_subscription = True:
branch_ref=PR 3b118a9
original_is_subscription=True
serialized_has_is_subscription=True
serialized_is_subscription=True
restored_is_subscription=True
instructions_is_none=False
formatted_inputs=[{"role": "user", "content": [{"type": "input_text", "text": "Context (system prompt):\nSYSTEM\n\n"}, {"type": "input_text", "text": "USER"}]}]
This shows the serialized transport payload now carries is_subscription=True, the rebuilt LLM keeps it, and subscription-specific message formatting still applies after the round trip.
Test 2: Exercise actual local agent-server transport
Step 1 — Establish baseline:
Test 1 established that the transport-style serialization boundary failed before the PR.
Step 2 — Apply the PR's changes and start software:
Started the local server with uv run python -m openhands.agent_server --host 127.0.0.1 --port 8123, confirmed /server_info returned HTTP 200, then created a real RemoteConversation with an Agent whose LLM had is_subscription=True.
Step 3 — Inspect server-side conversation state:
Queried GET /api/conversations/{conversation_id} from the running server:
conversation_id=fe5df9f3-2286-43ef-8125-e84266b66035
get_conversation_status=200
response_mentions_is_subscription=True
agent.llm.is_subscription=True
top_level_keys=activated_knowledge_skills,agent,agent_state,available_models,blocked_actions,blocked_messages,client_tools,confirmation_policy,created_at,current_model_id,execution_status,hook_config,id,invoked_skills,last_user_message_id,max_iterations,metrics,persistence_dir,secret_registry,security_analyzer,stats,stuck_detection,supports_runtime_model_switch,tags,title,updated_at,workspace
This verifies the PR's target remote transport path in running software: the agent-server rebuilt/stored the remote conversation with agent.llm.is_subscription=True.
Test 3: Legacy setter and plain LLM behavior
Step 1 — Baseline:
The base-branch run in Test 1 showed direct _is_subscription mutation worked only locally and did not survive serialization.
Step 2 — Re-run compatibility/default checks on the PR:
Ran a script that sets legacy._is_subscription = True, captures warnings, round-trips through serialization, and checks a plain LLM:
branch_ref=PR 3b118a9
legacy_write_warning_count=1
legacy_warning_types=DeprecatedWarning
legacy_restored_is_subscription=True
plain_serialized_is_subscription=False
plain_restored_is_subscription=False
legacy_warning_is_deprecated=True
This shows older direct writes still function with a deprecation warning, and non-subscription LLMs still default to False after serialization.
Issues Found
⚠️ CI status:Python APIandREST API (OpenAPI)breakage checks are currently failing in GitHub checks; I did not investigate or rerun CI jobs as part of functional QA.
This QA review was created by an AI agent (OpenHands) on behalf of the user.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review Summary
🟢 Good taste - Elegant, minimal fix that solves a real serialization problem with clean backward compatibility.
Changes Overview
This PR converts is_subscription from a private PrivateAttr to a public Field so it survives Pydantic serialization (model_dump/model_validate) when the LLM is transported to a remote agent-server.
Analysis
[CRITICAL ISSUES] - None
[IMPROVEMENT OPPORTUNITIES] - None identified
[TESTING]
test_is_subscription_survives_serialization_round_trip()- Verifies the core fix works as intendedtest_legacy_private_attr_write_redirects_with_deprecation_warning()- Verifies backward compatibility with deprecation warning- Existing tests updated to use the public field instead of private attr
Notable Design Decisions
-
Deprecation via
__setattr__interception - Clean approach to maintain backward compatibility while guiding users to the new API. The deprecation path is gradual (3 minor versions to removal). -
Documentation in field description - Good that the docstring explicitly mentions the remote transport use case.
-
Minimal surface area - Only 4 files changed, focused change.
[RISK ASSESSMENT]
- Overall PR: Risk Assessment: 🟢 LOW
- No breaking changes to existing APIs
- Backward compatible via deprecation warning
- Well-tested with serialization round-trip test
VERDICT:
✅ Worth merging - Clean fix that solves the serialization problem without breaking existing code.
KEY INSIGHT:
Converting a PrivateAttr to a public Field is the right solution here — it makes the flag serializable while the __setattr__ interception maintains backward compatibility for code that still writes to _is_subscription.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
…ote transport `is_subscription` was a plain property backed by the `_is_subscription` PrivateAttr, so it was silently dropped whenever an LLM was serialized — in particular when a RemoteConversation ships the agent's LLM to an agent-server. The server rebuilt the LLM with is_subscription == False, and every subscription-specific behavior stopped applying server-side: - the streaming on_token exemption in responses() (ValueError: Streaming requires an on_token callback), - the Codex system-prompt transform (transform_for_subscription), - reasoning-item stripping for store=false (follow-up requests reference unresolvable item IDs -> 404 from the Codex endpoint). Net effect: LLM.subscription_login() worked locally but silently broke with remote workspaces. Promote the property to a `@computed_field` (so it is included in model_dump) and add a wrap model_validator that restores the backing `_is_subscription` PrivateAttr on validation. The public read API (`llm.is_subscription`) and the existing `_is_subscription` write path are both unchanged, so this is additive and not an API break. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
xingyaoww
force-pushed
the
fix/serialize-is-subscription
branch
from
3b118a9 to
2daa3f8
Compare
|
Pushed First attempt made Current approach keeps Verified against the released baseline:
The red ❌ on Python API / REST API (OpenAPI) is not the breakage check — both of those steps pass. The jobs fail only at the final |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
⚠️ QA Report: PASS WITH ISSUES
The SDK behavior behind the PR goal works: subscription-mode LLMs now survive serialization/revalidation and retain the streaming exemption after transport-like round trips.
Does this PR achieve its stated goal?
Yes. The PR set out to serialize is_subscription so subscription mode survives remote agent-server transport; I exercised the SDK's LLM and Agent serialization/rebuild path and confirmed main loses the flag while this PR preserves it. I also called LLM.responses() after the round trip with stream=True and no on_token: main fails locally with ValueError: Streaming requires an on_token callback, while the PR keeps is_subscription=True and proceeds to the HTTP layer, proving the subscription streaming exemption remains active after serialization.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed and installed the uv environment successfully. |
| CI Status | Validate PR description was failing and qa-changes was still in progress when checked. |
| Functional Verification | ✅ Verified serialization, Agent payload transport shape, JSON wire-style round trip, plain non-subscription behavior, and streaming exemption behavior. |
Functional Verification
Test 1: Subscription flag survives SDK/Agent serialization round trip
Step 1 — Reproduce baseline on origin/main (e01557ee):
Ran a script that creates a Codex-style LLM, sets _is_subscription=True as the subscription auth flow does, serializes/revalidates the LLM, then serializes/revalidates an Agent containing that LLM:
baseline_llm_dump_has_is_subscription= False
baseline_llm_dump_value= None
baseline_restored_llm_is_subscription= False
baseline_agent_llm_dump_has_is_subscription= False
baseline_restored_agent_llm_is_subscription= False
baseline_plain_restored_is_subscription= False
This confirms the reported bug exists on main: the subscription flag is absent from serialized LLM/Agent payloads and comes back as False after reconstruction.
Step 2 — Apply the PR's changes:
Used the checked-out PR branch fix/serialize-is-subscription at 2daa3f83.
Step 3 — Re-run the same scenario on the PR:
pr_llm_dump_has_is_subscription= True
pr_llm_dump_value= True
pr_restored_llm_is_subscription= True
pr_agent_llm_dump_has_is_subscription= True
pr_restored_agent_llm_is_subscription= True
pr_plain_restored_is_subscription= False
This shows the fix works for the transport-relevant SDK objects: subscription LLMs serialize with the flag and restore it, while ordinary LLMs still restore as non-subscription.
Test 2: JSON wire-style Agent payload round trip
Ran an Agent payload through json.dumps / json.loads before model validation on the PR:
wire_llm_is_subscription_field= True
wire_restored_agent_llm_is_subscription= True
This confirms the field survives a JSON-shaped payload, matching the kind of boundary used by remote transport rather than only an in-memory Python dict.
Test 3: Streaming on_token exemption remains active after round trip
Step 1 — Reproduce baseline on origin/main:
With stream=True, num_retries=0, a dummy API key, and base_url=http://127.0.0.1:9, I called responses() after the serialization round trip without providing on_token:
baseline_after_roundtrip_is_subscription= False
baseline_responses_exception_type= ValueError
baseline_responses_exception_first_line= Streaming requires an on_token callback
This confirms the user-visible breakage described in the PR: after serialization, the LLM is no longer treated as subscription mode and the local streaming guard rejects the call before any HTTP request.
Step 2 — Re-run on the PR:
pr_after_roundtrip_is_subscription= True
pr_responses_exception_type= LLMServiceUnavailableError
pr_responses_exception_first_line= litellm.InternalServerError: InternalServerError: OpenAIException - [Errno 111] Connection refused
The connection refusal is expected because I intentionally pointed the request at a closed localhost port. The important behavior is that the local on_token error is gone and the call reaches the HTTP layer, which demonstrates the subscription-specific streaming exemption survived the serialization round trip.
Unable to Verify
I did not use real ChatGPT/Codex subscription credentials or run a live Codex request. The functional check intentionally used a dummy key and closed localhost endpoint to verify the SDK transport/streaming behavior without external credentials.
Issues Found
- 🟡 CI:
Validate PR descriptionis currently failing according togh pr checks; no functional issues were found in the changed SDK behavior.
This QA report was created by an AI agent (OpenHands) on behalf of the user.
2 participants
AGENT:
Why
is_subscriptionwas a plain@propertybacked by the_is_subscriptionPrivateAttr, so it was silently dropped whenever an LLM was serialized — in particular when aRemoteConversationships the agent's LLM to an agent-server. The server rebuilds the LLM withis_subscription == False, and every subscription-specific behavior stops applying server-side:on_tokenexemption inresponses()(raisesValueError: Streaming requires an on_token callback),transform_for_subscription),store=false(follow-up requests reference unresolvable item IDs → 404 from the Codex endpoint).Net effect:
LLM.subscription_login()works locally but silently breaks with remote workspaces.Summary
is_subscriptionproperty to a@computed_fieldso it is included inmodel_dump()._is_subscriptionPrivateAttrand its existing write path untouched (no call-site or API changes;OpenAISubscriptionAuth.create_llmstill sets it directly).model_validatorthat restores_is_subscriptionwhen validating serialized data, so the flag survives a dump → validate round trip (e.g. transport to a remote agent-server).Issue Number
N/A
How to Test
Round-trip is the core of the fix:
Automated:
A new regression test (
test_is_subscription_survives_serialization_round_trip) covers both the subscription and plain cases.ruff+pyrightclean on the touched files.Video/Screenshots
N/A (no UI surface).
Type
Notes
is_subscriptionbecomes an additive, read-only computed field in the serialized schema; the griffe API-breakage check sees noATTRIBUTE_CHANGED_VALUE(still a property), and the REST schema gains only the additive read-only field. Backward compatible: old payloads without the field validate toFalseas before.🤖 Generated with Claude Code
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:2daa3f8-pythonRun
All tags pushed for this build
About Multi-Architecture Support
2daa3f8-python) is a multi-arch manifest supporting both amd64 and arm642daa3f8-python-amd64) are also available if needed