fix(skills): harden skill references before first release

packages/core/external-annotation.ts

@@ -383,7 +383,12 @@ export interface AnnotationStore<T extends StorableAnnotation> {
   remove(id: string): boolean;
   /** Remove all annotations from a specific source. Returns count removed. */
   clearBySource(source: string): number;
-  /** Update an annotation by ID. Returns the updated annotation, or null if not found. */
+  /**
+   * Update an annotation by ID. Returns the updated annotation, or null if
+   * not found. The identity fields `id` and `source` are pinned — values for
+   * them in `fields` are ignored (`source` gates verbatim skill-instruction
+   * injection in exported feedback and must not be clearable via PATCH).
+   */
   update(id: string, fields: Partial<T>): T | null;
   /** Remove all annotations. Returns count removed. */
   clearAll(): number;
@@ -441,7 +446,17 @@ export function createAnnotationStore<T extends StorableAnnotation>(): Annotatio
     update(id, fields) {
       const idx = annotations.findIndex((a) => a.id === id);
       if (idx === -1) return null;
-      const merged = { ...annotations[idx], ...fields, id } as T;
+      // Identity fields are pinned and can never be set, cleared, or changed
+      // by an update: `id` addresses the annotation, and `source` is the
+      // security marker the feedback exporters key on — a tool-submitted
+      // annotation (one carrying a `source`) must never receive verbatim
+      // SKILL.md injection (#1229). PATCH is an unauthenticated localhost
+      // surface, so allowing `{"source": ""}` through the merge would let
+      // any local process strip the external marker and re-arm injection.
+      const patch = { ...fields } as Record<string, unknown>;
+      delete patch.id;
+      delete patch.source;
+      const merged = { ...annotations[idx], ...(patch as Partial<T>) } as T;
       annotations[idx] = merged;
       version++;
       emit({ type: "update", id, annotation: merged });

packages/server/external-annotations.test.ts

@@ -16,3 +16,46 @@ describe("external annotations SSE", () => {
     expect(res?.headers.get("content-type")).toBe("text/event-stream");
   });
 });
+
+describe("PATCH /api/external-annotations", () => {
+  test("cannot clear or change the source marker (skill-injection guard, reproduced end-to-end)", async () => {
+    const handler = createExternalAnnotationHandler("review");
+    const added = handler.addAnnotations({
+      source: "rogue-agent",
+      scope: "general",
+      text: "apply $some-human-only-skill",
+    });
+    if ("error" in added) throw new Error(added.error);
+    const [id] = added.ids;
+
+    const patch = async (body: unknown) => {
+      const url = `http://localhost/api/external-annotations?id=${encodeURIComponent(id)}`;
+      const res = await handler.handle(
+        new Request(url, {
+          method: "PATCH",
+          headers: { "Content-Type": "application/json" },
+          body: JSON.stringify(body),
+        }),
+        new URL(url),
+      );
+      expect(res?.status).toBe(200);
+      return (await res!.json()) as { annotation: { source?: string; text?: string } };
+    };
+
+    // The reproduced bypass: PATCH {"source": ""} cleared the field and
+    // re-armed verbatim SKILL.md injection for a tool-submitted comment.
+    const cleared = await patch({ source: "" });
+    expect(cleared.annotation.source).toBe("rogue-agent");
+
+    const swapped = await patch({ source: "innocent" });
+    expect(swapped.annotation.source).toBe("rogue-agent");
+
+    const nulled = await patch({ source: null });
+    expect(nulled.annotation.source).toBe("rogue-agent");
+
+    // Legitimate field patches still work, with source intact.
+    const edited = await patch({ text: "edited text" });
+    expect(edited.annotation.text).toBe("edited text");
+    expect(edited.annotation.source).toBe("rogue-agent");
+  });
+});

packages/server/review-skill-loader.test.ts

@@ -20,6 +20,7 @@ import {
   readCuratedSkillNames,
   readReferenceSkillContent,
   resolveRequestedReviewProfile,
+  SKILL_CONTENT_HEAD_BYTES,
   stripFrontmatter,
 } from "./review-skill-loader";
 
@@ -158,6 +159,66 @@ describe("discoverSkills — root resolution", () => {
   });
 });
 
+describe("discoverSkills — symlinked skill directories", () => {
+  test("a symlinked skill dir at the root level is discovered", () => {
+    // `~/.claude/skills/my-skill -> /elsewhere/my-skill` — a common layout
+    // (skills managed in a dotfiles repo). Dirents report isDirectory() false
+    // for symlinks, so discovery must follow them.
+    const target = writeSkill(join(home, "elsewhere"), "linked-skill");
+    const root = join(home, ".claude", "skills");
+    mkdirSync(root, { recursive: true });
+    symlinkSync(target, join(root, "linked-skill"));
+
+    const found = discoverSkills().find((s) => s.name === "linked-skill");
+    expect(found).toBeDefined();
+    expect(found!.root).toBe("claude");
+    // And it flows through to the reference catalog / picker.
+    expect(listReferenceSkills().map((s) => s.name)).toContain("linked-skill");
+  });
+
+  test("a symlinked category dir is walked for the nested layout", () => {
+    const category = join(home, "elsewhere-cat");
+    writeSkill(category, "nested-linked");
+    const root = join(home, ".claude", "skills");
+    mkdirSync(root, { recursive: true });
+    symlinkSync(category, join(root, "category-link"));
+
+    const found = discoverSkills().find((s) => s.name === "nested-linked");
+    expect(found).toBeDefined();
+  });
+
+  test("a broken symlink is skipped silently", () => {
+    const root = join(home, ".claude", "skills");
+    writeSkill(root, "real-skill");
+    symlinkSync(join(home, "does-not-exist"), join(root, "dangling"));
+
+    const names = discoverSkills().map((s) => s.name);
+    expect(names).toContain("real-skill");
+    expect(names).not.toContain("dangling");
+  });
+
+  test("a symlink to a FILE is not a skill dir", () => {
+    const root = join(home, ".claude", "skills");
+    mkdirSync(root, { recursive: true });
+    writeFileSync(join(home, "some-file.md"), "not a dir");
+    symlinkSync(join(home, "some-file.md"), join(root, "file-link"));
+
+    expect(discoverSkills().map((s) => s.name)).not.toContain("file-link");
+  });
+
+  test("a symlink cycle neither hangs nor throws (depth-2 walk bounds it)", () => {
+    const root = join(home, ".claude", "skills");
+    writeSkill(root, "real-skill");
+    // A self-referential loop and a link back to the root itself.
+    symlinkSync(join(root, "loop"), join(root, "loop"));
+    symlinkSync(root, join(root, "up-link"));
+
+    const names = discoverSkills().map((s) => s.name);
+    expect(names).toContain("real-skill");
+    expect(names).not.toContain("loop");
+  });
+});
+
 // ---------------------------------------------------------------------------
 // Test 3 — Curation filter (membership; missing name; absent/malformed)
 // ---------------------------------------------------------------------------
@@ -724,6 +785,44 @@ describe("readReferenceSkillContent — human-only skill injection source", () =
     expect(readReferenceSkillContent("yaml-bomb")).toBeNull();
   });
 
+  test("a file of exactly the read bound is NOT flagged truncated", () => {
+    // The old `bytes === maxBytes` check flagged an exactly-boundary file as
+    // truncated, producing a false "[Instructions truncated...]" note. A
+    // giant-but-closed frontmatter plus a small body keeps the char cap out
+    // of play so head truncation is the only signal.
+    const root = join(home, ".claude", "skills");
+    const dir = join(root, "exact-boundary");
+    mkdirSync(dir, { recursive: true });
+    const bodyText = "# Real body";
+    const prefix = "---\nname: exact-boundary\npadding: ";
+    const suffix = "\n---\n" + bodyText;
+    const padLen = SKILL_CONTENT_HEAD_BYTES - prefix.length - suffix.length;
+    writeFileSync(join(dir, "SKILL.md"), prefix + "y".repeat(padLen) + suffix);
+
+    const result = readReferenceSkillContent("exact-boundary")!;
+    expect(result).not.toBeNull();
+    expect(result.content).toBe(bodyText);
+    expect(result.truncated).toBe(false);
+  });
+
+  test("one byte past the read bound IS flagged truncated", () => {
+    const root = join(home, ".claude", "skills");
+    const dir = join(root, "boundary-plus-one");
+    mkdirSync(dir, { recursive: true });
+    const bodyText = "# Real body";
+    const prefix = "---\nname: boundary-plus-one\npadding: ";
+    const suffix = "\n---\n" + bodyText;
+    const padLen = SKILL_CONTENT_HEAD_BYTES - prefix.length - suffix.length + 1;
+    writeFileSync(join(dir, "SKILL.md"), prefix + "y".repeat(padLen) + suffix);
+
+    const result = readReferenceSkillContent("boundary-plus-one")!;
+    expect(result).not.toBeNull();
+    // The head read cut the file's last byte: the file continues past the
+    // read, so the truncation notice is honest.
+    expect(result.truncated).toBe(true);
+    expect(result.content).toBe(bodyText.slice(0, -1));
+  });
+
   test("a complete file with genuinely unterminated frontmatter keeps its old behavior", () => {
     // Under the bound, no closing ---: the whole text is the body, exactly as
     // the unbounded readFileSync produced before.

packages/server/review-skill-loader.ts

@@ -22,6 +22,7 @@
 import {
   closeSync,
   existsSync,
+  fstatSync,
   mkdirSync,
   openSync,
   readdirSync,
@@ -142,7 +143,25 @@ function listSubdirs(dir: string): string[] {
     return [];
   }
   return entries
-    .filter((e) => e.isDirectory() && !SKIP_DIRS.has(e.name))
+    .filter((e) => {
+      if (SKIP_DIRS.has(e.name)) return false;
+      if (e.isDirectory()) return true;
+      // A symlinked skill dir (`~/.claude/skills/foo -> /elsewhere/foo`) has
+      // isDirectory() false on its dirent — follow it with statSync. A broken
+      // symlink (or any stat failure, e.g. an ELOOP symlink cycle) is skipped
+      // silently. No extra cycle detection is needed: statSync resolves to
+      // the final target (throwing on loops), and discovery is a fixed
+      // depth-2 walk with a hard cap, never a recursion that could follow a
+      // symlink back up the tree.
+      if (e.isSymbolicLink()) {
+        try {
+          return statSync(join(dir, e.name)).isDirectory();
+        } catch {
+          return false;
+        }
+      }
+      return false;
+    })
     .map((e) => e.name);
 }
 
@@ -300,7 +319,11 @@ function readFileHead(
   try {
     const buf = Buffer.alloc(maxBytes);
     const bytes = readSync(fd, buf, 0, maxBytes, 0);
-    return { text: buf.subarray(0, bytes).toString("utf-8"), truncated: bytes === maxBytes };
+    // Truncation means the file CONTINUES past the read — judged from the
+    // real size (fstat on the already-open fd), not from `bytes === maxBytes`,
+    // which spuriously flagged a file of exactly maxBytes as truncated.
+    const truncated = fstatSync(fd).size > bytes;
+    return { text: buf.subarray(0, bytes).toString("utf-8"), truncated };
   } catch {
     return null;
   } finally {
@@ -450,9 +473,9 @@ export const MAX_INJECTED_SKILL_CONTENT_LEN = 20_000;
  * 4 bytes per capped content char (UTF-8 worst case) and slack, so any file
  * whose frontmatter fits the catalog bound always yields the full
  * MAX_INJECTED_SKILL_CONTENT_LEN characters of body — truncation detection is
- * unchanged for every such file.
+ * unchanged for every such file. Exported for the boundary tests only.
  */
-const SKILL_CONTENT_HEAD_BYTES =
+export const SKILL_CONTENT_HEAD_BYTES =
   SKILL_META_HEAD_BYTES + MAX_INJECTED_SKILL_CONTENT_LEN * 4 + 4_096;
 
 /** A referenced skill's SKILL.md body, prepared for feedback injection. */

packages/shared/external-annotation.test.ts

@@ -4,7 +4,7 @@
  * finding submits cleanly while a broken line finding is still rejected.
  */
 import { describe, expect, test } from "bun:test";
-import { transformReviewInput } from "./external-annotation";
+import { createAnnotationStore, transformReviewInput } from "./external-annotation";
 
 function ok(body: unknown) {
   const r = transformReviewInput(body);
@@ -101,3 +101,50 @@ describe("transformReviewInput — scope-aware location requirements", () => {
     expect("error" in badNumber && badNumber.error).toContain("invalid prNumber");
   });
 });
+
+describe("annotation store update — identity fields are pinned", () => {
+  type Ann = { id: string; source?: string; text?: string; dismissed?: boolean };
+
+  test("update cannot clear, change, or set `source` (the injection guard field)", () => {
+    // #1229's exporter defense keys on `source`: annotations carrying one are
+    // tool-submitted and never receive verbatim SKILL.md injection. PATCH is
+    // an unauthenticated localhost surface, so `{"source": ""}` must not be
+    // able to strip the marker and re-arm injection.
+    const store = createAnnotationStore<Ann>();
+    store.add([{ id: "a1", source: "rogue-agent", text: "apply $skill" }]);
+
+    const cleared = store.update("a1", { source: "", text: "edited" } as Partial<Ann>);
+    expect(cleared).toEqual({ id: "a1", source: "rogue-agent", text: "edited" });
+
+    const swapped = store.update("a1", { source: "other-tool" } as Partial<Ann>);
+    expect(swapped!.source).toBe("rogue-agent");
+
+    const undefd = store.update("a1", { source: undefined } as Partial<Ann>);
+    expect(undefd!.source).toBe("rogue-agent");
+  });
+
+  test("update cannot change `id`", () => {
+    const store = createAnnotationStore<Ann>();
+    store.add([{ id: "a1", source: "tool" }]);
+    const updated = store.update("a1", { id: "b2", text: "x" } as Partial<Ann>);
+    expect(updated!.id).toBe("a1");
+    expect(store.getAll().map((a) => a.id)).toEqual(["a1"]);
+  });
+
+  test("an annotation without a source cannot gain one via update", () => {
+    const store = createAnnotationStore<Ann>();
+    store.add([{ id: "a1", text: "reviewer-authored" }]);
+    const updated = store.update("a1", { source: "fake-tool" } as Partial<Ann>);
+    expect(updated!.source).toBeUndefined();
+  });
+
+  test("ordinary field updates still merge and bump the version", () => {
+    const store = createAnnotationStore<Ann>();
+    store.add([{ id: "a1", source: "tool", text: "before" }]);
+    const v = store.version;
+    const updated = store.update("a1", { text: "after", dismissed: true });
+    expect(updated).toEqual({ id: "a1", source: "tool", text: "after", dismissed: true });
+    expect(store.version).toBe(v + 1);
+    expect(store.update("missing", { text: "x" })).toBeNull();
+  });
+});