diff --git a/.github/pr-assets/pr-16860-01-skills-default.png b/.github/pr-assets/pr-16860-01-skills-default.png new file mode 100644 index 0000000000..91cb74c5dc Binary files /dev/null and b/.github/pr-assets/pr-16860-01-skills-default.png differ diff --git a/.github/pr-assets/pr-16860-02-recommended-facet.png b/.github/pr-assets/pr-16860-02-recommended-facet.png new file mode 100644 index 0000000000..62e5edbf04 Binary files /dev/null and b/.github/pr-assets/pr-16860-02-recommended-facet.png differ diff --git a/__tests__/components/features/skills/skill-filter.test.ts b/__tests__/components/features/skills/skill-filter.test.ts index 00e7f8b9fd..d88fbaf109 100644 --- a/__tests__/components/features/skills/skill-filter.test.ts +++ b/__tests__/components/features/skills/skill-filter.test.ts @@ -47,6 +47,16 @@ function buildMixedSkills(): SkillInfo[] { ]; } +/** + * The filter helpers take enablement as a predicate — resolving the allow-list + * and deny-list behind it is the caller's job — so these tests express the + * cases they care about as a deny-set. + */ +function enabledExcept(...names: string[]) { + const denied = new Set(names); + return (skill: SkillInfo) => !denied.has(skill.name); +} + function stateWith( overrides: Partial = {}, ): SkillFilterState { @@ -57,7 +67,7 @@ describe("parseSkillFilterState", () => { it("reads every facet plus the query", () => { const state = parseSkillFilterState( new URLSearchParams( - "q=deno&source=personal&source=public&category=environment&type=repo&state=disabled", + "q=deno&source=personal&source=public&category=environment&type=repo&state=disabled&recommendation=recommended", ), ); @@ -66,6 +76,7 @@ describe("parseSkillFilterState", () => { expect([...state.categories]).toEqual(["environment"]); expect([...state.types]).toEqual(["repo"]); expect([...state.states]).toEqual(["disabled"]); + expect([...state.recommendations]).toEqual(["recommended"]); }); it.each([ @@ -73,6 +84,7 @@ describe("parseSkillFilterState", () => { ["category", "category=code-hostig"], ["type", "type=bogus"], ["state", "state=maybe"], + ["recommendation", "recommendation=maybe"], ])("drops unknown %s values instead of erroring", (_label, search) => { const state = parseSkillFilterState(new URLSearchParams(search)); expect(countActiveFilters(state)).toBe(0); @@ -97,12 +109,12 @@ describe("parseSkillFilterState", () => { describe("applySkillFilters", () => { const skills = buildMixedSkills(); - const disabled = new Set(["prd"]); + const isEnabled = enabledExcept("prd"); it("ORs values within a group", () => { const result = applySkillFilters( skills, - disabled, + isEnabled, stateWith({ categories: new Set(["environment", "writing"]) }), ); expect(result.map((s) => s.name)).toEqual(["deno", "prd"]); @@ -111,7 +123,7 @@ describe("applySkillFilters", () => { it("ANDs across groups", () => { const result = applySkillFilters( skills, - disabled, + isEnabled, stateWith({ categories: new Set(["environment", "writing"]), states: new Set(["disabled"]), @@ -123,7 +135,7 @@ describe("applySkillFilters", () => { it("treats a category-less skill as other", () => { const result = applySkillFilters( skills, - disabled, + isEnabled, stateWith({ categories: new Set(["other"]) }), ); expect(result.map((s) => s.name)).toEqual(["house-rules"]); @@ -133,12 +145,12 @@ describe("applySkillFilters", () => { expect( applySkillFilters( skills, - disabled, + isEnabled, stateWith({ query: "Requirements" }), ).map((s) => s.name), ).toEqual(["prd"]); expect( - applySkillFilters(skills, disabled, stateWith({ query: "deno" })).map( + applySkillFilters(skills, isEnabled, stateWith({ query: "deno" })).map( (s) => s.name, ), ).toEqual(["deno"]); @@ -146,7 +158,7 @@ describe("applySkillFilters", () => { it("returns everything when no facet is selected", () => { expect( - applySkillFilters(skills, disabled, EMPTY_SKILL_FILTER_STATE), + applySkillFilters(skills, isEnabled, EMPTY_SKILL_FILTER_STATE), ).toHaveLength(3); }); }); @@ -160,7 +172,7 @@ describe("buildSkillFacetGroups", () => { const groups = buildSkillFacetGroups( publicOnly, - new Set(), + enabledExcept(), EMPTY_SKILL_FILTER_STATE, ); @@ -178,7 +190,7 @@ describe("buildSkillFacetGroups", () => { const groups = buildSkillFacetGroups( publicOnly, - new Set(), + enabledExcept(), stateWith({ sources: new Set(["project"]) }), ); @@ -193,7 +205,7 @@ describe("buildSkillFacetGroups", () => { it("shows groups once a second value exists", () => { const groups = buildSkillFacetGroups( buildMixedSkills(), - new Set(["prd"]), + enabledExcept("prd"), EMPTY_SKILL_FILTER_STATE, ); @@ -208,7 +220,7 @@ describe("buildSkillFacetGroups", () => { it("labels rows with i18n keys rather than translated text", () => { const groups = buildSkillFacetGroups( buildMixedSkills(), - new Set(["prd"]), + enabledExcept("prd"), EMPTY_SKILL_FILTER_STATE, ); const category = groups.find((g) => g.id === "category")!; @@ -227,7 +239,7 @@ describe("buildSkillFacetGroups", () => { it("counts rows against other groups but not the group's own selection", () => { const groups = buildSkillFacetGroups( buildMixedSkills(), - new Set(["prd"]), + enabledExcept("prd"), stateWith({ states: new Set(["enabled"]) }), ); @@ -246,11 +258,11 @@ describe("buildSkillFacetGroups", () => { it("keeps a different group's visibility stable while one facet narrows", () => { const skills = buildMixedSkills(); - const disabled = new Set(["prd"]); + const isEnabled = enabledExcept("prd"); const narrowed = buildSkillFacetGroups( skills, - disabled, + isEnabled, stateWith({ categories: new Set(["environment"]) }), ); @@ -262,27 +274,61 @@ describe("buildSkillFacetGroups", () => { it("keeps group visibility stable while a search query narrows results", () => { const skills = buildMixedSkills(); - const disabled = new Set(["prd"]); + const isEnabled = enabledExcept("prd"); const unfiltered = buildSkillFacetGroups( skills, - disabled, + isEnabled, EMPTY_SKILL_FILTER_STATE, ); // "Requirements" matches only `prd`, so a query-filtered denominator would collapse every group to one value. const narrowed = buildSkillFacetGroups( skills, - disabled, + isEnabled, stateWith({ query: "Requirements" }), ); expect(narrowed.map((g) => g.id)).toEqual(unfiltered.map((g) => g.id)); }); + it("adds a recommendation group once the catalog flags one of the skills", () => { + // `add-skill` carries `defaultEnabled` in the bundled catalog; `deno` does + // not, which is what gives the group two discriminating values. + const groups = buildSkillFacetGroups( + [buildSkill({ name: "add-skill" }), buildSkill({ name: "deno" })], + enabledExcept(), + EMPTY_SKILL_FILTER_STATE, + ); + + const recommendation = groups.find((g) => g.id === "recommendation")!; + expect(recommendation.labelKey).toBe( + I18nKey.SETTINGS$SKILLS_FACET_RECOMMENDATION, + ); + expect(recommendation.rows.map((r) => [r.value, r.count])).toEqual([ + ["recommended", 1], + ["other", 1], + ]); + }); + + it("filters down to the catalog's recommended skills", () => { + const skills = [ + buildSkill({ name: "add-skill" }), + buildSkill({ name: "deno" }), + ]; + + expect( + applySkillFilters( + skills, + enabledExcept(), + stateWith({ recommendations: new Set(["recommended"]) }), + ).map((s) => s.name), + ).toEqual(["add-skill"]); + }); + it("disables a zero-count row unless it is checked", () => { const groups = buildSkillFacetGroups( buildMixedSkills(), - new Set(["prd"]), + enabledExcept("prd"), stateWith({ states: new Set(["enabled"]) }), ); const rows = groups.find((g) => g.id === "category")!.rows; diff --git a/__tests__/components/modals/skills/skill-modal.test.tsx b/__tests__/components/modals/skills/skill-modal.test.tsx index 0b1e337f22..3661c71ce5 100644 --- a/__tests__/components/modals/skills/skill-modal.test.tsx +++ b/__tests__/components/modals/skills/skill-modal.test.tsx @@ -4,6 +4,8 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { renderWithProviders } from "test-utils"; import { SkillsModal } from "#/components/features/conversation-panel/skills-modal"; import SkillsService from "#/api/skills-service"; +import SettingsService from "#/api/settings-service/settings-service.api"; +import { MOCK_DEFAULT_USER_SETTINGS } from "#/mocks/handlers"; describe("SkillsModal", () => { const mockOnClose = vi.fn(); @@ -39,16 +41,45 @@ describe("SkillsModal", () => { vi.restoreAllMocks(); }); + describe("Enabled skills only", () => { + // The modal reports what the conversation actually has, so a catalog skill + // left off the allow-list must not be listed as available. + const CATALOG_ON = "add-skill"; + const CATALOG_OFF = "add-javadoc"; + + const catalogSkill = (name: string) => ({ + name, + type: "knowledge" as const, + source: "public", + triggers: [], + content: `content for ${name}`, + }); + + it("lists a catalog skill on the allow-list and omits one that is off", async () => { + vi.spyOn(SkillsService, "getSkills").mockResolvedValue([ + catalogSkill(CATALOG_ON), + catalogSkill(CATALOG_OFF), + ]); + vi.spyOn(SettingsService, "getSettings").mockResolvedValue({ + ...MOCK_DEFAULT_USER_SETTINGS, + enabled_skills: [CATALOG_ON], + disabled_skills: [], + }); + + renderWithProviders(); + + expect(await screen.findByText(CATALOG_ON)).toBeInTheDocument(); + expect(screen.queryByText(CATALOG_OFF)).not.toBeInTheDocument(); + }); + }); + describe("Refresh Button Rendering", () => { it("should render the refresh button as an icon-only control with accessible label", async () => { renderWithProviders(); const refreshButton = await screen.findByTestId("refresh-skills"); expect(refreshButton).toBeInTheDocument(); - expect(refreshButton).toHaveAttribute( - "aria-label", - "BUTTON$REFRESH", - ); + expect(refreshButton).toHaveAttribute("aria-label", "BUTTON$REFRESH"); expect(refreshButton).not.toHaveTextContent("BUTTON$REFRESH"); }); }); diff --git a/__tests__/hooks/use-migrate-enabled-skills.test.tsx b/__tests__/hooks/use-migrate-enabled-skills.test.tsx new file mode 100644 index 0000000000..9ce978a9ef --- /dev/null +++ b/__tests__/hooks/use-migrate-enabled-skills.test.tsx @@ -0,0 +1,102 @@ +import { renderHook, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { DEFAULT_ENABLED_SKILL_NAMES } from "@openhands/extensions/skills"; +import { useMigrateEnabledSkills } from "#/hooks/use-migrate-enabled-skills"; + +const useSettingsMock = vi.fn(); +const useActiveBackendMock = vi.fn(); +const saveSettingsMock = vi.fn(); + +vi.mock("#/hooks/query/use-settings", () => ({ + useSettings: () => useSettingsMock(), +})); +vi.mock("#/contexts/active-backend-context", () => ({ + useActiveBackend: () => useActiveBackendMock(), +})); +vi.mock("#/hooks/mutation/use-save-settings", () => ({ + useSaveSettings: () => ({ mutate: saveSettingsMock }), +})); + +function settings(overrides: Record = {}) { + return { data: { ...overrides }, isLoading: false, isError: false }; +} + +describe("useMigrateEnabledSkills", () => { + beforeEach(() => { + vi.clearAllMocks(); + useActiveBackendMock.mockReturnValue({ + backend: { kind: "local", id: "b1" }, + orgId: null, + }); + }); + + it("writes the curated default for a workspace that has never chosen", async () => { + useSettingsMock.mockReturnValue(settings({ disabled_skills: [] })); + + renderHook(() => useMigrateEnabledSkills()); + + await waitFor(() => + expect(saveSettingsMock).toHaveBeenCalledWith({ + enabled_skills: [...DEFAULT_ENABLED_SKILL_NAMES], + disabled_skills: [], + }), + ); + }); + + it("preserves what an existing workspace had switched on", async () => { + useSettingsMock.mockReturnValue(settings({ disabled_skills: ["deno"] })); + + renderHook(() => useMigrateEnabledSkills()); + + await waitFor(() => expect(saveSettingsMock).toHaveBeenCalled()); + const [payload] = saveSettingsMock.mock.calls[0]; + // The old default was everything-on, so a workspace that only turned off + // `deno` must not lose the rest of the catalog to the curated set. + expect(payload.enabled_skills).toContain("add-javadoc"); + expect(payload.enabled_skills).not.toContain("deno"); + }); + + it("leaves an already migrated workspace alone", async () => { + useSettingsMock.mockReturnValue( + settings({ enabled_skills: [], disabled_skills: [] }), + ); + + renderHook(() => useMigrateEnabledSkills()); + + await waitFor(() => expect(saveSettingsMock).not.toHaveBeenCalled()); + }); + + it("runs at most once while the save and refetch settle", async () => { + useSettingsMock.mockReturnValue(settings({ disabled_skills: [] })); + + const { rerender } = renderHook(() => useMigrateEnabledSkills()); + rerender(); + rerender(); + + await waitFor(() => expect(saveSettingsMock).toHaveBeenCalledTimes(1)); + }); + + it("skips cloud, which never reads enabled_skills", async () => { + useActiveBackendMock.mockReturnValue({ + backend: { kind: "cloud", id: "cloud" }, + orgId: null, + }); + useSettingsMock.mockReturnValue(settings({ disabled_skills: [] })); + + renderHook(() => useMigrateEnabledSkills()); + + await waitFor(() => expect(saveSettingsMock).not.toHaveBeenCalled()); + }); + + it("does not persist a default over settings that failed to load", async () => { + useSettingsMock.mockReturnValue({ + data: undefined, + isLoading: false, + isError: true, + }); + + renderHook(() => useMigrateEnabledSkills()); + + await waitFor(() => expect(saveSettingsMock).not.toHaveBeenCalled()); + }); +}); diff --git a/__tests__/routes/skills-settings.test.tsx b/__tests__/routes/skills-settings.test.tsx index 70dc7cd5e8..ba5750a411 100644 --- a/__tests__/routes/skills-settings.test.tsx +++ b/__tests__/routes/skills-settings.test.tsx @@ -493,4 +493,137 @@ Full skill body.`, expect(writeText).toHaveBeenCalledWith(ADD_SKILL_EXAMPLE_COMMAND); }); + // A skill from the bundled `@openhands/extensions` catalog: those are + // governed by the `enabled_skills` allow-list, not the deny-list. + const CATALOG_RECOMMENDED = "add-skill"; + const CATALOG_OPTIONAL = "add-javadoc"; + + it("shows the notice that skill changes only reach new conversations", async () => { + vi.spyOn(SkillsService, "getSkills").mockResolvedValue([]); + + renderSkillsSettingsScreen(); + + expect( + await screen.findByTestId("skills-new-conversation-notice"), + ).toHaveTextContent("SETTINGS$SKILLS_NEW_CONVERSATION_NOTICE"); + }); + + it("badges a catalog skill the catalog recommends", async () => { + vi.spyOn(SkillsService, "getSkills").mockResolvedValue([ + buildSkill({ name: CATALOG_RECOMMENDED }), + buildSkill({ name: CATALOG_OPTIONAL }), + ]); + + renderSkillsSettingsScreen(); + await screen.findByTestId(`skill-card-${CATALOG_RECOMMENDED}`); + + expect( + screen.getByTestId(`skill-recommended-${CATALOG_RECOMMENDED}`), + ).toHaveTextContent("SETTINGS$SKILLS_RECOMMENDED"); + expect( + screen.queryByTestId(`skill-recommended-${CATALOG_OPTIONAL}`), + ).not.toBeInTheDocument(); + }); + + it("renders an unlisted catalog skill as off without it being denied", async () => { + // The reported bug: everything in the catalog arrived switched on. + vi.spyOn(SkillsService, "getSkills").mockResolvedValue([ + buildSkill({ name: CATALOG_OPTIONAL }), + ]); + vi.spyOn(SettingsService, "getSettings").mockResolvedValue( + buildSettings({ enabled_skills: undefined, disabled_skills: [] }), + ); + + renderSkillsSettingsScreen(); + const card = await screen.findByTestId(`skill-card-${CATALOG_OPTIONAL}`); + + expect( + within(card).getByTestId(`skill-toggle-${CATALOG_OPTIONAL}`), + ).toHaveAttribute("aria-checked", "false"); + }); + + it("writes nothing back when the user toggles nothing", async () => { + // Persisting the hydrated state on mount would race the one-shot migration + // and could narrow a workspace it had just preserved. + vi.spyOn(SkillsService, "getSkills").mockResolvedValue([ + buildSkill({ name: CATALOG_OPTIONAL }), + ]); + vi.spyOn(SettingsService, "getSettings").mockResolvedValue( + buildSettings({ enabled_skills: undefined, disabled_skills: [] }), + ); + const saveSpy = vi + .spyOn(SettingsService, "saveSettings") + .mockResolvedValue(true); + + renderSkillsSettingsScreen(); + await screen.findByTestId(`skill-card-${CATALOG_OPTIONAL}`); + await waitFor(() => + expect(screen.getByTestId("skills-result-summary")).toBeInTheDocument(), + ); + + expect(saveSpy).not.toHaveBeenCalled(); + }); + + it("saves a catalog toggle to enabled_skills rather than the deny-list", async () => { + const user = userEvent.setup(); + vi.spyOn(SkillsService, "getSkills").mockResolvedValue([ + buildSkill({ name: CATALOG_RECOMMENDED }), + ]); + vi.spyOn(SettingsService, "getSettings").mockResolvedValue( + buildSettings({ enabled_skills: [CATALOG_RECOMMENDED] }), + ); + const saveSpy = vi + .spyOn(SettingsService, "saveSettings") + .mockResolvedValue(true); + + renderSkillsSettingsScreen(); + const card = await screen.findByTestId(`skill-card-${CATALOG_RECOMMENDED}`); + + await user.click( + within(card).getByTestId(`skill-toggle-${CATALOG_RECOMMENDED}`), + ); + + await waitFor(() => + expect(saveSpy).toHaveBeenCalledWith( + expect.objectContaining({ + enabled_skills: [], + disabled_skills: [], + }), + ), + ); + }); + + it("drops a re-enabled catalog skill from an unmigrated deny-list", async () => { + // A stale deny entry would otherwise veto the allow-list entry we just + // wrote, leaving the toggle on and the skill still absent. + const user = userEvent.setup(); + vi.spyOn(SkillsService, "getSkills").mockResolvedValue([ + buildSkill({ name: CATALOG_RECOMMENDED }), + ]); + vi.spyOn(SettingsService, "getSettings").mockResolvedValue( + buildSettings({ + enabled_skills: undefined, + disabled_skills: [CATALOG_RECOMMENDED], + }), + ); + const saveSpy = vi + .spyOn(SettingsService, "saveSettings") + .mockResolvedValue(true); + + renderSkillsSettingsScreen(); + const card = await screen.findByTestId(`skill-card-${CATALOG_RECOMMENDED}`); + + await user.click( + within(card).getByTestId(`skill-toggle-${CATALOG_RECOMMENDED}`), + ); + + await waitFor(() => + expect(saveSpy).toHaveBeenCalledWith( + expect.objectContaining({ + enabled_skills: expect.arrayContaining([CATALOG_RECOMMENDED]), + disabled_skills: [], + }), + ), + ); + }); }); diff --git a/__tests__/utils/skill-enablement.test.ts b/__tests__/utils/skill-enablement.test.ts new file mode 100644 index 0000000000..93e7821018 --- /dev/null +++ b/__tests__/utils/skill-enablement.test.ts @@ -0,0 +1,161 @@ +import { describe, expect, it } from "vitest"; +import { + DEFAULT_ENABLED_SKILL_NAMES, + SKILLS_CATALOG, +} from "@openhands/extensions/skills"; +import { + buildSkillEnablementFilter, + findInvokedCatalogSkill, + isCatalogSkill, + isRecommendedSkill, + migrateSkillEnablement, + resolveEnabledCatalogSkills, + toSkillEnablement, +} from "#/utils/skill-enablement"; + +// Two anchors from the bundled catalog: `add-skill` carries `defaultEnabled`, +// `add-javadoc` is the language-specific kind of skill OpenHands#16302 asked +// not to be opted into. +const RECOMMENDED = "add-skill"; +const OPTIONAL = "add-javadoc"; +const LOCAL = "house-rules"; + +describe("catalog membership", () => { + it("recognises bundled skills and ignores locally authored ones", () => { + expect(isCatalogSkill(RECOMMENDED)).toBe(true); + expect(isCatalogSkill(OPTIONAL)).toBe(true); + expect(isCatalogSkill(LOCAL)).toBe(false); + }); + + it("marks only the catalog's default-enabled entries as recommended", () => { + expect(isRecommendedSkill(RECOMMENDED)).toBe(true); + expect(isRecommendedSkill(OPTIONAL)).toBe(false); + expect(DEFAULT_ENABLED_SKILL_NAMES.length).toBeLessThan( + SKILLS_CATALOG.length, + ); + }); +}); + +describe("resolveEnabledCatalogSkills", () => { + it("falls back to the curated default when nothing is persisted", () => { + expect(resolveEnabledCatalogSkills({})).toEqual([ + ...DEFAULT_ENABLED_SKILL_NAMES, + ]); + }); + + it("uses the persisted allow-list verbatim, empty list included", () => { + expect(resolveEnabledCatalogSkills({ enabledSkills: [OPTIONAL] })).toEqual([ + OPTIONAL, + ]); + expect(resolveEnabledCatalogSkills({ enabledSkills: [] })).toEqual([]); + }); +}); + +describe("buildSkillEnablementFilter", () => { + it("keeps user- and project-authored skills on unless denied", () => { + expect(buildSkillEnablementFilter({ enabledSkills: [] })(LOCAL)).toBe(true); + expect(buildSkillEnablementFilter({ disabledSkills: [LOCAL] })(LOCAL)).toBe( + false, + ); + }); + + it("requires a catalog skill to be on the allow-list", () => { + expect(buildSkillEnablementFilter({})(OPTIONAL)).toBe(false); + expect(buildSkillEnablementFilter({})(RECOMMENDED)).toBe(true); + expect( + buildSkillEnablementFilter({ enabledSkills: [OPTIONAL] })(OPTIONAL), + ).toBe(true); + }); + + it("lets an unmigrated deny-list veto a default-enabled skill", () => { + // Until the migration runs, an existing "I turned this off" lives in the + // deny-list alone; the allow-list fallback must not switch it back on. + expect( + buildSkillEnablementFilter({ disabledSkills: [RECOMMENDED] })( + RECOMMENDED, + ), + ).toBe(false); + }); +}); + +describe("migrateSkillEnablement", () => { + it("does nothing once an allow-list is persisted", () => { + expect(migrateSkillEnablement({ enabledSkills: [] })).toBeUndefined(); + expect( + migrateSkillEnablement({ enabledSkills: [RECOMMENDED] }), + ).toBeUndefined(); + }); + + it("preserves an existing workspace's all-on-minus-deny-list set", () => { + const migrated = migrateSkillEnablement({ disabledSkills: [OPTIONAL] }); + + expect(migrated?.enabled_skills).toHaveLength(SKILLS_CATALOG.length - 1); + expect(migrated?.enabled_skills).not.toContain(OPTIONAL); + expect(migrated?.enabled_skills).toContain(RECOMMENDED); + // The catalog name has moved to the allow-list; a leftover deny entry + // would veto the skill if the user switched it back on later. + expect(migrated?.disabled_skills).toEqual([]); + }); + + it("keeps local deny entries, which the allow-list does not cover", () => { + const migrated = migrateSkillEnablement({ + disabledSkills: [OPTIONAL, LOCAL], + }); + + expect(migrated?.disabled_skills).toEqual([LOCAL]); + }); + + it("gives a fresh workspace the curated default", () => { + expect(migrateSkillEnablement({})?.enabled_skills).toEqual([ + ...DEFAULT_ENABLED_SKILL_NAMES, + ]); + // A deny-list naming only local skills says nothing about the catalog. + expect( + migrateSkillEnablement({ disabledSkills: [LOCAL] })?.enabled_skills, + ).toEqual([...DEFAULT_ENABLED_SKILL_NAMES]); + }); + + it("persists the set even for a fresh workspace", () => { + // Without an explicit write, a catalog addition marked `defaultEnabled` + // would switch itself on in a workspace that never asked for it. + expect(migrateSkillEnablement({})).toBeDefined(); + }); +}); + +describe("toSkillEnablement", () => { + it("keeps an absent allow-list undefined, the unmigrated sentinel", () => { + expect(toSkillEnablement({ disabled_skills: [LOCAL] })).toEqual({ + enabledSkills: undefined, + disabledSkills: [LOCAL], + }); + }); +}); + +describe("findInvokedCatalogSkill", () => { + it("resolves a skill's own slash command, which is what automation cards send", () => { + expect(findInvokedCatalogSkill("/standup-digest:setup")).toBe( + "slack-standup-digest", + ); + expect(findInvokedCatalogSkill("/codereview please look at src/")).toBe( + "code-review", + ); + }); + + it("resolves `/`, which the Use skill button inserts", () => { + expect(findInvokedCatalogSkill(`/${OPTIONAL} MyClass.java`)).toBe(OPTIONAL); + }); + + it("only counts the leading token", () => { + // Matching a `/word` anywhere in prose would re-admit most of the catalog. + expect( + findInvokedCatalogSkill("what would /standup-digest:setup do?"), + ).toBeUndefined(); + }); + + it("ignores an empty, absent, or unknown command", () => { + expect(findInvokedCatalogSkill(undefined)).toBeUndefined(); + expect(findInvokedCatalogSkill(" ")).toBeUndefined(); + expect(findInvokedCatalogSkill("just a message")).toBeUndefined(); + expect(findInvokedCatalogSkill("/not-a-real-skill")).toBeUndefined(); + }); +}); diff --git a/src/api/agent-server-adapter.test.ts b/src/api/agent-server-adapter.test.ts index 909f9289a1..42f8ac9dde 100644 --- a/src/api/agent-server-adapter.test.ts +++ b/src/api/agent-server-adapter.test.ts @@ -128,7 +128,7 @@ describe("buildStartConversationRequest", () => { expect(payload.secrets_encrypted).toBeUndefined(); }); - it("excludes disabled skills from OpenHands conversation context", () => { + it("ships only allow-listed catalog skills to a OpenHands conversation context", () => { const settings = makeSettings({ agent_kind: "openhands", llm: { @@ -147,10 +147,18 @@ describe("buildStartConversationRequest", () => { const payload = buildStartConversationRequest({ settings }); const skillNames = getAgentContextSkillNames(payload); + // `agent-memory` is default-enabled, so this asserts the deny-list still + // wins over the allow-list — the case that matters before the one-shot + // migration has run. expect(skillNames).not.toContain("agent-memory"); expect(skillNames).not.toContain("disabled-custom"); + // Skills already on the agent context are user-authored and stay opt-out. expect(skillNames).toContain("enabled-custom"); - expect(skillNames).toContain("add-javadoc"); + // No `enabled_skills` on the settings means the curated default applies: + // `add-skill` is flagged `defaultEnabled` in the catalog, `add-javadoc` is + // not, and shipping every catalog skill is what OpenHands#16302 reported. + expect(skillNames).toContain("add-skill"); + expect(skillNames).not.toContain("add-javadoc"); expect(payload.agent_settings?.agent_context?.disabled_skills).toEqual([ "agent-memory", @@ -158,7 +166,7 @@ describe("buildStartConversationRequest", () => { ]); }); - it("excludes disabled skills from ACP conversation context", () => { + it("ships only allow-listed catalog skills to a ACP conversation context", () => { const settings = makeSettings({ agent_kind: "acp", acp_server: "codex", @@ -176,16 +184,77 @@ describe("buildStartConversationRequest", () => { const payload = buildStartConversationRequest({ settings }); const skillNames = getAgentContextSkillNames(payload); + // `agent-memory` is default-enabled, so this asserts the deny-list still + // wins over the allow-list — the case that matters before the one-shot + // migration has run. expect(skillNames).not.toContain("agent-memory"); expect(skillNames).not.toContain("disabled-custom"); + // Skills already on the agent context are user-authored and stay opt-out. expect(skillNames).toContain("enabled-custom"); - expect(skillNames).toContain("add-javadoc"); + // No `enabled_skills` on the settings means the curated default applies: + // `add-skill` is flagged `defaultEnabled` in the catalog, `add-javadoc` is + // not, and shipping every catalog skill is what OpenHands#16302 reported. + expect(skillNames).toContain("add-skill"); + expect(skillNames).not.toContain("add-javadoc"); expect(payload.agent_settings?.agent_context?.disabled_skills).toEqual([ "agent-memory", "disabled-custom", ]); }); + + it("loads a catalog skill the opening slash command invokes", () => { + // An automation card fills the chat input with the skill's own command + // (`findAutomationCommand`), and 18 of the catalog's 24 slash commands + // belong to skills that are off by default — without this the card would + // silently do nothing. + const settings = makeSettings({ + agent_kind: "openhands", + llm: { model: "litellm_proxy/openai/gpt-5.5", api_key: "sk-test" }, + }); + + const payload = buildStartConversationRequest({ + settings, + query: "/standup-digest:setup", + }); + + expect(getAgentContextSkillNames(payload)).toContain( + "slack-standup-digest", + ); + }); + + it("does not admit a skill named mid-sentence rather than invoked", () => { + const settings = makeSettings({ + agent_kind: "openhands", + llm: { model: "litellm_proxy/openai/gpt-5.5", api_key: "sk-test" }, + }); + + const payload = buildStartConversationRequest({ + settings, + query: "tell me what /standup-digest:setup would do", + }); + + expect(getAgentContextSkillNames(payload)).not.toContain( + "slack-standup-digest", + ); + }); + + it("lets an invoked skill override a stored deny entry for that conversation", () => { + const settings = makeSettings({ + agent_kind: "openhands", + llm: { model: "litellm_proxy/openai/gpt-5.5", api_key: "sk-test" }, + }); + settings.disabled_skills = ["slack-standup-digest"]; + + const payload = buildStartConversationRequest({ + settings, + query: "/standup-digest:setup", + }); + + expect(getAgentContextSkillNames(payload)).toContain( + "slack-standup-digest", + ); + }); }); describe("buildStartConversationRequest — agentProfileId path", () => { diff --git a/src/api/agent-server-adapter.ts b/src/api/agent-server-adapter.ts index dfc0702d26..eb99c0389a 100644 --- a/src/api/agent-server-adapter.ts +++ b/src/api/agent-server-adapter.ts @@ -26,6 +26,12 @@ import { SandboxStatus, } from "./conversation-service/agent-server-conversation-service.types"; import { combineUsageMetrics } from "#/utils/conversation-metrics"; +import { + buildSkillEnablementFilter, + findInvokedCatalogSkill, + toSkillEnablement, + type SkillEnablement, +} from "#/utils/skill-enablement"; import SettingsService from "./settings-service/settings-service.api"; import { getStoredConversationMetadata } from "./conversation-metadata-store"; import LLMSubscriptionService from "./llm-subscription-service"; @@ -749,7 +755,8 @@ function buildBundledSkills(): BundledSkill[] { function buildAgentContext( agentSettings: SettingsRecord, runtimeServicesInfo?: RuntimeServicesInfo | null, - disabledSkills: string[] = [], + enablement: SkillEnablement = {}, + invokedCatalogSkill?: string, ): SettingsRecord { const runtimeServicesSuffix = buildRuntimeServicesSystemSuffix(runtimeServicesInfo); @@ -760,11 +767,25 @@ function buildAgentContext( const existingSkills = Array.isArray(existingContext.skills) ? (existingContext.skills as SettingsRecord[]) : []; + const disabledSkills = enablement.disabledSkills ?? []; const disabledSkillNames = new Set(disabledSkills); - const mergedSkills = [...existingSkills, ...buildBundledSkills()].filter( - (skill) => - typeof skill.name !== "string" || !disabledSkillNames.has(skill.name), - ); + const isSkillEnabled = buildSkillEnablementFilter(enablement); + + // The bundled catalog is allow-listed, not deny-listed: it is a build-time + // snapshot of ~60 skills, so a deny-list puts every future addition into + // every system prompt (OpenHands#16302). Skills the agent context already + // carries are user-authored and stay opt-out. A skill the opening message + // invokes by name overrides both, for this conversation only. + const mergedSkills = [ + ...existingSkills.filter( + (skill) => + typeof skill.name !== "string" || !disabledSkillNames.has(skill.name), + ), + ...buildBundledSkills().filter( + (skill) => + skill.name === invokedCatalogSkill || isSkillEnabled(skill.name), + ), + ]; return { ...existingContext, @@ -783,7 +804,8 @@ function buildAgentContext( load_project_skills: true, // The backend also auto-loads user/project skills; the deny-list must // travel with the context so those skills are excluded from the system - // prompt too. + // prompt too. The allow-list has no counterpart to send: the backend + // loads no catalog skills of its own (`load_public_skills` is false). disabled_skills: disabledSkills, ...(runtimeServicesSuffix ? { system_message_suffix: runtimeServicesSuffix } @@ -821,6 +843,7 @@ function resolveAcpCommand(agentSettings: SettingsRecord): unknown { function buildConfiguredAcpAgentSettings( settings: Settings, runtimeServicesInfo?: RuntimeServicesInfo | null, + query?: string, ): AgentSettingsPayload { const agentSettings = toRecord(settings.agent_settings); const payload: AgentSettingsPayload = { @@ -828,7 +851,8 @@ function buildConfiguredAcpAgentSettings( agent_context: buildAgentContext( agentSettings, runtimeServicesInfo, - settings.disabled_skills, + toSkillEnablement(settings), + findInvokedCatalogSkill(query), ), }; @@ -887,6 +911,7 @@ function buildConfiguredAcpAgentSettings( function buildConfiguredOpenHandsAgentSettings( settings: Settings, runtimeServicesInfo?: RuntimeServicesInfo | null, + query?: string, ): AgentSettingsPayload { const agentSettings = toRecord(settings.agent_settings); const llm = toRecord(agentSettings.llm); @@ -944,7 +969,8 @@ function buildConfiguredOpenHandsAgentSettings( agent_context: buildAgentContext( agentSettings, runtimeServicesInfo, - settings.disabled_skills, + toSkillEnablement(settings), + findInvokedCatalogSkill(query), ), tools: getAgentTools(agentSettings), }; @@ -953,10 +979,15 @@ function buildConfiguredOpenHandsAgentSettings( function buildConfiguredAgentSettings( settings: Settings, runtimeServicesInfo?: RuntimeServicesInfo | null, + query?: string, ): AgentSettingsPayload { return isAcpAgent(settings) - ? buildConfiguredAcpAgentSettings(settings, runtimeServicesInfo) - : buildConfiguredOpenHandsAgentSettings(settings, runtimeServicesInfo); + ? buildConfiguredAcpAgentSettings(settings, runtimeServicesInfo, query) + : buildConfiguredOpenHandsAgentSettings( + settings, + runtimeServicesInfo, + query, + ); } function buildConfiguredConversationSettings(options: { @@ -1067,6 +1098,7 @@ export function buildStartConversationRequest( const agentSettings = buildConfiguredAgentSettings( sourceAgentSettings, options.runtimeServicesInfo, + options.query, ); const acpServerTag = acpMode ? getAcpServerTag(sourceAgentSettings) diff --git a/src/api/settings-service/settings-service.api.ts b/src/api/settings-service/settings-service.api.ts index 6da2fc926c..f8f9cdec5e 100644 --- a/src/api/settings-service/settings-service.api.ts +++ b/src/api/settings-service/settings-service.api.ts @@ -30,6 +30,7 @@ export const APP_PREFERENCE_FIELDS = [ "git_user_email", "title_llm_profile", "disabled_skills", + "enabled_skills", ] as const; export type AppPreferenceField = (typeof APP_PREFERENCE_FIELDS)[number]; @@ -72,7 +73,8 @@ export interface SettingsApiResponse { * `conversation_settings_diff`. A partial diff like * `{ app_preferences: { language: "fr" } }` updates only `language` and * leaves every other `app_preferences` field alone. Lists - * (`disabled_skills`) are replaced wholesale rather than merged. + * (`disabled_skills`, `enabled_skills`) are replaced wholesale rather than + * merged. */ export interface SettingsUpdateRequest { agent_settings_diff?: Record; diff --git a/src/components/features/conversation-panel/skills-modal.tsx b/src/components/features/conversation-panel/skills-modal.tsx index d548ae6949..2dee4f6130 100644 --- a/src/components/features/conversation-panel/skills-modal.tsx +++ b/src/components/features/conversation-panel/skills-modal.tsx @@ -5,6 +5,7 @@ import { ModalBody } from "#/components/shared/modals/modal-body"; import { I18nKey } from "#/i18n/declaration"; import { getAgentServerWorkingDir } from "#/api/agent-server-config"; import { useConversationSkills } from "#/hooks/query/use-conversation-skills"; +import { useSkillEnabledFilter } from "#/hooks/use-skill-enablement"; import { groupSkillsByScope, SKILL_SCOPE_ORDER, @@ -41,10 +42,18 @@ export function SkillsModal({ onClose }: SkillsModalProps) { refetch, isRefetching, } = useConversationSkills(); + const isSkillEnabled = useSkillEnabledFilter(); + + // The modal reports what the conversation actually has, so it lists the + // enabled set rather than the whole catalog. + const visibleSkills = useMemo( + () => (skills ?? []).filter(isSkillEnabled), + [skills, isSkillEnabled], + ); const groupedSkills = useMemo( - () => (skills ? groupSkillsByScope(skills, projectDir) : null), - [skills, projectDir], + () => groupSkillsByScope(visibleSkills, projectDir), + [visibleSkills, projectDir], ); const toggleAgent = (agentName: string) => { @@ -71,7 +80,7 @@ export function SkillsModal({ onClose }: SkillsModalProps) {
{isLoading ? ( - ) : isError || !skills || skills.length === 0 ? ( + ) : isError || !skills || visibleSkills.length === 0 ? ( ) : ( groupedSkills && ( diff --git a/src/components/features/conversation/conversation-overview-skills-panel.tsx b/src/components/features/conversation/conversation-overview-skills-panel.tsx index 58cb6eda18..c483fafc85 100644 --- a/src/components/features/conversation/conversation-overview-skills-panel.tsx +++ b/src/components/features/conversation/conversation-overview-skills-panel.tsx @@ -1,10 +1,10 @@ import React, { useEffect, useMemo, useState } from "react"; import { useTranslation } from "react-i18next"; import { I18nKey } from "#/i18n/declaration"; -import { useSaveSettings } from "#/hooks/mutation/use-save-settings"; import { useSettings } from "#/hooks/query/use-settings"; import { useActiveConversation } from "#/hooks/query/use-active-conversation"; import { useConversationSkills } from "#/hooks/query/use-conversation-skills"; +import { useSkillEnablement } from "#/hooks/use-skill-enablement"; import { SkillCard } from "#/components/features/skills/skill-card"; import { SkillDetailModal } from "#/components/features/skills/skill-detail-modal"; import { AddSkillModal } from "#/components/features/skills/add-skill-modal"; @@ -24,8 +24,6 @@ import { extensionModuleCardGridContainerClassName, extensionModuleEmptyStateClassName, } from "#/utils/extension-module-card-classes"; -import { displayErrorToast } from "#/utils/custom-toast-handlers"; -import { retrieveAxiosErrorMessage } from "#/utils/retrieve-axios-error-message"; import type { SkillInfo } from "#/types/settings"; import { cn } from "#/utils/utils"; import { @@ -45,17 +43,14 @@ export function ConversationOverviewSkillsPanel({ openAdd, }: ConversationOverviewSkillsPanelProps) { const { t } = useTranslation("openhands"); - const { mutate: saveSettings } = useSaveSettings(); - const { data: settings, isLoading: settingsLoading } = useSettings(); + const { isLoading: settingsLoading } = useSettings(); const { data: conversation } = useActiveConversation(); const { data: skills, isLoading: skillsLoading } = useConversationSkills(); + const { isEnabled, setEnabled } = useSkillEnablement(); const addRequestKey = useConversationOverviewDrawerOptional()?.addRequestKey ?? 0; const projectDir = conversation?.selected_workspace ?? null; - const [disabledSet, setDisabledSet] = useState>(new Set()); - const [hasHydratedInitialSettings, setHasHydratedInitialSettings] = - useState(false); const [projectScope, setProjectScope] = useState( CONVERSATION_OVERVIEW_PROJECT_SCOPE.project, @@ -67,14 +62,6 @@ export function ConversationOverviewSkillsPanel({ const [selectedSkill, setSelectedSkill] = useState(null); const [showAddSkillModal, setShowAddSkillModal] = useState(false); - useEffect(() => { - if (settingsLoading || !settings) { - return; - } - setDisabledSet(new Set(settings.disabled_skills ?? [])); - setHasHydratedInitialSettings(true); - }, [settingsLoading, settings?.disabled_skills]); - useEffect(() => { if (!openAdd) { return; @@ -89,22 +76,6 @@ export function ConversationOverviewSkillsPanel({ setShowAddSkillModal(true); }, [addRequestKey]); - useEffect(() => { - if (!hasHydratedInitialSettings) { - return; - } - saveSettings( - { disabled_skills: Array.from(disabledSet) }, - { - onError: (error) => { - displayErrorToast( - retrieveAxiosErrorMessage(error) || t(I18nKey.ERROR$GENERIC), - ); - }, - }, - ); - }, [disabledSet, hasHydratedInitialSettings, saveSettings, t]); - const scopedSkills = useMemo(() => { if (!skills) { return []; @@ -120,22 +91,10 @@ export function ConversationOverviewSkillsPanel({ }, [skills, projectScope, projectDir]); const filteredSkills = useMemo( - () => applySkillFilters(scopedSkills, disabledSet, filter), - [scopedSkills, disabledSet, filter], + () => applySkillFilters(scopedSkills, isEnabled, filter), + [scopedSkills, isEnabled, filter], ); - const handleToggle = (skillName: string, enabled: boolean) => { - setDisabledSet((previous) => { - const next = new Set(previous); - if (enabled) { - next.delete(skillName); - } else { - next.add(skillName); - } - return next; - }); - }; - if (skillsLoading || settingsLoading) { return (
setSelectedSkill(skill)} - onToggle={(enabled) => handleToggle(skill.name, enabled)} + onToggle={(enabled) => setEnabled(skill.name, enabled)} /> ))}
@@ -229,8 +188,8 @@ export function ConversationOverviewSkillsPanel({ {selectedSkill ? ( handleToggle(selectedSkill.name, enabled)} + enabled={isEnabled(selectedSkill)} + onToggle={(enabled) => setEnabled(selectedSkill.name, enabled)} onClose={() => setSelectedSkill(null)} /> ) : null} @@ -241,7 +200,7 @@ export function ConversationOverviewSkillsPanel({ {isFiltersModalOpen ? ( setFilter((previous) => diff --git a/src/components/features/skills/build-skill-pills.tsx b/src/components/features/skills/build-skill-pills.tsx index 68501cc332..c18da4e033 100644 --- a/src/components/features/skills/build-skill-pills.tsx +++ b/src/components/features/skills/build-skill-pills.tsx @@ -13,6 +13,7 @@ import { SKILL_CATEGORY_LABEL_KEYS, UNCATEGORIZED_SKILL_CATEGORY, } from "#/utils/skill-category"; +import { isRecommendedSkill } from "#/utils/skill-enablement"; type SkillPillVariant = "card" | "detail"; @@ -46,6 +47,23 @@ export function buildSkillPills( }, ]; + if (isRecommendedSkill(skill.name)) { + pills.push({ + id: "recommended", + node: ( + + {translate(I18nKey.SETTINGS$SKILLS_RECOMMENDED)} + + ), + }); + } + // An "Other" pill says nothing, and every local skill would carry one: // only the catalog assigns categories. if (category !== UNCATEGORIZED_SKILL_CATEGORY) { diff --git a/src/components/features/skills/skill-filter.ts b/src/components/features/skills/skill-filter.ts index cb6e26048b..3407ad8082 100644 --- a/src/components/features/skills/skill-filter.ts +++ b/src/components/features/skills/skill-filter.ts @@ -11,6 +11,7 @@ import { SKILL_SCOPE_ORDER, type SkillScope, } from "#/utils/skill-scope"; +import { isRecommendedSkill } from "#/utils/skill-enablement"; import { getSkillCardDescription } from "./get-skill-card-description"; export const SKILL_FILTER_QUERY_PARAM = "q"; @@ -18,14 +19,25 @@ const SOURCE_PARAM = "source"; const CATEGORY_PARAM = "category"; const TYPE_PARAM = "type"; const STATE_PARAM = "state"; +const RECOMMENDATION_PARAM = "recommendation"; export type SkillEnabledState = "enabled" | "disabled"; -export type SkillFacetGroupId = "state" | "source" | "category" | "type"; +export type SkillRecommendation = "recommended" | "other"; +export type SkillFacetGroupId = + | "state" + | "recommendation" + | "source" + | "category" + | "type"; const SKILL_ENABLED_STATE_ORDER: readonly SkillEnabledState[] = [ "enabled", "disabled", ]; +const SKILL_RECOMMENDATION_ORDER: readonly SkillRecommendation[] = [ + "recommended", + "other", +]; const SKILL_TYPE_ORDER: readonly SkillType[] = [ "agentskills", "knowledge", @@ -38,6 +50,7 @@ export interface SkillFilterState { categories: Set; types: Set; states: Set; + recommendations: Set; } export interface SkillFacetRowModel { @@ -60,6 +73,7 @@ export const EMPTY_SKILL_FILTER_STATE: SkillFilterState = { categories: new Set(), types: new Set(), states: new Set(), + recommendations: new Set(), }; /** Carrying `labelKey`s rather than translated strings keeps this module pure, so its tests need no i18n. */ @@ -75,13 +89,16 @@ function labelledValues( return order.map((value) => ({ value, labelKey: labelKeys[value] })); } +/** Resolving the two lists behind enablement is the caller's job, not this module's. */ +type SkillEnabledPredicate = (skill: SkillInfo) => boolean; + /** A new facet group is declared here and nowhere else: `selected` / `withSelected` are what keep every operation below driven by this table alone. */ interface GroupDef { id: SkillFacetGroupId; labelKey: I18nKey; param: string; values: readonly GroupValue[]; - valueOf: (skill: SkillInfo, disabledSet: Set) => string; + valueOf: (skill: SkillInfo, isEnabled: SkillEnabledPredicate) => string; selected: (state: SkillFilterState) => Set; withSelected: ( state: SkillFilterState, @@ -114,20 +131,40 @@ const STATE_LABEL_KEYS: Record = { disabled: I18nKey.SETTINGS$SKILLS_DISABLED, }; +const RECOMMENDATION_LABEL_KEYS: Record = { + recommended: I18nKey.SETTINGS$SKILLS_RECOMMENDED, + other: I18nKey.SETTINGS$SKILLS_RECOMMENDATION_OTHER, +}; + const GROUP_DEFS: readonly GroupDef[] = [ { id: "state", labelKey: I18nKey.SETTINGS$SKILLS_FACET_STATE, param: STATE_PARAM, values: labelledValues(SKILL_ENABLED_STATE_ORDER, STATE_LABEL_KEYS), - valueOf: (skill, disabledSet) => - disabledSet.has(skill.name) ? "disabled" : "enabled", + valueOf: (skill, isEnabled) => (isEnabled(skill) ? "enabled" : "disabled"), selected: (state) => state.states, withSelected: (state, next) => ({ ...state, states: narrowSet(SKILL_ENABLED_STATE_ORDER, next), }), }, + { + id: "recommendation", + labelKey: I18nKey.SETTINGS$SKILLS_FACET_RECOMMENDATION, + param: RECOMMENDATION_PARAM, + values: labelledValues( + SKILL_RECOMMENDATION_ORDER, + RECOMMENDATION_LABEL_KEYS, + ), + valueOf: (skill) => + isRecommendedSkill(skill.name) ? "recommended" : "other", + selected: (state) => state.recommendations, + withSelected: (state, next) => ({ + ...state, + recommendations: narrowSet(SKILL_RECOMMENDATION_ORDER, next), + }), + }, { id: "source", labelKey: I18nKey.SETTINGS$SKILLS_FACET_SOURCE, @@ -235,7 +272,7 @@ function matchesQuery(skill: SkillInfo, query: string): boolean { */ function matchesFacets( skill: SkillInfo, - disabledSet: Set, + isEnabled: SkillEnabledPredicate, state: SkillFilterState, exclude?: SkillFacetGroupId, ): boolean { @@ -243,30 +280,30 @@ function matchesFacets( if (def.id === exclude) return true; const selected = def.selected(state); if (selected.size === 0) return true; - return selected.has(def.valueOf(skill, disabledSet)); + return selected.has(def.valueOf(skill, isEnabled)); }); } export function applySkillFilters( skills: SkillInfo[], - disabledSet: Set, + isEnabled: SkillEnabledPredicate, state: SkillFilterState, ): SkillInfo[] { return skills.filter( (skill) => matchesQuery(skill, state.query) && - matchesFacets(skill, disabledSet, state), + matchesFacets(skill, isEnabled, state), ); } function countByValue( skills: SkillInfo[], def: GroupDef, - disabledSet: Set, + isEnabled: SkillEnabledPredicate, ): Record { const counts: Record = {}; for (const skill of skills) { - const value = def.valueOf(skill, disabledSet); + const value = def.valueOf(skill, isEnabled); counts[value] = (counts[value] ?? 0) + 1; } return counts; @@ -276,13 +313,13 @@ function buildGroup( def: GroupDef, allSkills: SkillInfo[], searched: SkillInfo[], - disabledSet: Set, + isEnabled: SkillEnabledPredicate, state: SkillFilterState, ): SkillFacetGroup | null { // Visibility and the row set come from the raw list so the rail's shape // depends only on what the user has, never on the active filters. If they // tracked filtered counts, narrowing could make a group vanish mid-click. - const rawCounts = countByValue(allSkills, def, disabledSet); + const rawCounts = countByValue(allSkills, def, isEnabled); const discriminating = def.values.filter( ({ value }) => (rawCounts[value] ?? 0) > 0, ); @@ -298,9 +335,9 @@ function buildGroup( ); const candidates = searched.filter((skill) => - matchesFacets(skill, disabledSet, state, def.id), + matchesFacets(skill, isEnabled, state, def.id), ); - const counts = countByValue(candidates, def, disabledSet); + const counts = countByValue(candidates, def, isEnabled); return { id: def.id, @@ -321,13 +358,13 @@ function buildGroup( export function buildSkillFacetGroups( skills: SkillInfo[], - disabledSet: Set, + isEnabled: SkillEnabledPredicate, state: SkillFilterState, ): SkillFacetGroup[] { const searched = skills.filter((skill) => matchesQuery(skill, state.query)); return GROUP_DEFS.map((def) => - buildGroup(def, skills, searched, disabledSet, state), + buildGroup(def, skills, searched, isEnabled, state), ).filter((group): group is SkillFacetGroup => group !== null); } diff --git a/src/hooks/use-migrate-enabled-skills.ts b/src/hooks/use-migrate-enabled-skills.ts new file mode 100644 index 0000000000..e2a6300819 --- /dev/null +++ b/src/hooks/use-migrate-enabled-skills.ts @@ -0,0 +1,43 @@ +import { useEffect, useRef } from "react"; +import { isNoBackend } from "#/api/backend-registry/active-store"; +import { useActiveBackend } from "#/contexts/active-backend-context"; +import { useSaveSettings } from "#/hooks/mutation/use-save-settings"; +import { useSettings } from "#/hooks/query/use-settings"; +import { + migrateSkillEnablement, + toSkillEnablement, +} from "#/utils/skill-enablement"; + +/** + * Move a workspace to an explicit `enabled_skills` allow-list, once. + * + * It runs at the app root, not on the Customize page: until it has run the + * resolver falls back to the curated default, which would silently drop + * catalog skills an existing workspace had switched on. Local only — cloud + * never reads the field. + */ +export function useMigrateEnabledSkills(): void { + const { backend } = useActiveBackend(); + const isLocal = backend.kind === "local" && !isNoBackend(backend); + const { data: settings, isLoading, isError } = useSettings(); + const { mutate: saveSettings } = useSaveSettings(); + + // One attempt per backend: the save invalidates the settings query, so + // without this the refetch would re-enter before the write is visible. + const migratedBackendRef = useRef(null); + + useEffect(() => { + migratedBackendRef.current = null; + }, [backend.id]); + + useEffect(() => { + if (!isLocal || isLoading || isError || !settings) return; + if (migratedBackendRef.current === backend.id) return; + + migratedBackendRef.current = backend.id; + const migrated = migrateSkillEnablement(toSkillEnablement(settings)); + // Silent on failure: the resolver's fallback keeps the session working and + // the next app load retries. + if (migrated) saveSettings(migrated); + }, [isLocal, isLoading, isError, settings, backend.id, saveSettings]); +} diff --git a/src/hooks/use-skill-enablement.ts b/src/hooks/use-skill-enablement.ts new file mode 100644 index 0000000000..02734bc17e --- /dev/null +++ b/src/hooks/use-skill-enablement.ts @@ -0,0 +1,160 @@ +import React from "react"; +import { useTranslation } from "react-i18next"; +import { useActiveBackend } from "#/contexts/active-backend-context"; +import { useSaveSettings } from "#/hooks/mutation/use-save-settings"; +import { useSettings } from "#/hooks/query/use-settings"; +import { I18nKey } from "#/i18n/declaration"; +import type { Settings, SkillInfo } from "#/types/settings"; +import { displayErrorToast } from "#/utils/custom-toast-handlers"; +import { retrieveAxiosErrorMessage } from "#/utils/retrieve-axios-error-message"; +import { + buildSkillEnablementFilter, + CATALOG_SKILL_NAMES, + isCatalogSkill, + resolveEnabledCatalogSkills, + type SkillEnablement, +} from "#/utils/skill-enablement"; + +/** + * Settings → the two lists. Cloud creates conversations from its own + * server-side catalog and never reads `enabled_skills`, so the catalog stays + * deny-list governed there. + */ +function readSkillEnablement( + settings: Settings | undefined, + usesCatalogAllowList: boolean, +): SkillEnablement { + return { + enabledSkills: usesCatalogAllowList + ? settings?.enabled_skills + : [...CATALOG_SKILL_NAMES], + disabledSkills: settings?.disabled_skills ?? [], + }; +} + +/** Read-only view of the rule, for surfaces that list skills without toggling. */ +export function useSkillEnabledFilter(): (skill: SkillInfo) => boolean { + const { backend } = useActiveBackend(); + const { data: settings } = useSettings(); + const usesCatalogAllowList = backend.kind !== "cloud"; + + return React.useMemo(() => { + const isEnabled = buildSkillEnablementFilter( + readSkillEnablement(settings, usesCatalogAllowList), + ); + return (skill: SkillInfo) => isEnabled(skill.name); + }, [ + settings?.enabled_skills, + settings?.disabled_skills, + usesCatalogAllowList, + ]); +} + +export interface SkillEnablementController { + isEnabled: (skill: SkillInfo) => boolean; + setEnabled: (skillName: string, enabled: boolean) => void; +} + +/** Stable form of both lists, so an unchanged set can skip the save. */ +function snapshot(enablement: SkillEnablement): string { + return JSON.stringify([ + [...resolveEnabledCatalogSkills(enablement)].sort(), + [...(enablement.disabledSkills ?? [])].sort(), + ]); +} + +/** Same array reference when the membership already matches, to skip a save. */ +function withMembership( + list: string[], + name: string, + present: boolean, +): string[] { + if (list.includes(name) === present) return list; + return present ? [...list, name] : list.filter((entry) => entry !== name); +} + +/** + * Shared toggle state for every surface that switches skills on and off. + * + * Which of the two lists a skill belongs to is decided here alone, so one + * surface cannot write a preference another cannot see. + */ +export function useSkillEnablement(): SkillEnablementController { + const { t } = useTranslation("openhands"); + const { backend } = useActiveBackend(); + const usesCatalogAllowList = backend.kind !== "cloud"; + const { data: settings, isLoading: settingsLoading } = useSettings(); + const { mutate: saveSettings } = useSaveSettings(); + + const [enablement, setEnablement] = React.useState({}); + const savedRef = React.useRef(null); + + React.useEffect(() => { + if (settingsLoading || !settings) return; + const hydrated = readSkillEnablement(settings, usesCatalogAllowList); + savedRef.current = snapshot(hydrated); + setEnablement(hydrated); + }, [ + settingsLoading, + settings?.enabled_skills, + settings?.disabled_skills, + usesCatalogAllowList, + ]); + + React.useEffect(() => { + // Writing the hydrated value straight back would race the one-shot + // migration and could narrow a workspace it had just preserved. + const next = snapshot(enablement); + if (savedRef.current === null || savedRef.current === next) return; + savedRef.current = next; + + const disabledSkills = enablement.disabledSkills ?? []; + saveSettings( + usesCatalogAllowList + ? { + enabled_skills: resolveEnabledCatalogSkills(enablement), + disabled_skills: disabledSkills, + } + : { disabled_skills: disabledSkills }, + { + onError: (error) => { + displayErrorToast( + retrieveAxiosErrorMessage(error) || t(I18nKey.ERROR$GENERIC), + ); + }, + }, + ); + }, [enablement, usesCatalogAllowList, saveSettings, t]); + + const isEnabled = React.useMemo(() => { + const enabled = buildSkillEnablementFilter(enablement); + return (skill: SkillInfo) => enabled(skill.name); + }, [enablement]); + + const setEnabled = React.useCallback( + (skillName: string, enabled: boolean) => { + setEnablement((previous) => { + const allowListed = usesCatalogAllowList && isCatalogSkill(skillName); + return { + enabledSkills: allowListed + ? withMembership( + resolveEnabledCatalogSkills(previous), + skillName, + enabled, + ) + : previous.enabledSkills, + // A catalog skill switched back on also leaves the deny-list, where + // an unmigrated workspace may still hold it. + disabledSkills: withMembership( + previous.disabledSkills ?? [], + skillName, + !enabled && !allowListed, + ), + }; + }); + }, + [usesCatalogAllowList], + ); + + return { isEnabled, setEnabled }; +} diff --git a/src/i18n/translation.json b/src/i18n/translation.json index 9f96b10f1f..03a5565b6f 100644 --- a/src/i18n/translation.json +++ b/src/i18n/translation.json @@ -33183,6 +33183,74 @@ "uk": "Тип", "ca": "Tipus" }, + "SETTINGS$SKILLS_FACET_RECOMMENDATION": { + "ar": "التوصية", + "ca": "Recomanació", + "de": "Empfehlung", + "en": "Recommendation", + "es": "Recomendación", + "fr": "Recommandation", + "it": "Consiglio", + "ja": "推奨状況", + "ko-KR": "추천 여부", + "no": "Anbefaling", + "pt": "Recomendação", + "tr": "Öneri", + "uk": "Рекомендація", + "zh-CN": "推荐情况", + "zh-TW": "推薦狀態" + }, + "SETTINGS$SKILLS_RECOMMENDED": { + "ar": "موصى به", + "ca": "Recomanat", + "de": "Empfohlen", + "en": "Recommended", + "es": "Recomendado", + "fr": "Recommandé", + "it": "Consigliato", + "ja": "推奨", + "ko-KR": "권장", + "no": "Anbefalt", + "pt": "Recomendado", + "tr": "Önerilen", + "uk": "Рекомендовано", + "zh-CN": "推荐", + "zh-TW": "建議" + }, + "SETTINGS$SKILLS_RECOMMENDATION_OTHER": { + "ar": "أخرى", + "ca": "Altres", + "de": "Andere", + "en": "Other", + "es": "Otros", + "fr": "Autres", + "it": "Altri", + "ja": "その他", + "ko-KR": "기타", + "no": "Andre", + "pt": "Outros", + "tr": "Diğer", + "uk": "Інші", + "zh-CN": "其他", + "zh-TW": "其他" + }, + "SETTINGS$SKILLS_NEW_CONVERSATION_NOTICE": { + "ar": "تنطبق تغييرات المهارات على المحادثات الجديدة فقط.", + "ca": "Els canvis d'habilitats només s'apliquen a les converses noves.", + "de": "Änderungen an Fähigkeiten gelten nur für neue Unterhaltungen.", + "en": "Skill changes apply to new conversations only.", + "es": "Los cambios de habilidades solo se aplican a las conversaciones nuevas.", + "fr": "Les modifications des compétences s'appliquent uniquement aux nouvelles conversations.", + "it": "Le modifiche alle competenze si applicano solo alle nuove conversazioni.", + "ja": "スキルの変更は新しい会話にのみ適用されます。", + "ko-KR": "스킬 변경 사항은 새 대화에만 적용됩니다.", + "no": "Endringer i ferdigheter gjelder bare nye samtaler.", + "pt": "As alterações de habilidades aplicam-se apenas a novas conversas.", + "tr": "Yetenek değişiklikleri yalnızca yeni sohbetlere uygulanır.", + "uk": "Зміни навичок застосовуються лише до нових розмов.", + "zh-CN": "技能更改仅对新会话生效。", + "zh-TW": "技能變更僅對新對話生效。" + }, "SETTINGS$SKILLS_SOURCE_PROJECT": { "en": "This project", "ja": "このプロジェクト", diff --git a/src/routes/root-layout.tsx b/src/routes/root-layout.tsx index f5121d245d..d9cbcdd5ec 100644 --- a/src/routes/root-layout.tsx +++ b/src/routes/root-layout.tsx @@ -14,6 +14,7 @@ import { SidebarMobileNavProvider } from "#/components/features/sidebar/sidebar- import { SidebarMobileMenuBar } from "#/components/features/sidebar/sidebar-mobile-menu-bar"; import { useSettings } from "#/hooks/query/use-settings"; import { useEnsureActiveProfile } from "#/hooks/use-ensure-active-profile"; +import { useMigrateEnabledSkills } from "#/hooks/use-migrate-enabled-skills"; import { useSyncTelemetryConsent } from "#/hooks/use-sync-telemetry-consent"; import { useSyncAutomationTelemetryConsent } from "#/hooks/use-sync-automation-telemetry-consent"; @@ -83,6 +84,8 @@ export default function MainApp() { useTelemetryIdentity(); // Local-mode policy: keep a profile active so a usable LLM is always selected. useEnsureActiveProfile(); + // One-shot move from the catalog deny-list to an explicit allow-list. + useMigrateEnabledSkills(); React.useEffect(() => { if (settings?.language) { diff --git a/src/routes/skills-settings.tsx b/src/routes/skills-settings.tsx index 832dbcd8c5..09166b1945 100644 --- a/src/routes/skills-settings.tsx +++ b/src/routes/skills-settings.tsx @@ -21,18 +21,16 @@ import { type SkillFilterState, } from "#/components/features/skills/skill-filter"; import { SkillsToolbar } from "#/components/features/skills/skills-toolbar"; -import { useSaveSettings } from "#/hooks/mutation/use-save-settings"; import { useSettings } from "#/hooks/query/use-settings"; import { useSkills } from "#/hooks/query/use-skills"; +import { useSkillEnablement } from "#/hooks/use-skill-enablement"; import { I18nKey } from "#/i18n/declaration"; import type { SkillInfo } from "#/types/settings"; -import { displayErrorToast } from "#/utils/custom-toast-handlers"; import { extensionModuleCardGridClassName, extensionModuleCardGridContainerClassName, extensionModuleEmptyStateClassName, } from "#/utils/extension-module-card-classes"; -import { retrieveAxiosErrorMessage } from "#/utils/retrieve-axios-error-message"; import { settingsLikeMainScrollClassName } from "#/utils/settings-like-page-layout-classes"; import { cn } from "#/utils/utils"; @@ -41,9 +39,9 @@ const SEARCH_URL_SYNC_DELAY_MS = 300; function SkillsSettingsScreen() { const { t } = useTranslation("openhands"); - const { mutate: saveSettings } = useSaveSettings(); const { data: settings, isLoading: settingsLoading } = useSettings(); const { data: skills, isLoading: skillsLoading } = useSkills(); + const { isEnabled, setEnabled } = useSkillEnablement(); const [searchParams, setSearchParams] = useSearchParams(); const [queryInput, setQueryInput] = React.useState( @@ -52,9 +50,6 @@ function SkillsSettingsScreen() { // The last `q` this component put in the URL, which is what lets the sync effect below tell a history navigation apart from the lagging echo of its own debounced write. const lastWrittenQuery = React.useRef(queryInput); - const [disabledSet, setDisabledSet] = React.useState>(new Set()); - const [hasHydratedInitialSettings, setHasHydratedInitialSettings] = - React.useState(false); const [selectedSkill, setSelectedSkill] = React.useState( null, ); @@ -70,38 +65,17 @@ function SkillsSettingsScreen() { ); const groups = React.useMemo( - () => buildSkillFacetGroups(allSkills, disabledSet, filter), - [allSkills, disabledSet, filter], + () => buildSkillFacetGroups(allSkills, isEnabled, filter), + [allSkills, isEnabled, filter], ); const visibleSkills = React.useMemo( - () => applySkillFilters(allSkills, disabledSet, filter), - [allSkills, disabledSet, filter], + () => applySkillFilters(allSkills, isEnabled, filter), + [allSkills, isEnabled, filter], ); const activeFilterCount = countActiveFilters(filter); - // Sync local state with server settings when data first arrives - React.useEffect(() => { - if (settingsLoading || !settings) return; - setDisabledSet(new Set(settings.disabled_skills ?? [])); - setHasHydratedInitialSettings(true); - }, [settingsLoading, settings?.disabled_skills]); - - // Auto-save skill toggles once initial settings are loaded. - React.useEffect(() => { - if (!hasHydratedInitialSettings) return; - saveSettings( - { disabled_skills: Array.from(disabledSet) }, - { - onError: (error) => { - const errorMessage = retrieveAxiosErrorMessage(error); - displayErrorToast(errorMessage || t(I18nKey.ERROR$GENERIC)); - }, - }, - ); - }, [disabledSet, hasHydratedInitialSettings, saveSettings, t]); - // Back and forward move `q` under a route that stays mounted, so the input has to take the URL's value back or it would keep showing — and debounce back — a query the user has already navigated away from. React.useEffect(() => { const current = searchParams.get(SKILL_FILTER_QUERY_PARAM) ?? ""; @@ -150,18 +124,6 @@ function SkillsSettingsScreen() { const handleClearFacets = () => handleFilterChange(clearSkillFilterFacets(filter)); - const handleToggle = (skillName: string, enabled: boolean) => { - setDisabledSet((prev) => { - const next = new Set(prev); - if (enabled) { - next.delete(skillName); - } else { - next.add(skillName); - } - return next; - }); - }; - return (
{t(I18nKey.SETTINGS$SKILLS_PAGE_DESCRIPTION)}
+
+ {t(I18nKey.SETTINGS$SKILLS_NEW_CONVERSATION_NOTICE)} +
setSelectedSkill(skill)} onToggle={(enabled) => - handleToggle(skill.name, enabled) + setEnabled(skill.name, enabled) } /> ))} @@ -302,8 +270,8 @@ function SkillsSettingsScreen() { {selectedSkill && ( handleToggle(selectedSkill.name, enabled)} + enabled={isEnabled(selectedSkill)} + onToggle={(enabled) => setEnabled(selectedSkill.name, enabled)} onClose={() => setSelectedSkill(null)} /> )} diff --git a/src/types/settings.ts b/src/types/settings.ts index d4b3d574b5..5cbef5c02e 100644 --- a/src/types/settings.ts +++ b/src/types/settings.ts @@ -139,7 +139,14 @@ export type Settings = { search_api_key?: string; is_new_user?: boolean; mcp_config?: MCPConfig; + /** Deny-list over user- and project-authored skills. */ disabled_skills?: string[]; + /** + * Allow-list over the bundled `@openhands/extensions` catalog. `undefined` + * means "never migrated", the only signal `migrateSkillEnablement` has, so + * it is deliberately absent from `DEFAULT_SETTINGS`. + */ + enabled_skills?: string[]; max_budget_per_task: number | null; email?: string; email_verified?: boolean; diff --git a/src/utils/skill-enablement.ts b/src/utils/skill-enablement.ts new file mode 100644 index 0000000000..22f9c54411 --- /dev/null +++ b/src/utils/skill-enablement.ts @@ -0,0 +1,124 @@ +import { + DEFAULT_ENABLED_SKILL_NAMES, + SKILLS_CATALOG, +} from "@openhands/extensions/skills"; + +/** Every skill bundled from `@openhands/extensions`, in catalog order. */ +export const CATALOG_SKILL_NAMES: readonly string[] = SKILLS_CATALOG.map( + (entry) => entry.name, +); + +const CATALOG_SKILL_NAME_SET = new Set(CATALOG_SKILL_NAMES); +const RECOMMENDED_SKILL_NAME_SET = new Set(DEFAULT_ENABLED_SKILL_NAMES); + +export function isCatalogSkill(name: string): boolean { + return CATALOG_SKILL_NAME_SET.has(name); +} + +export function isRecommendedSkill(name: string): boolean { + return RECOMMENDED_SKILL_NAME_SET.has(name); +} + +/** + * The two persisted lists. They cover different populations: `enabledSkills` + * allow-lists the bundled catalog, whose every future addition would otherwise + * be on for everyone (#16302), while `disabledSkills` keeps denying user- and + * project-authored skills, which should be on the moment they appear. + * + * `undefined` means "never migrated" and must survive settings hydration. + */ +export interface SkillEnablement { + enabledSkills?: string[]; + disabledSkills?: string[]; +} + +export function resolveEnabledCatalogSkills( + enablement: SkillEnablement, +): string[] { + return enablement.enabledSkills ?? [...DEFAULT_ENABLED_SKILL_NAMES]; +} + +/** + * The one rule for "will this skill be loaded", resolved once per caller so + * per-skill checks stay cheap. + * + * The deny-list still wins over the allow-list, which only matters before the + * migration runs: until then a pre-existing "I turned this off" lives in the + * deny-list alone. + */ +export function buildSkillEnablementFilter( + enablement: SkillEnablement, +): (skillName: string) => boolean { + const enabled = new Set(resolveEnabledCatalogSkills(enablement)); + const disabled = new Set(enablement.disabledSkills ?? []); + + return (skillName) => { + if (disabled.has(skillName)) return false; + return !isCatalogSkill(skillName) || enabled.has(skillName); + }; +} + +/** + * One-shot conversion from "all catalog skills on, minus a deny-list" to an + * explicit allow-list; `undefined` once already migrated. + * + * A fresh workspace is migrated too, even though the resolver's fallback would + * give it the same set: persisting an explicit list is what stops a later + * `defaultEnabled` catalog addition from switching itself on. + */ +export function migrateSkillEnablement( + enablement: SkillEnablement, +): { enabled_skills: string[]; disabled_skills: string[] } | undefined { + if (enablement.enabledSkills !== undefined) return undefined; + + const disabled = new Set(enablement.disabledSkills ?? []); + // A deny-list naming a catalog skill is the only evidence that this + // workspace predates the allow-list; one holding local names alone says + // nothing about the catalog. + const isExistingWorkspace = [...disabled].some(isCatalogSkill); + + return { + enabled_skills: isExistingWorkspace + ? CATALOG_SKILL_NAMES.filter((name) => !disabled.has(name)) + : [...DEFAULT_ENABLED_SKILL_NAMES], + // Catalog names move to the allow-list; a leftover deny entry would veto + // a skill the user later switches back on. + disabled_skills: [...disabled].filter((name) => !isCatalogSkill(name)), + }; +} + +export function toSkillEnablement(settings: { + enabled_skills?: string[]; + disabled_skills?: string[]; +}): SkillEnablement { + return { + enabledSkills: settings.enabled_skills, + disabledSkills: settings.disabled_skills, + }; +} + +// Both forms a user can send: the commands a skill declares in its own +// `triggers` (what an automation card fills in — see `findAutomationCommand`), +// and `/`, which the detail modal's "Use skill" button inserts. +const CATALOG_SKILL_BY_SLASH_COMMAND = new Map( + SKILLS_CATALOG.flatMap((entry) => + [ + `/${entry.name}`, + ...(entry.triggers ?? []).filter((trigger) => trigger.startsWith("/")), + ].map((command) => [command.toLowerCase(), entry.name] as const), + ), +); + +/** + * The catalog skill a message invokes by name, if any. + * + * 18 of the catalog's 24 slash commands belong to skills that are off by + * default, so without this an automation card would send its command with none + * of the instructions behind it. Only the leading token counts: matching a + * `/word` anywhere in prose would re-admit most of the catalog. + */ +export function findInvokedCatalogSkill(query?: string): string | undefined { + const firstToken = query?.trim().split(/\s+/, 1)[0]; + if (!firstToken?.startsWith("/")) return undefined; + return CATALOG_SKILL_BY_SLASH_COMMAND.get(firstToken.toLowerCase()); +}