settings: persist app preferences and disabled_skills on the agent-server#1191
Open
chuckbutkus wants to merge 8 commits into
Open
settings: persist app preferences and disabled_skills on the agent-server#1191chuckbutkus wants to merge 8 commits into
disabled_skills on the agent-server#1191chuckbutkus wants to merge 8 commits into
Conversation
…rver The local agent-server now exposes app_preferences on the persisted settings (OpenHands/software-agent-sdk#3539): language, sound notifications, analytics consent, git identity, and disabled_skills are returned on GET /api/settings under app_preferences and updated via a new app_preferences_diff field on PATCH /api/settings. This brings the local agent-server to parity with the cloud, which has always accepted the same keys at the top level. Drops the localStorage workaround that mirrored these fields in two keys (openhands-agent-server-app-preferences and openhands-agent-server-disabled-skills), along with the app-preferences-store.ts module and the DISABLED_SKILLS_STORAGE_KEY helpers it depended on. - SettingsService.transformApiResponse reads app_preferences from the server response and hoists each field onto the flat Settings shape so consumers (settings.language, settings.disabled_skills, …) keep working unchanged. - SettingsService.saveSettings routes the same set of fields through the new app_preferences_diff for local backends and through the existing app_preferences flat-spread path for cloud backends. - New legacy-app-preferences-migration.ts runs once on first getSettings() after upgrade: when the server reports an app_preferences block AND legacy localStorage values are still present, it pushes them up via app_preferences_diff and clears the legacy keys. Pre-1.27 servers (which omit app_preferences entirely) cause the migration to no-op so existing data isn't dropped before the server can accept it. - Updated MSW handlers to round-trip app_preferences and app_preferences_diff so the mock backend matches production. - Test coverage: 5 new tests in __tests__/api/settings-service.test.ts for the local round-trip, the mixed diff routing, the legacy migration, and the pre-1.27 skip path. Closes the localStorage workaround called out in the recent audit of agent-canvas localStorage usage (items 3 and 4: disabled_skills and app-preferences fields). Depends on agent-server 1.27 / SDK PR #3539. Co-authored-by: openhands <openhands@all-hands.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
approved these changes
Jun 5, 2026
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
There was a problem hiding this comment.
Code Review: PR #1191 — settings: persist app preferences and disabled_skills on the agent-server
🟡 Acceptable — The solution is sound and the migration approach handles edge cases well, but a few patterns could be cleaner.
[IMPROVEMENT OPPORTUNITIES]
-
[src/api/settings-service/legacy-app-preferences-migration.ts, Line 87] DRY: The
buildDifffunction at line 87 iterates overAPP_PREFERENCE_KEYSto extract known fields. This same pattern appears intransformApiResponse(settings-service.api.ts, line 184). Consider extracting a shared constant or helper to avoid drift if a new app preference field is added. -
[src/api/settings-service/settings-service.api.ts, Lines 346-362] Verbosity: The cloud payload construction uses 5 sequential
ifblocks with identical shape. This is acceptable but could use a helper if it grows further. Not blocking — current form is readable. -
[src/api/settings-service/settings-service.api.ts, Line 199] Stale comment:
syncDerivedSettingspreviously noted it reads app preferences from localStorage. The comment block at line 199-208 still references "App-level user preferences (language, git identity, sound notifications, analytics consent)" being merged fromstoredAppPrefs, but that variable no longer exists. The function body is correct; the stale comment may confuse future readers.
[TESTING]
Tests are comprehensive and cover:
disabled_skillsrouting toapp_preferences_diff(local backend)app_preferencessurfacing ingetSettings- Mixed app/agent fields routing to correct diff buckets
- Legacy localStorage migration on first
getSettings - Migration skipped for pre-1.27 servers (no
app_preferencesfield) - Cloud preferences surfacing via mock
- MCP config rollback scenarios (existing coverage preserved)
No testing gaps identified.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
Risk factors:
- Data migration: This PR moves user preferences from localStorage to server-side storage. The migration is one-shot and idempotent — failures leave legacy keys for retry, and pre-1.27 servers skip migration until upgraded. This is well-handled.
- Breaking change: Old agent-canvas localStorage data must migrate to the server. Users upgrading from pre-1.27 agent-canvas without a 1.27+ agent-server will have their preferences reset (acceptable degradation).
- No test evidence of manual end-to-end run: Tests are comprehensive but this PR has no
Evidencesection with screenshots or commands demonstrating the feature works in a real browser session. For a UI-affecting change, this would normally be blocking — however, the migration path is well-tested and low-risk, so I'm noting it without blocking.
Recommendation: Safe to merge. The migration logic is well-tested and the risk is bounded.
VERDICT
✅ Worth merging: Core logic is sound, migration approach is safe, tests are comprehensive.
KEY INSIGHT
The migration from localStorage to server-side storage is a well-scoped refactor: the legacy-app-preferences-migration.ts module cleanly encapsulates the one-shot migration logic, and the app_preferences_diff API contract provides a clean interface for both cloud and local backends to share.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
chuckbutkus
added
the
update-snapshots
6 tasks
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>
Contributor
✅ Mock-LLM E2E Tests38/38 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
🔶 Mock-LLM Docker E2E Test Results33/38 passed · 5 skipped Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM E2E Tests38/38 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
🔶 Mock-LLM Docker E2E Test Results33/38 passed · 5 skipped Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM E2E Tests42/42 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
🔶 Mock-LLM Docker E2E Test Results37/42 passed · 5 skipped Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
|
PR Artifacts Notice This PR contains a
|
Contributor
✅ Mock-LLM E2E Tests43/43 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
🔶 Mock-LLM Docker E2E Test Results38/43 passed · 5 skipped Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM E2E Tests44/44 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
🔶 Mock-LLM Docker E2E Test Results39/44 passed · 5 skipped Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
|
| Status | Test | Duration |
|---|
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses)
Contributor
|
| Status | Test | Duration |
|---|
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses)
Contributor
📸 Snapshot Test ReportWarning Snapshot comparison step crashed (timeout, OOM, or runner error) — diff results below may be incomplete or absent. ✅ 20 snapshots changed — acknowledged via the
🔴 Changed snapshots (20)
|
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
automations — 3 snapshots
automations-list-active-inactive
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
automations-no-automations
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
automations-search-no-results
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
backends-extended — 2 snapshots
backend-add-two-column-layout
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
backend-dropdown-two-backends
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
backends
backend-manage-modal
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
changes-tab
changes-empty
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
mcp-page — 2 snapshots
mcp-search-filtered
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
mcp-slack-install-1-marketplace
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
onboarding
onboarding-step-1-choose-agent
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
settings-page — 4 snapshots
analytics-consent-modal
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
home-screen
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
settings-app-page
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
settings-page
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
settings-verification
condenser-settings
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
skills-page — 4 snapshots
skills-empty
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
skills-loaded
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
skills-no-match
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
skills-search-filtered
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
✅ Unchanged snapshots (54)
archived-conversation
- conversation-panel-with-archived-badges
- conversation-view-archived
automations
- automations-delete-modal
backends-extended
- backend-add-blank-disabled
- backend-add-cloud-advanced-open
- backend-add-cloud-no-key-disabled
- backend-add-cloud-with-key-enabled
- backend-add-form-partially-filled
- backend-add-invalid-url-disabled
- backend-add-local-ready
- backend-add-name-only-disabled
- backend-add-whitespace-host-disabled
- backend-after-switch
- backend-cancel-nothing-saved
- backend-edit-prefilled
- backend-manage-after-removal
- backend-manage-two-listed
- backend-remove-cancelled
- backend-remove-confirmation
- backend-switch-overlay
backends
- backend-add-modal
- backend-selector-open
changes-tab
- changes-deleted-file
- changes-diff-viewer
collapsible-thinking
- reasoning-content-collapsed
- reasoning-content-expanded
- think-action-collapsed
- think-action-expanded
mcp-page
- mcp-custom-server-1-editor-open
- mcp-custom-server-2-url-filled
- mcp-custom-server-3-all-filled
- mcp-custom-server-4-installed
- mcp-custom-server-editor
- mcp-empty-installed
- mcp-slack-install-2-modal
- mcp-slack-install-3-filled
- mcp-slack-install-4-installed
onboarding
- onboarding-step-0-check-backend
- onboarding-step-2-setup-llm
- onboarding-step-3-say-hello
projects-workspace-browser
- projects-workspace-browser
settings-page
- add-backend-modal
settings-secrets
- secrets-add-form-filled
- secrets-add-form
- secrets-after-save
- secrets-delete-confirm
- secrets-list
settings-verification
- verification-settings-critic-enabled
- verification-settings-off
- verification-settings-on
sidebar
- sidebar-collapsed
- sidebar-conversation-panel
- sidebar-filter-menu
skills-page
- skills-type-filter
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Removes the localStorage shim for app preferences and
disabled_skillsand routes them through the agent-server's settings store, where they live asmisc_settings.app_preferences.This is the agent-canvas side of a two-PR change:
misc_settingsas an opaquedict[str, Any]container onSettingsResponse/PersistedSettings, with deep-merge semantics onmisc_settings_diff. This supersedes the unreleased flatapp_preferencesshape from #3539; both ship underPERSISTED_SETTINGS_SCHEMA_VERSION=2, only the wire shape differs.AppPreferencesschema end-to-end on the TypeScript side, posts it through the opaquemisc_settingspipe, migrates leftover localStorage on first read, and deletes the old localStorage code paths.Why
misc_settings(and why opaque)A flat top-level
app_preferencesfield on the SDK would pin the API to a single "frontend-owned" namespace and force the SDK to declare names for fields it has no business interpreting. Withmisc_settingsas an opaque container:app_preferencesshape lives where the schema lives — here, in TypeScript.Wire-shape summary
{ ..., misc_settings: { app_preferences: {...} } }{ misc_settings_diff: { app_preferences: {...} } }(deep-merged)Deep-merge means partial diffs like
{ misc_settings_diff: { app_preferences: { language: "fr" } } }only updatelanguage; siblingapp_preferencesfields are preserved.disabled_skills(a list) is still replaced wholesale.The fact that the inner block is named
app_preferencesis now purely an agent-canvas convention — the agent-server only sees it as one key in an opaque dict it doesn't read.Frontend changes in this PR
openhands-agent-server-app-preferencesandopenhands-agent-server-disabled-skills. The keys are read once and pushed up by a one-shot migration on nextgetSettings()(src/api/settings-service/legacy-app-preferences-migration.ts), then deleted.src/api/settings-service/settings-service.api.ts: typedAppPreferences(frontend-owned), GET readsresponse.misc_settings?.app_preferences, PATCH emitsmisc_settings_diff.saveSettingsextracts app-preference fields from a flatSettingspayload, then wraps them as{ misc_settings_diff: { app_preferences } }for the local PATCH (or flat top-level keys for the cloud POST).src/api/app-preferences-store.tsdeleted — the localStorage shim is gone.misc_settingsto match the wire shape end-to-end.misc_settings: documents deep-merge semantics, that the agent-server treats the contents as opaque, and reminds future contributors not to reintroduce a localStorage fallback for these fields.Cloud path is unchanged
saveCloudSettingsstill spreads app-preference fields as flat top-level keys onPOST /api/v1/settings(cloud convention). The split happens insidesaveSettings: local goes throughmisc_settings_diff, cloud goes throughcloudPayload.app_preferences.Verification
Merge order
Wait for SDK #3543 to merge and ship in a release the frontend pins; only then merge this PR. Until then this PR points at a wire shape the agent-server does not yet speak.
This PR was created by an AI agent (OpenHands) on behalf of @rbren.
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.26.0-pythonopenhands-automation==1.0.0a6b54e9fbb543e171551c6797260f63cad0cc0ce85Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-b54e9fbRun
All tags pushed for this build
About Multi-Architecture Support
sha-b54e9fb) is a multi-arch manifest supporting both amd64 and arm64sha-b54e9fb-amd64) are also available if needed