Prepare AgentSettings removal helpers#3208
Conversation
Coverage Report •
|
||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
[CRITICAL ISSUES]
- [openhands-sdk/openhands/sdk/settings/model.py] Deprecation Policy:
LLMAgentSettingsremoval appears to violate the 5-release deprecation policy (only 4 releases since 1.19.0). See inline comment for details.
[RISK ASSESSMENT]
This is a breaking API change removing long-deprecated aliases. Risk factors:
- ✅ Migration logic properly preserved in
validate_agent_settings() - ✅ Tests comprehensively updated
- ✅
AgentSettings(deprecated 1.17.0) meets 5-release requirement ⚠️ LLMAgentSettingstimeline needs verification (appears 1 release short)- The PR stacks on rel-1.22.0 and will be retargeted, which mitigates timing concerns
Recommendation: Verify deprecation timeline before merge. If policy violation confirmed, either remove only AgentSettings in this PR or update removal target to v1.24.0.
VERDICT:
⏸️ Needs clarification: Core implementation is solid, but deprecation policy compliance must be confirmed by maintainer.
KEY INSIGHT:
The migration strategy correctly preserves backward compatibility for persisted data (old "llm" discriminators still work) while preventing new code from using deprecated variants—a textbook deprecation removal, contingent on timeline verification.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
All changes verified successfully through functional testing.
Does this PR achieve its stated goal?
Yes. The PR successfully removes the deprecated AgentSettings and LLMAgentSettings aliases and makes validate_agent_settings() the canonical migration-aware loader for persisted agent settings. The agent-server persistence layer correctly uses the new API, and legacy payloads (including agent_kind: "llm") are properly migrated to the current schema.
| Phase | Result |
|---|---|
| Environment Setup | ✅ Dependencies installed, project builds successfully |
| CI Status | ✅ All checks passing (sdk-tests, agent-server-tests, cross-tests, pre-commit, deprecation check) |
| Functional Verification | ✅ 9 functional tests + 3 integration tests all passing |
Functional Verification
Test 1: Verify deprecated classes are removed
Baseline: Before this PR, AgentSettings and LLMAgentSettings could be imported from openhands.sdk and openhands.sdk.settings (though they emitted deprecation warnings).
After PR changes: Attempted to import both classes:
from openhands.sdk import AgentSettings
from openhands.sdk import LLMAgentSettings
from openhands.sdk.settings import AgentSettings
from openhands.sdk.settings import LLMAgentSettingsAll import attempts correctly raise ImportError with message cannot import name 'AgentSettings' / cannot import name 'LLMAgentSettings'. The deprecated aliases have been successfully removed from the public API.
Test 2: Current agent settings payloads work
Test: Validated current agent_kind: "openhands" and agent_kind: "acp" payloads with validate_agent_settings():
# OpenHands agent
result = validate_agent_settings({
"agent_kind": "openhands",
"llm": {"model": "anthropic/claude-sonnet-4-5-20250929"}
})
assert result.agent_kind == "openhands"
# ACP agent
result = validate_agent_settings({
"agent_kind": "acp",
"acp_server": "custom",
"acp_command": ["npx", "-y", "claude-agent-acp"]
})
assert result.agent_kind == "acp"Both payloads validate correctly and return the expected settings instances.
Test 3: Legacy 'llm' kind migration
Test: Verified that legacy agent_kind: "llm" payloads are migrated to "openhands":
legacy_payload = {
"schema_version": 1,
"agent_kind": "llm",
"llm": {"model": "legacy-model"}
}
result = validate_agent_settings(legacy_payload)Result: result.agent_kind == "openhands" (migrated from "llm")
The v1→v2 migration (_migrate_agent_settings_v1_to_v2) correctly rewrites the discriminator from "llm" to "openhands", allowing legacy persisted payloads to load into the canonical OpenHandsAgentSettings class.
Test 4: v0 payload handling
Test: Verified that v0 payloads (no schema_version, no agent_kind) default correctly:
v0_payload = {"llm": {"model": "v0-model"}}
result = validate_agent_settings(v0_payload)Result: Validates successfully with agent_kind == "openhands" (default discriminator).
Test 5: Agent-server persistence uses validate_agent_settings()
Verification: Inspected openhands.agent_server.persistence.models.PersistedSettings.update() source code:
- ✅ Imports
validate_agent_settingsfromopenhands.sdk.settings - ✅ Line 139 calls
validate_agent_settings(agent_merged)for agent settings updates - ✅ No references to deprecated
AgentSettings.from_persisted()remain in the code
The persistence layer has been successfully updated to use the new canonical loader.
Test 6: validate_agent_settings() accepts model instances
Test: Verified that validate_agent_settings() is idempotent—passing an already-validated instance returns it unchanged:
instance = OpenHandsAgentSettings(llm=LLM(model="test-model"))
result = validate_agent_settings(instance)
assert result is instanceThis allows the function to be used safely in code paths that may receive either raw payloads or already-validated instances.
Integration Test 1: Create agent from legacy settings
Test: End-to-end scenario simulating loading persisted legacy settings and creating an agent:
legacy_payload = {
"schema_version": 1,
"agent_kind": "llm",
"llm": {"model": "anthropic/claude-sonnet-4-5-20250929"},
"tools": []
}
settings = validate_agent_settings(legacy_payload)
agent = settings.create_agent()Result:
- ✅ Settings loaded with
agent_kind="openhands"(migrated) - ✅ Agent created successfully (type:
Agent) - ✅ Agent configured with specified model
Integration Test 2: default_agent_settings() helper
Test: Verified the replacement for the old AgentSettings() constructor:
settings = default_agent_settings()Result: Returns OpenHandsAgentSettings instance with agent_kind="openhands" and default model.
Integration Test 3: AgentSettingsConfig union
Test: Verified the discriminated union behavior:
- ✅ Accepts
"openhands"discriminator →OpenHandsAgentSettings - ✅ Accepts
"acp"discriminator →ACPAgentSettings - ✅ Rejects
"llm"discriminator withValidationError(expected—migration only happens viavalidate_agent_settings())
Automated Test Suite
Command: uv run pytest tests/sdk/test_settings.py -k "validate_agent_settings or from_persisted"
Result: All 9 test cases passing:
test_validate_agent_settings_defaults_to_openhands_when_discriminator_missing✅test_validate_agent_settings_dispatches_on_agent_kind✅test_validate_agent_settings_migrates_v0_llm_payload✅test_validate_agent_settings_dispatches_current_acp_payload✅test_validate_agent_settings_canonicalizes_legacy_llm_kind✅test_validate_agent_settings_drops_legacy_verification_fields✅test_validate_agent_settings_rejects_newer_schema_version✅test_conversation_settings_from_persisted_migrates_v0_payload✅test_conversation_settings_from_persisted_rejects_newer_schema_version✅
Deprecation Check
Command: uv run --with packaging python .github/scripts/check_deprecations.py
Result: ✅ Checked 9 deprecation metadata entries across 4 packages—all valid.
Issues Found
None.
neubig
force-pushed
the
remove-legacy-agent-settings
branch
from
687bd10 to
b977b29
Compare
neubig
changed the title
|
Addressed the policy feedback by splitting the removal and keeping LLMAgentSettings until v1.24.0; also fixed the compatibility test import in 7f04cd7. Ready for another look. This comment was created by an AI agent (OpenHands) on behalf of the user. |
neubig
added
the
review-this
OpenHands AI
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟢 Good taste
Clean deprecation removal that follows the repo's 5-release policy (1.17.0 → 1.22.0). Migration logic properly preserved in validate_agent_settings(), tests comprehensive, agent-server updated correctly.
[RISK ASSESSMENT]
Breaking API change removing a long-deprecated public class. Risk factors:
- ✅ Migration path preserved via
validate_agent_settings() - ✅ 5-release deprecation runway satisfied
- ✅ Tests cover legacy payload migration (including
agent_kind: "llm"canonicalization) - ✅ Agent-server persistence correctly updated
⚠️ External code that ignored deprecation warnings will break
VERDICT: ✅ Worth merging
KEY INSIGHT: The refactor elevates validate_agent_settings() from a wrapper to the canonical migration-aware loader, simplifying the API surface while maintaining full backward compatibility through migrations.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
All PR goals achieved. The deprecated AgentSettings alias has been successfully removed while maintaining backward compatibility for persisted settings and keeping LLMAgentSettings available through v1.24.0.
Does this PR achieve its stated goal?
Yes. The PR set out to remove the deprecated AgentSettings public alias while preserving migration support for persisted settings. All four stated goals were verified:
- ✅
AgentSettingsis no longer importable fromopenhands.sdk - ✅
LLMAgentSettingsremains available with proper deprecation warnings (scheduled for v1.24.0 removal) - ✅
validate_agent_settings()correctly handles migration from legacy payloads - ✅ Agent-server persistence layer successfully uses
validate_agent_settings()instead of the removedAgentSettings.from_persisted()
Additionally verified that core agent runtime functionality remains intact with no breaking changes.
| Phase | Result |
|---|---|
| Environment Setup | ✅ Built successfully with make build |
| CI Status | ✅ All core tests passing (sdk-tests, agent-server-tests, tools-tests, workspace-tests, cross-tests, windows-tests, pre-commit) |
| Functional Verification | ✅ All manual tests passed |
Functional Verification
Test 1: Verify AgentSettings is no longer importable
Attempt to import the removed alias:
from openhands.sdk import AgentSettingsResult:
ImportError: cannot import name 'AgentSettings' from 'openhands.sdk'
✅ Confirmed: The deprecated AgentSettings alias has been successfully removed from the public API.
Test 2: Verify LLMAgentSettings still exists with deprecation warning
Import the still-supported alias:
import warnings
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
from openhands.sdk.settings import LLMAgentSettingsResult:
DeprecationWarning: Importing 'LLMAgentSettings' from openhands.sdk.settings is deprecated as of 1.19.0 and will be removed in 1.24.0. Use OpenHandsAgentSettings directly. LLMAgentSettings was renamed in v1.19.0.
✅ Confirmed: LLMAgentSettings remains available with proper deprecation notice through v1.24.0.
Test 3: Verify validate_agent_settings handles migration
Test 3a - Legacy v0 payload (no schema_version):
from openhands.sdk.settings import validate_agent_settings
legacy_v0 = {"llm": {"model": "test-model"}}
settings = validate_agent_settings(legacy_v0)
print(f"Model: {settings.llm.model}")
print(f"Schema version: {settings.schema_version}")
print(f"Agent kind: {settings.agent_kind}")Result:
Model: test-model
Schema version: 3
Agent kind: openhands
✅ Confirmed: Legacy v0 payloads are correctly migrated to current schema version.
Test 3b - Legacy 'llm' agent_kind (should canonicalize to 'openhands'):
legacy_llm_kind = {
"schema_version": 1,
"agent_kind": "llm",
"llm": {"model": "legacy-model"}
}
settings = validate_agent_settings(legacy_llm_kind)
print(f"Agent kind: {settings.agent_kind}")
print(f"Model: {settings.llm.model}")Result:
Agent kind: openhands
Model: legacy-model
✅ Confirmed: Legacy agent_kind: "llm" payloads are migrated to the canonical "openhands" discriminator.
Test 3c - Current OpenHandsAgentSettings payload:
current_payload = {
"schema_version": 3,
"agent_kind": "openhands",
"llm": {"model": "current-model"}
}
settings = validate_agent_settings(current_payload)
print(f"Agent kind: {settings.agent_kind}")
print(f"Model: {settings.llm.model}")Result:
Agent kind: openhands
Model: current-model
✅ Confirmed: Current payloads validate successfully without modification.
Test 4: Verify core agent creation still works
Create an agent using OpenHandsAgentSettings:
from openhands.sdk import LLM, Tool
from openhands.sdk.settings import OpenHandsAgentSettings
from openhands.tools.terminal import TerminalTool
llm = LLM(
model="anthropic/claude-3-5-sonnet-20241022",
api_key="dummy-key-for-testing"
)
settings = OpenHandsAgentSettings(
llm=llm,
tools=[Tool(name=TerminalTool.name)]
)
agent = settings.create_agent()
print(f"Agent class: {agent.__class__.__name__}")
print(f"LLM model: {agent.llm.model}")
print(f"Tools: {[tool.name for tool in agent.tools]}")Result:
Agent class: Agent
LLM model: anthropic/claude-3-5-sonnet-20241022
Tools: ['terminal']
✅ Confirmed: Core agent creation works without breaking changes.
Test settings serialization round-trip:
settings_dict = settings.model_dump()
restored_settings = validate_agent_settings(settings_dict)
print(f"Original agent kind: {settings.agent_kind}")
print(f"Restored agent kind: {restored_settings.agent_kind}")
print(f"Models match: {restored_settings.llm.model == settings.llm.model}")Result:
Original agent kind: openhands
Restored agent kind: openhands
Models match: True
✅ Confirmed: Settings serialization and deserialization round-trip successfully.
Test 5: Verify agent-server persistence logic
Test PersistedSettings.update with legacy payload:
from openhands.agent_server.persistence.models import PersistedSettings
settings = PersistedSettings()
update_payload = {
"agent_settings_diff": {
"schema_version": 1,
"agent_kind": "llm",
"llm": {"model": "updated-model"}
}
}
settings.update(update_payload)
print(f"Agent kind: {settings.agent_settings.agent_kind}")
print(f"Model: {settings.agent_settings.llm.model}")Result:
Agent kind: openhands
Model: updated-model
✅ Confirmed: The agent-server persistence layer correctly uses validate_agent_settings() to handle legacy payloads. The key change in persistence/models.py line 139 works as intended.
Issues Found
None.
Summary: This PR successfully removes the deprecated AgentSettings alias as planned, migrates all internal usage to validate_agent_settings(), and maintains full backward compatibility for persisted settings. All goals achieved with no breaking changes.
neubig
force-pushed
the
remove-legacy-agent-settings
branch
from
7f04cd7 to
f18f5b7
Compare
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
neubig
changed the title
Co-authored-by: openhands <openhands@all-hands.dev>
neubig
force-pushed
the
remove-legacy-agent-settings
branch
from
6a766ec to
455fdd6
Compare
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟢 Good taste
Clean refactoring that consolidates migration logic into validate_agent_settings() as the canonical entry point. The deprecated AgentSettings.from_persisted() still works via delegation, preserving backward compatibility while simplifying the codebase.
Key improvements:
- Migration logic centralized in one place (
validate_agent_settings) - Agent-server persistence paths now use the canonical helper
- Tests properly updated to focus on the canonical entry point
- The semantic change (validate_agent_settings now always applies migrations) improves robustness by handling legacy payloads automatically
[RISK ASSESSMENT]
Internal refactoring with no behavior changes. Migration logic moved but functionality identical. Tests comprehensive, backward compatibility preserved via delegation. No eval risk - this is purely internal reorganization.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Successfully verified that the refactoring consolidates migration logic into validate_agent_settings() while preserving backward compatibility through the deprecated AgentSettings.from_persisted() alias.
Does this PR achieve its stated goal?
Yes. The PR successfully moves persisted settings migration logic from AgentSettings.from_persisted() into validate_agent_settings(), making it the canonical entry point for all agent settings validation and migration. The deprecated AgentSettings.from_persisted() now simply delegates to validate_agent_settings(), maintaining backward compatibility until the scheduled v1.23.0 removal. Both agent-server persistence paths (PersistedSettings.update() and PersistedSettings._normalize_inputs()) now correctly use validate_agent_settings() for migration and validation.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully |
| CI Status | ✅ 22 checks passed, 7 pending (Docker builds), 0 failing |
| Functional Verification | ✅ All 10 functional tests passed |
Functional Verification
Verification Strategy
This PR is a refactoring that moves migration logic from one location to another while maintaining identical behavior. To verify correctness, I:
- Established the baseline: Confirmed the refactoring goal from the PR description
- Exercised the new code path: Tested
validate_agent_settings()directly with various payloads requiring migration - Verified backward compatibility: Tested
AgentSettings.from_persisted()still works and produces identical results - Tested integration points: Verified agent-server persistence models use the new function correctly
Test 1: Core Migration Functionality (6 tests)
Ran comprehensive tests of validate_agent_settings() migration behavior:
$ uv run python test_pr_validation.pyResults:
- ✅ v0 payloads (no schema_version, no agent_kind) migrate to current schema (v3, agent_kind='openhands')
- ✅ Legacy
agent_kind='llm'payloads canonicalize toagent_kind='openhands' - ✅
AgentSettings.from_persisted()still works (backward compatibility) - ✅ Both methods produce identical results for all payload types
- ✅ ACP agent settings validate correctly through discriminated union
- ✅ Already-validated instances pass through as no-op
Key observation: The migration logic now lives in validate_agent_settings() (lines 1327-1332 in model.py), and AgentSettings.from_persisted() simply delegates to it (line 1375).
Test 2: Persistence Layer Integration (4 tests)
Verified agent-server persistence models use the refactored function:
$ uv run python test_persistence_integration.pyResults:
- ✅
PersistedSettings.update()correctly usesvalidate_agent_settings()for merging and migrating partial updates - ✅
PersistedSettings._normalize_inputs()applies migrations during deserialization viavalidate_agent_settings() - ✅ ACP agent settings deserialize correctly through the persistence layer
- ✅
PersistedSettings.from_persisted()applies nested migrations (both top-level and agent settings migrations)
Key observation: Both code paths in the agent-server that previously called AgentSettings.from_persisted() now call validate_agent_settings() directly:
- Line 142:
new_agent = validate_agent_settings(agent_merged) - Line 232:
data["agent_settings"] = validate_agent_settings(coerced, context=info.context)
Before/After Evidence
Before (pre-refactoring behavior):
- Migration logic lived in
AgentSettings.from_persisted() validate_agent_settings()only validated, did not migrate- Agent-server called
AgentSettings.from_persisted()for migrations
After (current PR):
- Migration logic lives in
validate_agent_settings()(consolidated) AgentSettings.from_persisted()delegates tovalidate_agent_settings()- Agent-server calls
validate_agent_settings()directly - Result: Same migrations applied, identical output, simpler call graph
Example migration flow:
# Input: v0 payload (no schema_version)
{"llm": {"model": "test-model"}}
# After validate_agent_settings():
OpenHandsAgentSettings(
schema_version=3, # migrated
agent_kind="openhands", # migrated
llm=LLM(model="test-model")
)Test Coverage Summary
✅ 10 functional tests passed:
- 6 tests for core
validate_agent_settings()behavior - 4 tests for agent-server persistence integration
All test scenarios:
- v0 → v3 migration (no schema_version)
- v1 → v3 migration (legacy agent_kind='llm')
- Current v3 payloads (no migration needed)
- ACP agent settings discriminated union
- Already-validated instance pass-through
- Backward compatibility via
AgentSettings.from_persisted() - Equivalence of both methods
PersistedSettings.update()merge and migratePersistedSettings._normalize_inputs()during deserialization- Nested migrations in
PersistedSettings.from_persisted()
Issues Found
None.
3 participants
Summary
AgentSettingspublic export while main remains at v1.22.x, so the API breakage policy stays satisfied until the scheduled removal releasevalidate_agent_settings()and canonical helpers in agent-server persistence pathsvalidate_agent_settings()apply persisted settings migrations directly while preserving validation context for encrypted secretsvalidate_agent_settings()entry pointThe actual
AgentSettingspublic export removal must wait for a version bump to v1.23.0 or later, because its scheduled removal is v1.23.0.Testing
uv run pytest tests/sdk/test_settings.py -k 'agent_settings or llm_agent_settings or validate_agent_settings or acp_agent_settings_acp_env_encrypts_with_cipher' -quv run pre-commit run --files openhands-agent-server/openhands/agent_server/persistence/models.py openhands-sdk/openhands/sdk/settings/model.py tests/sdk/test_settings.pyThis PR was updated by an AI agent (OpenHands) on behalf of the user.
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:455fdd6-pythonRun
All tags pushed for this build
About Multi-Architecture Support
455fdd6-python) is a multi-arch manifest supporting both amd64 and arm64455fdd6-python-amd64) are also available if needed