fix: resolve a skill's SKILL.md by its declared name, not by its directory name (#1372)

## 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 <noreply@anthropic.com>
Co-authored-by: Happy <yesreply@happy.engineering>
This commit is contained in:
nikita-ashihmin
2026-10-19 08:05:25 +04:00
committed by GitHub
parent dbf386d8d3
commit 424db87535
3 changed files with 471 additions and 21 deletions

View File

@@ -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<string, string | undefined>,
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;
};

View File

@@ -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);
});
});
});
});

View File

@@ -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<string, ScannedSkill>();
/**
* 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<string, Map<string, string>>;
/**
* 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<string, string> {
const scanned = scans?.get(containerDir);
if (scanned) {
return scanned;
}
const byName = new Map<string, string>();
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 `<prefix>:<name>` spelling either a plugin (`.claude/plugins/<prefix>/skills/<name>`) or a
* directory-scoped skill (`<prefix>/.claude/skills/<name>`), 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 `<prefix>:<name>` spelling either a plugin or a directory-scoped skill (`<prefix>/.claude/skills/<name>`),
* 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 `<prefix>:<name>` 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"));
const addScopedContainers = (base: string) => {
if (scope === undefined) {
return;
}
addCandidates(cwd);
addCandidates(os.homedir());
return candidates.find((candidate) => existsSync(candidate));
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 `<prefix>` 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;
}
}
// 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;
}