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.
|
@OpenHands Look at the PR template in this repo, and at the workflow that fails here in CI:
|
|
I'm on it! enyst can track my progress at all-hands.dev |
|
@enyst I checked the failing
Validation for #3693:
This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @xingyaoww, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. This comment was created by an AI agent (OpenHands) on behalf of the user. |
3 participants
HUMAN:
This PR proposes a mechanism to keep
is_subscriptionacross the wire.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