address skill bundle review feedback

Author: charlesvien

packages/api-client/src/loops.ts

@@ -204,8 +204,10 @@ export namespace LoopSchemas {
     updated_at: string;
     triggers: Array<LoopTrigger>;
     /** Skill bundles attached to this loop, seeded into every fired run's sandbox.
-     * Replaced wholesale via `replaceLoopSkillBundles`, never through the loop write. */
-    skill_bundles: Array<LoopSkillBundle>;
+     * Replaced wholesale via `replaceLoopSkillBundles`, never through the loop write.
+     * Optional because a backend that predates skill bundles omits the field; treat
+     * absence as an empty list. */
+    skill_bundles?: Array<LoopSkillBundle>;
   };
 
   /** A skill bundle attached to a loop. `content_sha256` is the stored snapshot's

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

@@ -47,7 +47,7 @@ import {
   nextScheduleRun,
   summarizeNotificationDestinations,
 } from "../loopDisplay";
-import { primaryLoopSkillBundle } from "../loopSkill";
+import { loopSkillBundles, primaryLoopSkillBundle } from "../loopSkill";
 import { LoopLoadError } from "./LoopFallbacks";
 import { LoopRunRow } from "./LoopRunRow";
 
@@ -322,7 +322,7 @@ function ConfigSummarySection({ loop }: { loop: LoopSchemas.Loop }) {
             .join(" · ")}
         </SummaryRow>
 
-        {loop.skill_bundles.length > 0 ? (
+        {loopSkillBundles(loop).length > 0 ? (
           <SummaryRow label="Skill">
             <LoopSkillSummary loop={loop} />
           </SummaryRow>
@@ -374,18 +374,26 @@ function LoopSkillSummary({ loop }: { loop: LoopSchemas.Loop }) {
 
   const primary = primaryLoopSkillBundle(loop);
   if (!primary) return null;
-  const dependencyCount = loop.skill_bundles.length - 1;
-
-  const localMatch = (skillsQuery.data ?? []).find(
+  const dependencyCount = loopSkillBundles(loop).length - 1;
+
+  // The one-click refresh must be unambiguous about which skill it snapshots: it
+  // requires exactly one local skill matching the stored name AND source, so a
+  // 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(
     (skill) =>
       skill.name === primary.skill_name &&
-      isUploadableSkillSource(skill.source),
+      skill.source === primary.skill_source,
   );
+  const localMatch = candidates.length === 1 ? candidates[0] : undefined;
   const updateDisabledReason = !localWorkspaces
     ? "updating the snapshot needs the desktop app"
     : localMatch
       ? null
-      : `no local skill named ${primary.skill_name} was found on this machine`;
+      : candidates.length > 1
+        ? `several local skills are named ${primary.skill_name}; pick the right one from the edit form`
+        : `no local ${primary.skill_source} skill named ${primary.skill_name} was found on this machine`;
 
   const handleUpdate = () => {
     if (!localMatch || !isUploadableSkillSource(localMatch.source)) return;

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

@@ -16,8 +16,15 @@ import {
 import { Box, Flex, Text, TextField } from "@radix-ui/themes";
 import { type ReactNode, useEffect, useState } from "react";
 import { useAuthStateValue } from "../../auth/store";
-import { useCreateLoop, useUpdateLoop } from "../hooks/useLoopMutations";
-import { useSyncLoopSkillBundles } from "../hooks/useLoopSkillBundles";
+import {
+  useCreateLoop,
+  useDeleteLoop,
+  useUpdateLoop,
+} from "../hooks/useLoopMutations";
+import {
+  useBundleLocalSkill,
+  useReplaceLoopSkillBundles,
+} from "../hooks/useLoopSkillBundles";
 import { summarizeTrigger } from "../loopDisplay";
 import { useLoopDraftStore } from "../loopDraftStore";
 import {
@@ -31,7 +38,7 @@ import {
   loopToFormValues,
   normalizeLoopFormValues,
 } from "../loopFormTypes";
-import { buildSkillInstructions } from "../loopSkill";
+import { buildSkillInstructions, loopSkillBundles } from "../loopSkill";
 import { LoopBehaviorFields } from "./LoopBehaviorFields";
 import { LoopContextFields } from "./LoopContextFields";
 import { Field } from "./LoopFormPrimitives";
@@ -88,10 +95,14 @@ export function LoopForm({ loop }: LoopFormProps) {
 
   const createLoop = useCreateLoop();
   const updateLoop = useUpdateLoop(loop?.id ?? "");
-  const syncSkillBundles = useSyncLoopSkillBundles();
+  const deleteLoop = useDeleteLoop();
+  const bundleSkill = useBundleLocalSkill();
+  const replaceSkillBundles = useReplaceLoopSkillBundles();
   const isSubmitting =
     (isEdit ? updateLoop.isPending : createLoop.isPending) ||
-    syncSkillBundles.isPending;
+    bundleSkill.isPending ||
+    replaceSkillBundles.isPending ||
+    deleteLoop.isPending;
   const canSubmit = isLoopFormValid(values) && !isSubmitting;
 
   // Per-step gate for the Next button. The final Create button is gated on the
@@ -130,26 +141,52 @@ export function LoopForm({ loop }: LoopFormProps) {
   const handleSubmit = async () => {
     if (!canSubmit) return;
     const body = formValuesToLoopWrite(values);
+
+    // Bundling runs before anything is persisted: a missing or broken local
+    // skill fails here with no partial state, instead of leaving a saved loop
+    // whose `/skill-name` instructions have no matching bundle.
+    let uploads: LoopSchemas.LoopSkillBundleUpload[] | null = null;
+    if (values.skill?.kind === "local") {
+      try {
+        uploads = await bundleSkill.mutateAsync(values.skill);
+      } catch (error) {
+        toast.error("Failed to bundle the skill", {
+          description: error instanceof Error ? error.message : undefined,
+        });
+        return;
+      }
+    }
+
     try {
       const saved = isEdit
         ? await updateLoop.mutateAsync(body)
         : await createLoop.mutateAsync(body);
-      // The skill snapshot rides separately from the loop write: a locally
-      // picked skill is bundled and uploaded, removing the skill detaches the
-      // stored bundles, and an unchanged attached snapshot is left alone.
-      const skillSync =
-        values.skill?.kind === "local"
-          ? { loopId: saved.id, skill: values.skill }
-          : values.skill === null && saved.skill_bundles.length > 0
-            ? { loopId: saved.id, skill: null }
-            : null;
-      if (skillSync) {
+      const needsDetach =
+        values.skill === null && loopSkillBundles(saved).length > 0;
+      if (uploads || needsDetach) {
         try {
-          await syncSkillBundles.mutateAsync(skillSync);
+          await replaceSkillBundles.mutateAsync({
+            loopId: saved.id,
+            uploads: uploads ?? [],
+          });
         } catch (error) {
+          const description =
+            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 });
+            return;
+          }
+          // Keep the form open with its state intact: saving again retries
+          // both the loop write and the skill upload.
           toast.error("Loop saved, but updating its skill failed", {
-            description: error instanceof Error ? error.message : undefined,
+            description: [description, "Save again to retry."]
+              .filter(Boolean)
+              .join(" "),
           });
+          return;
         }
       }
       navigateToLoopDetail(saved.id);

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

@@ -14,6 +14,20 @@ interface PickableSkill {
   name: string;
   source: LoopSkillDraft["source"];
   path: string;
+  repoName?: string;
+}
+
+/** Same-named skills from different sources must be tellable apart in the picker,
+ * or the user can't know which one the loop will snapshot. */
+function pickableSkillLabel(
+  skill: PickableSkill,
+  duplicatedNames: Set<string>,
+): string {
+  if (!duplicatedNames.has(skill.name)) return skill.name;
+  const origin = skill.repoName
+    ? `${skill.source}: ${skill.repoName}`
+    : skill.source;
+  return `${skill.name} (${origin})`;
 }
 
 export function LoopInstructionsFields({
@@ -34,9 +48,24 @@ export function LoopInstructionsFields({
   const localSkills: PickableSkill[] = (skillsQuery.data ?? []).flatMap(
     (skill) =>
       isUploadableSkillSource(skill.source)
-        ? [{ name: skill.name, source: skill.source, path: skill.path }]
+        ? [
+            {
+              name: skill.name,
+              source: skill.source,
+              path: skill.path,
+              repoName: skill.repoName,
+            },
+          ]
         : [],
   );
+  const duplicatedNames = new Set(
+    localSkills
+      .filter(
+        (skill, _, all) =>
+          all.filter((other) => other.name === skill.name).length > 1,
+      )
+      .map((skill) => skill.name),
+  );
 
   const skill = values.skill;
   const skillUnavailableHint = localWorkspaces
@@ -59,8 +88,8 @@ export function LoopInstructionsFields({
       value: local.path,
       label:
         skill?.kind === "attached" && local.name === skill.name
-          ? `${local.name} (pick again to refresh the snapshot)`
-          : local.name,
+          ? `${pickableSkillLabel(local, duplicatedNames)} (pick again to refresh the snapshot)`
+          : pickableSkillLabel(local, duplicatedNames),
     })),
   ];
 

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

@@ -13,11 +13,78 @@ export interface LoopLocalSkillRef {
   path: string;
 }
 
+type HostClient = ReturnType<typeof useHostTRPCClient>;
+
+async function buildSkillUploads(
+  hostClient: HostClient,
+  skill: LoopLocalSkillRef,
+): Promise<LoopSchemas.LoopSkillBundleUpload[]> {
+  const refs = await hostClient.skills.resolveDependencies.query([skill]);
+  const ordered = [skill, ...refs.filter((ref) => ref.name !== skill.name)];
+  const bundles = await Promise.all(
+    ordered.map((ref) => hostClient.skills.bundleLocal.query(ref)),
+  );
+  return bundles.map((bundle) => ({
+    file_name: bundle.fileName,
+    skill_name: bundle.name,
+    skill_source: bundle.source,
+    content_sha256: bundle.contentSha256,
+    bundle_format: "zip" as const,
+    content_base64: bundle.contentBase64,
+  }));
+}
+
+/**
+ * Bundles a local skill plus its skill dependencies via workspace-server into
+ * upload payloads. Host-only: nothing is persisted anywhere, so callers can run
+ * it before writing the loop and fail fast with no partial state.
+ */
+export function useBundleLocalSkill() {
+  const hostClient = useHostTRPCClient();
+  return useMutation<
+    LoopSchemas.LoopSkillBundleUpload[],
+    Error,
+    LoopLocalSkillRef
+  >({
+    mutationFn: (skill) => buildSkillUploads(hostClient, skill),
+  });
+}
+
+/** Replaces a loop's stored bundles wholesale; an empty list detaches them all. */
+export function useReplaceLoopSkillBundles() {
+  const loopsClient = useLoopsClient();
+  const queryClient = useQueryClient();
+
+  return useMutation<
+    LoopSchemas.Loop,
+    Error,
+    { loopId: string; uploads: LoopSchemas.LoopSkillBundleUpload[] }
+  >({
+    mutationFn: async ({ loopId, uploads }) => {
+      if (!loopsClient) throw new Error("Not authenticated");
+      return await replaceLoopSkillBundles(
+        loopsClient.client,
+        loopsClient.projectId,
+        loopId,
+        uploads,
+      );
+    },
+    onSuccess: (loop) => {
+      queryClient.setQueryData(
+        loopsKeys.detail(loopsClient?.projectId ?? null, loop.id),
+        loop,
+      );
+      void queryClient.invalidateQueries({
+        queryKey: loopsKeys.list(loopsClient?.projectId ?? null),
+      });
+    },
+  });
+}
+
 /**
- * Replaces a loop's attached skill bundles: bundles the given local skill plus
- * its skill dependencies via workspace-server and uploads the set wholesale, or
- * detaches every bundle when `skill` is null. Detaching never touches the host,
- * so it also works on hosts without a local filesystem.
+ * Bundle-and-replace in one step, for refreshing an existing loop's snapshot
+ * from the detail page. Detaching (null skill) never touches the host, so it
+ * also works on hosts without a local filesystem.
  */
 export function useSyncLoopSkillBundles() {
   const loopsClient = useLoopsClient();
@@ -31,25 +98,7 @@ export function useSyncLoopSkillBundles() {
   >({
     mutationFn: async ({ loopId, skill }) => {
       if (!loopsClient) throw new Error("Not authenticated");
-      let uploads: LoopSchemas.LoopSkillBundleUpload[] = [];
-      if (skill) {
-        const refs = await hostClient.skills.resolveDependencies.query([skill]);
-        const ordered = [
-          skill,
-          ...refs.filter((ref) => ref.name !== skill.name),
-        ];
-        const bundles = await Promise.all(
-          ordered.map((ref) => hostClient.skills.bundleLocal.query(ref)),
-        );
-        uploads = bundles.map((bundle) => ({
-          file_name: bundle.fileName,
-          skill_name: bundle.name,
-          skill_source: bundle.source,
-          content_sha256: bundle.contentSha256,
-          bundle_format: "zip" as const,
-          content_base64: bundle.contentBase64,
-        }));
-      }
+      const uploads = skill ? await buildSkillUploads(hostClient, skill) : [];
       return await replaceLoopSkillBundles(
         loopsClient.client,
         loopsClient.projectId,

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

@@ -45,9 +45,16 @@ export function parseSkillContext(
   return trimmed;
 }
 
+/** The loop's attached bundles, tolerating a backend that predates the field. */
+export function loopSkillBundles(
+  loop: LoopSchemas.Loop,
+): LoopSchemas.LoopSkillBundle[] {
+  return loop.skill_bundles ?? [];
+}
+
 /** The bundle whose skill the loop's instructions invoke; dependency bundles follow it. */
 export function primaryLoopSkillBundle(
   loop: LoopSchemas.Loop,
 ): LoopSchemas.LoopSkillBundle | null {
-  return loop.skill_bundles[0] ?? null;
+  return loopSkillBundles(loop)[0] ?? null;
 }