From 424db8753581bfaf4c5bf278f8df8c089eec35da Mon Sep 17 00:00:00 2001 From: nikita-ashihmin Date: Mon, 19 Oct 2026 08:05:25 +0400 Subject: [PATCH] fix: resolve a skill's SKILL.md by its declared name, not by its directory name (#1372) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Claude Code advertises a skill by the `name:` of its `SKILL.md` frontmatter, which does not have to match the name of the directory — or of the symlink — that holds the file. `resolveSkillPath` joined the advertised name into each container path and only checked whether the result exists, so every such skill resolved to nothing. For an AIR client the consequence is visible: `getAvailableSlashCommands` attaches `_meta.jetbrains.air.kind = "skill"` and the `SKILL.md` path only when the path resolves, so the skill reached the client as a plain command and disappeared from the skill completion, while `/name` kept working. Symlinked skills hit this most often, because the link name is usually chosen independently of the skill's declared name. Reproduced with a project whose `.claude/skills/mismatch/SKILL.md` declares `name: frontname`, and with `.claude/skills/aliased2 -> …/target2` declaring `name: front2`: both were advertised without any `_meta`. The same happens in a large repo where `.claude/skills/rubymine` declares `name: rubymine-workflows`. ## Fix The existing join-by-name probe stays the first attempt, so nothing changes for the common layout. When it misses, the same containers are scanned, in the same order, for a `SKILL.md` whose frontmatter declares the requested name: - `statSync`/`existsSync` follow links, so a skill behind a symlinked directory is scanned like any other; - each container is listed per lookup, and a `SKILL.md` whose modification time has not changed keeps the name of the previous scan, so an unchanged file is parsed once; - only the frontmatter block of at most 8 KB is parsed, so a `name:` in the body is not a name; - a directory that is itself named like the skill still wins over another skill that merely declares the name. ## Tests Eight cases in `src/tests/tools.test.ts` cover a directory/declared-name mismatch, a symlink with a differing link name in both `.claude/skills` and `.agents/skills`, the precedence of the directory that carries the name, scoped and plugin spellings resolved by declared name, a file without frontmatter, and a declared name that changes after the first resolution. --------- Co-authored-by: Claude Co-authored-by: Happy --- src/acp-agent.ts | 6 +- src/tests/tools.test.ts | 213 +++++++++++++++++- src/tool-calls/reporters/interaction.ts | 273 ++++++++++++++++++++++-- 3 files changed, 471 insertions(+), 21 deletions(-) diff --git a/src/acp-agent.ts b/src/acp-agent.ts index 6b773492..a8472173 100644 --- a/src/acp-agent.ts +++ b/src/acp-agent.ts @@ -295,7 +295,7 @@ import { type ToolCallUpdate, type ToolUpdateMeta, } from "./tool-calls/renderer.js"; -import { resolveSkillPath } from "./tool-calls/reporters/interaction.js"; +import { resolveSkillPath, type SkillScanCache } from "./tool-calls/reporters/interaction.js"; import { messageLoopYielder, nodeToWebReadable, @@ -12552,10 +12552,12 @@ function getAvailableSlashCommands( skillPaths?: Map, query?: Query, ): AvailableCommand[] { + // The container listings of this one pass, so a list of a hundred commands lists each container once. + const skillScans: SkillScanCache = new Map(); const skillPath = (name: string, cwd: string): string | undefined => { const key = `${cwd}\0${name}`; if (skillPaths?.has(key)) return skillPaths.get(key); - const resolved = resolveSkillPath(name, cwd); + const resolved = resolveSkillPath(name, cwd, skillScans); skillPaths?.set(key, resolved); return resolved; }; diff --git a/src/tests/tools.test.ts b/src/tests/tools.test.ts index bd083f9a..6b786218 100644 --- a/src/tests/tools.test.ts +++ b/src/tests/tools.test.ts @@ -1,7 +1,16 @@ import { describe, it, expect, afterEach, beforeEach, vi } from "vitest"; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { + mkdirSync, + mkdtempSync, + rmSync, + statSync, + symlinkSync, + utimesSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import * as path from "node:path"; +import { resolveSkillPath, type SkillScanCache } from "../tool-calls/reporters/interaction.js"; import { ClientCapabilities } from "@agentclientprotocol/sdk"; import { ImageBlockParam, ToolResultBlockParam } from "@anthropic-ai/sdk/resources"; import { @@ -4580,23 +4589,56 @@ describe("Skill tool rendering", () => { )?._meta?.jetbrains?.air?.skill; let root: string; + let home: string; + let realHome: string | undefined; beforeEach(() => { root = mkdtempSync(path.join(tmpdir(), "acp-skill-path-")); + // A miss scans the user-level containers, so the real home of whoever runs this must stay out of it. + home = path.join(root, "home"); + mkdirSync(home, { recursive: true }); + realHome = process.env.HOME; + process.env.HOME = home; }); afterEach(() => { + if (realHome === undefined) { + delete process.env.HOME; + } else { + process.env.HOME = realHome; + } rmSync(root, { recursive: true, force: true }); }); - const writeSkill = (relativeDir: string) => { + const writeSkill = (relativeDir: string, declaredName?: string) => { const dir = path.join(root, relativeDir); mkdirSync(dir, { recursive: true }); const file = path.join(dir, "SKILL.md"); - writeFileSync(file, "# skill\n"); + writeFileSync( + file, + declaredName ? `---\nname: ${declaredName}\n---\n# skill\n` : "# skill\n", + ); return file; }; + /** A skill whose whole `SKILL.md` is given, for the frontmatter spellings `writeSkill` cannot express. */ + const writeSkillFile = (relativeDir: string, content: string) => { + const dir = path.join(root, relativeDir); + mkdirSync(dir, { recursive: true }); + const file = path.join(dir, "SKILL.md"); + writeFileSync(file, content); + return file; + }; + + // The skill directory is outside the container, so only a link into the container can hold it. + const linkSkill = (relativeLink: string, declaredName?: string) => { + const file = writeSkill(`external/${relativeLink.replaceAll("/", "-")}-target`, declaredName); + const link = path.join(root, relativeLink); + mkdirSync(path.dirname(link), { recursive: true }); + symlinkSync(path.dirname(file), link); + return path.join(link, "SKILL.md"); + }; + it("resolves a project-level .claude/skills skill", () => { const file = writeSkill(".claude/skills/commits"); expect(skillMeta("commits", root).path).toBe(file); @@ -4626,6 +4668,171 @@ describe("Skill tool rendering", () => { writeSkill(".claude/skills/commits"); expect(skillMeta("commits").path).toBeUndefined(); }); + + it("resolves a skill whose directory name differs from its declared name", () => { + const file = writeSkill(".claude/skills/rubymine", "rubymine-workflows"); + expect(skillMeta("rubymine-workflows", root).path).toBe(file); + }); + + it("resolves a symlinked skill whose link name differs from its declared name", () => { + const file = linkSkill(".claude/skills/linked", "commits"); + expect(skillMeta("commits", root).path).toBe(file); + }); + + it("resolves a symlinked skill of .agents/skills by its declared name", () => { + const file = linkSkill(".agents/skills/linked", "commits"); + expect(skillMeta("commits", root).path).toBe(file); + }); + + it("prefers the directory that carries the name when its file declares no name", () => { + const byName = writeSkill(".claude/skills/commits"); + writeSkill(".agents/skills/other", "commits"); + expect(skillMeta("commits", root).path).toBe(byName); + }); + + it("prefers the file that declares the name over a directory of that name declaring another", () => { + writeSkill(".claude/skills/commits", "git-commits"); + const declared = writeSkill(".agents/skills/other", "commits"); + expect(skillMeta("commits", root).path).toBe(declared); + }); + + it("omits the path when the directory of that name belongs to another skill", () => { + writeSkill(".claude/skills/commits", "git-commits"); + expect(skillMeta("commits", root)).toEqual({ name: "commits" }); + }); + + it("resolves a directory-scoped skill by its declared name", () => { + const file = writeSkill("apps/web/.claude/skills/deployment", "deploy"); + expect(skillMeta("apps/web:deploy", root).path).toBe(file); + }); + + it("resolves a plugin skill by its declared name", () => { + const file = writeSkill(".claude/plugins/reviewer/skills/auditor", "audit"); + expect(skillMeta("reviewer:audit", root).path).toBe(file); + }); + + it("ignores a name that the frontmatter does not declare", () => { + writeSkill(".claude/skills/commits"); + expect(skillMeta("rubymine-workflows", root)).toEqual({ name: "rubymine-workflows" }); + }); + + it("picks up a declared name that changed after the first resolution", () => { + const file = writeSkill(".claude/skills/commits", "commits"); + expect(skillMeta("renamed", root).path).toBeUndefined(); + writeFileSync(file, "---\nname: renamed\n---\n# skill\n"); + utimesSync(file, new Date(), new Date(Date.now() + 2000)); + expect(skillMeta("renamed", root).path).toBe(file); + }); + + it("picks up a rewrite that restored the modification time of the file", () => { + const file = writeSkill(".claude/skills/commits", "commits"); + const stamp = new Date(Math.floor(Date.now() / 1000) * 1000); + utimesSync(file, stamp, stamp); + const before = statSync(file).mtimeMs; + expect(skillMeta("renamed", root).path).toBeUndefined(); + // The rename keeps the length of the file, so only the inode change time tells the two apart. + writeFileSync(file, "---\nname: renamed\n---\n# skill\n"); + // `git checkout`, `cp -p`, and a filesystem of one-second time resolution all leave the same trace. + utimesSync(file, stamp, stamp); + expect(statSync(file).mtimeMs).toBe(before); + expect(skillMeta("renamed", root).path).toBe(file); + }); + + it("resolves a user-level skill by its declared name", () => { + const dir = path.join(home, ".claude/skills/rubymine"); + mkdirSync(dir, { recursive: true }); + const file = path.join(dir, "SKILL.md"); + writeFileSync(file, "---\nname: rubymine-workflows\n---\n# skill\n"); + expect(skillMeta("rubymine-workflows", root).path).toBe(file); + }); + + it("resolves a marketplace plugin skill by its declared name", () => { + const file = writeSkill( + ".claude/plugins/marketplaces/official/plugins/reviewer/skills/auditor", + "audit", + ); + expect(skillMeta("reviewer:audit", root).path).toBe(file); + }); + + it("gives one name to the same skill on every machine", () => { + const first = writeSkill(".claude/skills/a-first", "commits"); + writeSkill(".claude/skills/z-last", "commits"); + expect(skillMeta("commits", root).path).toBe(first); + }); + + it("leaves a prompt command of .claude/commands unresolved", () => { + writeSkill(".agents/skills/other", "deploy"); + mkdirSync(path.join(root, ".claude/commands"), { recursive: true }); + writeFileSync(path.join(root, ".claude/commands/deploy.md"), "# prompt\n"); + expect(skillMeta("deploy", root)).toEqual({ name: "deploy" }); + }); + + it("keeps a name that escapes its container off the filesystem", () => { + writeSkill(".claude/skills/commits"); + expect(skillMeta("../commits", root).path).toBeUndefined(); + expect(skillMeta("..:commits", root).path).toBeUndefined(); + expect(skillMeta("a/b", root).path).toBeUndefined(); + }); + + // One list of slash commands is resolved in one pass, and the containers are listed once for the whole + // pass rather than once per command. + it("lists a container once for the whole resolution pass", () => { + const scans: SkillScanCache = new Map(); + writeSkill(".claude/skills/holder", "first"); + expect(resolveSkillPath("first", root, scans)).toBe( + path.join(root, ".claude/skills/holder/SKILL.md"), + ); + const added = writeSkill(".claude/skills/later", "second"); + expect(resolveSkillPath("second", root, scans)).toBeUndefined(); + expect(resolveSkillPath("second", root)).toBe(added); + }); + + describe("frontmatter", () => { + const resolves = (content: string, name = "commits") => { + const file = writeSkillFile(".claude/skills/holder", content); + return skillMeta(name, root).path === file; + }; + + it("reads a quoted name", () => { + expect(resolves('---\nname: "commits"\n---\n')).toBe(true); + }); + + it("reads a name with a trailing comment", () => { + expect(resolves("---\nname: commits # the one\n---\n")).toBe(true); + }); + + it("keeps a hash that starts no comment", () => { + expect(resolves("---\nname: c#1\n---\n", "c#1")).toBe(true); + }); + + it("reads a name written with CRLF line endings", () => { + expect(resolves("---\r\nname: commits\r\n---\r\n")).toBe(true); + }); + + it("reads a name of a block that ends with ...", () => { + expect(resolves("---\nname: commits\n...\n")).toBe(true); + }); + + it("ignores a value that no space separates from the key", () => { + expect(resolves("---\nname:commits\n---\n")).toBe(false); + }); + + it("ignores a name nested under another key", () => { + expect(resolves("---\nmetadata:\n name: commits\n---\n")).toBe(false); + }); + + it("ignores a name below the frontmatter", () => { + expect(resolves("---\ndescription: d\n---\nname: commits\n")).toBe(false); + }); + + it("ignores a file with no frontmatter", () => { + expect(resolves("name: commits\n")).toBe(false); + }); + + it("ignores a name past the frontmatter the reader covers", () => { + expect(resolves(`---\ndescription: ${"d".repeat(9000)}\nname: commits\n---\n`)).toBe(false); + }); + }); }); }); diff --git a/src/tool-calls/reporters/interaction.ts b/src/tool-calls/reporters/interaction.ts index 00131cd8..45c4e012 100644 --- a/src/tool-calls/reporters/interaction.ts +++ b/src/tool-calls/reporters/interaction.ts @@ -5,7 +5,15 @@ import type { TaskUpdateInput, TodoWriteInput, } from "@anthropic-ai/claude-agent-sdk/sdk-tools.js"; -import { existsSync, statSync } from "node:fs"; +import { + closeSync, + existsSync, + openSync, + readSync, + readdirSync, + statSync, + type Stats, +} from "node:fs"; import os from "node:os"; import path from "node:path"; import { exitPlanModeRawOutput } from "../../exit-plan.js"; @@ -213,37 +221,270 @@ export class AgentControlReporter extends GenericReporter { /** Roots a skill's directory may sit under, relative to the directory the scope resolves to. */ const SKILL_CONTAINER_DIRS = [".claude/skills", ".agents/skills"] as const; +/** Bytes of a `SKILL.md` that are read. A frontmatter block is far shorter, and the body is of no interest. */ +const MAX_FRONTMATTER_BYTES = 8192; + +/** `SKILL.md` files whose declared name is remembered. The cap keeps a long-lived adapter from growing forever. */ +const MAX_CACHED_SKILLS = 1024; + +/** The head of [file], up to [MAX_FRONTMATTER_BYTES], cut back to its last complete line. */ +function frontmatterHead(file: string): string | undefined { + let descriptor: number; + try { + descriptor = openSync(file, "r"); + } catch { + return undefined; + } + try { + const buffer = Buffer.allocUnsafe(MAX_FRONTMATTER_BYTES); + const read = readSync(descriptor, buffer, 0, MAX_FRONTMATTER_BYTES, 0); + const head = buffer.toString("utf8", 0, read); + if (read < MAX_FRONTMATTER_BYTES) { + return head; + } + // The last line of a head that filled the buffer may be cut in half, and half a name is the wrong name. + const lastBreak = head.lastIndexOf("\n"); + return lastBreak < 0 ? "" : head.slice(0, lastBreak); + } catch { + return undefined; + } finally { + closeSync(descriptor); + } +} + +/** + * The value [raw] carries, with its quotes resolved and a trailing comment dropped. + * + * This is the plain-scalar and single-line-quoted part of YAML, which is all a `name:` ever is. An unterminated + * quote keeps what it opened, as the frontmatter of a broken file is still worth a guess. + */ +function frontmatterScalar(raw: string): string { + const quote = raw[0]; + if (quote !== '"' && quote !== "'") { + // A `#` only starts a comment at the start of the value or after a space, so `a#b` is one scalar. + const comment = raw.search(/(?:^|[ \t])#/); + return (comment < 0 ? raw : raw.slice(0, comment)).trim(); + } + let value = ""; + for (let index = 1; index < raw.length; index++) { + const character = raw[index]; + if (character === quote) { + if (quote === "'" && raw[index + 1] === "'") { + value += "'"; + index++; + continue; + } + return value; + } + if (quote === '"' && character === "\\" && index + 1 < raw.length) { + value += raw[++index]; + continue; + } + value += character; + } + return value; +} + +/** + * The `name:` that the frontmatter of [skillMdPath] declares, or `undefined` when it declares none. + * + * Only the frontmatter block is parsed, so a `name:` further down the body is not a name. + */ +function declaredSkillName(skillMdPath: string): string | undefined { + const head = frontmatterHead(skillMdPath); + if (head === undefined) { + return undefined; + } + const lines = head.split(/\r?\n/); + if (lines[0]?.trim() !== "---") { + return undefined; + } + for (const line of lines.slice(1)) { + const trimmed = line.trim(); + if (trimmed === "---" || trimmed === "...") { + break; + } + // A key at column 0, so a `name:` nested under another key is not the skill's name. A value needs a space + // after the colon, as `name:value` is a plain scalar in YAML rather than a mapping. + const match = /^name[ \t]*:(?:[ \t]+(.*))?$/.exec(line); + if (match) { + return frontmatterScalar(match[1]?.trim() ?? "") || undefined; + } + } + return undefined; +} + +/** What one read of a `SKILL.md` found, and the state of the file it found it in. */ +type ScannedSkill = { signature: string; declaredName: string | undefined }; + +const scannedSkills = new Map(); + +/** + * The declared name of a `SKILL.md` whose [stats] are already known, read at most once per state of the file. + * + * The signature carries the inode change time next to the modification time, because a write that restores the + * old modification time — `git checkout`, `cp -p`, a filesystem whose times have a one-second resolution — + * still moves the change time, and a name that silently stayed behind resolves to the wrong skill forever. + */ +function cachedDeclaredName(skillMdPath: string, stats: Stats): string | undefined { + const signature = `${stats.mtimeMs}:${stats.ctimeMs}:${stats.size}:${stats.ino}:${stats.dev}`; + const cached = scannedSkills.get(skillMdPath); + if (cached?.signature === signature) { + return cached.declaredName; + } + const declaredName = declaredSkillName(skillMdPath); + if (scannedSkills.size >= MAX_CACHED_SKILLS) { + scannedSkills.clear(); + } + scannedSkills.set(skillMdPath, { signature, declaredName }); + return declaredName; +} + +/** + * The container scans of one resolution, so that a list of commands lists each container once. + * + * A list of slash commands is resolved in one synchronous pass, and nothing on disk changes within it. The cache + * therefore lives exactly as long as that pass: every later pass sees the directories as they are. + */ +export type SkillScanCache = Map>; + +/** + * The `SKILL.md` paths of [containerDir], keyed by the name their frontmatter declares. + * + * `statSync` follows links, so a skill reached through a symlinked directory is scanned like any other. The + * listing is sorted, so that two skills declaring one name resolve to the same file on every machine. + */ +function skillsByDeclaredName(containerDir: string, scans?: SkillScanCache): Map { + const scanned = scans?.get(containerDir); + if (scanned) { + return scanned; + } + const byName = new Map(); + let entries: string[]; + try { + entries = readdirSync(containerDir).sort(); + } catch { + entries = []; + } + for (const entry of entries) { + const file = path.join(containerDir, entry, "SKILL.md"); + let stats: Stats; + try { + stats = statSync(file); + } catch { + continue; + } + const name = cachedDeclaredName(file, stats); + if (name !== undefined && !byName.has(name)) { + byName.set(name, file); + } + } + scans?.set(containerDir, byName); + return byName; +} + +/** Whether [segment] would take a join outside the directory it is joined to. */ +function escapesContainer(segment: string): boolean { + return path.isAbsolute(segment) || segment.split(/[/\\]/).includes(".."); +} + /** * Absolute path of a skill's `SKILL.md`, or `undefined` when none of the known layouts holds one. * * The `Skill` tool reports only the skill's name, so the file has to be located by probing the layouts skills * actually use: project- and user-level `.claude/skills` (plus this repo's `.agents/skills` source of truth), and - * for a `:` spelling either a plugin (`.claude/plugins//skills/`) or a - * directory-scoped skill (`/.claude/skills/`), which share that spelling. Only a path that exists - * on disk is returned, so a wrong guess costs nothing and clients never render a link to a missing file. + * for a `:` spelling either a plugin or a directory-scoped skill (`/.claude/skills/`), + * which share that spelling. Only a path that exists on disk is returned, so a wrong guess costs nothing and + * clients never render a link to a missing file. + * + * Claude Code advertises a skill by the `name:` of its frontmatter, which can differ from the name of the + * directory, or of the symlink, that holds the file. A directory that carries the name therefore counts only + * while the file in it declares that name or declares none; otherwise the same containers are scanned, in the + * same order, for the `SKILL.md` that does declare it. + * + * [scans] holds the container listings of one resolution pass. Pass the same cache for every command of one + * list, and each container is listed once instead of once per command. */ -export function resolveSkillPath(skillName: string, cwd?: string): string | undefined { +export function resolveSkillPath( + skillName: string, + cwd?: string, + scans?: SkillScanCache, +): string | undefined { if (!cwd) { return undefined; } const colon = skillName.indexOf(":"); const scope = colon < 0 ? undefined : skillName.slice(0, colon); const name = colon < 0 ? skillName : skillName.slice(colon + 1); - if (!name) { + // The name comes from the agent, so a separator or a `..` in it must never reach the filesystem. + if ( + !name || + /[/\\]/.test(name) || + name === ".." || + (scope !== undefined && escapesContainer(scope)) + ) { return undefined; } - const candidates: string[] = []; - const addCandidates = (base: string) => { + const containers: string[] = []; + const addContainers = (base: string) => { for (const container of SKILL_CONTAINER_DIRS) { - candidates.push(path.join(base, container, name, "SKILL.md")); + containers.push(path.join(base, container)); } }; - if (scope) { - // A `:` skill is either directory-scoped or a plugin's; both spellings look identical. - addCandidates(path.join(cwd, scope)); - candidates.push(path.join(cwd, ".claude/plugins", scope, "skills", name, "SKILL.md")); + // A `:` skill is either directory-scoped or a plugin's; both spellings look identical. + const addScopedContainers = (base: string) => { + if (scope === undefined) { + return; + } + addContainers(path.join(base, scope)); + containers.push(path.join(base, ".claude/plugins", scope, "skills")); + // An installed plugin lives under the marketplace it came from, which its `` does not name. + const marketplaces = path.join(base, ".claude/plugins/marketplaces"); + let installed: string[]; + try { + installed = readdirSync(marketplaces).sort(); + } catch { + return; + } + for (const marketplace of installed) { + containers.push(path.join(marketplaces, marketplace, "plugins", scope, "skills")); + } + }; + addScopedContainers(cwd); + addScopedContainers(os.homedir()); + addContainers(cwd); + addContainers(os.homedir()); + for (const container of containers) { + const candidate = path.join(container, name, "SKILL.md"); + let stats: Stats; + try { + stats = statSync(candidate); + } catch { + continue; + } + if (!stats.isFile()) { + continue; + } + const declared = cachedDeclaredName(candidate, stats); + if (declared === undefined || declared === name) { + return candidate; + } } - addCandidates(cwd); - addCandidates(os.homedir()); - return candidates.find((candidate) => existsSync(candidate)); + // A prompt command of `.claude/commands` is no skill, so its name must not be matched against a skill that + // declares it somewhere else. Only the directory probe above may answer for it, by the name it carries. + if ( + scope === undefined && + [cwd, os.homedir()].some((base) => + existsSync(path.join(base, ".claude/commands", `${name}.md`)), + ) + ) { + return undefined; + } + for (const container of containers) { + const declared = skillsByDeclaredName(container, scans).get(name); + if (declared) { + return declared; + } + } + return undefined; }