From 0206d0e9eee32890e5673b8a50c8630ffbc83d39 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Wed, 27 May 2026 17:49:25 -0600 Subject: [PATCH] fix(skills): save disabled_skills when server omits the field (#837) * fix(skills): save disabled_skills when settings has no prior disabled_skills field The hydration effect in SkillsSettingsScreen only set hasHydratedInitialSettings=true when settings?.disabled_skills was truthy. When no skills had ever been disabled, the server omits the field entirely (undefined), so the condition was always false: if (settings?.disabled_skills) { ... } // skipped when field is absent Because settings?.disabled_skills is undefined both before *and* after settings load, the dependency array [settings?.disabled_skills] never changed value on load either, so the effect never ran a second time. hasHydratedInitialSettings stayed false, and the save effect's early-return guard blocked every toggle. Fix by gating on settingsLoading instead and defaulting the missing field with ?? []. Also add a localStorage stub for Node.js 25+ which ships a built-in localStorage that is non-functional without --localstorage-file, breaking the zustand persist middleware in the test environment. Co-authored-by: openhands * fix(tests): use mockResolvedValue(true) for saveSettings spy saveSettings returns Promise, not Promise, so mockResolvedValue(undefined) caused TS2345 type errors in CI. Co-authored-by: openhands * fix(skills): persist disabled_skills to localStorage for local backend The local agent-server has no concept of disabled_skills and its PATCH /api/settings strips the field before sending. As a result, toggling a skill off appeared to work in the UI but was never persisted -- the value was silently discarded on every save and page refresh reverted the toggle. Fix by mirroring the existing app-preferences pattern: - saveSettings (local path): call writeStoredDisabledSkills before stripping the field from the PATCH payload. - transformApiResponse: call readStoredDisabledSkills and overlay it onto the partial Settings returned from the API, so every getSettings call (cached or fresh) surfaces the stored value. Export DISABLED_SKILLS_STORAGE_KEY so tests can reference the key without duplicating the string literal. Co-authored-by: openhands --------- Co-authored-by: openhands --- __tests__/api/settings-service.test.ts | 31 +++++++---- __tests__/routes/skills-settings.test.tsx | 52 ++++++++++++++++++- .../settings-service/settings-service.api.ts | 44 +++++++++++++++- src/routes/skills-settings.tsx | 9 ++-- vitest.setup.ts | 15 ++++++ 5 files changed, 135 insertions(+), 16 deletions(-) diff --git a/__tests__/api/settings-service.test.ts b/__tests__/api/settings-service.test.ts index f3089ff740..974bfe012e 100644 --- a/__tests__/api/settings-service.test.ts +++ b/__tests__/api/settings-service.test.ts @@ -1,7 +1,9 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { http, HttpResponse } from "msw"; -import SettingsService from "#/api/settings-service/settings-service.api"; +import SettingsService, { + DISABLED_SKILLS_STORAGE_KEY, +} from "#/api/settings-service/settings-service.api"; import { APP_PREFERENCES_STORAGE_KEY } from "#/api/app-preferences-store"; import { __resetActiveStoreForTests, @@ -139,26 +141,37 @@ describe("SettingsService", () => { fetchSpy.mockRestore(); }); - it("skips PATCH for a skills-only save against a local backend", async () => { - // Arrange: skills are a cloud-only feature. The local agent-server's - // PATCH /api/settings rejects payloads without agent/conversation diffs - // (the MSW handler returns 400 in that case), so a successful no-op here - // also confirms disabled_skills is not leaked to the local backend. + it("persists disabled_skills to localStorage and skips PATCH on a local backend", async () => { + // The local agent-server has no endpoint for disabled_skills, so we store + // them in localStorage instead and never send them in the PATCH body. const fetchSpy = vi.spyOn(SettingsService, "fetchSettingsFromApi"); - // Act const result = await SettingsService.saveSettings({ disabled_skills: ["SSH Microagent"], }); - // Assert: returns true and never fires the PATCH (no fetch invalidation - // either, because the cache wasn't cleared). expect(result).toBe(true); + // The PATCH must not be called — disabled_skills is not a server field. expect(fetchSpy).not.toHaveBeenCalled(); + // The value must be written to localStorage so getSettings can read it back. + const raw = window.localStorage.getItem(DISABLED_SKILLS_STORAGE_KEY); + expect(raw && JSON.parse(raw)).toEqual(["SSH Microagent"]); fetchSpy.mockRestore(); }); + it("surfaces stored disabled_skills in getSettings on a local backend", async () => { + // Pre-seed localStorage as if a previous save had persisted them. + window.localStorage.setItem( + DISABLED_SKILLS_STORAGE_KEY, + JSON.stringify(["SSH Microagent"]), + ); + + const settings = await SettingsService.getSettings(); + + expect(settings.disabled_skills).toEqual(["SSH Microagent"]); + }); + it("persists app-level preferences to localStorage when saving on a local backend", async () => { // Arrange: no diffs, only the 5 app-level preference fields. const appPrefs = { diff --git a/__tests__/routes/skills-settings.test.tsx b/__tests__/routes/skills-settings.test.tsx index 21c1edd19c..deacf69ebf 100644 --- a/__tests__/routes/skills-settings.test.tsx +++ b/__tests__/routes/skills-settings.test.tsx @@ -1,5 +1,5 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; -import { render, screen, within, fireEvent } from "@testing-library/react"; +import { render, screen, within, fireEvent, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { beforeEach, describe, expect, it, vi } from "vitest"; import SkillsSettingsScreen from "#/routes/skills-settings"; @@ -277,6 +277,56 @@ Full skill body.`, expect(within(modal).getByText("SETTINGS$SKILLS_DISABLED")).toBeInTheDocument(); }); + it("saves disabled_skills to the server when a skill is toggled off and settings has no prior disabled_skills field", async () => { + // Reproduces the bug where disabled_skills is absent from settings (undefined), + // causing hasHydratedInitialSettings to never become true and the save to be silently skipped. + const user = userEvent.setup(); + const skill = buildSkill({ name: "save-me" }); + vi.spyOn(SkillsService, "getSkills").mockResolvedValue([skill]); + vi.spyOn(SettingsService, "getSettings").mockResolvedValue( + buildSettings({ disabled_skills: undefined }), + ); + const saveSpy = vi + .spyOn(SettingsService, "saveSettings") + .mockResolvedValue(true); + + renderSkillsSettingsScreen(); + await screen.findByTestId(`skill-card-${skill.name}`); + const card = screen.getByTestId(`skill-card-${skill.name}`); + + await user.click(within(card).getByTestId(`skill-toggle-${skill.name}`)); + + await waitFor(() => + expect(saveSpy).toHaveBeenCalledWith( + expect.objectContaining({ disabled_skills: [skill.name] }), + ), + ); + }); + + it("saves an updated disabled list when a skill is toggled off and settings already has disabled_skills", async () => { + const user = userEvent.setup(); + const skill = buildSkill({ name: "another-skill" }); + vi.spyOn(SkillsService, "getSkills").mockResolvedValue([skill]); + vi.spyOn(SettingsService, "getSettings").mockResolvedValue( + buildSettings({ disabled_skills: [] }), + ); + const saveSpy = vi + .spyOn(SettingsService, "saveSettings") + .mockResolvedValue(true); + + renderSkillsSettingsScreen(); + await screen.findByTestId(`skill-card-${skill.name}`); + const card = screen.getByTestId(`skill-card-${skill.name}`); + + await user.click(within(card).getByTestId(`skill-toggle-${skill.name}`)); + + await waitFor(() => + expect(saveSpy).toHaveBeenCalledWith( + expect.objectContaining({ disabled_skills: [skill.name] }), + ), + ); + }); + it("toggles a skill from the card without opening the modal", async () => { const user = userEvent.setup(); const skill = buildSkill({ name: "card-toggle" }); diff --git a/src/api/settings-service/settings-service.api.ts b/src/api/settings-service/settings-service.api.ts index 8896429a73..f2e60b23b4 100644 --- a/src/api/settings-service/settings-service.api.ts +++ b/src/api/settings-service/settings-service.api.ts @@ -45,6 +45,36 @@ export type ExposeSecretsMode = "encrypted" | "plaintext" | undefined; const deepClone = (value: T): T => JSON.parse(JSON.stringify(value)) as T; +// disabled_skills is not persisted by the local agent-server, so we mirror +// the app-preferences pattern: write to localStorage on save, read back on fetch. +export const DISABLED_SKILLS_STORAGE_KEY = + "openhands-agent-server-disabled-skills"; + +const readStoredDisabledSkills = (): string[] | null => { + if (typeof window === "undefined") return null; + try { + const raw = window.localStorage.getItem(DISABLED_SKILLS_STORAGE_KEY); + if (!raw) return null; + const parsed: unknown = JSON.parse(raw); + if (!Array.isArray(parsed)) return null; + return parsed.filter((v): v is string => typeof v === "string"); + } catch { + return null; + } +}; + +const writeStoredDisabledSkills = (skills: string[]): void => { + if (typeof window === "undefined") return; + try { + window.localStorage.setItem( + DISABLED_SKILLS_STORAGE_KEY, + JSON.stringify(skills), + ); + } catch { + // ignore write failures (e.g. private-browsing quota exceeded) + } +}; + const mergeRecords = ( base: Record | null | undefined, next: Record | null | undefined, @@ -111,11 +141,20 @@ const transformApiResponse = ( const agentSettings = response.agent_settings ?? {}; const conversationSettings = response.conversation_settings ?? {}; - return { + const partial: Partial = { agent_settings: agentSettings, conversation_settings: conversationSettings, llm_api_key_set: response.llm_api_key_is_set, }; + + // The local agent-server never returns disabled_skills; read it from + // localStorage where saveSettings writes it for local-mode clients. + const stored = readStoredDisabledSkills(); + if (stored !== null) { + partial.disabled_skills = stored; + } + + return partial; }; /** @@ -458,6 +497,9 @@ class SettingsService { // requires at least one of the two diff fields. Strip disabled_skills // and skip the request entirely if no diffs remain. App preferences // are persisted to localStorage above and never sent to this endpoint. + if (Array.isArray(disabledSkills)) { + writeStoredDisabledSkills(disabledSkills); + } const localPayload = { ...payload }; delete localPayload.disabled_skills; const hasLocalDiffs = diff --git a/src/routes/skills-settings.tsx b/src/routes/skills-settings.tsx index 18d77f52da..6052b3839a 100644 --- a/src/routes/skills-settings.tsx +++ b/src/routes/skills-settings.tsx @@ -57,11 +57,10 @@ function SkillsSettingsScreen() { // Sync local state with server settings when data first arrives React.useEffect(() => { - if (settings?.disabled_skills) { - setDisabledSet(new Set(settings.disabled_skills)); - setHasHydratedInitialSettings(true); - } - }, [settings?.disabled_skills]); + if (settingsLoading || !settings) return; + setDisabledSet(new Set(settings.disabled_skills ?? [])); + setHasHydratedInitialSettings(true); + }, [settingsLoading, settings?.disabled_skills]); const handleToggle = (skillName: string, enabled: boolean) => { setDisabledSet((prev) => { diff --git a/vitest.setup.ts b/vitest.setup.ts index 2c0f9c10de..e57d397f2d 100644 --- a/vitest.setup.ts +++ b/vitest.setup.ts @@ -19,6 +19,21 @@ const windowStub = vi.stubGlobal("window", windowStub); windowStub.scrollTo = vi.fn(); +// Node.js 25+ ships a built-in localStorage that requires --localstorage-file +// and is not functional without it. Stub it with a plain in-memory +// implementation so zustand's persist middleware works in tests. +if (typeof localStorage === "undefined" || typeof localStorage.setItem !== "function") { + const store: Record = {}; + vi.stubGlobal("localStorage", { + getItem: (key: string) => store[key] ?? null, + setItem: (key: string, value: string) => { store[key] = String(value); }, + removeItem: (key: string) => { delete store[key]; }, + clear: () => { Object.keys(store).forEach((k) => delete store[k]); }, + get length() { return Object.keys(store).length; }, + key: (index: number) => Object.keys(store)[index] ?? null, + }); +} + if (typeof requestAnimationFrame === "undefined") { vi.stubGlobal("requestAnimationFrame", (callback: FrameRequestCallback) => setTimeout(() => callback(0), 0),