fix(skills): allow symlinked local skills in cloud tasks

packages/workspace-server/src/services/skills/skill-bundler.ts

@@ -22,16 +22,17 @@ function getSafeSkillFileName(name: string): string {
   return safeName.length > 0 ? safeName : "skill";
 }
 
-async function assertSkillRoot(skillPath: string): Promise<string> {
+async function assertSkillRoot(
+  skillPath: string,
+  allowRootSymlink: boolean,
+): Promise<string> {
   const lexical = path.resolve(skillPath);
   const parentReal = await fs.promises.realpath(path.dirname(lexical));
   const root = await fs.promises.realpath(lexical);
-  // A symlinked skill root bundles whatever it points at, so a repository could
-  // commit `.claude/skills/foo -> ~/.claude/skills/foo` and exfiltrate a
-  // directory from outside the repo into an uploaded bundle. Only the skill
-  // directory itself must be real; symlinked ancestors (e.g. /tmp on macOS)
-  // stay legal.
-  if (root !== path.join(parentReal, path.basename(lexical))) {
+  if (
+    !allowRootSymlink &&
+    root !== path.join(parentReal, path.basename(lexical))
+  ) {
     throw new Error(
       "Local skill bundle root must be a real directory, not a symlink",
     );
@@ -134,12 +135,14 @@ export async function bundleLocalSkill({
   name,
   source,
   skillPath,
+  allowRootSymlink = false,
 }: {
   name: string;
   source: UploadableSkillSource;
   skillPath: string;
+  allowRootSymlink?: boolean;
 }): Promise<BundleLocalSkillOutput> {
-  const root = await assertSkillRoot(skillPath);
+  const root = await assertSkillRoot(skillPath, allowRootSymlink);
   const acc: SkillFileAccumulator = { files: {}, totalBytes: 0 };
   await collectSkillFiles(root, root, acc);
   const files = acc.files;

packages/workspace-server/src/services/skills/skills.test.ts

@@ -9,6 +9,7 @@ import { WatcherService } from "../watcher/service";
 import { SkillsService } from "./skills";
 
 const codexHome = vi.hoisted(() => ({ dir: "" }));
+const marketplaceHome = vi.hoisted(() => ({ dir: "" }));
 const userSkillsHome = vi.hoisted(() => ({ dir: "" }));
 
 vi.mock("../posthog-plugin/codex-mirror", async (importOriginal) => {
@@ -19,20 +20,28 @@ vi.mock("../posthog-plugin/codex-mirror", async (importOriginal) => {
 
 vi.mock("./skill-discovery", async (importOriginal) => {
   const actual = await importOriginal<typeof import("./skill-discovery")>();
-  return { ...actual, getUserSkillsDir: () => userSkillsHome.dir };
+  return {
+    ...actual,
+    getMarketplaceInstallPaths: async () => [marketplaceHome.dir],
+    getUserSkillsDir: () => userSkillsHome.dir,
+  };
 });
 
 let root: string;
 let pluginPath: string;
 let folderPath: string;
 let repoSkillsDir: string;
 
-function makeService(): SkillsService {
+function makeService(openFolders: string[] = [folderPath]): SkillsService {
   const plugin = {
     getPluginPath: () => pluginPath,
   } as unknown as PosthogPluginService;
   const folders = {
-    getFolders: async () => [{ path: folderPath, name: "my-repo" }],
+    getFolders: async () =>
+      openFolders.map((folder, index) => ({
+        path: folder,
+        name: `repo-${index}`,
+      })),
   } as unknown as FoldersService;
   return new SkillsService(plugin, folders, new WatcherService());
 }
@@ -54,6 +63,7 @@ beforeEach(async () => {
   folderPath = path.join(root, "repo");
   repoSkillsDir = path.join(folderPath, ".claude", "skills");
   codexHome.dir = path.join(root, "codex-skills");
+  marketplaceHome.dir = path.join(root, "marketplace");
   userSkillsHome.dir = path.join(root, "user-skills");
   await mkdir(path.join(pluginPath, "skills"), { recursive: true });
   await mkdir(repoSkillsDir, { recursive: true });
@@ -520,6 +530,23 @@ describe("write-path guard", () => {
     ).rejects.toThrow("resolves outside its repository");
   });
 
+  it("rejects a repo skill that resolves into another open repository", async () => {
+    const targetRepo = path.join(root, "target-repo");
+    const targetSkillsDir = path.join(targetRepo, ".claude", "skills");
+    await createSkill(targetSkillsDir, "escapee");
+    await rm(repoSkillsDir, { recursive: true, force: true });
+    await symlink(targetSkillsDir, repoSkillsDir, "dir");
+    const service = makeService([targetRepo, folderPath]);
+
+    await expect(
+      service.bundleLocalSkill({
+        name: "escapee",
+        source: "repo",
+        path: path.join(repoSkillsDir, "escapee"),
+      }),
+    ).rejects.toThrow("resolves outside its repository");
+  });
+
   it("rejects a symlinked repo skill root", async () => {
     // A repository could commit `.claude/skills/foo` as a symlink to a
     // directory outside the repo; bundling must refuse to follow it rather
@@ -538,21 +565,67 @@ describe("write-path guard", () => {
     ).rejects.toThrow("resolves outside its repository");
   });
 
-  it("rejects a symlinked skill root outside repo roots", async () => {
-    // Non-repo roots have no repository anchor, so the bundler's own
-    // leaf-symlink check is the guard there.
+  it("bundles a symlinked user skill root", async () => {
     const target = await createSkill(root, "linked");
     await mkdir(userSkillsHome.dir, { recursive: true });
     const linkPath = path.join(userSkillsHome.dir, "linked");
     await symlink(target, linkPath, "dir");
     const service = makeService();
 
+    const bundled = await service.bundleLocalSkill({
+      name: "linked",
+      source: "user",
+      path: linkPath,
+    });
+
+    expect(bundled.fileName).toBe("linked.zip");
+  });
+
+  it("bundles a user skill through a symlinked user skills directory", async () => {
+    const realUserSkillsDir = path.join(root, "real-user-skills");
+    const target = await createSkill(root, "linked");
+    await mkdir(realUserSkillsDir, { recursive: true });
+    await symlink(realUserSkillsDir, userSkillsHome.dir, "dir");
+    const linkPath = path.join(userSkillsHome.dir, "linked");
+    await symlink(target, linkPath, "dir");
+
+    const bundled = await makeService().bundleLocalSkill({
+      name: "linked",
+      source: "user",
+      path: linkPath,
+    });
+
+    expect(bundled.fileName).toBe("linked.zip");
+  });
+
+  it("rejects an open workspace root reached through a user skill symlink", async () => {
+    await writeFile(path.join(folderPath, "SKILL.md"), "workspace");
+    await mkdir(userSkillsHome.dir, { recursive: true });
+    const linkPath = path.join(userSkillsHome.dir, "workspace");
+    await symlink(folderPath, linkPath, "dir");
+
     await expect(
-      service.bundleLocalSkill({
-        name: "linked",
+      makeService().bundleLocalSkill({
+        name: "workspace",
         source: "user",
         path: linkPath,
       }),
+    ).rejects.toThrow("resolves outside its repository");
+  });
+
+  it("rejects a symlinked marketplace skill root", async () => {
+    const target = await createSkill(root, "linked");
+    const marketplaceSkillsDir = path.join(marketplaceHome.dir, "skills");
+    await mkdir(marketplaceSkillsDir, { recursive: true });
+    const linkPath = path.join(marketplaceSkillsDir, "linked");
+    await symlink(target, linkPath, "dir");
+
+    await expect(
+      makeService().bundleLocalSkill({
+        name: "linked",
+        source: "marketplace",
+        path: linkPath,
+      }),
     ).rejects.toThrow("not a symlink");
   });
 
@@ -719,6 +792,42 @@ describe("resolveSkillBundleDependencies", () => {
     expect(resolved.map((r) => r.path)).toEqual([primary, repoHelper]);
   });
 
+  it("rejects an implicitly resolved symlinked user dependency", async () => {
+    const primary = await createSkill(
+      repoSkillsDir,
+      "scoped-parent",
+      withDeps("scoped-parent", ["helper"]),
+    );
+    const target = await createSkill(root, "helper");
+    await mkdir(userSkillsHome.dir, { recursive: true });
+    await symlink(target, path.join(userSkillsHome.dir, "helper"), "dir");
+
+    await expect(
+      makeService().resolveSkillBundleDependencies([
+        ref("scoped-parent", primary),
+      ]),
+    ).rejects.toThrow("Select helper explicitly");
+  });
+
+  it("allows an explicitly selected symlinked user dependency", async () => {
+    const primary = await createSkill(
+      repoSkillsDir,
+      "scoped-parent",
+      withDeps("scoped-parent", ["helper"]),
+    );
+    const target = await createSkill(root, "helper");
+    await mkdir(userSkillsHome.dir, { recursive: true });
+    const helper = path.join(userSkillsHome.dir, "helper");
+    await symlink(target, helper, "dir");
+
+    const resolved = await makeService().resolveSkillBundleDependencies([
+      ref("scoped-parent", primary),
+      { name: "helper", source: "user", path: helper },
+    ]);
+
+    expect(resolved.map((skill) => skill.path)).toEqual([primary, helper]);
+  });
+
   it("expands a tagged skill to include its transitive dependencies", async () => {
     const primary = await createSkill(
       repoSkillsDir,

packages/workspace-server/src/services/skills/skills.ts

@@ -500,10 +500,12 @@ export class SkillsService {
   ): Promise<BundleLocalSkillOutput> {
     const skillDir = await this.resolveKnownSkillDir(input.path);
     await this.assertRepoSkillStaysInRepo(skillDir);
+    const allowRootSymlink = await this.isUnderUserSkillRoot(skillDir);
     return bundleLocalSkill({
       name: input.name,
       source: input.source,
       skillPath: skillDir,
+      allowRootSymlink,
     });
   }
 
@@ -518,22 +520,47 @@ export class SkillsService {
   private async assertRepoSkillStaysInRepo(skillDir: string): Promise<void> {
     const parent = path.dirname(skillDir);
     const folders = await this.folders.getFolders();
-    const owningFolder = folders.find(
-      (folder) =>
+    const realSkill = await fs.promises.realpath(skillDir);
+    const foldersWithRealPaths = await Promise.all(
+      folders.map(async (folder) => ({
+        folder,
+        realPath: await fs.promises.realpath(path.resolve(folder.path)),
+      })),
+    );
+    const lexicalOwner = foldersWithRealPaths.find(
+      ({ folder }) =>
         path.resolve(path.join(folder.path, ".claude", "skills")) === parent,
     );
-    if (!owningFolder) return;
-    const [realSkill, realFolder] = await Promise.all([
-      fs.promises.realpath(skillDir),
-      fs.promises.realpath(path.resolve(owningFolder.path)),
-    ]);
-    if (!realSkill.startsWith(realFolder + path.sep)) {
+    if (lexicalOwner) {
+      if (!realSkill.startsWith(lexicalOwner.realPath + path.sep)) {
+        throw new Error(
+          "Access denied: repository skill resolves outside its repository",
+        );
+      }
+      return;
+    }
+
+    const resolvedOwner = foldersWithRealPaths.find(
+      ({ realPath }) =>
+        realSkill === realPath || realSkill.startsWith(realPath + path.sep),
+    );
+    if (resolvedOwner && realSkill === resolvedOwner.realPath) {
       throw new Error(
         "Access denied: repository skill resolves outside its repository",
       );
     }
   }
 
+  private async isUnderUserSkillRoot(skillDir: string): Promise<boolean> {
+    const parent = await fs.promises.realpath(path.dirname(skillDir));
+    const roots = await Promise.all(
+      [getUserSkillsDir(), getCodexSkillsDir()].map((root) =>
+        fs.promises.realpath(root).catch(() => path.resolve(root)),
+      ),
+    );
+    return roots.includes(parent);
+  }
+
   /**
    * Expand a set of tagged skill refs to include their transitive dependency
    * skills, so a skill that needs another pulls it into the same cloud run.
@@ -574,6 +601,9 @@ export class SkillsService {
     );
 
     const seen = new Set<string>();
+    const explicitRefs = new Set(
+      refs.map((ref) => `${ref.source}:${ref.path}`),
+    );
     const resolved: SkillBundleRef[] = [];
     const queue: SkillBundleRef[] = [...refs];
     // Sanity ceiling on the dependency closure. The `seen` set already
@@ -617,10 +647,19 @@ export class SkillsService {
 
       for (const dependencyName of dependencyNames) {
         const dependencyRef = findUploadableByName(dependencyName, ref);
-        if (
-          dependencyRef &&
-          !seen.has(`${dependencyRef.source}:${dependencyRef.path}`)
-        ) {
+        const dependencyKey = dependencyRef
+          ? `${dependencyRef.source}:${dependencyRef.path}`
+          : null;
+        if (dependencyRef && dependencyKey && !seen.has(dependencyKey)) {
+          const isImplicitSymlink =
+            !explicitRefs.has(dependencyKey) &&
+            (await fs.promises.lstat(dependencyRef.path)).isSymbolicLink();
+          if (isImplicitSymlink) {
+            throw new Error(
+              `The ${ref.name} skill references /${dependencyRef.name}, which is a symlinked local skill. ` +
+                `Select ${dependencyRef.name} explicitly to include it in this cloud run.`,
+            );
+          }
           queue.push(dependencyRef);
         }
       }