From a1089dbae524b47b2aba17608d911ffc223dfeff Mon Sep 17 00:00:00 2001 From: Graham Neubig <398875+neubig@users.noreply.github.com> Date: Fri, 5 Jun 2026 09:39:55 -0400 Subject: [PATCH 1/5] Harden settings persistence validation --- .../check_settings_schema_migrations.py | 233 ++++++++++++++++++ .github/workflows/api-breakage.yml | 4 + .../agent_server/persistence/models.py | 6 +- .../agent_server/persistence/store.py | 8 + tests/agent_server/test_settings_router.py | 57 ++++- 5 files changed, 306 insertions(+), 2 deletions(-) create mode 100644 .github/scripts/check_settings_schema_migrations.py diff --git a/.github/scripts/check_settings_schema_migrations.py b/.github/scripts/check_settings_schema_migrations.py new file mode 100644 index 0000000000..a8919bfe7c --- /dev/null +++ b/.github/scripts/check_settings_schema_migrations.py @@ -0,0 +1,233 @@ +#!/usr/bin/env python3 +"""Require SDK settings migrations for incompatible persisted schema changes.""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +import tempfile +from pathlib import Path +from typing import Any + + +REPO_ROOT = Path(__file__).resolve().parents[2] + +SCHEMA_PROGRAM = r""" +import json +import sys +from pathlib import Path + +source_tree = Path(sys.argv[1]) +sys.path = [str(source_tree / path) for path in ( + "openhands-sdk", + "openhands-tools", + "openhands-workspace", + "openhands-agent-server", +)] + sys.path + +from openhands.sdk.settings import ConversationSettings, export_agent_settings_schema +import openhands.sdk.settings.model as settings_model + +print(json.dumps({ + "agent_schema": export_agent_settings_schema().model_dump(mode="json"), + "conversation_schema": ConversationSettings.export_schema().model_dump(mode="json"), + "agent_version": settings_model.AGENT_SETTINGS_SCHEMA_VERSION, + "conversation_version": settings_model.CONVERSATION_SETTINGS_SCHEMA_VERSION, + "agent_migrations": sorted(settings_model._AGENT_SETTINGS_MIGRATIONS), + "conversation_migrations": sorted(settings_model._CONVERSATION_SETTINGS_MIGRATIONS), +})) +""" + + +def _run(command: list[str], *, cwd: Path = REPO_ROOT) -> subprocess.CompletedProcess: + return subprocess.run(command, cwd=cwd, capture_output=True, text=True) + + +def _candidate_refs(ref: str) -> list[str]: + candidates = [ref] + if not ref.startswith("origin/"): + candidates.insert(0, f"origin/{ref}") + return list(dict.fromkeys(candidates)) + + +def _archive_ref(ref: str) -> Path | None: + for candidate in _candidate_refs(ref): + archive = subprocess.run( + ["git", "archive", candidate], + cwd=REPO_ROOT, + capture_output=True, + ) + if archive.returncode != 0: + continue + + source_tree = Path(tempfile.mkdtemp(prefix="settings-schema-base-src-")) + extract = subprocess.run( + ["tar", "-x", "-C", str(source_tree)], + input=archive.stdout, + capture_output=True, + ) + if extract.returncode == 0: + return source_tree + return None + + +def _capture(source_tree: Path) -> dict[str, Any]: + result = _run([sys.executable, "-c", SCHEMA_PROGRAM, str(source_tree)]) + if result.returncode != 0: + raise RuntimeError(result.stderr[-2000:] or result.stdout[-2000:]) + return json.loads(result.stdout) + + +def _flatten_fields(schema: dict[str, Any]) -> dict[str, dict[str, Any]]: + fields: dict[str, dict[str, Any]] = {} + for section in schema.get("sections", []): + section_variant = section.get("variant") + for field in section.get("fields", []): + variant = field.get("variant") or section_variant or "*" + field_id = f"{variant}:{field.get('key')}" + fields[field_id] = field + return fields + + +def _choice_values(field: dict[str, Any]) -> set[Any]: + return {choice.get("value") for choice in field.get("choices", [])} + + +def _incompatible_changes( + old_schema: dict[str, Any], + new_schema: dict[str, Any], +) -> list[str]: + old_fields = _flatten_fields(old_schema) + new_fields = _flatten_fields(new_schema) + + changes: list[str] = [] + for field_id, old_field in sorted(old_fields.items()): + new_field = new_fields.get(field_id) + if new_field is None: + changes.append(f"removed field {field_id}") + continue + + if old_field.get("value_type") != new_field.get("value_type"): + changes.append( + "changed field type " + f"{field_id}: {old_field.get('value_type')} -> " + f"{new_field.get('value_type')}" + ) + + removed_choices = _choice_values(old_field) - _choice_values(new_field) + if removed_choices: + changes.append( + f"removed choices from {field_id}: {sorted(removed_choices)!r}" + ) + + return changes + + +def _check_model( + *, + name: str, + old_info: dict[str, Any], + new_info: dict[str, Any], + schema_key: str, + version_key: str, + migrations_key: str, +) -> list[str]: + errors: list[str] = [] + changes = _incompatible_changes(old_info[schema_key], new_info[schema_key]) + old_version = int(old_info[version_key]) + new_version = int(new_info[version_key]) + migrations = set(new_info[migrations_key]) + + if new_version < old_version: + errors.append( + f"{name} schema version went backwards: {old_version} -> {new_version}" + ) + + if not changes: + return errors + + print(f"{name} incompatible settings schema changes:") + for change in changes: + print(f" - {change}") + + if new_version <= old_version: + errors.append( + f"{name} has incompatible settings schema changes but " + f"{version_key} did not increase ({old_version} -> {new_version})." + ) + return errors + + missing = [ + version + for version in range(old_version, new_version) + if version not in migrations + ] + if missing: + errors.append( + f"{name} schema version increased ({old_version} -> {new_version}) " + f"but migrations are missing for version(s): {missing}." + ) + + return errors + + +def _base_ref() -> str: + return ( + os.environ.get("SETTINGS_SCHEMA_BASE_REF") + or os.environ.get("GITHUB_BASE_REF") + or "origin/main" + ) + + +def main() -> int: + base_ref = _base_ref() + base_tree = _archive_ref(base_ref) + if base_tree is None: + print( + f"::warning title=Settings schema::Unable to read base ref {base_ref}; " + "skipping settings schema migration check." + ) + return 0 + + try: + old_info = _capture(base_tree) + new_info = _capture(REPO_ROOT) + except Exception as exc: + print(f"::error title=Settings schema::Failed to capture schemas: {exc}") + return 1 + + errors: list[str] = [] + errors.extend( + _check_model( + name="AgentSettings", + old_info=old_info, + new_info=new_info, + schema_key="agent_schema", + version_key="agent_version", + migrations_key="agent_migrations", + ) + ) + errors.extend( + _check_model( + name="ConversationSettings", + old_info=old_info, + new_info=new_info, + schema_key="conversation_schema", + version_key="conversation_version", + migrations_key="conversation_migrations", + ) + ) + + if errors: + for error in errors: + print(f"::error title=Settings schema::{error}") + return 1 + + print("Settings schema migration check passed.") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/workflows/api-breakage.yml b/.github/workflows/api-breakage.yml index 4350342654..ee8fd05743 100644 --- a/.github/workflows/api-breakage.yml +++ b/.github/workflows/api-breakage.yml @@ -25,6 +25,10 @@ jobs: enable-cache: true - name: Install workspace deps (dev) run: uv sync --frozen --group dev + - name: Run settings schema migration check + env: + SETTINGS_SCHEMA_BASE_REF: ${{ github.event_name == 'pull_request' && github.base_ref || github.event.before }} + run: uv run python .github/scripts/check_settings_schema_migrations.py - name: Run Python API breakage check id: api_breakage # Let this step fail so CI is visibly red on breakage. diff --git a/openhands-agent-server/openhands/agent_server/persistence/models.py b/openhands-agent-server/openhands/agent_server/persistence/models.py index 26c4f3ae42..ae4acf58f1 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/models.py +++ b/openhands-agent-server/openhands/agent_server/persistence/models.py @@ -125,7 +125,11 @@ class PersistedSettings(BaseModel): description="Name of the currently active LLM profile.", ) - model_config = ConfigDict(populate_by_name=True) + model_config = ConfigDict( + populate_by_name=True, + validate_assignment=True, + revalidate_instances="always", + ) @property def llm_api_key_is_set(self) -> bool: diff --git a/openhands-agent-server/openhands/agent_server/persistence/store.py b/openhands-agent-server/openhands/agent_server/persistence/store.py index 673d81c01a..c54ef5c347 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/store.py +++ b/openhands-agent-server/openhands/agent_server/persistence/store.py @@ -360,6 +360,14 @@ def save(self, settings: PersistedSettings) -> None: If no cipher is provided, secrets are stored in plaintext. This is logged as a security warning on first save. """ + if not isinstance(settings, PersistedSettings): + raise TypeError("FileSettingsStore.save() requires PersistedSettings") + + # Revalidate at the persistence boundary. Pydantic assignment or + # model_construct() can bypass normal field validation; the store must + # never serialize that malformed state to disk. + settings = PersistedSettings.model_validate(settings) + _ensure_secure_directory(self.persistence_dir) # Pass cipher in context for automatic encryption of all secret fields diff --git a/tests/agent_server/test_settings_router.py b/tests/agent_server/test_settings_router.py index 01e82c9ecf..37a94bb788 100644 --- a/tests/agent_server/test_settings_router.py +++ b/tests/agent_server/test_settings_router.py @@ -6,7 +6,7 @@ import pytest from fastapi.testclient import TestClient -from pydantic import SecretStr +from pydantic import SecretStr, ValidationError from openhands.agent_server.api import create_app from openhands.agent_server.config import Config @@ -22,6 +22,7 @@ AGENT_SETTINGS_SCHEMA_VERSION, CONVERSATION_SETTINGS_SCHEMA_VERSION, ACPAgentSettings, + ConversationSettings, OpenHandsAgentSettings, ) from openhands.sdk.utils.cipher import Cipher @@ -1009,6 +1010,60 @@ def test_patch_settings_validation_error_does_not_leak_secrets(client_with_setti assert error_detail == "Settings validation failed" +def test_patch_settings_validation_error_does_not_persist_partial_update( + client_with_settings, temp_persistence_dir +): + """A mixed valid/invalid PATCH must not write any part of the payload.""" + response = client_with_settings.patch( + "/api/settings", + json={"agent_settings_diff": {"llm": {"model": "stable-model"}}}, + ) + assert response.status_code == 200, response.text + + response = client_with_settings.patch( + "/api/settings", + json={ + "agent_settings_diff": { + "llm": { + "model": "should-not-persist", + "api_key": "sk-should-not-persist", + } + }, + "conversation_settings_diff": {"max_iterations": -1}, + }, + ) + + assert response.status_code == 422 + settings_text = (temp_persistence_dir / "settings.json").read_text() + assert "should-not-persist" not in settings_text + assert "sk-should-not-persist" not in settings_text + + response = client_with_settings.get( + "/api/settings", headers={"X-Expose-Secrets": "plaintext"} + ) + assert response.status_code == 200 + body = response.json() + assert body["agent_settings"]["llm"]["model"] == "stable-model" + assert body["conversation_settings"]["max_iterations"] == 500 + + +def test_file_settings_store_save_rejects_malformed_constructed_settings( + temp_persistence_dir, +): + """The store must reject malformed settings even if callers bypass Pydantic.""" + store = FileSettingsStore(persistence_dir=temp_persistence_dir, cipher=None) + malformed = PersistedSettings.model_construct( + schema_version=PERSISTED_SETTINGS_SCHEMA_VERSION, + agent_settings={"schema_version": 999, "agent_kind": "bogus"}, + conversation_settings=ConversationSettings(), + ) + + with pytest.raises(ValidationError): + store.save(malformed) + + assert not (temp_persistence_dir / "settings.json").exists() + + def test_secret_upsert_updates_existing(client_with_settings): """PUT /api/settings/secrets updates existing secret (upsert behavior).""" # Create initial secret From ec91f3354b7804dfbaf19933e8ce63e9c2875108 Mon Sep 17 00:00:00 2001 From: openhands Date: Sat, 6 Jun 2026 02:30:25 +0000 Subject: [PATCH 2/5] fix: revalidate nested sub-models at the persistence boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QA on PR #3529 found that calling `PersistedSettings.model_validate(settings)` on an outer instance does NOT revalidate nested sub-models that were built via `model_construct()` — Pydantic v2 treats already-instantiated sub-model instances as opaque on that pass. As a result a malformed `ConversationSettings.model_construct(max_iterations=-1)` would slip through `FileSettingsStore.save()` and end up on disk with an invalid value. Add a second validation step that round-trips through `model_dump()` so the nested sub-models are revalidated against their field constraints. The dump uses `expose_secrets=plaintext` context so SecretStr fields are not redacted during the round-trip; the real encrypt-or-redact dump still happens below for the on-disk write. Add a regression test that constructs a nested `max_iterations=-1` and asserts `store.save()` raises `ValidationError` and never writes `settings.json`. Co-authored-by: openhands --- .../agent_server/persistence/store.py | 19 +++++++++++++ tests/agent_server/test_settings_router.py | 27 +++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/openhands-agent-server/openhands/agent_server/persistence/store.py b/openhands-agent-server/openhands/agent_server/persistence/store.py index c54ef5c347..436b06c058 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/store.py +++ b/openhands-agent-server/openhands/agent_server/persistence/store.py @@ -366,7 +366,26 @@ def save(self, settings: PersistedSettings) -> None: # Revalidate at the persistence boundary. Pydantic assignment or # model_construct() can bypass normal field validation; the store must # never serialize that malformed state to disk. + # + # We validate in two steps because each step catches a different class + # of bypass: + # 1. ``model_validate(settings)`` rejects raw-dict payloads sitting in + # slots typed as a sub-model (e.g. ``model_construct`` was called + # with ``agent_settings={"agent_kind": "bogus"}``). It does *not* + # revalidate already-instantiated sub-model instances. + # 2. ``model_validate(model_dump(...))`` then revalidates nested + # sub-models that *were* constructed via ``model_construct()`` — + # Pydantic v2 treats them as opaque on the first pass, so a + # ``ConversationSettings.model_construct(max_iterations=-1)`` would + # slip through step 1 alone. + # + # The dump must use ``expose_secrets="plaintext"`` so SecretStr fields + # round-trip without being redacted/encrypted by ``serialize_secret``; + # the real encrypt-or-redact dump happens below for the actual write. settings = PersistedSettings.model_validate(settings) + settings = PersistedSettings.model_validate( + settings.model_dump(context={"expose_secrets": "plaintext"}) + ) _ensure_secure_directory(self.persistence_dir) diff --git a/tests/agent_server/test_settings_router.py b/tests/agent_server/test_settings_router.py index 37a94bb788..678c3757c2 100644 --- a/tests/agent_server/test_settings_router.py +++ b/tests/agent_server/test_settings_router.py @@ -1064,6 +1064,33 @@ def test_file_settings_store_save_rejects_malformed_constructed_settings( assert not (temp_persistence_dir / "settings.json").exists() +def test_file_settings_store_save_rejects_nested_model_construct_bypass( + temp_persistence_dir, +): + """The store must reject settings whose *nested* sub-models bypassed Pydantic. + + Pydantic v2 does not re-validate already-instantiated sub-model instances + when ``model_validate(instance)`` is called on the outer model. That means a + ``ConversationSettings.model_construct(max_iterations=-1)`` would slip + through a naive ``PersistedSettings.model_validate(settings)`` boundary + check and reach disk. The store must dump-and-revalidate so nested invalid + state is caught. + """ + store = FileSettingsStore(persistence_dir=temp_persistence_dir, cipher=None) + bad_conv = ConversationSettings.model_construct(max_iterations=-1) + assert bad_conv.max_iterations == -1 # model_construct bypassed validation + malformed = PersistedSettings.model_construct( + schema_version=PERSISTED_SETTINGS_SCHEMA_VERSION, + agent_settings=OpenHandsAgentSettings(), + conversation_settings=bad_conv, + ) + + with pytest.raises(ValidationError): + store.save(malformed) + + assert not (temp_persistence_dir / "settings.json").exists() + + def test_secret_upsert_updates_existing(client_with_settings): """PUT /api/settings/secrets updates existing secret (upsert behavior).""" # Create initial secret From cc085d95193c00b4c252b9c8f5b013f227095fd0 Mon Sep 17 00:00:00 2001 From: Graham Neubig <398875+neubig@users.noreply.github.com> Date: Tue, 9 Jun 2026 17:37:53 -0400 Subject: [PATCH 3/5] fix: reject malformed ACP settings switch --- .../agent_server/persistence/models.py | 37 ++----------------- openhands-sdk/openhands/sdk/settings/model.py | 4 ++ tests/agent_server/test_settings_router.py | 30 +++++++++++++++ tests/sdk/test_apply_agent_settings_diff.py | 23 ++++++++++-- 4 files changed, 58 insertions(+), 36 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/persistence/models.py b/openhands-agent-server/openhands/agent_server/persistence/models.py index e9cd3b6a51..1bfbabdb6e 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/models.py +++ b/openhands-agent-server/openhands/agent_server/persistence/models.py @@ -25,6 +25,7 @@ from openhands.sdk.settings import ( AgentSettingsConfig, ConversationSettings, + apply_agent_settings_diff, default_agent_settings, validate_agent_settings, ) @@ -201,42 +202,14 @@ def update(self, payload: SettingsUpdatePayload) -> None: # Phase 1: Validate all updates before any mutations new_agent: AgentSettingsConfig | None = None new_conv: ConversationSettings | None = None - agent_merged: dict | None = None conv_merged: dict | None = None try: if isinstance(agent_update, dict): - # Check if this is a variant (agent_kind) switch - old_kind = self.agent_settings.agent_kind - new_kind = agent_update.get("agent_kind") - is_kind_switch = new_kind is not None and new_kind != old_kind - - if is_kind_switch: - # Variant replacement: validate the diff as-is rather than - # deep-merging it onto the old variant. A kind switch picks a - # different member of the AgentSettingsConfig union, and the - # old variant's serialized fields are not a valid base for the - # new one (e.g. ACP's acp_command has no place in - # OpenHandsAgentSettings and would fail validation). - # - # Consequence (intentional): fields the two variants happen to - # share (e.g. ``llm``) are NOT carried over — they fall back to - # the new variant's defaults unless the caller restates them in - # this same diff. Switching kinds is a fresh start on the new - # variant, mirroring the frontend's "fresh base on kind switch" - # behaviour. Callers that want to preserve a shared field must - # include it in the switch payload. - agent_merged = agent_update - else: - # Same-kind update: deep-merge for incremental field edits - agent_merged = _deep_merge( - self.agent_settings.model_dump( - mode="json", context={"expose_secrets": "plaintext"} - ), - agent_update, - ) try: - new_agent = validate_agent_settings(agent_merged) + new_agent = apply_agent_settings_diff( + self.agent_settings, agent_update + ) except Exception as e: # Use 'from None' to break exception chain - the original # exception may contain secret values in Pydantic errors @@ -280,8 +253,6 @@ def update(self, payload: SettingsUpdatePayload) -> None: self.active_profile = payload["active_profile"] finally: # Clear merged dicts to minimize plaintext exposure window - if agent_merged is not None: - agent_merged.clear() if conv_merged is not None: conv_merged.clear() diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index cc33b26fb6..5b494ff923 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -1845,6 +1845,10 @@ def apply_agent_settings_diff( return base_settings new_kind = diff.get("agent_kind") if new_kind and new_kind != base_settings.agent_kind: + if new_kind == "acp" and diff.get("agent_context") is None: + raise ValueError( + "agent_context is required when switching agent_kind to acp" + ) merged = _merge_patch({"agent_kind": new_kind}, diff) else: merged = _merge_patch( diff --git a/tests/agent_server/test_settings_router.py b/tests/agent_server/test_settings_router.py index 2882e05b48..df452cb9b3 100644 --- a/tests/agent_server/test_settings_router.py +++ b/tests/agent_server/test_settings_router.py @@ -1016,6 +1016,7 @@ def test_patch_settings_switch_agent_kind_from_openhands_to_acp(client_with_sett json={ "agent_settings_diff": { "agent_kind": "acp", + "agent_context": {}, "acp_command": ["echo", "hello"], } }, @@ -1026,6 +1027,35 @@ def test_patch_settings_switch_agent_kind_from_openhands_to_acp(client_with_sett body = response.json() assert body["agent_settings"]["agent_kind"] == "acp" assert body["agent_settings"]["acp_command"] == ["echo", "hello"] + assert body["agent_settings"]["agent_context"] is not None + + +@pytest.mark.parametrize( + "agent_settings_diff", + [ + {"agent_kind": "acp"}, + {"agent_kind": "acp", "agent_context": None}, + ], +) +def test_patch_settings_switch_to_acp_requires_agent_context( + client_with_settings, temp_persistence_dir, agent_settings_diff +): + """PATCH /api/settings rejects malformed ACP switches before persistence.""" + response = client_with_settings.patch( + "/api/settings", + json={"agent_settings_diff": {"llm": {"model": "stable-model"}}}, + ) + assert response.status_code == 200, response.text + + response = client_with_settings.patch( + "/api/settings", + json={"agent_settings_diff": agent_settings_diff}, + ) + + assert response.status_code == 422 + persisted = json.loads((temp_persistence_dir / "settings.json").read_text()) + assert persisted["agent_settings"]["agent_kind"] == "openhands" + assert persisted["agent_settings"]["llm"]["model"] == "stable-model" def test_patch_settings_same_kind_restated_still_deep_merges(client_with_settings): diff --git a/tests/sdk/test_apply_agent_settings_diff.py b/tests/sdk/test_apply_agent_settings_diff.py index 2841348d49..c39884ce25 100644 --- a/tests/sdk/test_apply_agent_settings_diff.py +++ b/tests/sdk/test_apply_agent_settings_diff.py @@ -6,6 +6,7 @@ this in several places; this exercises the single SDK owner. """ +import pytest from pydantic import SecretStr from openhands.sdk import apply_agent_settings_diff, validate_agent_settings @@ -24,13 +25,29 @@ def test_switch_openhands_to_acp_replaces_with_fresh_variant() -> None: base = {"agent_kind": "openhands", "llm": {"model": "gpt"}} result = apply_agent_settings_diff( - base, {"agent_kind": "acp", "acp_server": "claude-code"} + base, + { + "agent_kind": "acp", + "agent_context": {}, + "acp_server": "claude-code", + }, ) assert isinstance(result, ACPAgentSettings) assert result.acp_server == "claude-code" - # ACP's agent_context is nullable; the openhands llm.model is not carried. - assert result.agent_context is None + # The OpenHands llm.model is not carried across the variant boundary. + assert result.agent_context is not None + + +@pytest.mark.parametrize( + "diff", + [{"agent_kind": "acp"}, {"agent_kind": "acp", "agent_context": None}], +) +def test_switch_openhands_to_acp_requires_agent_context(diff) -> None: + base = {"agent_kind": "openhands", "llm": {"model": "gpt"}} + + with pytest.raises(ValueError, match="agent_context is required"): + apply_agent_settings_diff(base, diff) def test_switch_acp_to_openhands_replaces_with_fresh_variant() -> None: From 12ae6d3beb0f7561eda4fb20f8f7a4a9ef2936c9 Mon Sep 17 00:00:00 2001 From: Graham Neubig <398875+neubig@users.noreply.github.com> Date: Tue, 9 Jun 2026 17:43:12 -0400 Subject: [PATCH 4/5] fix: preserve ACP merge validation semantics --- openhands-sdk/openhands/sdk/settings/model.py | 17 +++++++++---- tests/agent_server/test_settings_router.py | 1 + tests/sdk/test_apply_agent_settings_diff.py | 24 +++++++++++++++++++ 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index 5b494ff923..d48b990be5 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -1798,7 +1798,12 @@ def validate_agent_settings( return _AGENT_SETTINGS_ADAPTER.validate_python(payload, context=context) -def _merge_patch(base: dict[str, Any], diff: Mapping[str, Any]) -> dict[str, Any]: +def _merge_patch( + base: dict[str, Any], + diff: Mapping[str, Any], + *, + remove_nulls: bool = False, +) -> dict[str, Any]: """Apply an RFC 7386 JSON Merge Patch. Nested mappings merge recursively; ``None`` deletes a key; every other value @@ -1807,10 +1812,12 @@ def _merge_patch(base: dict[str, Any], diff: Mapping[str, Any]) -> dict[str, Any """ result = dict(base) for key, value in diff.items(): - if value is None: + if value is None and remove_nulls: result.pop(key, None) + elif value is None: + result[key] = None elif isinstance(value, Mapping) and isinstance(result.get(key), Mapping): - result[key] = _merge_patch(result[key], value) + result[key] = _merge_patch(result[key], value, remove_nulls=True) else: result[key] = value return result @@ -1832,7 +1839,9 @@ def apply_agent_settings_diff( silently drop the outgoing variant's fields (the variants ignore unknown keys), producing a mongrel row. * When ``agent_kind`` is unchanged or omitted, deep-merge the diff within - the variant (``None`` unsets a key, per :func:`_merge_patch`). + the variant. Top-level ``None`` values are validated as ``None`` so + non-optional fields fail loudly; nested mapping ``None`` values remove + individual entries. ``base`` may be a raw persisted mapping or a settings instance; it is migrated and validated first. The merged result is re-validated against diff --git a/tests/agent_server/test_settings_router.py b/tests/agent_server/test_settings_router.py index df452cb9b3..0e5d3e436b 100644 --- a/tests/agent_server/test_settings_router.py +++ b/tests/agent_server/test_settings_router.py @@ -1104,6 +1104,7 @@ def test_patch_settings_same_kind_merge_after_a_switch(client_with_settings): json={ "agent_settings_diff": { "agent_kind": "acp", + "agent_context": {}, "acp_command": ["my-cli"], "acp_env": {"FOO": "bar"}, } diff --git a/tests/sdk/test_apply_agent_settings_diff.py b/tests/sdk/test_apply_agent_settings_diff.py index c39884ce25..918ba84479 100644 --- a/tests/sdk/test_apply_agent_settings_diff.py +++ b/tests/sdk/test_apply_agent_settings_diff.py @@ -109,6 +109,30 @@ def test_none_in_diff_unsets_key_merge_patch() -> None: assert result.acp_model is None +def test_null_top_level_required_field_is_rejected() -> None: + base = { + "agent_kind": "acp", + "acp_server": "claude-code", + "acp_env": {"KEEP_ME": "keep"}, + } + + with pytest.raises(ValueError): + apply_agent_settings_diff(base, {"acp_env": None}) + + +def test_null_nested_mapping_entry_is_removed() -> None: + base = { + "agent_kind": "acp", + "acp_server": "claude-code", + "acp_env": {"KEEP_ME": "keep", "DROP_ME": "drop"}, + } + + result = apply_agent_settings_diff(base, {"acp_env": {"DROP_ME": None}}) + + assert isinstance(result, ACPAgentSettings) + assert result.acp_env == {"KEEP_ME": "keep"} + + def test_empty_diff_returns_validated_base() -> None: base = {"agent_kind": "acp", "acp_server": "claude-code"} From 8ba9668bfc609feca57d8789b5c17efc533a0d9e Mon Sep 17 00:00:00 2001 From: Graham Neubig <398875+neubig@users.noreply.github.com> Date: Tue, 9 Jun 2026 17:44:49 -0400 Subject: [PATCH 5/5] fix: preserve ACP switch compatibility --- openhands-sdk/openhands/sdk/settings/model.py | 10 +++++++++- tests/agent_server/test_settings_router.py | 15 ++++++++++++++- tests/sdk/test_apply_agent_settings_diff.py | 13 +++++++++++-- 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index d48b990be5..689f6f1a41 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -1854,9 +1854,17 @@ def apply_agent_settings_diff( return base_settings new_kind = diff.get("agent_kind") if new_kind and new_kind != base_settings.agent_kind: + if new_kind == "acp" and "agent_context" in diff: + if diff["agent_context"] is None: + raise ValueError( + "agent_context cannot be null when switching agent_kind to acp" + ) + elif new_kind == "acp": + diff = dict(diff) + diff["agent_context"] = {} if new_kind == "acp" and diff.get("agent_context") is None: raise ValueError( - "agent_context is required when switching agent_kind to acp" + "agent_context cannot be null when switching agent_kind to acp" ) merged = _merge_patch({"agent_kind": new_kind}, diff) else: diff --git a/tests/agent_server/test_settings_router.py b/tests/agent_server/test_settings_router.py index 0e5d3e436b..a126199c7d 100644 --- a/tests/agent_server/test_settings_router.py +++ b/tests/agent_server/test_settings_router.py @@ -1033,7 +1033,6 @@ def test_patch_settings_switch_agent_kind_from_openhands_to_acp(client_with_sett @pytest.mark.parametrize( "agent_settings_diff", [ - {"agent_kind": "acp"}, {"agent_kind": "acp", "agent_context": None}, ], ) @@ -1058,6 +1057,20 @@ def test_patch_settings_switch_to_acp_requires_agent_context( assert persisted["agent_settings"]["llm"]["model"] == "stable-model" +def test_patch_settings_switch_to_acp_defaults_missing_agent_context( + client_with_settings, +): + response = client_with_settings.patch( + "/api/settings", + json={"agent_settings_diff": {"agent_kind": "acp"}}, + ) + + assert response.status_code == 200, response.text + body = response.json() + assert body["agent_settings"]["agent_kind"] == "acp" + assert body["agent_settings"]["agent_context"] is not None + + def test_patch_settings_same_kind_restated_still_deep_merges(client_with_settings): """Re-stating the current ``agent_kind`` is NOT a variant switch: the diff must still deep-merge so unrelated fields survive. diff --git a/tests/sdk/test_apply_agent_settings_diff.py b/tests/sdk/test_apply_agent_settings_diff.py index 918ba84479..2a10108801 100644 --- a/tests/sdk/test_apply_agent_settings_diff.py +++ b/tests/sdk/test_apply_agent_settings_diff.py @@ -41,15 +41,24 @@ def test_switch_openhands_to_acp_replaces_with_fresh_variant() -> None: @pytest.mark.parametrize( "diff", - [{"agent_kind": "acp"}, {"agent_kind": "acp", "agent_context": None}], + [{"agent_kind": "acp", "agent_context": None}], ) def test_switch_openhands_to_acp_requires_agent_context(diff) -> None: base = {"agent_kind": "openhands", "llm": {"model": "gpt"}} - with pytest.raises(ValueError, match="agent_context is required"): + with pytest.raises(ValueError, match="agent_context cannot be null"): apply_agent_settings_diff(base, diff) +def test_switch_openhands_to_acp_defaults_missing_agent_context() -> None: + base = {"agent_kind": "openhands", "llm": {"model": "gpt"}} + + result = apply_agent_settings_diff(base, {"agent_kind": "acp"}) + + assert isinstance(result, ACPAgentSettings) + assert result.agent_context is not None + + def test_switch_acp_to_openhands_replaces_with_fresh_variant() -> None: base = {"agent_kind": "acp", "acp_server": "claude-code"}