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 <openhands@all-hands.dev>

* fix(tests): use mockResolvedValue(true) for saveSettings spy

saveSettings returns Promise<boolean>, not Promise<void>, so
mockResolvedValue(undefined) caused TS2345 type errors in CI.

Co-authored-by: openhands <openhands@all-hands.dev>

* 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 <openhands@all-hands.dev>

---------

Co-authored-by: openhands <openhands@all-hands.dev>

__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 = {

__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" });

src/api/settings-service/settings-service.api.ts

@@ -45,6 +45,36 @@ export type ExposeSecretsMode = "encrypted" | "plaintext" | undefined;
 
 const deepClone = <T>(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<string, SettingsValue> | null | undefined,
   next: Record<string, SettingsValue> | null | undefined,
@@ -111,11 +141,20 @@ const transformApiResponse = (
   const agentSettings = response.agent_settings ?? {};
   const conversationSettings = response.conversation_settings ?? {};
 
-  return {
+  const partial: Partial<Settings> = {
     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 =

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) => {

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<string, string> = {};
+  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),