guard skill deps and fix react-doctor findings

Author: charlesvien

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

@@ -366,7 +366,7 @@ function ConfigSummarySection({ loop }: { loop: LoopSchemas.Loop }) {
 function LoopSkillSummary({ loop }: { loop: LoopSchemas.Loop }) {
   const { localWorkspaces } = useHostCapabilities();
   const trpc = useHostTRPC();
-  const skillsQuery = useQuery({
+  const { data: localSkillData } = useQuery({
     ...trpc.skills.list.queryOptions(),
     enabled: localWorkspaces,
   });
@@ -381,7 +381,7 @@ function LoopSkillSummary({ loop }: { loop: LoopSchemas.Loop }) {
   // same-named skill from another source (say, an opened repo) can never silently
   // replace the loop's snapshot. Ambiguous cases go through the edit form, where
   // the picker shows each candidate.
-  const candidates = (skillsQuery.data ?? []).filter(
+  const candidates = (localSkillData ?? []).filter(
     (skill) =>
       skill.name === primary.skill_name &&
       skill.source === primary.skill_source,

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

@@ -41,11 +41,11 @@ export function LoopInstructionsFields({
 }) {
   const { localWorkspaces } = useHostCapabilities();
   const trpc = useHostTRPC();
-  const skillsQuery = useQuery({
+  const { data: localSkillData } = useQuery({
     ...trpc.skills.list.queryOptions(),
     enabled: localWorkspaces,
   });
-  const localSkills: PickableSkill[] = (skillsQuery.data ?? []).flatMap(
+  const localSkills: PickableSkill[] = (localSkillData ?? []).flatMap(
     (skill) =>
       isUploadableSkillSource(skill.source)
         ? [
@@ -58,13 +58,12 @@ export function LoopInstructionsFields({
           ]
         : [],
   );
+  const nameCounts = new Map<string, number>();
+  for (const skill of localSkills) {
+    nameCounts.set(skill.name, (nameCounts.get(skill.name) ?? 0) + 1);
+  }
   const duplicatedNames = new Set(
-    localSkills
-      .filter(
-        (skill, _, all) =>
-          all.filter((other) => other.name === skill.name).length > 1,
-      )
-      .map((skill) => skill.name),
+    [...nameCounts].flatMap(([name, count]) => (count > 1 ? [name] : [])),
   );
 
   const skill = values.skill;

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

@@ -4,6 +4,7 @@ import {
 } from "@posthog/api-client/loops";
 import { useHostTRPCClient } from "@posthog/host-router/react";
 import { useMutation, useQueryClient } from "@tanstack/react-query";
+import { isTrustedSkillDependency } from "../loopSkill";
 import { loopsKeys } from "./loopsKeys";
 import { useLoopsClient } from "./useLoopsClient";
 
@@ -20,7 +21,18 @@ async function buildSkillUploads(
   skill: LoopLocalSkillRef,
 ): Promise<LoopSchemas.LoopSkillBundleUpload[]> {
   const refs = await hostClient.skills.resolveDependencies.query([skill]);
-  const ordered = [skill, ...refs.filter((ref) => ref.name !== skill.name)];
+  const dependencies = refs.filter((ref) => ref.name !== skill.name);
+  const untrusted = dependencies.find(
+    (dep) => !isTrustedSkillDependency(dep, skill),
+  );
+  if (untrusted) {
+    throw new Error(
+      `The ${skill.name} skill references /${untrusted.name}, which resolved to a skill ` +
+        `outside its own scope (${untrusted.source}: ${untrusted.path}). Install ` +
+        `${untrusted.name} as one of your user skills, or alongside ${skill.name}, and retry.`,
+    );
+  }
+  const ordered = [skill, ...dependencies];
   const bundles = await Promise.all(
     ordered.map((ref) => hostClient.skills.bundleLocal.query(ref)),
   );

packages/ui/src/features/loops/loopSkill.test.ts

@@ -1,5 +1,9 @@
 import { describe, expect, it } from "vitest";
-import { buildSkillInstructions, parseSkillContext } from "./loopSkill";
+import {
+  buildSkillInstructions,
+  isTrustedSkillDependency,
+  parseSkillContext,
+} from "./loopSkill";
 
 describe("buildSkillInstructions", () => {
   it.each([
@@ -14,6 +18,60 @@ describe("buildSkillInstructions", () => {
   });
 });
 
+describe("isTrustedSkillDependency", () => {
+  const repoPrimary = {
+    source: "repo" as const,
+    path: "/work/app/.claude/skills/deploy-checks",
+  };
+
+  it.each([
+    {
+      name: "user dependency is always trusted",
+      dep: { source: "user" as const, path: "/home/me/.claude/skills/helper" },
+      primary: repoPrimary,
+      expected: true,
+    },
+    {
+      name: "sibling from the same repo skills dir is trusted",
+      dep: { source: "repo" as const, path: "/work/app/.claude/skills/helper" },
+      primary: repoPrimary,
+      expected: true,
+    },
+    {
+      name: "same-source skill from another repo is rejected",
+      dep: {
+        source: "repo" as const,
+        path: "/work/other-repo/.claude/skills/helper",
+      },
+      primary: repoPrimary,
+      expected: false,
+    },
+    {
+      name: "cross-source non-user match is rejected",
+      dep: {
+        source: "marketplace" as const,
+        path: "/plugins/x/skills/helper",
+      },
+      primary: repoPrimary,
+      expected: false,
+    },
+    {
+      name: "windows-style sibling paths are compared by directory",
+      dep: {
+        source: "repo" as const,
+        path: "C:\\app\\.claude\\skills\\helper",
+      },
+      primary: {
+        source: "repo" as const,
+        path: "C:\\app\\.claude\\skills\\deploy-checks",
+      },
+      expected: true,
+    },
+  ])("$name", ({ dep, primary, expected }) => {
+    expect(isTrustedSkillDependency(dep, primary)).toBe(expected);
+  });
+});
+
 describe("parseSkillContext", () => {
   it.each([
     { name: "bare invocation", instructions: "/deploy-checks", expected: "" },

packages/ui/src/features/loops/loopSkill.ts

@@ -45,6 +45,33 @@ export function parseSkillContext(
   return trimmed;
 }
 
+function parentDir(skillPath: string): string {
+  const separatorIndex = Math.max(
+    skillPath.lastIndexOf("/"),
+    skillPath.lastIndexOf("\\"),
+  );
+  return separatorIndex > 0 ? skillPath.slice(0, separatorIndex) : skillPath;
+}
+
+/**
+ * Whether a resolved skill dependency may ride along in a loop's bundle set.
+ * The resolver matches dependencies by name across every local skill, so a
+ * same-named skill in another opened repository could otherwise shadow the
+ * real dependency and get itself installed into every recurring run. Trusted:
+ * the user's own machine-level skills, and siblings from the same skills
+ * directory as the skill that referenced them.
+ */
+export function isTrustedSkillDependency(
+  dep: { source: LoopSchemas.LoopSkillSourceEnum; path: string },
+  primary: { source: LoopSchemas.LoopSkillSourceEnum; path: string },
+): boolean {
+  if (dep.source === "user") return true;
+  return (
+    dep.source === primary.source &&
+    parentDir(dep.path) === parentDir(primary.path)
+  );
+}
+
 /** The loop's attached bundles, tolerating a backend that predates the field. */
 export function loopSkillBundles(
   loop: LoopSchemas.Loop,