fix: reject unreadable skill manifests (#1482)

Fixes #1478.

Skip SKILL.md candidates when opening or reading the file fails, while
preserving readable unnamed manifests. Do not cache failed reads so
later resolutions can recover.

Adds nine resolver regressions for open/read errors, descriptor cleanup,
recovery, and valid alternatives.

Validation (Windows, Node 24.18.0):
- Nine new tests, TypeScript build, and ESLint pass.
- Full suite: 3,354 passed, 57 failed, 52 skipped. The same 57 failures
reproduce on unmodified 52c241e in the affected files.
- npm run check fails only on unchanged CLAUDE.md: Windows checked out
the symlink as a plain target file. The baseline has the same issue.

Co-authored-by: xiaofeilovecoding <fxy850328059@gmail.com>

src/tests/skill-manifest-read-errors.test.ts

@@ -0,0 +1,104 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import {
+  closeSync,
+  mkdirSync,
+  mkdtempSync,
+  openSync,
+  readSync,
+  rmSync,
+  statSync,
+  writeFileSync,
+} from "node:fs";
+import os from "node:os";
+import path from "node:path";
+import { resolveSkillPath } from "../tool-calls/reporters/interaction.js";
+
+vi.mock("node:fs", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("node:fs")>();
+  return {
+    ...actual,
+    openSync: vi.fn(actual.openSync),
+    readSync: vi.fn(actual.readSync),
+    closeSync: vi.fn(actual.closeSync),
+  };
+});
+
+const actualFs = await vi.importActual<typeof import("node:fs")>("node:fs");
+
+describe("skill manifest read errors", () => {
+  let root: string;
+
+  beforeEach(() => {
+    vi.mocked(openSync).mockReset().mockImplementation(actualFs.openSync);
+    vi.mocked(readSync).mockReset().mockImplementation(actualFs.readSync);
+    vi.mocked(closeSync).mockReset().mockImplementation(actualFs.closeSync);
+    root = mkdtempSync(path.join(os.tmpdir(), "acp-skill-read-"));
+    vi.spyOn(os, "homedir").mockReturnValue(path.join(root, "home"));
+  });
+
+  afterEach(() => {
+    vi.restoreAllMocks();
+    rmSync(root, { recursive: true, force: true });
+  });
+
+  const writeSkill = (directory: string, content: string) => {
+    const file = path.join(root, directory, "SKILL.md");
+    mkdirSync(path.dirname(file), { recursive: true });
+    writeFileSync(file, content);
+    return file;
+  };
+
+  it.each(["EACCES", "ENOENT"])("rejects a manifest when opening it fails with %s", (code) => {
+    writeSkill(".claude/skills/review", "---\nname: another-skill\n---\n");
+    vi.mocked(openSync).mockImplementation(() => {
+      throw Object.assign(new Error("Cannot open fixture manifest"), { code });
+    });
+
+    expect(resolveSkillPath("review", root)).toBeUndefined();
+  });
+
+  it("rejects a manifest when reading it fails and closes the descriptor", () => {
+    writeSkill(".claude/skills/review", "---\nname: another-skill\n---\n");
+    vi.mocked(readSync).mockImplementation(() => {
+      throw Object.assign(new Error("Cannot read fixture manifest"), { code: "EIO" });
+    });
+
+    expect(resolveSkillPath("review", root)).toBeUndefined();
+    expect(closeSync).toHaveBeenCalled();
+    expect(vi.mocked(closeSync).mock.calls.length).toBe(vi.mocked(openSync).mock.calls.length);
+  });
+
+  it("retries a failed read even when the manifest's stat signature is unchanged", () => {
+    const file = writeSkill(".claude/skills/review", "# Skill without a declared name\n");
+    const before = statSync(file);
+    vi.mocked(readSync).mockImplementation(() => {
+      throw Object.assign(new Error("Temporary read failure"), { code: "EIO" });
+    });
+
+    expect(resolveSkillPath("review", root)).toBeUndefined();
+    vi.mocked(readSync).mockImplementation(actualFs.readSync);
+    expect(statSync(file)).toMatchObject({ mtimeMs: before.mtimeMs, ctimeMs: before.ctimeMs });
+    expect(resolveSkillPath("review", root)).toBe(file);
+  });
+
+  it("continues to a readable skill declaring the requested name", () => {
+    const unreadable = writeSkill(".claude/skills/review", "---\nname: another-skill\n---\n");
+    const readable = writeSkill(".agents/skills/other", "---\nname: review\n---\n");
+    vi.mocked(openSync).mockImplementation((file, flags, mode) => {
+      if (file === unreadable) {
+        throw Object.assign(new Error("Cannot open fixture manifest"), { code: "EACCES" });
+      }
+      return actualFs.openSync(file, flags, mode);
+    });
+
+    expect(resolveSkillPath("review", root)).toBe(readable);
+  });
+
+  it.each(["", "# Skill\n", "---\ndescription: review\n---\n", "---\nname: review\n---\n"])(
+    "keeps resolving a readable manifest with content %j",
+    (content) => {
+      const file = writeSkill(".claude/skills/review", content);
+      expect(resolveSkillPath("review", root)).toBe(file);
+    },
+  );
+});

src/tool-calls/reporters/interaction.ts

@@ -293,14 +293,15 @@ function frontmatterScalar(raw: string): string {
 }
 
 /**
- * The `name:` that the frontmatter of [skillMdPath] declares, or `undefined` when it declares none.
+ * The `name:` that the frontmatter of [skillMdPath] declares, `undefined` when it declares none,
+ * or `null` when the file could not be read.
  *
  * Only the frontmatter block is parsed, so a `name:` further down the body is not a name.
  */
-function declaredSkillName(skillMdPath: string): string | undefined {
+function declaredSkillName(skillMdPath: string): string | null | undefined {
   const head = frontmatterHead(skillMdPath);
   if (head === undefined) {
-    return undefined;
+    return null;
   }
   const lines = head.split(/\r?\n/);
   if (lines[0]?.trim() !== "---") {
@@ -333,13 +334,17 @@ const scannedSkills = new Map<string, ScannedSkill>();
  * 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 {
+function cachedDeclaredName(skillMdPath: string, stats: Stats): string | null | 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);
+  // A transient read error can clear without changing the stat signature, so the next pass must retry it.
+  if (declaredName === null) {
+    return null;
+  }
   if (scannedSkills.size >= MAX_CACHED_SKILLS) {
     scannedSkills.clear();
   }
@@ -382,7 +387,7 @@ function skillsByDeclaredName(containerDir: string, scans?: SkillScanCache): Map
       continue;
     }
     const name = cachedDeclaredName(file, stats);
-    if (name !== undefined && !byName.has(name)) {
+    if (typeof name === "string" && !byName.has(name)) {
       byName.set(name, file);
     }
   }