refactor(settings): nest AppPreferences under new misc_settings container#3543
Conversation
…iner Follow-up to #3539. Replaces the top-level app_preferences block with a misc_settings container that wraps it, giving the API one extension point for frontend-owned settings the agent doesn't interpret. API change: Before (v2): After (v3): ---------------------- ------------------------------ SettingsResponse SettingsResponse .app_preferences .misc_settings .app_preferences SettingsUpdateRequest SettingsUpdateRequest .app_preferences_diff .misc_settings_diff misc_settings_diff is deep-merged into the persisted block, consistent with agent_settings_diff and conversation_settings_diff. Partial diffs like {"misc_settings_diff": {"app_preferences": {"language": "fr"}}} update only the named nested field; sibling app_preferences fields are left alone. Lists (disabled_skills) are replaced wholesale by the deep-merge. Why this matters: the previous shape locked the API into a single "frontend-owned" namespace. Adding a second category (e.g. ui_preferences for sidebar layout / view modes) would have required either another top-level field or shoehorning it into AppPreferences. With misc_settings as a container, new categories drop in as additional nested fields without churning the top-level shape. Schema migration: - Bumps PERSISTED_SETTINGS_SCHEMA_VERSION 2 → 3. - from_persisted() lifts any legacy top-level app_preferences block into misc_settings.app_preferences on load. v1 (pre-app-preferences) and v2 (the now-superseded flat shape from #3539) both load transparently. - If both legacy and new shapes appear on disk (hand-edited file), misc_settings wins. Removes the prior shallow-overlay semantics for app_preferences updates in favour of consistent deep-merge across all three diff fields. Tests: $ uv run pytest tests/agent_server/test_settings_router.py 57 passed $ uv run pytest tests/sdk/settings 45 passed $ uv run ruff check && uv run pyright openhands-sdk/openhands/sdk/settings/ \ openhands-agent-server/openhands/agent_server/persistence/models.py \ openhands-agent-server/openhands/agent_server/settings_router.py All checks pass Note for downstream clients (agent-canvas, etc.): the wire shape changed in a non-backwards-compatible way. Clients reading or writing settings need to update to misc_settings / misc_settings_diff. On-disk v2 files are migrated automatically on first read. Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED Behavioral default changes detectedThese public
|
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||
Follow-up to the localStorage cleanup in this PR + SDK refactor in OpenHands/software-agent-sdk#3543. The agent-server now exposes frontend-owned settings under a generic misc_settings container instead of a top-level app_preferences field. Wire shape changes: Before: After: GET /api/settings GET /api/settings -> { app_preferences: {...} } -> { misc_settings: { app_preferences: {...} } } PATCH /api/settings PATCH /api/settings body.app_preferences_diff (shallow body.misc_settings_diff (deep-merged, overlay, replaces named fields) same semantics as agent_settings_diff) Why the rename to misc_settings: the previous name pinned the API to a single 'frontend-owned' namespace. Adding a future category like ui_preferences (sidebar layout / view modes) would have required either yet another top-level field or shoehorning unrelated UI state into AppPreferences. With misc_settings as a container, new categories drop in as nested fields without churning the top-level shape. Changes: - settings-service.api.ts * SettingsApiResponse.app_preferences -> .misc_settings (typed) * SettingsUpdateRequest.app_preferences_diff -> .misc_settings_diff * Add MiscSettings interface * transformApiResponse reads response.misc_settings?.app_preferences * saveSettings emits { misc_settings_diff: { app_preferences } } * Local 'has any diffs' check tracks misc_settings_diff * Doc comments updated; semantics noted as deep-merge - legacy-app-preferences-migration.ts * Gate on serverResponse.misc_settings, not .app_preferences * pushDiff callback now wraps the diff in { app_preferences: ... } - src/mocks/settings-handlers.ts * GET handler returns misc_settings.app_preferences * PATCH handler accepts misc_settings_diff; deep-merges nested app_preferences into the persisted block * Internal mock state stores under misc_settings to match wire shape - __tests__/api/settings-service.test.ts * Four tests updated to assert the new wire shape (local PATCH body, GET round-trip, mixed-diff routing, legacy localStorage migration) * Pre-1.27 detection test now keys off missing misc_settings - AGENTS.md * App-preferences note rewritten for the misc_settings container, explains deep-merge semantics, and documents the in-flight rename (flat shape introduced in #3539 never shipped to users) Cloud path is unchanged: cloud /api/v1/settings still accepts the fields as flat top-level keys, mirrored by saveCloudSettings. Verification: $ npm run typecheck exit 0 $ npm test -- __tests__/api/settings-service.test.ts \ __tests__/api/mock-settings-handlers.test.ts 23 tests passed $ npm test 3009 passed | 12 skipped | 9 todo $ npm run lint All matched files use Prettier code style! $ npm run build built in 1.50s Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
🟢 Good taste — Clean, well-designed refactor.
Summary
This PR introduces a misc_settings container that wraps app_preferences, providing an extension point for future frontend-owned settings without changing the top-level API shape. Schema version bumps from 2 → 3 with automatic migration for existing persisted files.
Analysis
[CRITICAL ISSUES] — None found.
[IMPROVEMENT OPPORTUNITIES] — None. This is solid work.
[STYLE NOTES]
- The extensive docstring in
from_persisted()documenting the schema-version history is appropriate here — this is genuinely non-obvious migration logic that benefits from explanation. - The migration code at line 330-332 is well-guarded:
misc_settingswins when both keys exist on disk.
What Works Well
- Migration integrity: v1 files (no
app_preferences) load with empty defaults; v2 files (flatapp_preferences) are automatically nested undermisc_settings.app_preferences. - Behavioral upgrade:
app_preferences_diffwas a shallow overlay;misc_settings_diffnow deep-merges consistently withagent_settings_diffandconversation_settings_diff. This makes the API more predictable. - Extension point:
MiscSettingsis a thin wrapper today but allows future categories (e.g., UI preferences) without API churn. - Test coverage: Migration paths are explicitly tested for v1, v2, and the conflict case (both keys present). The deep-merge semantic for
disabled_skillsis also verified.
Minor Note
The PR is marked as draft — this is appropriate given the breaking API change (clients must update to use misc_settings instead of app_preferences).
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
This is an internal schema refactor with automatic migration. Existing persisted settings are preserved. The only surface-area change is the API response shape and the diff field name, both of which clients can update at their own pace (the legacy app_preferences_diff field now correctly returns 400).
VERDICT:
✅ Worth merging — Solid migration design, good test coverage, low risk.
KEY INSIGHT:
The schema-version migration pattern here is a good template: detect legacy shape, lift into new shape, prefer new shape when both exist, persist the migrated form.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
The settings API, SDK typed model, and legacy settings migration all behaved as described when exercised through a live agent-server and direct SDK usage.
Does this PR achieve its stated goal?
Yes. The PR set out to replace the top-level app_preferences API shape with misc_settings.app_preferences, switch writes to misc_settings_diff, deep-merge partial nested updates, loudly reject the legacy diff key, and migrate v2 persisted settings. I verified those behaviors by running the server on both main and this PR branch, making real HTTP requests to /api/settings, and importing the SDK SettingsResponse model directly.
| Phase | Result |
|---|---|
| Environment Setup | ✅ uv run created/synced the project environment and agent-server started successfully. No tests/linters were run. |
| CI Status | gh pr checks: most checks are passing, but Validate PR description and Deprecation deadlines are failing; qa-changes was in progress. |
| Functional Verification | ✅ Live API requests and SDK calls matched the PR description, including migration and error cases. |
Functional Verification
Test 1: API wire shape changed from app_preferences to misc_settings.app_preferences
Step 1 — Establish baseline on main:
Ran git checkout origin/main, started uv run agent-server --host 127.0.0.1 --port 18081, then called GET /api/settings and PATCH /api/settings with app_preferences_diff:
--- main GET shape ---
top_level_has_app_preferences= True
top_level_has_misc_settings= False
app_preferences= {'language': None, ..., 'disabled_skills': []}
--- main PATCH app_preferences_diff ---
status=200
app_preferences= {'language': 'fr', ..., 'disabled_skills': ['openhands/snake']}
--- main PATCH misc_settings_diff ---
{"detail":"At least one of agent_settings_diff, conversation_settings_diff, or app_preferences_diff must be provided"}
http_status=400
This confirms the old branch exposed and accepted the flat app_preferences shape, and did not accept misc_settings_diff.
Step 2 — Apply PR changes:
Checked out refactor-misc-settings-container at 96ba270d and restarted agent-server with an isolated OH_PERSISTENCE_DIR.
Step 3 — Re-run against the PR:
Called GET /api/settings, then PATCH /api/settings with misc_settings_diff:
--- pr GET default shape ---
top_level_has_app_preferences= False
top_level_has_misc_settings= True
misc_settings= {'app_preferences': {'language': None, ..., 'disabled_skills': []}}
--- pr PATCH misc_settings_diff initial write ---
status=200
prefs= {'language': 'fr', 'user_consents_to_analytics': True, 'enable_sound_notifications': False, 'git_user_name': 'Ada Lovelace', 'git_user_email': 'ada@example.com', 'disabled_skills': ['openhands/snake', 'openhands/python']}
This shows the PR exposes the new nested response shape and accepts the new write shape end-to-end.
Test 2: misc_settings_diff deep-merges nested fields and replaces lists wholesale
Step 1 — Baseline / prior state:
On main, there was no accepted misc_settings_diff; PATCH returned 400 as shown above. That establishes the new nested merge behavior did not exist in the old API.
Step 2 — Apply PR changes:
Used the running PR server from Test 1.
Step 3 — Exercise partial update and list replacement:
Ran partial nested PATCH requests:
--- pr PATCH misc_settings_diff partial deep merge ---
status=200
language= de
git_user_name_still= Ada Lovelace
disabled_skills_still= ['openhands/snake', 'openhands/python']
--- pr PATCH disabled_skills list replacement ---
status=200
language_still= de
disabled_skills= []
--- pr refetch persists during server session ---
language= de
git_user_email= ada@example.com
disabled_skills= []
This confirms partial diffs update only the requested nested field, preserve siblings, and replace the disabled_skills list rather than merging it.
Test 3: Legacy request key and invalid nested values fail loudly
Step 1 — Baseline / old behavior:
On main, app_preferences_diff returned 200 and updated preferences as shown in Test 1.
Step 2 — Apply PR changes:
Used the PR server at 96ba270d.
Step 3 — Re-run the legacy and invalid requests:
--- pr PATCH legacy app_preferences_diff rejected ---
{"detail":"At least one of agent_settings_diff, conversation_settings_diff, or misc_settings_diff must be provided"}
http_status=400
--- pr PATCH invalid nested type rejected ---
{"detail":"Settings validation failed"}
http_status=422
This confirms the PR does not silently accept the old client field and still validates nested app_preferences data.
Test 4: v2 persisted settings migrate to v3 response shape
Step 1 — Reproduce legacy persisted input:
Created an isolated settings.json with schema version 2 and top-level app_preferences:
{"schema_version": 2, "app_preferences": {"language": "fr", "git_user_name": "Ada Lovelace", "disabled_skills": ["openhands/snake"]}, ...}This represents the v2 on-disk shape the PR claims to migrate.
Step 2 — Apply PR changes:
Started the PR server with OH_PERSISTENCE_DIR pointing at that file.
Step 3 — Read settings via the live API:
--- pr GET from v2 settings file ---
top_level_has_app_preferences= False
top_level_has_misc_settings= True
language= fr
git_user_name= Ada Lovelace
disabled_skills= ['openhands/snake']
I also tested a hand-edited file containing both old and new keys:
--- pr GET from v2 file containing both keys ---
top_level_has_app_preferences= False
language= fr
git_user_email= ada@example.com
This confirms legacy data is surfaced through misc_settings.app_preferences, and the new shape wins when both are present.
Test 5: SDK typed access moved to response.misc_settings.app_preferences
Step 1 — Establish typed baseline on main:
Ran a direct SDK import/call:
--- main typed SettingsResponse ---
has_app_preferences= True
has_misc_settings= False
language= fr
Step 2 — Apply PR changes:
Checked out the PR branch.
Step 3 — Re-run typed access on PR:
--- pr typed SettingsResponse ---
has_app_preferences= False
has_misc_settings= True
language= fr
This confirms typed clients now access preferences through SettingsResponse.misc_settings.app_preferences as described.
Issues Found
None from functional QA.
This review was created by an AI agent (OpenHands) on behalf of the requester.
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
Taste Rating
🟢 Good taste — Clean, well-scoped refactor that adds a future-proof extension point without breaking the core functionality.
Analysis
Overall: This is a solid structural improvement. Wrapping app_preferences in a MiscSettings container before shipping stable APIs is the right call — it gives us an extension point for future categories (UI preferences, layout state) without churning the top-level wire shape. The migration logic is straightforward and the test coverage is thorough.
Data Structure: The new MiscSettings container is minimal and correct — one nested field today, easy to extend tomorrow. Good choice.
Migration Logic: The v2→v3 migration in from_persisted() handles the tiebreaker case well (misc_settings wins over legacy app_preferences). The pop() + conditional pattern is idiomatic.
Breaking Change: Properly documented as breaking. The decision to reject (400) the legacy app_preferences_diff field rather than silently accepting it is correct — it surfaces the migration requirement loudly rather than silently succeeding with stale behavior.
Testing Gaps
- None. The test suite covers:
- Happy path: GET/PATCH round-trip through
misc_settings.app_preferences - Deep-merge preservation of sibling fields
- v2 migration (flat→nested)
- Tiebreaker when both keys are present (
misc_settingswins) - Rejection of legacy
app_preferences_difffield
- Happy path: GET/PATCH round-trip through
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
This is a well-tested refactor with clear migration semantics. The breaking change is intentional and properly surfaced. Agent-canvas side has been updated and its test suite passes, providing cross-repo e2e validation.
Verdict
✅ Worth merging — The design is sound, the migration is safe, and the test coverage is comprehensive. One minor suggestion below.
Minor Suggestion
🟡 Suggestion: The _empty_app_preferences() helper in the test file (line 554) creates a fresh dict each time. Since it's used as an expected value in multiple assertions, consider whether conftest.py would be a better home for this fixture, keeping the test module focused on behavior rather than data construction. This is optional — the current approach is clear and readable.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
⚠️ QA Report: PASS WITH ISSUES
The settings API behavior works as described end-to-end, including the new misc_settings wire shape, deep-merged partial updates, legacy diff rejection, and v2-on-disk migration; CI is not fully green.
Does this PR achieve its stated goal?
Yes. I ran the local agent-server and exercised /api/settings as an API user: the response moved from flat app_preferences to nested misc_settings.app_preferences, misc_settings_diff preserved sibling preference fields across partial PATCHes, app_preferences_diff now returns the expected 400, and a hand-written v2 settings file was loaded and rewritten as schema v3 without losing app preferences.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed and installed the uv workspace dependencies. |
| CI Status | gh pr checks showed 32 successful, 1 failing (Deprecation deadlines/check), 1 pending, 3 skipped. |
| Functional Verification | ✅ Real local agent-server HTTP requests verified the changed behavior. |
Functional Verification
Test 1: Wire shape and PATCH semantics
Step 1 — Establish baseline on origin/main:
Ran a local base server and queried /api/settings:
git checkout origin/main
OPENHANDS_SUPPRESS_BANNER=1 uv run python -m openhands.agent_server --host 127.0.0.1 --port 18101
curl -sS http://127.0.0.1:18101/api/settings | jq '{has_app_preferences: has("app_preferences"), has_misc_settings: has("misc_settings"), app_preferences: .app_preferences, misc_settings: .misc_settings}'Observed:
{
"has_app_preferences": true,
"has_misc_settings": false,
"app_preferences": {
"language": null,
"user_consents_to_analytics": null,
"enable_sound_notifications": null,
"git_user_name": null,
"git_user_email": null,
"disabled_skills": []
},
"misc_settings": null
}A flat update worked on base:
{
"status_like": "ok",
"app_preferences": {
"language": "fr",
"disabled_skills": ["openhands/snake"]
},
"misc_settings": null
}Sending misc_settings_diff to base returned:
HTTP/1.1 400 Bad Request
{"detail":"At least one of agent_settings_diff, conversation_settings_diff, or app_preferences_diff must be provided"}This confirms the old user-facing API shape was flat and did not accept the new nested diff field.
Step 2 — Apply the PR changes:
Checked out refactor-misc-settings-container at 96ba270d, removed the generated settings file for a clean start, and ran a local PR server:
git checkout refactor-misc-settings-container
rm -f workspace/.openhands/settings.json
OPENHANDS_SUPPRESS_BANNER=1 uv run python -m openhands.agent_server --host 127.0.0.1 --port 18104Step 3 — Re-run with the PR:
Fresh GET now returned the nested shape only:
{
"has_app_preferences": false,
"has_misc_settings": true,
"misc_settings": {
"app_preferences": {
"language": null,
"user_consents_to_analytics": null,
"enable_sound_notifications": null,
"git_user_name": null,
"git_user_email": null,
"disabled_skills": []
}
}
}PATCHing misc_settings_diff with realistic user preferences worked:
{
"language": "fr",
"user_consents_to_analytics": null,
"enable_sound_notifications": null,
"git_user_name": null,
"git_user_email": null,
"disabled_skills": ["openhands/snake"]
}A second partial PATCH preserved sibling fields while adding git_user_name:
{
"language": "fr",
"user_consents_to_analytics": null,
"enable_sound_notifications": null,
"git_user_name": "Ada Lovelace",
"git_user_email": null,
"disabled_skills": ["openhands/snake"]
}A re-GET confirmed persistence:
{
"language": "fr",
"git_user_name": "Ada Lovelace",
"disabled_skills": ["openhands/snake"]
}List replacement also behaved as claimed:
[]The legacy flat diff was rejected loudly:
HTTP/1.1 400 Bad Request
{"detail":"At least one of agent_settings_diff, conversation_settings_diff, or misc_settings_diff must be provided"}This confirms the PR delivers the new wire shape and update semantics.
Test 2: v2 flat settings file migrates to v3 nested settings
Step 1 — Establish legacy on-disk input:
Wrote a v2-style settings file with top-level app_preferences:
jq '.schema_version=2 | .app_preferences={"language":"fr","git_user_name":"Ada Lovelace","disabled_skills":["openhands/snake"]} | del(.misc_settings)' workspace/.openhands/settings.json > /tmp/v2-settings.json
mv /tmp/v2-settings.json workspace/.openhands/settings.json
jq '{schema_version, has_app_preferences: has("app_preferences"), has_misc_settings: has("misc_settings"), app_preferences}' workspace/.openhands/settings.jsonObserved:
{
"schema_version": 2,
"has_app_preferences": true,
"has_misc_settings": false,
"app_preferences": {
"language": "fr",
"git_user_name": "Ada Lovelace",
"disabled_skills": ["openhands/snake"]
}
}This is the legacy flat shape the PR promises to migrate.
Step 2 — Start the PR server and read settings:
OPENHANDS_SUPPRESS_BANNER=1 uv run python -m openhands.agent_server --host 127.0.0.1 --port 18105
curl -sS http://127.0.0.1:18105/api/settings | jq '{has_app_preferences: has("app_preferences"), has_misc_settings: has("misc_settings"), prefs: .misc_settings.app_preferences}'Observed:
{
"has_app_preferences": false,
"has_misc_settings": true,
"prefs": {
"language": "fr",
"user_consents_to_analytics": null,
"enable_sound_notifications": null,
"git_user_name": "Ada Lovelace",
"git_user_email": null,
"disabled_skills": ["openhands/snake"]
}
}This confirms the server loads the legacy v2 preferences into the new nested response shape without data loss.
Step 3 — Force a write and inspect persisted v3 file:
curl -sS -X PATCH -H 'Content-Type: application/json' -d '{"misc_settings_diff":{"app_preferences":{"git_user_email":"ada@example.com"}}}' http://127.0.0.1:18105/api/settings
jq '{schema_version, has_app_preferences: has("app_preferences"), has_misc_settings: has("misc_settings"), prefs: .misc_settings.app_preferences}' workspace/.openhands/settings.jsonObserved:
{
"schema_version": 3,
"has_app_preferences": false,
"has_misc_settings": true,
"prefs": {
"language": "fr",
"user_consents_to_analytics": null,
"enable_sound_notifications": null,
"git_user_name": "Ada Lovelace",
"git_user_email": "ada@example.com",
"disabled_skills": ["openhands/snake"]
}
}This confirms the v2 file is rewritten into the v3 nested persisted shape on the next update.
Issues Found
- 🟡 CI not green:
Deprecation deadlines/checkis failing becauseopenhands-toolsstill has a cleanup-deadline workaround atopenhands-tools/openhands/tools/browser_use/logging_fix.py:16. This did not block functional verification of the settings API, but the PR is not merge-ready until CI is addressed or the check is resolved.
This review was created by an AI agent (OpenHands) on behalf of the user.
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
Thank you for the work on this! It seems the agent is confused now, without revert it thinks version v2 is “real”, but I think maybe best is to replace last night’s version completely
…es history PR #3539 (flat top-level app_preferences at schema v2) was never released to users, so we treat its on-disk shape as if it never existed. This PR now stands on its own as the introduction of misc_settings at schema_version=2 -- not a v2 -> v3 refactor. Changes against the previous commit on this branch: - persistence/models.py * PERSISTED_SETTINGS_SCHEMA_VERSION: 3 -> 2 (stays at 2). * Remove the v2 -> v3 migration block from from_persisted() and the accompanying legacy_prefs local + 'both shapes' tiebreaker. A file written by the unreleased #3539 shape would now fail to load cleanly, but no such file exists in the wild. * Rewrite the from_persisted() docstring to describe the simpler v1 (pre-misc_settings) -> v2 (current, with misc_settings) history. - openhands-sdk/.../settings/api_models.py * AppPreferences and MiscSettings docstrings: 'schema v3' -> 'schema v2'. - tests/agent_server/test_settings_router.py * Drop test_legacy_app_preferences_diff_field_is_rejected. * Drop test_persisted_settings_v2_app_preferences_migrate_to_misc_settings and test_persisted_settings_v2_migration_prefers_existing_misc_settings. * Tighten the v1-loads-with-empty-misc-settings docstring. Co-authored-by: openhands <openhands@all-hands.dev>
The SDK had no business knowing what's nested inside misc_settings. Drop
both AppPreferences and MiscSettings entirely and store misc_settings as
a plain dict[str, Any]. The agent-server now persists and deep-merges
whatever the frontend sends, without interpreting any inner field names.
openhands-sdk/openhands/sdk/settings/api_models.py
- Remove AppPreferences class (the 6 field declarations + docstring).
- Remove MiscSettings class (the wrapper that exposed app_preferences).
- SettingsResponse.misc_settings: MiscSettings -> dict[str, Any].
- Drop unused ConfigDict import.
- Update SettingsResponse / SettingsUpdateRequest docstrings to
describe misc_settings as an opaque frontend-owned container.
openhands-sdk/openhands/sdk/settings/__init__.py
- Drop AppPreferences and MiscSettings from imports and __all__.
openhands-agent-server/openhands/agent_server/persistence/models.py
- Drop MiscSettings import.
- PersistedSettings.misc_settings: MiscSettings -> dict[str, Any]
with default_factory=dict.
- update(): replace MiscSettings.model_validate(merged) with direct
assignment of the deep-merged dict; there's nothing to validate.
- Drop misc_merged from the finally clear-down: the merged dict is now
assigned directly into self.misc_settings, so clearing it would wipe
the persisted state. The clear-down was a secrets-defense mechanism
that does not apply to a non-secret opaque container.
- Rewrite SettingsUpdatePayload, PersistedSettings, and from_persisted
docstrings to describe misc_settings as opaque frontend-owned data.
openhands-agent-server/openhands/agent_server/settings_router.py
- Rewrite PATCH /api/settings docstring for misc_settings_diff to
describe it as deep-merged into an opaque container.
tests/agent_server/test_settings_router.py
- Rename misc_settings test block and helpers to drop app_preferences.
- Rewrite tests with neutral payloads (theme, ui, tags, count) that
exercise the same deep-merge / list-replacement / atomic-update
behaviour the original tests covered, without referencing any
specific frontend schema.
- Drop test_patch_settings_misc_settings_rejects_invalid_type. There
is no schema to violate now -- replaced with
test_patch_settings_misc_settings_accepts_arbitrary_payloads, which
documents the opaque contract by sending a payload that would have
failed the old typed validation and asserting it round-trips.
- Update test_persisted_settings_v1_loads_with_empty_misc_settings to
assert misc_settings == {} instead of {'app_preferences': ...}.
Verification:
$ uv run pytest tests/agent_server/test_settings_router.py
54 passed in 10.62s
$ uv run pytest tests/sdk/settings tests/sdk/utils/test_pydantic_secrets.py
87 passed in 0.39s
$ uv run ruff check + format PASS
$ uv run pyright (settings + persistence + router) 0 errors
Co-authored-by: openhands <openhands@all-hands.dev>
The PR removes app_preferences/AppPreferences from the SDK and re-uses schema_version=2 for the opaque misc_settings container. The v2 baseline fixture still carried the old typed app_preferences shape (and an __expected__ map referencing app_preferences.language etc.), so the persisted-settings compatibility check failed with: Missing expected persisted field 'app_preferences.language'. Replace the inner block with a neutral opaque payload (theme/ui/tags), matching the deep-merge tests in the router. Now zero app_preferences references remain anywhere in the SDK, agent-server, or tests. Co-authored-by: openhands <openhands@all-hands.dev>
4 participants
HUMAN:
Settings-API follow-up to #3539: that PR was merged but never released, so we're using the same
schema_version=2slot to ship a slightly different shape and to make the field fully opaque to the SDK. Please QA the settings GET/PATCH round-trip against a freshly built agent-server (no on-disk migration to test, since v2 has not shipped). Once you're happy, tick the box below.AGENT:
Why
PR #3539 added a top-level
app_preferencesfield toSettingsResponse/SettingsUpdateRequest(introducingschema_version=2). It is merged but has not shipped in any release, so no production user has the flat shape on disk and no production client speaks the flat wire shape.That gives us a free window to fix two things before they lock in:
app_preferencesfield pins the API to one "frontend-owned" namespace. Adding any second category (e.g. UI / sidebar / view-mode state, which is the next bucket in the agent-canvas localStorage audit) would either need yet another top-level field or have to shoehorn unrelated UI state intoAppPreferences. Lift it into a genericmisc_settingscontainer so future categories slot in as nested fields without churning the top-level shape.app_preferencesblock to persisted settings #3539 had the SDK definingAppPreferenceswith six typed fields (language,git_user_name, …) that are pure frontend concerns. The SDK had no reason to know those names — they're just bytes it persists on behalf of a particular client. This PR makesmisc_settingsadict[str, Any]that the agent-server stores and deep-merges but never reads. The frontend owns the schema end-to-end.It also brings the PATCH semantics in line with the other two diff fields —
misc_settings_diffis deep-merged likeagent_settings_diff/conversation_settings_diffinstead of being a shallow overlay, so a partial update like{"misc_settings_diff": {"some_block": {"key": "value"}}}preserves the sibling keys rather than silently leaving stale state behind.Since #3539 never shipped, this PR re-uses
schema_version=2. There is no v2→v3 migration; the prior in-tree v2 shape from #3539 is treated as if it never existed.Summary
SettingsResponse.misc_settingsis nowdict[str, Any](was the typedMiscSettingsmodel from the earlier round of this PR).SettingsUpdateRequest.misc_settings_diffstaysdict[str, Any] | Noneand is deep-merged into the persisted block.AppPreferencesandMiscSettingsclasses are removed fromopenhands-sdkentirely — including from the public__init__.pyexports. The SDK no longer knows anything about the inner shape.PersistedSettings.misc_settings: dict[str, Any]. Theupdate()method deep-merges the diff and stores the result verbatim — nomodel_validate, no schema, no inner clear-down. (The merged dict is nowself.misc_settingsitself; the secrets-defenceclear()in thefinallyblock does not apply to a non-secret opaque container.)PERSISTED_SETTINGS_SCHEMA_VERSIONstays at 2. A v1 file (pre-feat(settings): addapp_preferencesblock to persisted settings #3539) still loads withmisc_settings = {}; the flat v2 shape from the unreleased feat(settings): addapp_preferencesblock to persisted settings #3539 is not supported and would error on load.theme,ui,tags,count) that exercise the deep-merge / list-replacement / atomic-update properties without referencing any specific frontend schema. The "rejects invalid nested type" test from the earlier round is dropped (there's nothing to reject); replaced withtest_patch_settings_misc_settings_accepts_arbitrary_payloads, which documents the opaque contract by sending a payload that would have failed the old typed validation and asserting it round-trips cleanly.Net: +91 / -193 lines vs the previous round of this PR; zero remaining references to
app_preferencesorAppPreferencesanywhere inopenhands-sdk/,openhands-agent-server/, ortests/.Issue Number
N/A — follow-up to #3539.
How to Test
End-to-end check against a real local agent-server:
Manual round-trip:
The matching agent-canvas update (#1191) is built against this wire shape — it owns the
AppPreferencestyping on the TypeScript side and posts{"misc_settings_diff": {"app_preferences": {...}}}through this exact opaque pipe. Its full test suite (3009 tests) passes, providing the cross-repo end-to-end signal.Video/Screenshots
N/A — settings-API change, no UI surface.
Type
Not a user-facing breaking change because the predecessor shape (#3539) never shipped.
Notes
If a developer was running off
mainbetween #3539 merging and this PR landing, their on-disk settings file may contain the flat top-levelapp_preferencesshape atschema_version=2. After this PR lands, that file will not load — the simplest fix is to delete the file. This affects internal-only audience; no released agent-server reads or writes the flat shape.The matching agent-canvas update is OpenHands/agent-canvas#1191.
This PR was created by an AI agent (OpenHands) on behalf of @rbren.
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:c7ff5f1-pythonRun
All tags pushed for this build
About Multi-Architecture Support
c7ff5f1-python) is a multi-arch manifest supporting both amd64 and arm64c7ff5f1-python-amd64) are also available if needed