Skip to content

Commit 2e8e0fa

Browse files
fix(sdk): simplify settings schema section handling
- remove the public CURRENT_PERSISTED_VERSION constants - default missing section metadata to the general section - update settings tests to assert schema_version directly Co-authored-by: openhands <openhands@all-hands.dev>
1 parent 8fed9d1 commit 2e8e0fa

File tree

3 files changed

+38
-60
lines changed

3 files changed

+38
-60
lines changed

AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@ When reviewing code, provide constructive feedback:
107107
- `AgentSettings.tools` is part of the exported settings schema so the schema stays aligned with the settings payload that round-trips through `AgentSettings` and drives `create_agent()`.
108108
- `AgentSettings.mcp_config` now uses FastMCP's typed `MCPConfig` at runtime. When serializing settings back to plain data (e.g. `model_dump()` or `create_agent()`), keep the output compact with `exclude_none=True, exclude_defaults=True` so callers still see the familiar `.mcp.json`-style dict shape.
109109
- Persisted SDK settings should use the direct `model_dump()` shape with a top-level `schema_version`; avoid adding wrapped payload formats or legacy migration shims in `openhands/sdk/settings/model.py`.
110+
- Do not expose settings schema versions as public `CURRENT_PERSISTED_VERSION` class constants on `AgentSettings` or `ConversationSettings`; keep versioning internal to the `schema_version` field/defaults and private module constants.
110111
- `ConversationSettings` owns the conversation-scoped confirmation controls directly (`confirmation_mode`, `security_analyzer`); keep those fields top-level on the model and grouped into the exported `verification` section via schema metadata rather than nested helper models, and prefer the direct settings-model constructor `create_request(...)` over separate request-wrapper helpers.
111112
- Anthropic malformed tool-use/tool-result history errors (for example, missing or duplicated ``tool_result`` blocks) are intentionally mapped to a dedicated `LLMMalformedConversationHistoryError` and caught separately in `Agent.step()`, so recovery can still use condensation while logs preserve that this was malformed history rather than a true context-window overflow.
112113
- AgentSkills progressive disclosure goes through `AgentContext.get_system_message_suffix()` into `<available_skills>`, and `openhands.sdk.context.skills.to_prompt()` truncates each prompt description to 1024 characters because the AgentSkills specification caps `description` at 1-1024 characters.

openhands-sdk/openhands/sdk/settings/model.py

Lines changed: 33 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
from collections.abc import Callable
44
from enum import Enum
55
from pathlib import Path
6-
from typing import TYPE_CHECKING, Any, ClassVar, Literal, TypeVar, get_args, get_origin
6+
from typing import TYPE_CHECKING, Any, Literal, TypeVar, get_args, get_origin
77

88
from fastmcp.mcp_config import MCPConfig
99
from pydantic import (
@@ -243,8 +243,6 @@ def _default_llm_settings() -> LLM:
243243

244244

245245
class ConversationSettings(BaseModel):
246-
CURRENT_PERSISTED_VERSION: ClassVar[int] = _CONVERSATION_SETTINGS_SCHEMA_VERSION
247-
248246
schema_version: int = Field(default=_CONVERSATION_SETTINGS_SCHEMA_VERSION, ge=1)
249247
max_iterations: int = Field(
250248
default=500,
@@ -334,8 +332,6 @@ def create_request(
334332

335333

336334
class AgentSettings(BaseModel):
337-
CURRENT_PERSISTED_VERSION: ClassVar[int] = _AGENT_SETTINGS_SCHEMA_VERSION
338-
339335
schema_version: int = Field(default=_AGENT_SETTINGS_SCHEMA_VERSION, ge=1)
340336
agent: str = Field(
341337
default="CodeActAgent",
@@ -493,7 +489,9 @@ def build_critic(self) -> CriticBase | None:
493489
)
494490

495491

496-
def settings_section_metadata(field: FieldInfo) -> SettingsSectionMetadata | None:
492+
def _explicit_settings_section_metadata(
493+
field: FieldInfo,
494+
) -> SettingsSectionMetadata | None:
497495
extra = field.json_schema_extra
498496
if not isinstance(extra, dict):
499497
return None
@@ -504,6 +502,13 @@ def settings_section_metadata(field: FieldInfo) -> SettingsSectionMetadata | Non
504502
return SettingsSectionMetadata.model_validate(metadata)
505503

506504

505+
def settings_section_metadata(field: FieldInfo) -> SettingsSectionMetadata:
506+
metadata = _explicit_settings_section_metadata(field)
507+
if metadata is not None:
508+
return metadata
509+
return _GENERAL_SECTION_METADATA
510+
511+
507512
def settings_metadata(field: FieldInfo) -> SettingsFieldMetadata | None:
508513
extra = field.json_schema_extra
509514
if not isinstance(extra, dict):
@@ -517,6 +522,10 @@ def settings_metadata(field: FieldInfo) -> SettingsFieldMetadata | None:
517522

518523
_GENERAL_SECTION_KEY = "general"
519524
_GENERAL_SECTION_LABEL = "General"
525+
_GENERAL_SECTION_METADATA = SettingsSectionMetadata(
526+
key=_GENERAL_SECTION_KEY,
527+
label=_GENERAL_SECTION_LABEL,
528+
)
520529

521530

522531
_HIDDEN_SETTINGS_SCHEMA_FIELDS: dict[type[BaseModel], set[str]] = {
@@ -541,28 +550,29 @@ def export_settings_schema(model: type[BaseModel]) -> SettingsSchema:
541550
"""
542551
sections: list[SettingsSectionSchema] = []
543552
sections_by_key: dict[str, SettingsSectionSchema] = {}
544-
general_fields: list[SettingsFieldSchema] = []
545553

546-
def ensure_section(key: str, label: str) -> SettingsSectionSchema:
547-
section = sections_by_key.get(key)
554+
def ensure_section(metadata: SettingsSectionMetadata) -> SettingsSectionSchema:
555+
section = sections_by_key.get(metadata.key)
548556
if section is not None:
549557
return section
550-
section = SettingsSectionSchema(key=key, label=label, fields=[])
551-
sections_by_key[key] = section
558+
section = SettingsSectionSchema(
559+
key=metadata.key,
560+
label=metadata.label or _humanize_name(metadata.key),
561+
fields=[],
562+
)
563+
sections_by_key[metadata.key] = section
552564
sections.append(section)
553565
return section
554566

555567
for field_name, field in model.model_fields.items():
556568
section_metadata = settings_section_metadata(field)
557569
nested_model = _nested_model_type(field.annotation)
570+
explicit_section_metadata = _explicit_settings_section_metadata(field)
558571

559572
# Nested section (e.g., llm, condenser, critic)
560-
if section_metadata is not None and nested_model is not None:
573+
if explicit_section_metadata is not None and nested_model is not None:
561574
section_default = field.get_default(call_default_factory=True)
562-
section_label = section_metadata.label or _humanize_name(
563-
section_metadata.key
564-
)
565-
section = ensure_section(section_metadata.key, section_label)
575+
section = ensure_section(explicit_section_metadata)
566576
for nested_key, nested_field in nested_model.model_fields.items():
567577
if _is_hidden_settings_schema_field(
568578
nested_model, nested_key, nested_field
@@ -574,15 +584,15 @@ def ensure_section(key: str, label: str) -> SettingsSectionSchema:
574584
default_value = getattr(section_default, nested_key)
575585
section.fields.append(
576586
SettingsFieldSchema(
577-
key=f"{section_metadata.key}.{nested_key}",
587+
key=f"{explicit_section_metadata.key}.{nested_key}",
578588
label=(
579589
metadata.label
580590
if metadata is not None and metadata.label is not None
581591
else _humanize_name(nested_key)
582592
),
583593
description=nested_field.description,
584-
section=section_metadata.key,
585-
section_label=section_label,
594+
section=section.key,
595+
section_label=section.label,
586596
value_type=_infer_value_type(nested_field.annotation),
587597
default=_normalize_default(default_value),
588598
prominence=(
@@ -591,7 +601,7 @@ def ensure_section(key: str, label: str) -> SettingsSectionSchema:
591601
else SettingProminence.MINOR
592602
),
593603
depends_on=[
594-
f"{section_metadata.key}.{dependency}"
604+
f"{explicit_section_metadata.key}.{dependency}"
595605
for dependency in (
596606
metadata.depends_on if metadata is not None else ()
597607
)
@@ -607,30 +617,7 @@ def ensure_section(key: str, label: str) -> SettingsSectionSchema:
607617
continue
608618

609619
default_value = field.get_default(call_default_factory=True)
610-
if section_metadata is None:
611-
general_fields.append(
612-
SettingsFieldSchema(
613-
key=field_name,
614-
label=(
615-
metadata.label
616-
if metadata.label is not None
617-
else _humanize_name(field_name)
618-
),
619-
description=field.description,
620-
section=_GENERAL_SECTION_KEY,
621-
section_label=_GENERAL_SECTION_LABEL,
622-
value_type=_infer_value_type(field.annotation),
623-
default=_normalize_default(default_value),
624-
prominence=metadata.prominence,
625-
depends_on=list(metadata.depends_on),
626-
secret=_contains_secret(field.annotation),
627-
choices=_extract_choices(field.annotation),
628-
)
629-
)
630-
continue
631-
632-
section_label = section_metadata.label or _humanize_name(section_metadata.key)
633-
section = ensure_section(section_metadata.key, section_label)
620+
section = ensure_section(section_metadata)
634621
section.fields.append(
635622
SettingsFieldSchema(
636623
key=field_name,
@@ -640,8 +627,8 @@ def ensure_section(key: str, label: str) -> SettingsSectionSchema:
640627
else _humanize_name(field_name)
641628
),
642629
description=field.description,
643-
section=section_metadata.key,
644-
section_label=section_label,
630+
section=section.key,
631+
section_label=section.label,
645632
value_type=_infer_value_type(field.annotation),
646633
default=_normalize_default(default_value),
647634
prominence=metadata.prominence,
@@ -651,16 +638,6 @@ def ensure_section(key: str, label: str) -> SettingsSectionSchema:
651638
)
652639
)
653640

654-
if general_fields:
655-
sections.insert(
656-
0,
657-
SettingsSectionSchema(
658-
key=_GENERAL_SECTION_KEY,
659-
label=_GENERAL_SECTION_LABEL,
660-
fields=general_fields,
661-
),
662-
)
663-
664641
return SettingsSchema(model_name=model.__name__, sections=sections)
665642

666643

tests/sdk/test_settings.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,7 @@ def test_roundtrip_preserves_llm_model() -> None:
365365
def test_agent_settings_model_validate_accepts_nested_payload() -> None:
366366
settings = AgentSettings.model_validate(
367367
{
368-
"schema_version": AgentSettings.CURRENT_PERSISTED_VERSION,
368+
"schema_version": 1,
369369
"llm": {
370370
"model": "test-model",
371371
"api_key": "secret-key",
@@ -374,7 +374,7 @@ def test_agent_settings_model_validate_accepts_nested_payload() -> None:
374374
}
375375
)
376376

377-
assert settings.schema_version == AgentSettings.CURRENT_PERSISTED_VERSION
377+
assert settings.schema_version == 1
378378
assert settings.llm.model == "test-model"
379379
assert isinstance(settings.llm.api_key, SecretStr)
380380
assert settings.llm.api_key.get_secret_value() == "secret-key"
@@ -384,7 +384,7 @@ def test_agent_settings_model_validate_accepts_nested_payload() -> None:
384384
def test_agent_settings_model_dump_roundtrip_preserves_sparse_updates() -> None:
385385
settings = AgentSettings.model_validate(
386386
{
387-
"schema_version": AgentSettings.CURRENT_PERSISTED_VERSION,
387+
"schema_version": 1,
388388
"llm": {"model": "base-model", "base_url": "https://example.com"},
389389
}
390390
)
@@ -402,4 +402,4 @@ def test_agent_settings_model_dump_roundtrip_preserves_sparse_updates() -> None:
402402

403403
assert restored.llm.model == "personal-model"
404404
assert restored.llm.base_url is None
405-
assert restored.schema_version == AgentSettings.CURRENT_PERSISTED_VERSION
405+
assert restored.schema_version == 1

0 commit comments

Comments
 (0)