From a81704e622ee397cecd8055ea8c2987c713a2c24 Mon Sep 17 00:00:00 2001 From: Altay Date: Tue, 3 Mar 2026 02:00:05 +0300 Subject: [PATCH] fix(skills): scope skill-command APIs to respect agent allowlists (#32155) * refactor(skills): use explicit skill-command scope APIs * test(skills): cover scoped listing and telegram allowlist * fix(skills): add mergeSkillFilters edge-case tests and simplify dead code Cover unrestricted-co-tenant and empty-allowlist merge paths in skill-commands tests. Remove dead ternary in bot-handlers pagination. Add clarifying comments on undefined vs [] filter semantics. Co-Authored-By: Claude Opus 4.6 * refactor(skills): collapse scope functions into single listSkillCommandsForAgents Replace listSkillCommandsForAgentIds, listSkillCommandsForAllAgents, and the deprecated listSkillCommandsForAgents with a single function that accepts optional agentIds and falls back to all agents when omitted. Co-Authored-By: Claude Opus 4.6 * fix(skills): harden realpathSync race and add missing test coverage - Wrap fs.realpathSync in try-catch to gracefully skip workspaces that disappear between existsSync and realpathSync (TOCTOU race). - Log verbose diagnostics for missing/unresolvable workspace paths. - Add test for overlapping allowlists deduplication on shared workspaces. - Add test for graceful skip of missing workspaces. - Add test for pagination callback without agent suffix (default agent). - Clean up temp directories in skill-commands tests. Co-Authored-By: Claude Opus 4.6 * fix(telegram): warn when nativeSkillsEnabled but no agent route is bound Co-Authored-By: Claude Opus 4.6 * fix: use runtime.log instead of nonexistent runtime.warn Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- src/auto-reply/skill-commands.test.ts | 178 +++++++++++++++++- src/auto-reply/skill-commands.ts | 55 +++++- src/telegram/bot-handlers.ts | 4 +- ...t-native-commands.skills-allowlist.test.ts | 105 +++++++++++ src/telegram/bot-native-commands.ts | 10 +- src/telegram/bot.test.ts | 32 ++++ 6 files changed, 365 insertions(+), 19 deletions(-) create mode 100644 src/telegram/bot-native-commands.skills-allowlist.test.ts diff --git a/src/auto-reply/skill-commands.test.ts b/src/auto-reply/skill-commands.test.ts index 999ee9f84fc..e16446e5092 100644 --- a/src/auto-reply/skill-commands.test.ts +++ b/src/auto-reply/skill-commands.test.ts @@ -1,7 +1,7 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { beforeAll, describe, expect, it, vi } from "vitest"; +import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; // Avoid importing the full chat command registry for reserved-name calculation. vi.mock("./commands-registry.js", () => ({ @@ -44,14 +44,21 @@ vi.mock("../agents/skills.js", () => { return { buildWorkspaceSkillCommandSpecs: ( workspaceDir: string, - opts?: { reservedNames?: Set }, + opts?: { reservedNames?: Set; skillFilter?: string[] }, ) => { const used = new Set(); for (const reserved of opts?.reservedNames ?? []) { used.add(String(reserved).toLowerCase()); } + const filter = opts?.skillFilter; + const entries = + filter === undefined + ? resolveWorkspaceSkills(workspaceDir) + : resolveWorkspaceSkills(workspaceDir).filter((entry) => + filter.some((skillName) => skillName === entry.skillName), + ); - return resolveWorkspaceSkills(workspaceDir).map((entry) => { + return entries.map((entry) => { const base = entry.skillName.replace(/-/g, "_"); const name = resolveUniqueName(base, used); return { name, skillName: entry.skillName, description: entry.description }; @@ -106,8 +113,20 @@ describe("resolveSkillCommandInvocation", () => { }); describe("listSkillCommandsForAgents", () => { - it("merges command names across agents and de-duplicates", async () => { - const baseDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skills-")); + const tempDirs: string[] = []; + const makeTempDir = async (prefix: string) => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); + tempDirs.push(dir); + return dir; + }; + afterAll(async () => { + await Promise.all( + tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true })), + ); + }); + + it("lists all agents when agentIds is omitted", async () => { + const baseDir = await makeTempDir("openclaw-skills-"); const mainWorkspace = path.join(baseDir, "main"); const researchWorkspace = path.join(baseDir, "research"); await fs.mkdir(mainWorkspace, { recursive: true }); @@ -128,4 +147,153 @@ describe("listSkillCommandsForAgents", () => { expect(names).toContain("demo_skill_2"); expect(names).toContain("extra_skill"); }); + + it("scopes to specific agents when agentIds is provided", async () => { + const baseDir = await makeTempDir("openclaw-skills-filter-"); + const researchWorkspace = path.join(baseDir, "research"); + await fs.mkdir(researchWorkspace, { recursive: true }); + + const commands = listSkillCommandsForAgents({ + cfg: { + agents: { + list: [{ id: "research", workspace: researchWorkspace, skills: ["extra-skill"] }], + }, + }, + agentIds: ["research"], + }); + + expect(commands.map((entry) => entry.name)).toEqual(["extra_skill"]); + expect(commands.map((entry) => entry.skillName)).toEqual(["extra-skill"]); + }); + + it("prevents cross-agent skill leakage when each agent has an allowlist", async () => { + const baseDir = await makeTempDir("openclaw-skills-leak-"); + const mainWorkspace = path.join(baseDir, "main"); + const researchWorkspace = path.join(baseDir, "research"); + await fs.mkdir(mainWorkspace, { recursive: true }); + await fs.mkdir(researchWorkspace, { recursive: true }); + + const commands = listSkillCommandsForAgents({ + cfg: { + agents: { + list: [ + { id: "main", workspace: mainWorkspace, skills: ["demo-skill"] }, + { id: "research", workspace: researchWorkspace, skills: ["extra-skill"] }, + ], + }, + }, + agentIds: ["main", "research"], + }); + + expect(commands.map((entry) => entry.skillName)).toEqual(["demo-skill", "extra-skill"]); + expect(commands.map((entry) => entry.name)).toEqual(["demo_skill", "extra_skill"]); + }); + + it("merges allowlists for agents that share one workspace", async () => { + const baseDir = await makeTempDir("openclaw-skills-shared-"); + const sharedWorkspace = path.join(baseDir, "research"); + await fs.mkdir(sharedWorkspace, { recursive: true }); + + const commands = listSkillCommandsForAgents({ + cfg: { + agents: { + list: [ + { id: "main", workspace: sharedWorkspace, skills: ["demo-skill"] }, + { id: "research", workspace: sharedWorkspace, skills: ["extra-skill"] }, + ], + }, + }, + agentIds: ["main", "research"], + }); + + expect(commands.map((entry) => entry.skillName)).toEqual(["demo-skill", "extra-skill"]); + expect(commands.map((entry) => entry.name)).toEqual(["demo_skill", "extra_skill"]); + }); + + it("deduplicates overlapping allowlists for shared workspace", async () => { + const baseDir = await makeTempDir("openclaw-skills-overlap-"); + const sharedWorkspace = path.join(baseDir, "research"); + await fs.mkdir(sharedWorkspace, { recursive: true }); + + const commands = listSkillCommandsForAgents({ + cfg: { + agents: { + list: [ + { id: "agent-a", workspace: sharedWorkspace, skills: ["extra-skill"] }, + { id: "agent-b", workspace: sharedWorkspace, skills: ["extra-skill", "demo-skill"] }, + ], + }, + }, + agentIds: ["agent-a", "agent-b"], + }); + + // Both agents allowlist "extra-skill"; it should appear once, not twice. + expect(commands.map((entry) => entry.skillName)).toEqual(["demo-skill", "extra-skill"]); + expect(commands.map((entry) => entry.name)).toEqual(["demo_skill", "extra_skill"]); + }); + + it("keeps workspace unrestricted when one co-tenant agent has no skills filter", async () => { + const baseDir = await makeTempDir("openclaw-skills-unfiltered-"); + const sharedWorkspace = path.join(baseDir, "research"); + await fs.mkdir(sharedWorkspace, { recursive: true }); + + const commands = listSkillCommandsForAgents({ + cfg: { + agents: { + list: [ + { id: "restricted", workspace: sharedWorkspace, skills: ["extra-skill"] }, + { id: "unrestricted", workspace: sharedWorkspace }, + ], + }, + }, + agentIds: ["restricted", "unrestricted"], + }); + + const skillNames = commands.map((entry) => entry.skillName); + expect(skillNames).toContain("demo-skill"); + expect(skillNames).toContain("extra-skill"); + }); + + it("merges empty allowlist with non-empty allowlist for shared workspace", async () => { + const baseDir = await makeTempDir("openclaw-skills-empty-"); + const sharedWorkspace = path.join(baseDir, "research"); + await fs.mkdir(sharedWorkspace, { recursive: true }); + + const commands = listSkillCommandsForAgents({ + cfg: { + agents: { + list: [ + { id: "locked", workspace: sharedWorkspace, skills: [] }, + { id: "partial", workspace: sharedWorkspace, skills: ["extra-skill"] }, + ], + }, + }, + agentIds: ["locked", "partial"], + }); + + expect(commands.map((entry) => entry.skillName)).toEqual(["extra-skill"]); + }); + + it("skips agents with missing workspaces gracefully", async () => { + const baseDir = await makeTempDir("openclaw-skills-missing-"); + const validWorkspace = path.join(baseDir, "research"); + const missingWorkspace = path.join(baseDir, "nonexistent"); + await fs.mkdir(validWorkspace, { recursive: true }); + + const commands = listSkillCommandsForAgents({ + cfg: { + agents: { + list: [ + { id: "valid", workspace: validWorkspace }, + { id: "broken", workspace: missingWorkspace }, + ], + }, + }, + agentIds: ["valid", "broken"], + }); + + // The valid agent's skills should still be listed despite the broken one. + expect(commands.length).toBeGreaterThan(0); + expect(commands.map((entry) => entry.skillName)).toContain("demo-skill"); + }); }); diff --git a/src/auto-reply/skill-commands.ts b/src/auto-reply/skill-commands.ts index 49b851389d9..63c99e9ed03 100644 --- a/src/auto-reply/skill-commands.ts +++ b/src/auto-reply/skill-commands.ts @@ -1,7 +1,12 @@ import fs from "node:fs"; -import { listAgentIds, resolveAgentWorkspaceDir } from "../agents/agent-scope.js"; +import { + listAgentIds, + resolveAgentSkillsFilter, + resolveAgentWorkspaceDir, +} from "../agents/agent-scope.js"; import { buildWorkspaceSkillCommandSpecs, type SkillCommandSpec } from "../agents/skills.js"; import type { OpenClawConfig } from "../config/config.js"; +import { logVerbose } from "../globals.js"; import { getRemoteSkillEligibility } from "../infra/skills-remote.js"; import { listChatCommands } from "./commands-registry.js"; @@ -45,25 +50,57 @@ export function listSkillCommandsForAgents(params: { cfg: OpenClawConfig; agentIds?: string[]; }): SkillCommandSpec[] { + const mergeSkillFilters = (existing?: string[], incoming?: string[]): string[] | undefined => { + // undefined = no allowlist (unrestricted); [] = explicit empty allowlist (no skills). + // If any agent is unrestricted for this workspace, keep command discovery unrestricted. + if (existing === undefined || incoming === undefined) { + return undefined; + } + // An empty allowlist contributes no skills but does not widen the merge to unrestricted. + if (existing.length === 0) { + return Array.from(new Set(incoming)); + } + if (incoming.length === 0) { + return Array.from(new Set(existing)); + } + return Array.from(new Set([...existing, ...incoming])); + }; + + const agentIds = params.agentIds ?? listAgentIds(params.cfg); const used = listReservedChatSlashCommandNames(); const entries: SkillCommandSpec[] = []; - const agentIds = params.agentIds ?? listAgentIds(params.cfg); - // Track visited workspace dirs to avoid registering duplicate commands - // when multiple agents share the same workspace directory (#5717). - const visitedDirs = new Set(); + // Group by canonical workspace to avoid duplicate registration when multiple + // agents share the same directory (#5717), while still honoring per-agent filters. + const workspaceFilters = new Map(); for (const agentId of agentIds) { const workspaceDir = resolveAgentWorkspaceDir(params.cfg, agentId); if (!fs.existsSync(workspaceDir)) { + logVerbose(`Skipping agent "${agentId}": workspace does not exist: ${workspaceDir}`); continue; } - // Resolve to canonical path to handle symlinks and relative paths - const canonicalDir = fs.realpathSync(workspaceDir); - if (visitedDirs.has(canonicalDir)) { + let canonicalDir: string; + try { + canonicalDir = fs.realpathSync(workspaceDir); + } catch { + logVerbose(`Skipping agent "${agentId}": cannot resolve workspace: ${workspaceDir}`); continue; } - visitedDirs.add(canonicalDir); + const skillFilter = resolveAgentSkillsFilter(params.cfg, agentId); + const existing = workspaceFilters.get(canonicalDir); + if (existing) { + existing.skillFilter = mergeSkillFilters(existing.skillFilter, skillFilter); + continue; + } + workspaceFilters.set(canonicalDir, { + workspaceDir, + skillFilter, + }); + } + + for (const { workspaceDir, skillFilter } of workspaceFilters.values()) { const commands = buildWorkspaceSkillCommandSpecs(workspaceDir, { config: params.cfg, + skillFilter, eligibility: { remote: getRemoteSkillEligibility() }, reservedNames: used, }); diff --git a/src/telegram/bot-handlers.ts b/src/telegram/bot-handlers.ts index 1f91a58ba23..0b953989563 100644 --- a/src/telegram/bot-handlers.ts +++ b/src/telegram/bot-handlers.ts @@ -1142,10 +1142,10 @@ export const registerTelegramHandlers = ({ return; } - const agentId = paginationMatch[2]?.trim() || resolveDefaultAgentId(cfg) || undefined; + const agentId = paginationMatch[2]?.trim() || resolveDefaultAgentId(cfg); const skillCommands = listSkillCommandsForAgents({ cfg, - agentIds: agentId ? [agentId] : undefined, + agentIds: [agentId], }); const result = buildCommandsMessagePaginated(cfg, skillCommands, { page, diff --git a/src/telegram/bot-native-commands.skills-allowlist.test.ts b/src/telegram/bot-native-commands.skills-allowlist.test.ts new file mode 100644 index 00000000000..9c5fce1295c --- /dev/null +++ b/src/telegram/bot-native-commands.skills-allowlist.test.ts @@ -0,0 +1,105 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { writeSkill } from "../agents/skills.e2e-test-helpers.js"; +import type { OpenClawConfig } from "../config/config.js"; +import type { TelegramAccountConfig } from "../config/types.js"; +import { registerTelegramNativeCommands } from "./bot-native-commands.js"; +import { createNativeCommandTestParams } from "./bot-native-commands.test-helpers.js"; + +const pluginCommandMocks = vi.hoisted(() => ({ + getPluginCommandSpecs: vi.fn(() => []), + matchPluginCommand: vi.fn(() => null), + executePluginCommand: vi.fn(async () => ({ text: "ok" })), +})); +const deliveryMocks = vi.hoisted(() => ({ + deliverReplies: vi.fn(async () => ({ delivered: true })), +})); + +vi.mock("../plugins/commands.js", () => ({ + getPluginCommandSpecs: pluginCommandMocks.getPluginCommandSpecs, + matchPluginCommand: pluginCommandMocks.matchPluginCommand, + executePluginCommand: pluginCommandMocks.executePluginCommand, +})); +vi.mock("./bot/delivery.js", () => ({ + deliverReplies: deliveryMocks.deliverReplies, +})); + +const tempDirs: string[] = []; + +async function makeWorkspace(prefix: string) { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); + tempDirs.push(dir); + return dir; +} + +describe("registerTelegramNativeCommands skill allowlist integration", () => { + afterEach(async () => { + pluginCommandMocks.getPluginCommandSpecs.mockClear().mockReturnValue([]); + pluginCommandMocks.matchPluginCommand.mockClear().mockReturnValue(null); + pluginCommandMocks.executePluginCommand.mockClear().mockResolvedValue({ text: "ok" }); + deliveryMocks.deliverReplies.mockClear().mockResolvedValue({ delivered: true }); + await Promise.all( + tempDirs + .splice(0, tempDirs.length) + .map((dir) => fs.rm(dir, { recursive: true, force: true })), + ); + }); + + it("registers only allowlisted skills for the bound agent menu", async () => { + const workspaceDir = await makeWorkspace("openclaw-telegram-skills-"); + await writeSkill({ + dir: path.join(workspaceDir, "skills", "alpha-skill"), + name: "alpha-skill", + description: "Alpha skill", + }); + await writeSkill({ + dir: path.join(workspaceDir, "skills", "beta-skill"), + name: "beta-skill", + description: "Beta skill", + }); + + const setMyCommands = vi.fn().mockResolvedValue(undefined); + const cfg: OpenClawConfig = { + agents: { + list: [ + { id: "alpha", workspace: workspaceDir, skills: ["alpha-skill"] }, + { id: "beta", workspace: workspaceDir, skills: ["beta-skill"] }, + ], + }, + bindings: [ + { + agentId: "alpha", + match: { channel: "telegram", accountId: "bot-a" }, + }, + ], + }; + + registerTelegramNativeCommands({ + ...createNativeCommandTestParams({ + bot: { + api: { + setMyCommands, + sendMessage: vi.fn().mockResolvedValue(undefined), + }, + command: vi.fn(), + } as unknown as Parameters[0]["bot"], + cfg, + accountId: "bot-a", + telegramCfg: {} as TelegramAccountConfig, + }), + }); + + await vi.waitFor(() => { + expect(setMyCommands).toHaveBeenCalled(); + }); + const registeredCommands = setMyCommands.mock.calls[0]?.[0] as Array<{ + command: string; + description: string; + }>; + + expect(registeredCommands.some((entry) => entry.command === "alpha_skill")).toBe(true); + expect(registeredCommands.some((entry) => entry.command === "beta_skill")).toBe(false); + }); +}); diff --git a/src/telegram/bot-native-commands.ts b/src/telegram/bot-native-commands.ts index 071a0c1fa37..1c6ec8767e9 100644 --- a/src/telegram/bot-native-commands.ts +++ b/src/telegram/bot-native-commands.ts @@ -324,10 +324,14 @@ export const registerTelegramNativeCommands = ({ nativeEnabled && nativeSkillsEnabled ? resolveAgentRoute({ cfg, channel: "telegram", accountId }) : null; - const boundAgentIds = boundRoute ? [boundRoute.agentId] : null; + if (nativeEnabled && nativeSkillsEnabled && !boundRoute) { + runtime.log?.( + "nativeSkillsEnabled is true but no agent route is bound for this Telegram account; skill commands will not appear in the native menu.", + ); + } const skillCommands = - nativeEnabled && nativeSkillsEnabled - ? listSkillCommandsForAgents(boundAgentIds ? { cfg, agentIds: boundAgentIds } : { cfg }) + nativeEnabled && nativeSkillsEnabled && boundRoute + ? listSkillCommandsForAgents({ cfg, agentIds: [boundRoute.agentId] }) : []; const nativeCommands = nativeEnabled ? listNativeCommandSpecsForConfig(cfg, { diff --git a/src/telegram/bot.test.ts b/src/telegram/bot.test.ts index 2fe9636ee9b..69a94c3e200 100644 --- a/src/telegram/bot.test.ts +++ b/src/telegram/bot.test.ts @@ -294,6 +294,38 @@ describe("createTelegramBot", () => { ); }); + it("falls back to default agent for pagination callbacks without agent suffix", async () => { + onSpy.mockClear(); + listSkillCommandsForAgents.mockClear(); + + createTelegramBot({ token: "tok" }); + const callbackHandler = onSpy.mock.calls.find((call) => call[0] === "callback_query")?.[1] as ( + ctx: Record, + ) => Promise; + expect(callbackHandler).toBeDefined(); + + await callbackHandler({ + callbackQuery: { + id: "cbq-no-suffix", + data: "commands_page_2", + from: { id: 9, first_name: "Ada", username: "ada_bot" }, + message: { + chat: { id: 1234, type: "private" }, + date: 1736380800, + message_id: 14, + }, + }, + me: { username: "openclaw_bot" }, + getFile: async () => ({ download: async () => new Uint8Array() }), + }); + + expect(listSkillCommandsForAgents).toHaveBeenCalledWith({ + cfg: expect.any(Object), + agentIds: ["main"], + }); + expect(editMessageTextSpy).toHaveBeenCalledTimes(1); + }); + it("blocks pagination callbacks when allowlist rejects sender", async () => { onSpy.mockClear(); editMessageTextSpy.mockClear();