feat(skills): replace the all-on skill catalog with an explicit allow-list (#16860)

This commit is contained in:
Vasco Schiavo
2026-08-24 20:53:41 +02:00
committed by GitHub
parent c146a9e75a
commit c4c5bb7467
21 changed files with 1128 additions and 156 deletions
Binary file not shown.

After

Width:  |  Height:  |  Size: 197 KiB

Binary file not shown.

After

Width:  |  Height:  |  Size: 199 KiB

@@ -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> = {},
): 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;
@@ -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(<SkillsModal {...defaultProps} />);
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(<SkillsModal {...defaultProps} />);
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");
});
});
@@ -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<string, unknown> = {}) {
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());
});
});
+133
View File
@@ -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: [],
}),
),
);
});
});
+161
View File
@@ -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 `/<skill-name>`, 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();
});
});
+73 -4
View File
@@ -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", () => {
+42 -10
View File
@@ -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)
@@ -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<string, SettingsValue>;
@@ -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) {
<div className="w-full h-[60vh] overflow-auto rounded-md border border-[var(--oh-border)] bg-surface-raised custom-scrollbar-always">
{isLoading ? (
<SkillsLoadingState />
) : isError || !skills || skills.length === 0 ? (
) : isError || !skills || visibleSkills.length === 0 ? (
<SkillsEmptyState isError={isError} />
) : (
groupedSkills && (
@@ -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<Set<string>>(new Set());
const [hasHydratedInitialSettings, setHasHydratedInitialSettings] =
useState(false);
const [projectScope, setProjectScope] =
useState<ConversationOverviewProjectScope>(
CONVERSATION_OVERVIEW_PROJECT_SCOPE.project,
@@ -67,14 +62,6 @@ export function ConversationOverviewSkillsPanel({
const [selectedSkill, setSelectedSkill] = useState<SkillInfo | null>(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 (
<div
@@ -217,9 +176,9 @@ export function ConversationOverviewSkillsPanel({
<SkillCard
key={skill.name}
skill={skill}
enabled={!disabledSet.has(skill.name)}
enabled={isEnabled(skill)}
onOpen={() => setSelectedSkill(skill)}
onToggle={(enabled) => handleToggle(skill.name, enabled)}
onToggle={(enabled) => setEnabled(skill.name, enabled)}
/>
))}
</div>
@@ -229,8 +188,8 @@ export function ConversationOverviewSkillsPanel({
{selectedSkill ? (
<SkillDetailModal
skill={selectedSkill}
enabled={!disabledSet.has(selectedSkill.name)}
onToggle={(enabled) => 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 ? (
<SkillFiltersModal
groups={buildSkillFacetGroups(scopedSkills, disabledSet, filter)}
groups={buildSkillFacetGroups(scopedSkills, isEnabled, filter)}
activeCount={countActiveFilters(filter)}
onToggle={(groupId, value) =>
setFilter((previous) =>
@@ -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: (
<span
data-testid={
pillTestId(testIdPrefix, skill.name, "recommended") ??
`skill-recommended-${skill.name}`
}
className={SKILL_CARD_PILL_CLASS}
>
{translate(I18nKey.SETTINGS$SKILLS_RECOMMENDED)}
</span>
),
});
}
// An "Other" pill says nothing, and every local skill would carry one:
// only the catalog assigns categories.
if (category !== UNCATEGORIZED_SKILL_CATEGORY) {
+53 -16
View File
@@ -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<SkillCategoryId>;
types: Set<SkillType>;
states: Set<SkillEnabledState>;
recommendations: Set<SkillRecommendation>;
}
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<TValue extends string>(
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>) => string;
valueOf: (skill: SkillInfo, isEnabled: SkillEnabledPredicate) => string;
selected: (state: SkillFilterState) => Set<string>;
withSelected: (
state: SkillFilterState,
@@ -114,20 +131,40 @@ const STATE_LABEL_KEYS: Record<SkillEnabledState, I18nKey> = {
disabled: I18nKey.SETTINGS$SKILLS_DISABLED,
};
const RECOMMENDATION_LABEL_KEYS: Record<SkillRecommendation, I18nKey> = {
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<string>,
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<string>,
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<string>,
isEnabled: SkillEnabledPredicate,
): Record<string, number> {
const counts: Record<string, number> = {};
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<string>,
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<string>,
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);
}
+43
View File
@@ -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<string | null>(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]);
}
+160
View File
@@ -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<SkillEnablement>({});
const savedRef = React.useRef<string | null>(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 };
}
+68
View File
@@ -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": "このプロジェクト",
+3
View File
@@ -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) {
+16 -48
View File
@@ -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<Set<string>>(new Set());
const [hasHydratedInitialSettings, setHasHydratedInitialSettings] =
React.useState(false);
const [selectedSkill, setSelectedSkill] = React.useState<SkillInfo | null>(
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 (
<div
data-testid="skills-settings-screen"
@@ -184,6 +146,12 @@ function SkillsSettingsScreen() {
>
{t(I18nKey.SETTINGS$SKILLS_PAGE_DESCRIPTION)}
</div>
<div
data-testid="skills-new-conversation-notice"
className="max-w-2xl text-xs text-tertiary-alt"
>
{t(I18nKey.SETTINGS$SKILLS_NEW_CONVERSATION_NOTICE)}
</div>
</div>
<BrandButton
type="button"
@@ -273,10 +241,10 @@ function SkillsSettingsScreen() {
<SkillCard
key={skill.name}
skill={skill}
enabled={!disabledSet.has(skill.name)}
enabled={isEnabled(skill)}
onOpen={() => setSelectedSkill(skill)}
onToggle={(enabled) =>
handleToggle(skill.name, enabled)
setEnabled(skill.name, enabled)
}
/>
))}
@@ -302,8 +270,8 @@ function SkillsSettingsScreen() {
{selectedSkill && (
<SkillDetailModal
skill={selectedSkill}
enabled={!disabledSet.has(selectedSkill.name)}
onToggle={(enabled) => handleToggle(selectedSkill.name, enabled)}
enabled={isEnabled(selectedSkill)}
onToggle={(enabled) => setEnabled(selectedSkill.name, enabled)}
onClose={() => setSelectedSkill(null)}
/>
)}
+7
View File
@@ -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;
+124
View File
@@ -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 `/<skill-name>`, 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());
}