close review gaps in skill bundling flow

Author: charlesvien

packages/ui/src/features/loops/components/LoopDetailView.tsx

@@ -455,6 +455,7 @@ function LoopSkillSummary({ loop }: { loop: LoopSchemas.Loop }) {
 
 function InstructionsSection({ loop }: { loop: LoopSchemas.Loop }) {
   const updateLoop = useUpdateLoop(loop.id);
+  const primarySkill = primaryLoopSkillBundle(loop);
   const [draft, setDraft] = useState<string | null>(null);
   // Escape reverts and blurs; skip the resulting onBlur save.
   const skipCommit = useRef(false);
@@ -516,6 +517,13 @@ function InstructionsSection({ loop }: { loop: LoopSchemas.Loop }) {
           }
         }}
       />
+      {primarySkill ? (
+        <Text className="text-[11px] text-gray-10 leading-snug">
+          This loop runs the {primarySkill.skill_name} skill: the leading /
+          {primarySkill.skill_name} line invokes its attached snapshot. Editing
+          here changes only the text; use Edit to change or detach the skill.
+        </Text>
+      ) : null}
     </Flex>
   );
 }

packages/ui/src/features/loops/components/LoopForm.tsx

@@ -188,9 +188,22 @@ export function LoopForm({ loop }: LoopFormProps) {
             error instanceof Error ? error.message : undefined;
           if (!isEdit) {
             // Roll the just-created loop back rather than leaving one that
-            // fires `/skill-name` with no bundle behind it.
-            await deleteLoop.mutateAsync(saved.id).catch(() => {});
-            toast.error("Failed to create loop", { description });
+            // fires `/skill-name` with no bundle behind it. If the rollback
+            // itself fails, an orphaned loop exists — say so instead of
+            // pretending nothing was created.
+            try {
+              await deleteLoop.mutateAsync(saved.id);
+              toast.error("Failed to create loop", { description });
+            } catch {
+              toast.error("Loop created, but attaching its skill failed", {
+                description: [
+                  description,
+                  `Delete "${saved.name}" or re-save it from Edit.`,
+                ]
+                  .filter(Boolean)
+                  .join(" "),
+              });
+            }
             return;
           }
           // Keep the form open with its state intact: saving again retries

packages/ui/src/features/loops/hooks/useLoopSkillBundles.ts

@@ -3,11 +3,24 @@ import {
   replaceLoopSkillBundles,
 } from "@posthog/api-client/loops";
 import { useHostTRPCClient } from "@posthog/host-router/react";
-import { useMutation, useQueryClient } from "@tanstack/react-query";
+import {
+  type QueryClient,
+  useMutation,
+  useQueryClient,
+} from "@tanstack/react-query";
 import { isTrustedSkillDependency } from "../loopSkill";
 import { loopsKeys } from "./loopsKeys";
 import { useLoopsClient } from "./useLoopsClient";
 
+function applyLoopToCache(
+  queryClient: QueryClient,
+  projectId: string | null,
+  loop: LoopSchemas.Loop,
+): void {
+  queryClient.setQueryData(loopsKeys.detail(projectId, loop.id), loop);
+  void queryClient.invalidateQueries({ queryKey: loopsKeys.list(projectId) });
+}
+
 export interface LoopLocalSkillRef {
   name: string;
   source: LoopSchemas.LoopSkillSourceEnum;
@@ -81,15 +94,8 @@ export function useReplaceLoopSkillBundles() {
         uploads,
       );
     },
-    onSuccess: (loop) => {
-      queryClient.setQueryData(
-        loopsKeys.detail(loopsClient?.projectId ?? null, loop.id),
-        loop,
-      );
-      void queryClient.invalidateQueries({
-        queryKey: loopsKeys.list(loopsClient?.projectId ?? null),
-      });
-    },
+    onSuccess: (loop) =>
+      applyLoopToCache(queryClient, loopsClient?.projectId ?? null, loop),
   });
 }
 
@@ -118,14 +124,7 @@ export function useSyncLoopSkillBundles() {
         uploads,
       );
     },
-    onSuccess: (loop) => {
-      queryClient.setQueryData(
-        loopsKeys.detail(loopsClient?.projectId ?? null, loop.id),
-        loop,
-      );
-      void queryClient.invalidateQueries({
-        queryKey: loopsKeys.list(loopsClient?.projectId ?? null),
-      });
-    },
+    onSuccess: (loop) =>
+      applyLoopToCache(queryClient, loopsClient?.projectId ?? null, loop),
   });
 }

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

@@ -502,7 +502,25 @@ describe("write-path guard", () => {
     ).rejects.toThrow("not a known skill directory");
   });
 
-  it("rejects a symlinked skill root", async () => {
+  it("rejects a repo skill reached through a symlinked skills directory", async () => {
+    // The leaf check alone misses this: a repo committing `.claude/skills`
+    // itself as a symlink makes every skill under it resolve outside the repo.
+    const outside = path.join(root, "outside-skills");
+    await createSkill(outside, "escapee");
+    await rm(repoSkillsDir, { recursive: true, force: true });
+    await symlink(outside, repoSkillsDir, "dir");
+    const service = makeService();
+
+    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
     // than upload the external target.
@@ -517,6 +535,24 @@ describe("write-path guard", () => {
         source: "repo",
         path: linkPath,
       }),
+    ).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.
+    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();
+
+    await expect(
+      service.bundleLocalSkill({
+        name: "linked",
+        source: "user",
+        path: linkPath,
+      }),
     ).rejects.toThrow("not a symlink");
   });
 

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

@@ -499,13 +499,41 @@ export class SkillsService {
     input: BundleLocalSkillInput,
   ): Promise<BundleLocalSkillOutput> {
     const skillDir = await this.resolveKnownSkillDir(input.path);
+    await this.assertRepoSkillStaysInRepo(skillDir);
     return bundleLocalSkill({
       name: input.name,
       source: input.source,
       skillPath: skillDir,
     });
   }
 
+  /**
+   * A repository can commit any ancestor of its skills (`.claude` or
+   * `.claude/skills`) as a symlink pointing outside the repo, which passes the
+   * lexical known-root check and the bundler's leaf-symlink check yet bundles
+   * an external directory. Uploads must stay inside the real repository, so
+   * anchor the check on the workspace folder itself — the one path segment a
+   * repo author cannot control.
+   */
+  private async assertRepoSkillStaysInRepo(skillDir: string): Promise<void> {
+    const parent = path.dirname(skillDir);
+    const folders = await this.folders.getFolders();
+    const owningFolder = folders.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)) {
+      throw new Error(
+        "Access denied: repository skill resolves outside its repository",
+      );
+    }
+  }
+
   /**
    * 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.