feat(sdk): split AgentSettings into LLM/ACP discriminated union#2861
feat(sdk): split AgentSettings into LLM/ACP discriminated union#2861simonrosenberg merged 7 commits intomainfrom
Conversation
Replaces the monolithic ``AgentSettings`` (which silently ignored ``llm``/``tools``/``mcp_config``/``condenser``/``verification`` when ``acp_command`` was set) with a discriminated union over two concrete variants: - ``LLMAgentSettings`` — the long-standing LLM-agent shape. - ``ACPAgentSettings`` — ACP subprocess agent; keeps ``llm`` (for cost/token metrics attribution) and owns the ACP-specific fields (``acp_command``/``acp_args``/``acp_env``/``acp_model``/ ``acp_session_mode``/``acp_prompt_timeout``). ``AgentSettings`` is now a TypeAlias using ``Annotated[... , Discriminator(...)]`` with a discriminator that defaults missing ``agent_kind`` to ``"llm"`` so existing persisted payloads round-trip without migration. ``create_agent()`` lives on each variant with a narrowed return type (``Agent`` vs ``ACPAgent``) — no more ``AgentBase`` fallback or silently-ignored fields. Adds ``export_agent_settings_schema()`` which produces a single schema with a synthetic ``agent_kind`` critical choice field in ``general`` and sections tagged with a new ``variant`` field (``"llm"`` / ``"acp"`` / ``None``) so GUIs can show only the sections that apply to the selected variant. New helpers: ``validate_agent_settings(data)``, ``default_agent_settings()``, and ``create_agent_from_settings(settings)``. Closes #2853
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
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.
🟡 Acceptable - Solid discriminated union refactor that properly models two distinct agent types. The backward-compatible discriminator and comprehensive tests reduce risk. A few areas could be clearer but the trade-offs are reasonable.
Risk Assessment🟡 MEDIUM Key Factors:
Benchmark Impact: Recommendation: Overall Verdict: The refactor is architecturally sound and solves a real problem. The schema merge complexity and semantic oddity of |
Coverage Report •
|
||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
The discriminated union successfully replaces monolithic AgentSettings, provides type-safe variant selection, and maintains backward compatibility.
Does this PR achieve its stated goal?
Yes. This PR delivers exactly what it promises: a discriminated union over LLMAgentSettings and ACPAgentSettings that eliminates silent field ignoring, provides narrowed return types from create_agent(), exports a schema with variant tags for GUI filtering, and maintains backward compatibility for existing persisted payloads that lack the agent_kind discriminator.
| Phase | Result |
|---|---|
| Environment Setup | ✅ Build successful, dependencies installed |
| CI & Tests | ✅ sdk-tests pass (2m48s), agent-server-tests pass (46s), 21 new tests pass |
| Functional Verification | ✅ All 8 behavioral scenarios verified end-to-end |
Functional Verification
Test 1: Discriminator defaults to 'llm' for backward compatibility
Step 1 — Establish baseline (old payload without agent_kind):
Created a payload simulating persisted AgentSettings from before this PR:
old_payload = {
"schema_version": 1,
"llm": {"model": "test-model"},
# No agent_kind field
}Step 2 — Validate the payload:
Ran validate_agent_settings(old_payload):
Type: LLMAgentSettings
agent_kind: llm
This confirms the discriminator function correctly defaults missing agent_kind to 'llm', preserving backward compatibility for existing persisted settings.
Step 3 — Round-trip and verify agent_kind is added:
Serialized the validated settings back:
roundtrip = validated.model_dump(mode="json")
print(roundtrip["agent_kind"]) # Output: llmThe round-trip adds the agent_kind field, so future saves will include it explicitly.
Test 2: Explicit agent_kind='llm' creates LLMAgentSettings
Validated payload:
llm_payload = {
"agent_kind": "llm",
"llm": {"model": "claude-3-5-sonnet"},
}
validated = validate_agent_settings(llm_payload)Result:
Type: LLMAgentSettings
Model: claude-3-5-sonnet
Confirms explicit agent_kind='llm' dispatches to the LLM variant.
Test 3: Explicit agent_kind='acp' creates ACPAgentSettings
Validated payload:
acp_payload = {
"agent_kind": "acp",
"acp_command": ["npx", "-y", "claude-agent-acp"],
"acp_model": "claude-opus-4-6",
}
validated = validate_agent_settings(acp_payload)Result:
Type: ACPAgentSettings
Command: ['npx', '-y', 'claude-agent-acp']
Model: claude-opus-4-6
Confirms explicit agent_kind='acp' dispatches to the ACP variant.
Test 4: LLMAgentSettings.create_agent() returns narrowed Agent type
Created settings and agent:
settings = LLMAgentSettings(llm=LLM(model="test-model", api_key="fake-key"))
agent = settings.create_agent()Result:
Type: Agent
Confirms the LLM variant's create_agent() returns the narrowed Agent type (not AgentBase).
Test 5: ACPAgentSettings.create_agent() returns narrowed ACPAgent type
Created settings and agent:
settings = ACPAgentSettings(acp_command=["echo", "test"], acp_model="claude-opus-4-6")
agent = settings.create_agent()Result:
Type: ACPAgent
acp_command: ['echo', 'test']
acp_model: claude-opus-4-6
Confirms the ACP variant's create_agent() returns the narrowed ACPAgent type.
Test 6: Schema export includes variant-tagged sections
Exported schema:
schema = export_agent_settings_schema()
print(f"model_name: {schema.model_name}") # Output: AgentSettingsVariant tags found:
Sections with variants:
general (variant=None)
llm (variant=llm)
condenser (variant=llm)
verification (variant=llm)
acp (variant=acp)
llm (variant=acp) # LLM section for metrics
Synthetic agent_kind field:
general_section = next(s for s in schema.sections if s.key == "general")
agent_kind_field = next(f for f in general_section.fields if f.key == "agent_kind")
print(f"Choices: {[c.value for c in agent_kind_field.choices]}")
# Output: ['llm', 'acp']This confirms the schema export adds a synthetic agent_kind selector field and tags sections with their applicable variant so GUIs can filter by the active agent type.
Test 7: ACP variant includes llm field for metrics, excludes LLM-only fields
Created ACP settings:
acp = ACPAgentSettings(
acp_command=["npx", "-y", "claude-agent-acp"],
acp_model="claude-opus-4-6",
llm=LLM(model="claude-opus-4-6", api_key="test-key")
)LLM field is present:
llm.model: claude-opus-4-6
As documented in the PR description, the ACP variant keeps llm for cost/token attribution even though the ACP subprocess makes its own model calls.
LLM-only fields are absent:
acp_payload = acp.model_dump()
assert "tools" not in acp_payload
assert "mcp_config" not in acp_payload
assert "agent_context" not in acp_payload
assert "condenser" not in acp_payload
assert "verification" not in acp_payloadAll assertions pass, confirming the fields that ACPAgent.init_state() rejects are correctly excluded from the ACP variant.
Test 8: Breaking change — old AgentSettings() constructor fails
Attempted direct construction:
from openhands.sdk import AgentSettings
settings = AgentSettings(llm=LLM(model="test"))Result:
TypeError: Cannot instantiate typing.Union
Confirms the documented breaking change: direct AgentSettings() construction no longer works because AgentSettings is now a TypeAlias for a discriminated union. Callers must use LLMAgentSettings() or ACPAgentSettings() explicitly, or use validate_agent_settings() / default_agent_settings() helpers.
Test 9: Updated example runs successfully
Ran examples/01_standalone_sdk/46_agent_settings.py:
✓ Roundtrip deserialization successful — all fields preserved
Agent created: llm.model=litellm_proxy/claude-sonnet-4-5-20250929
tools=['terminal', 'file_editor']
condenser=LLMSummarizingCondenser
The example was updated to use LLMAgentSettings and runs without errors, demonstrating the migration path for existing code.
Issues Found
None. The pre-commit formatting issue (ruff-format wanted to split a long generator expression) has been fixed.
…t_kind UX iteration on the discriminated union — the raw ``acp_command`` JSON array and the stray ``agent_kind`` / ``CodeActAgent`` fields were confusing on the ACP settings page. Three changes: 1. ``acp_server`` CRITICAL choice field (``claude-code`` / ``codex`` / ``gemini-cli`` / ``custom``) drives the subprocess command. The raw ``acp_command`` is demoted to MINOR and is only needed when ``acp_server='custom'`` (or when overriding the default install path). ``ACPAgentSettings.resolve_acp_command()`` maps known servers to their npx defaults (e.g. ``claude-code`` → ``npx -y @agentclientprotocol/claude-agent-acp``). 2. Field-level ``variant`` on ``SettingsFieldMetadata`` + ``SettingsFieldSchema``. The LLM-only general fields (``agent``, ``tools``, ``mcp_config``) now carry ``variant='llm'`` so the GUI can hide them on the ACP settings page. Nested fields inherit their section's variant. 3. ``export_agent_settings_schema()`` no longer emits a synthetic ``agent_kind`` choice field. Each variant lives on its own GUI settings page and that page injects ``agent_kind`` on save — the sidebar-route is the discriminator. Simpler than asking users to set both. ``acp_model`` is also promoted to CRITICAL (it's the second thing users want to set after picking the server).
…AgentSettingsConfig Addresses the Python-API-breakage CI check on #2861, which flagged the 12 ``AgentSettings.*`` members (``schema_version``, ``agent``, ``llm``, ``tools``, ``mcp_config``, ``agent_context``, ``condenser``, ``verification``, ``export_schema``, ``create_agent``, ``build_condenser``, ``build_critic``) as removed without deprecation. Instead of making ``AgentSettings`` a TypeAlias (which strips its attribute surface), keep it as a concrete class and make it a :class:`LLMAgentSettings` subclass. Every v1.17.0 attribute and method resolves through inheritance, so griffe sees them as preserved. The class body calls ``warn_deprecated("AgentSettings", deprecated_in="1.17.0", removed_in="1.22.0", …)`` in ``__init__`` to emit a runtime warning and mark the name as deprecated in the AST so the CI policy is satisfied. The discriminated-union TypeAlias moves to the new name ``AgentSettingsConfig``. Fields that previously read agent_settings: AgentSettings | None now read agent_settings: AgentSettingsConfig | None and the Pydantic discriminator resolves either variant on validation. Downstream callers should: - use ``LLMAgentSettings`` / ``ACPAgentSettings`` for explicit variants, - use ``AgentSettingsConfig`` as the type for fields accepting either, - use ``validate_agent_settings(...)`` to validate raw payloads. ``AgentSettings(...)`` construction continues to work for the v1.17 compatibility window (through v1.22).
The SDK companion PR (OpenHands/software-agent-sdk#2861) keeps ``AgentSettings`` as a deprecated v1.17-compat class and renames the discriminated-union TypeAlias to ``AgentSettingsConfig``. Update the downstream type annotation accordingly so the ``agent_settings`` field continues to accept either the LLM or ACP variant at validation time. - ``Settings.agent_settings: AgentSettingsConfig`` (was ``AgentSettings``). - ``Settings.to_agent_settings`` return type narrows to the explicit ``LLMAgentSettings | ACPAgentSettings`` union. - ``agent_settings_serializer`` likewise.
…essage CI's ``pre-commit`` job flagged two ruff-format fixes: - ``from __future__ import annotations`` is already in effect, so ``agent_settings: "AgentSettingsConfig | None"`` no longer needs the forward-reference string. - A long assertion message is reflowed across three lines per ruff-format's line-length policy. No behavior changes.
…ault ``@google/gemini-cli`` 0.38+ renamed the ACP activation flag from ``--experimental-acp`` to ``--acp`` (the old flag still works, with a deprecation notice). Update the default ``acp_server='gemini-cli'`` command so fresh users hit the supported path. Verified by probing each ACP server's ``initialize`` response: - claude-agent-acp 0.29.0 — no ACP-level auth (uses env vars directly) - codex-acp 0.9.4 — auth methods: chatgpt / codex-api-key / openai-api-key - gemini-cli 0.38.1 — auth methods: oauth-personal / gemini-api-key / vertex-ai / gateway (the "gateway" method is what the SDK's ``_select_auth_method`` targets with ``GEMINI_BASE_URL``)
The frontend's ``depends_on`` filter evaluates each dependency as a boolean (``values[dep] === true``). That works for real booleans (e.g. ``condenser.enabled``) but evaluates to false for the string-valued ``acp_server``, hiding ``acp_command`` outright — even in the "all" view of the ACP Server page — so the ``acp_server=custom`` workflow was unreachable from the GUI. Drop the bogus metadata and rely on prominence + view filtering. The field remains MINOR (visible in the "all" view). No behavior change for CLI / API callers.
|
@OpenHands /codereview this PR. Provide feedback as a review with gh api Then verify what exactly changes for existing clients of the SDK via Python API and REST API, and if our two CI workflows for API breakages succeeded, wdyt, is it correct that they should succeed or do you think we should make them more stringent and why? |
|
I'm on it! enyst can track my progress at all-hands.dev |
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
🟡 Acceptable
[IMPROVEMENT OPPORTUNITIES]
- [PR description] The body is stale after the compatibility follow-up: it still says direct
AgentSettings(...)construction no longer works, but the shipped code keepsAgentSettingsas a deprecatedLLMAgentSettingssubclass. I would update the PR description before merge so reviewers and downstream users get the real migration story. - [REST contract:
/api/settings/agent-schema] The OpenAPI diff is additive only, so the REST breakage workflow passing is mechanically correct today. But the runtime payload semantics did change in a way OpenAPI does not capture: the endpoint now returns variant-tagged sections and duplicate logical section keys (for examplellmexists for both thellmandacpvariants). Any client indexing sections bysection.keyalone could break even though oasdiff stays green.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
The Python API compatibility story is good: existingAgentSettingscallers keep working and get a deprecation runway, which is exactly what the SDK breakage policy wants. The remaining risk is on the schema-serving REST endpoint, where the wire schema is still compatible but the returned schema content now has new semantics that current CI does not model.
VERDICT:
✅ Worth merging: core logic is sound, the deprecation shim is the right way to preserve Python userspace, and the main follow-up is tightening how we review/schema-check the settings endpoint semantics.
KEY INSIGHT:
The important compatibility win here is that Python userspace is preserved; the subtle compatibility risk lives in schema endpoint semantics, not in the exported Python symbols.
This review was generated by an AI assistant (OpenHands) on behalf of enyst.
|
Done.
Conciseness:
|
Integration layer for ACP-in-GUI, building on the discriminated-union AgentSettings merged as OpenHands/software-agent-sdk#2861. When ``AgentSettings`` is an ``ACPAgentSettings`` the app server now: - persists ``agent_kind='acp'`` on ``AppConversationInfo`` (new column via Alembic ``009``; existing rows coerce NULL → ``'llm'``) - POSTs conversation-start to ``/api/acp/conversations`` on the agent-server, skipping LLM-only concerns (skills, hooks, server overrides) - plumbs UI-saved credentials from ``llm.api_key`` / ``llm.base_url`` into the subprocess env as provider-native vars (``ANTHROPIC_API_KEY`` etc.), gated on ``api_key`` being set so a provider-default ``base_url`` alone never clobbers an explicit ``OH_AGENT_SERVER_ENV`` passthrough - branches ``conversation_url`` and live-status polling to the right router so clients don't 404 on ``/api/conversations/{id}`` Also redirects the agent-server subprocess stdout/stderr to a log file at ``{working_dir}/.openhands-agent-server.log`` — the previous ``PIPE`` approach deadlocked once the 64 KB kernel buffer filled without a drain loop. This is the backend half of OpenHands#13983; the settings UX lives in a follow-up PR to keep this one small and reviewable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@simonrosenberg Just wondering, so we know if we need to fix anything. Is this going to affect CLI or Web clients? |
3 participants
Summary
Closes #2853.
Replaces the monolithic
AgentSettings— which silently ignoredllm,tools,mcp_config,condenser, andverificationwhenacp_commandwas set — with a discriminated union over two concrete variants.LLMAgentSettings: the long-standing LLM-agent shape (agent,llm,tools,mcp_config,agent_context,condenser,verification).ACPAgentSettings: ACP-delegating subprocess agent. Keepsllm(for cost/token metrics attribution — the ACP subprocess does its own completions, but we still need a model id for pricing and telemetry) plus the ACP-specific fields:acp_command,acp_args,acp_env,acp_model,acp_session_mode,acp_prompt_timeout.AgentSettingsis now a TypeAlias:Annotated[LLMAgentSettings | ACPAgentSettings, Discriminator(...)]. The discriminator defaults missingagent_kind→"llm"so existing persisted payloads round-trip without a migration.create_agent()lives on each variant with a narrowed return type (AgentvsACPAgent) — no moreAgentBasefallback.export_agent_settings_schema()produces a single schema with a syntheticagent_kindcritical choice field ingeneraland sections tagged with a newvariantfield ("llm"/"acp"/None) so GUIs can show only the sections that apply to the active variant.validate_agent_settings(data),default_agent_settings(),create_agent_from_settings(settings).Note on the issue text
The issue described
llmas "silently-ignored" in ACP mode. In practice,ACPAgentdoes accept anllmfield — it's a dummy"acp-managed"placeholder by default, but gets used for cost/token attribution (llm.metrics.model_nameis rewritten toacp_modelinmodel_post_init). So the redesign here keepsllmonACPAgentSettingsrather than dropping it outright as Option 1 in the issue suggested. The LLM-only fields (tools,mcp_config,agent_context,condenser,verification) thatACPAgent.init_state()already rejects are the ones actually removed from the ACP variant.Breaking change
Direct construction
AgentSettings(...)no longer works — callers must pickLLMAgentSettings(...)orACPAgentSettings(...). The OpenHands repo companion PR demonstrates the migration (tests usefrom openhands.sdk.settings import LLMAgentSettings as AgentSettingsas a temporary alias).Test plan
tests/sdk/test_settings.py(per-variant schema export, discriminated-union validation including missing-discriminator default,create_agent()narrowing,ConversationSettings.create_requestfor both request types).tests/agent_server/test_settings_router.py—AgentSettingsschema endpoint still serves a schema named"AgentSettings"withllm/condenser/verificationsections (now variant-tagged).🤖 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:f13a4ba-pythonRun
All tags pushed for this build
About Multi-Architecture Support
f13a4ba-python) is a multi-arch manifest supporting both amd64 and arm64f13a4ba-python-amd64) are also available if needed