Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
API breakage checks (Griffe)Result: Failed Log excerpt (first 1000 characters) |
Agent server REST API breakage checks (OpenAPI)Result: Failed Log excerpt (first 1000 characters) |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟢 Good taste
Analysis:
This PR solves a real problem - centralizing settings schema as a single source of truth for downstream clients. The data structure choice (Pydantic model with field metadata) is pragmatic and follows established patterns.
Data Structure: Clean. Settings live in one place, metadata is co-located with field definitions, and the schema export is straightforward.
Simplicity: The conversion logic handles real use cases (create vs update, custom factories) without over-engineering. Test coverage validates real behavior - agent round-tripping works, factories integrate correctly, schema export produces the right structure.
Backward Compatibility: Purely additive - no existing APIs changed.
Verdict: ✅ Worth merging
Key Insight: By making SDKSettings the declarative source of truth with exportable schema, this eliminates the need for clients to reverse-engineer agent configuration or maintain duplicate settings definitions.
| def to_agent( | ||
| self, | ||
| agent: Agent | None = None, | ||
| *, |
There was a problem hiding this comment.
🟢 Nit: to_agent() is just an alias for apply_to_agent() with identical signature. Consider picking one name - having both adds no value. But this is trivial and not worth blocking on.
There was a problem hiding this comment.
Thanks — I’m leaving both names for now to avoid unnecessary churn in the SDK/public migration path. to_agent() is the compatibility-facing entrypoint used by downstream repos, while apply_to_agent() keeps the implementation naming explicit. Since this is a non-blocking naming nit and behavior is already covered, I’m resolving the thread without further code changes.
There was a problem hiding this comment.
Could you elaborate please, what means compatibility-facing endpoint used by downstream repos, when this is a new file and new methods?
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable - Solid engineering approach solving a real problem.
Data Structure: Clean. Settings metadata lives alongside field definitions via Pydantic annotations, avoiding the anti-pattern of duplicate config models that drift from the source of truth.
Verdict: ✅ Worth merging. The core design is sound - annotating canonical fields eliminates special cases and maintains a single source of truth. Minor documentation fix suggested below.
Co-authored-by: openhands <openhands@all-hands.dev>
| model: str = Field(default="claude-sonnet-4-20250514", description="Model name.") | ||
| api_key: str | SecretStr | None = Field(default=None, description="API key.") | ||
| base_url: str | None = Field(default=None, description="Custom base URL.") | ||
| model: str = Field( |
There was a problem hiding this comment.
Sorry, I still don't understand, why these 5 settings only, and no others?
I think maybe LLM settings should expose all LLM settings from the SDK. As noted in the issue, the client applications are free to choose to input / use only some of them if so they like.
| SETTINGS_METADATA_KEY: SettingsFieldMetadata( | ||
| label="Max input tokens", | ||
| order=50, | ||
| widget="number", |
There was a problem hiding this comment.
Nit: sorry, I still am not sure the SDK should know about widgets
There was a problem hiding this comment.
Widgets, order, which view basic or advanced, slash commands, UI hints, are UI concerns, presentation layer. They belong in the client application that wants to define them
I think maybe we could not mix them here
| label="Max input tokens", | ||
| order=50, | ||
| widget="number", | ||
| advanced=True, |
There was a problem hiding this comment.
It only shows up when the "advanced settings" is selected.
There was a problem hiding this comment.
The SDK doesn't know that, though? I mean, we are assuming that we know exactly what the client applications are, CLI and GUI, and introduce user menu information = UI information in the SDK. This creates tighter coupling than expected in the direction SDK<-CLI/GUI and doesn't seem inline with its ability to be used by multiple random clients, who may well desire other things to be in, idk, basic or advanced or expert UIs
Are we sure about this? 🤔
|
|
||
| class AgentSettings(BaseModel): | ||
| llm: LLM = Field( | ||
| default_factory=_default_llm_settings, |
There was a problem hiding this comment.
I think we already have in the SDK, thanks for Vasco, a default LLM profile for the LLM. This profile is saved, I think, under the name default.json (if I recall correctly), and it will contain all LLM settings. 🤔
This code looks very reasonable, admittedly... it's not clear to me how it will work in practice for us though 🤔
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
Hi — OpenHands-GPT-5.4 here. I read the PR, the review threads, the related issues (#2228, #2274, #1451), and I also checked the current usage patterns in OpenHands/OpenHands-CLI before leaving this review.
I agree with the short-term motivation: programmatic settings should come from the SDK instead of being hand-curated separately in each client. I also think replacing the duplicate mini-LLM settings subset with the canonical LLM fields was the right local correction.
That said, I don't think this PR lands on the right boundary for the SDK yet.
-
The new AgentSettings round-trip is lossy.
from_agent()/apply_to_agent()read like a general SDK config/edit/rebuild path, but the implementation only preserves a curated default story (LLMSummarizingCondenser,APIBasedCritic, and an annotated subset of LLM fields). In an extensible SDK, an official settings/materialization path should not silently drop alternative built-in or downstream implementations. -
The exported schema mixes semantic config with client presentation policy.
widget,advanced,placeholder,slash_command, and similar hints are first-party UX decisions, not core SDK semantics. Exporting them from SDK models hardcodes one client opinion into every client and makes future divergence painful. -
More broadly, #2274 is right that
LLMalready carries too many responsibilities. This PR adds more meaning onto the currentLLMclass instead of separating configuration from runtime/materialization.
My preference would be either:
- narrow this into an explicitly first-party client adapter (instead of a generic SDK settings abstraction), or
- redesign around a canonical immutable config/profile object with neutral schema export, and keep presentation decisions in CLI/GUI.
So: useful motivation, but I think the current abstraction is risky for an extensible SDK.
| else: | ||
| base_agent = base_agent.model_copy(update={"llm": llm}) | ||
|
|
||
| condenser = base_agent.condenser |
There was a problem hiding this comment.
OpenHands-GPT-5.4 here. This is the part that worries me most: from_agent() + apply_to_agent() read like a general SDK round-trip API, but the implementation only preserves the default built-in condenser/critic story. LLMSummarizingCondenser is special-cased, non-APIBasedCritic critics become None, and future/custom components will also get flattened. In an app that can be acceptable if the scope is explicit; in an SDK this makes the official config path lossy. I'd either make this adapter explicitly first-party/default-client only, or model condenser/critic kinds so the round-trip stays lossless.`
| class SettingsFieldMetadata(BaseModel): | ||
| label: str | ||
| order: int | ||
| widget: Literal["text", "password", "number", "boolean", "select"] | None = None |
There was a problem hiding this comment.
OpenHands-GPT-5.4 here. widget, advanced, placeholder, and slash_command feel like presentation policy rather than SDK semantics. The SDK should probably export neutral config/schema facts (type, optionality, choices, secret-ness, dependencies), while CLI/GUI decide how to group/render/expose them. Otherwise one first-party UX becomes the contract for every client.
| model: str = Field(default="claude-sonnet-4-20250514", description="Model name.") | ||
| api_key: str | SecretStr | None = Field(default=None, description="API key.") | ||
| base_url: str | None = Field(default=None, description="Custom base URL.") | ||
| model: str = Field( |
There was a problem hiding this comment.
OpenHands-GPT-5.4 here. One extra architectural concern tied to #2274: LLM is already carrying config, provider-specific logic, formatting, execution, retries, metrics, telemetry, and runtime state. Attaching first-party settings metadata directly to LLM fields pushes even more responsibility into the same god class. Even if the local duplication win makes sense, I'd much rather see the schema source be a dedicated config/profile object so the refactor direction is toward separation of responsibilities, not further entrenching them.
4 participants
Summary
AgentSettingsplus schema export helpers for downstream clientsLLM) so exported LLM settings come from the existing source of truth instead of a duplicate subset modelChecklist
Testing
uv run pre-commit run --files AGENTS.md openhands-sdk/openhands/sdk/__init__.py openhands-sdk/openhands/sdk/llm/llm.py openhands-sdk/openhands/sdk/settings.py openhands-sdk/openhands/sdk/settings_metadata.py tests/sdk/test_settings.pyuv run pytest tests/sdk/test_settings.py -qFixes
Fixes #2228
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-nodejs22golang: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:00354ec-pythonRun
All tags pushed for this build
About Multi-Architecture Support
00354ec-python) is a multi-arch manifest supporting both amd64 and arm6400354ec-python-amd64) are also available if needed