fix(desktop): harness form round-trip, delete blast-radius copy, comma-arg inline mirror

Frontend layer of the review fixes: - installHint survives edit: form seeding extracted to formValuesFromCatalogEntry (single source of truth for edit initialization — every user-editable field must appear there), input rendered, and a lossless entry -> form -> save-payload round-trip regression covers the full definition, not one field at a time. - Delete confirmation counts referencing agents (direct record pins plus persona-inherited runtime) and names the blast radius: 'N agents use this harness and will stop launching.' Queries only run while the confirmation is open. - Comma-in-args rejection mirrored inline in the form with an error naming the offending argument; the authoritative gate stays in validate_harness_definition on the Rust side. - ManagedAgent (TS) carries the record-level runtime pin; e2e bridge mirrors the native wire shape (key always present, null when unpinned). Co-authored-by: Tyler Longwell <tlongwell@block.xyz> Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
2026-08-18 06:50:31 +02:00 · 2026-07-25 22:43:28 -04:00
parent 9c0d097ce0
commit 29a1aa472e
9 changed files with 386 additions and 33 deletions
@@ -283,8 +283,9 @@ export function useBackendProvidersQuery(options?: { enabled?: boolean }) {
  });
 }

-export function usePersonasQuery() {
+export function usePersonasQuery(options?: { enabled?: boolean }) {
  return useQuery({
+    enabled: options?.enabled ?? true,
    queryKey: personasQueryKey,
    queryFn: listPersonas,
    staleTime: 30_000,
@@ -6,6 +6,8 @@ import {
  useAcpRuntimesQuery,
  useDeleteCustomHarnessMutation,
  useManagedAgentPrereqsQuery,
+  useManagedAgentsQuery,
+  usePersonasQuery,
  useSaveCustomHarnessMutation,
 } from "@/features/agents/hooks";
 import type { AcpRuntimeCatalogEntry } from "@/shared/api/types";
@@ -19,13 +21,16 @@ import {
  RuntimeIcon,
 } from "../../onboarding/ui/RuntimeIcon";
 import {
-  buildEnvRecord,
-  envPairsFromRecord,
-  filterArgs,
+  commaArgError,
+  type CustomFormValues,
+  definitionFromFormValues,
+  formValuesFromCatalogEntry,
  idFromLabel,
 } from "./harnessFormLogic";
 import {
+  countAgentsReferencingHarness,
  customEntries as getCustomEntries,
+  deleteHarnessConfirmMessage,
  sortedPresetEntries,
 } from "./harnessGalleryLogic";
 import { SettingsSectionHeader } from "./SettingsSectionHeader";
@@ -87,18 +92,6 @@ function PresetCard({ entry }: { entry: AcpRuntimeCatalogEntry }) {

 // ── Custom harness form ───────────────────────────────────────────────────────

-interface CustomFormValues {
-  id: string;
-  label: string;
-  command: string;
-  /** Each element is one argument; no space-splitting round-trip. */
-  args: string[];
-  /** KEY=VALUE pairs for env injection at spawn time. */
-  env: Array<{ key: string; value: string }>;
-  installInstructionsUrl: string;
-  installHint: string;
-}
-
 const EMPTY_FORM: CustomFormValues = {
  id: "",
  label: "",
@@ -298,17 +291,16 @@ function CustomHarnessForm({
  async function handleSubmit(e: React.FormEvent) {
    e.preventDefault();
    setError(null);
+    // Mirror the backend comma-in-args rejection so the user gets an inline
+    // error naming the offending argument before the round-trip.
+    const commaError = commaArgError(form.args);
+    if (commaError) {
+      setError(commaError);
+      return;
+    }
    try {
      await save.mutateAsync({
-        definition: {
-          id: form.id.trim(),
-          label: form.label.trim(),
-          command: form.command.trim(),
-          args: filterArgs(form.args),
-          env: buildEnvRecord(form.env),
-          installInstructionsUrl: form.installInstructionsUrl.trim(),
-          installHint: form.installHint.trim(),
-        },
+        definition: definitionFromFormValues(form),
        originalId,
      });
      onSaved();
@@ -413,6 +405,20 @@ function CustomHarnessForm({
        />
      </div>

+      <div className="space-y-1">
+        <p className="text-xs text-muted-foreground">
+          Install hint{" "}
+          <span className="text-muted-foreground/60">(optional)</span>
+        </p>
+        <Input
+          className="h-8 text-sm"
+          id="ch-install-hint"
+          onChange={field("installHint")}
+          placeholder="npm install -g my-harness"
+          value={form.installHint}
+        />
+      </div>
+
      {error ? (
        <p className="rounded-lg border border-destructive/30 bg-destructive/10 px-3 py-1.5 text-sm text-destructive">
          {error}
@@ -439,18 +445,20 @@ function CustomHarnessRow({ entry }: { entry: AcpRuntimeCatalogEntry }) {
  const [confirmingDelete, setConfirmingDelete] = React.useState(false);
  const [deleteError, setDeleteError] = React.useState<string | null>(null);
  const del = useDeleteCustomHarnessMutation();
+  // Blast-radius data for the delete confirmation — only fetched while the
+  // confirmation is open, so the row list doesn't poll agents.
+  const agentsQuery = useManagedAgentsQuery({ enabled: confirmingDelete });
+  const personasQuery = usePersonasQuery({ enabled: confirmingDelete });
+  const referencingAgents = countAgentsReferencingHarness(
+    entry.id,
+    agentsQuery.data ?? [],
+    personasQuery.data ?? [],
+  );

  if (editing) {
    return (
      <CustomHarnessForm
-        initial={{
-          id: entry.id,
-          label: entry.label,
-          command: entry.command ?? "",
-          args: entry.defaultArgs ?? [],
-          env: envPairsFromRecord(entry.definitionEnv),
-          installInstructionsUrl: entry.installInstructionsUrl,
-        }}
+        initial={formValuesFromCatalogEntry(entry)}
        originalId={entry.id}
        onCancel={() => setEditing(false)}
        onSaved={() => setEditing(false)}
@@ -547,6 +555,14 @@ function CustomHarnessRow({ entry }: { entry: AcpRuntimeCatalogEntry }) {
          )}
        </div>
      </div>
+      {confirmingDelete ? (
+        <p
+          className="rounded-lg border border-amber-500/30 bg-amber-500/10 px-3 py-1.5 text-sm text-amber-600 dark:text-amber-400"
+          data-testid={`custom-harness-delete-warning-${entry.id}`}
+        >
+          {deleteHarnessConfirmMessage(entry.label, referencingAgents)}
+        </p>
+      ) : null}
      {deleteError ? (
        <p className="rounded-lg border border-destructive/30 bg-destructive/10 px-3 py-1.5 text-sm text-destructive">
          {deleteError}
@@ -6,6 +6,9 @@ import {
  buildEnvRecord,
  filterArgs,
  envPairsFromRecord,
+  formValuesFromCatalogEntry,
+  definitionFromFormValues,
+  commaArgError,
 } from "./harnessFormLogic.ts";

 // ── idFromLabel ──────────────────────────────────────────────────────────────
@@ -170,3 +173,119 @@ test("envPairsFromRecord_roundTrip_buildEnvRecord_restoresOriginal", () => {
  const restored = buildEnvRecord(pairs);
  assert.deepEqual(restored, original);
 });
+
+// ── edit round-trip (entry → form → definition) ──────────────────────────────
+
+/** A fully-populated definition as it appears on a catalog entry. */
+const FULL_ENTRY = {
+  id: "my-harness",
+  label: "My Harness",
+  command: "my-harness",
+  defaultArgs: ["acp", "--verbose"],
+  definitionEnv: { FOO: "bar", BAZ: "qux" },
+  installInstructionsUrl: "https://example.com/docs",
+  installHint: "npm install -g my-harness",
+};
+
+test("formValuesFromCatalogEntry_fullEntry_populatesEveryField", () => {
+  const form = formValuesFromCatalogEntry(FULL_ENTRY);
+  assert.equal(form.id, "my-harness");
+  assert.equal(form.label, "My Harness");
+  assert.equal(form.command, "my-harness");
+  assert.deepEqual(form.args, ["acp", "--verbose"]);
+  assert.deepEqual(form.env, [
+    { key: "FOO", value: "bar" },
+    { key: "BAZ", value: "qux" },
+  ]);
+  assert.equal(form.installInstructionsUrl, "https://example.com/docs");
+  assert.equal(form.installHint, "npm install -g my-harness");
+});
+
+test("editRoundTrip_openThenSaveWithoutChanges_losesNothing", () => {
+  // Regression for the installHint edit-loss bug: opening the edit form and
+  // immediately saving must reproduce the original definition verbatim.
+  const definition = definitionFromFormValues(
+    formValuesFromCatalogEntry(FULL_ENTRY),
+  );
+  assert.deepEqual(definition, {
+    id: FULL_ENTRY.id,
+    label: FULL_ENTRY.label,
+    command: FULL_ENTRY.command,
+    args: FULL_ENTRY.defaultArgs,
+    env: FULL_ENTRY.definitionEnv,
+    installInstructionsUrl: FULL_ENTRY.installInstructionsUrl,
+    installHint: FULL_ENTRY.installHint,
+  });
+});
+
+test("editRoundTrip_definitionKeys_matchFormSeedFields", () => {
+  // Structural guard: any user-editable field added to the save payload must
+  // also be seeded on edit, or open-edit-then-save silently erases it (the
+  // installHint bug). `args`/`env` map to `defaultArgs`/`definitionEnv`.
+  const seeded = new Set(Object.keys(formValuesFromCatalogEntry(FULL_ENTRY)));
+  const saved = Object.keys(
+    definitionFromFormValues(formValuesFromCatalogEntry(FULL_ENTRY)),
+  );
+  for (const key of saved) {
+    assert.ok(
+      seeded.has(key),
+      `save payload field "${key}" not seeded on edit`,
+    );
+  }
+});
+
+test("formValuesFromCatalogEntry_nullishOptionalFields_defaultsSafely", () => {
+  const form = formValuesFromCatalogEntry({
+    id: "min",
+    label: "Min",
+    command: null,
+    defaultArgs: undefined,
+    definitionEnv: undefined,
+    installInstructionsUrl: "",
+    installHint: "",
+  });
+  assert.equal(form.command, "");
+  assert.deepEqual(form.args, []);
+  assert.deepEqual(form.env, []);
+});
+
+test("definitionFromFormValues_trimsScalarFields", () => {
+  const definition = definitionFromFormValues({
+    id: " my-id ",
+    label: " Label ",
+    command: " cmd ",
+    args: ["keep", "  "],
+    env: [{ key: " K ", value: "v" }],
+    installInstructionsUrl: " https://x ",
+    installHint: " hint ",
+  });
+  assert.equal(definition.id, "my-id");
+  assert.equal(definition.label, "Label");
+  assert.equal(definition.command, "cmd");
+  assert.deepEqual(definition.args, ["keep"]);
+  assert.deepEqual(definition.env, { K: "v" });
+  assert.equal(definition.installInstructionsUrl, "https://x");
+  assert.equal(definition.installHint, "hint");
+});
+
+// ── commaArgError ─────────────────────────────────────────────────────────────
+
+test("commaArgError_noCommas_returnsNull", () => {
+  assert.equal(commaArgError(["acp", "--verbose", "--name", "x y"]), null);
+});
+
+test("commaArgError_emptyArgs_returnsNull", () => {
+  assert.equal(commaArgError([]), null);
+});
+
+test("commaArgError_commaInArg_namesOffender", () => {
+  const err = commaArgError(["--models", "a,b"]);
+  assert.ok(err !== null);
+  assert.ok(err.includes('"a,b"'), "error must name the offending argument");
+  assert.ok(err.includes("comma"), "error must name the transport limit");
+});
+
+test("commaArgError_firstOffenderReported", () => {
+  const err = commaArgError(["x,y", "p,q"]);
+  assert.ok(err !== null && err.includes('"x,y"'));
+});
@@ -67,3 +67,92 @@ export function envPairsFromRecord(
 export function filterArgs(args: ReadonlyArray<string>): string[] {
  return args.filter((a) => a.trim() !== "");
 }
+
+/** Form state for the custom-harness create/edit form. */
+export interface CustomFormValues {
+  id: string;
+  label: string;
+  command: string;
+  /** Each element is one argument; no space-splitting round-trip. */
+  args: string[];
+  /** KEY=VALUE pairs for env injection at spawn time. */
+  env: Array<{ key: string; value: string }>;
+  installInstructionsUrl: string;
+  installHint: string;
+}
+
+/** The subset of a catalog entry that seeds the edit form. */
+export interface EditableCatalogEntry {
+  id: string;
+  label: string;
+  command: string | null;
+  defaultArgs: string[] | undefined;
+  definitionEnv?: Record<string, string>;
+  installInstructionsUrl: string;
+  installHint: string;
+}
+
+/** Definition payload sent to `save_custom_harness`. */
+export interface HarnessDefinitionPayload {
+  id: string;
+  label: string;
+  command: string;
+  args: string[];
+  env: Record<string, string>;
+  installInstructionsUrl: string;
+  installHint: string;
+}
+
+/**
+ * Seed form values from an existing catalog entry (edit flow).
+ *
+ * This is the single source of truth for edit initialization: every
+ * user-editable definition field must appear here, or an open-edit-then-save
+ * silently erases it. `formValuesRoundTrip` in the tests proves the
+ * entry → form → save-payload loop is lossless.
+ */
+export function formValuesFromCatalogEntry(
+  entry: EditableCatalogEntry,
+): CustomFormValues {
+  return {
+    id: entry.id,
+    label: entry.label,
+    command: entry.command ?? "",
+    args: entry.defaultArgs ?? [],
+    env: envPairsFromRecord(entry.definitionEnv),
+    installInstructionsUrl: entry.installInstructionsUrl,
+    installHint: entry.installHint,
+  };
+}
+
+/** Build the `save_custom_harness` definition payload from form values. */
+export function definitionFromFormValues(
+  form: CustomFormValues,
+): HarnessDefinitionPayload {
+  return {
+    id: form.id.trim(),
+    label: form.label.trim(),
+    command: form.command.trim(),
+    args: filterArgs(form.args),
+    env: buildEnvRecord(form.env),
+    installInstructionsUrl: form.installInstructionsUrl.trim(),
+    installHint: form.installHint.trim(),
+  };
+}
+
+/**
+ * Client-side mirror of the backend comma-in-args rejection.
+ *
+ * Args travel to the harness through the comma-delimited
+ * `BUZZ_ACP_AGENT_ARGS` env transport (clap `value_delimiter = ','` on the
+ * buzz-acp side), so a literal comma inside one argument would silently split
+ * into two at spawn time. The backend validator rejects it; this mirror gives
+ * the user an inline error naming the offending argument before the round-trip.
+ *
+ * Returns the error message, or `null` when all args are comma-free.
+ */
+export function commaArgError(args: ReadonlyArray<string>): string | null {
+  const offender = args.find((a) => a.includes(","));
+  if (offender === undefined) return null;
+  return `Argument "${offender}" contains a comma — arguments are passed via a comma-delimited transport and would be split at spawn time. Use separate argument entries instead.`;
+}
@@ -5,6 +5,8 @@ import {
  sortedPresetEntries,
  customEntries,
  isEditableEntry,
+  countAgentsReferencingHarness,
+  deleteHarnessConfirmMessage,
 } from "./harnessGalleryLogic.ts";

 // ── Minimal catalog entry factory ────────────────────────────────────────────
@@ -172,3 +174,75 @@ describe("isEditableEntry", () => {
    assert.ok(!isEditableEntry(entry({ source: "builtin" })));
  });
 });
+
+// ── countAgentsReferencingHarness ─────────────────────────────────────────────
+
+describe("countAgentsReferencingHarness", () => {
+  it("counts direct record-level runtime pins", () => {
+    const agents = [
+      { runtime: "my-harness", personaId: null },
+      { runtime: "other", personaId: null },
+      { runtime: "my-harness", personaId: "p1" },
+    ];
+    assert.equal(countAgentsReferencingHarness("my-harness", agents, []), 2);
+  });
+
+  it("counts persona-inherited references when record runtime is null", () => {
+    const agents = [{ runtime: null, personaId: "p1" }];
+    const personas = [{ id: "p1", runtime: "my-harness" }];
+    assert.equal(
+      countAgentsReferencingHarness("my-harness", agents, personas),
+      1,
+    );
+  });
+
+  it("record-level pin shadows persona runtime (no double counting, pin wins)", () => {
+    // Agent pinned to "other" whose persona uses "my-harness" does NOT count:
+    // the effective harness is the pin.
+    const agents = [{ runtime: "other", personaId: "p1" }];
+    const personas = [{ id: "p1", runtime: "my-harness" }];
+    assert.equal(
+      countAgentsReferencingHarness("my-harness", agents, personas),
+      0,
+    );
+  });
+
+  it("agents with no runtime and no persona do not count", () => {
+    const agents = [{ runtime: null, personaId: null }];
+    assert.equal(countAgentsReferencingHarness("my-harness", agents, []), 0);
+  });
+
+  it("persona reference to a different harness does not count", () => {
+    const agents = [{ runtime: null, personaId: "p1" }];
+    const personas = [{ id: "p1", runtime: "other" }];
+    assert.equal(
+      countAgentsReferencingHarness("my-harness", agents, personas),
+      0,
+    );
+  });
+});
+
+// ── deleteHarnessConfirmMessage ───────────────────────────────────────────────
+
+describe("deleteHarnessConfirmMessage", () => {
+  it("plain confirmation when nothing references the harness", () => {
+    assert.equal(
+      deleteHarnessConfirmMessage("My Harness", 0),
+      "Delete My Harness?",
+    );
+  });
+
+  it("singular copy for one referencing agent", () => {
+    assert.equal(
+      deleteHarnessConfirmMessage("My Harness", 1),
+      "1 agent uses this harness and will stop launching. Delete My Harness?",
+    );
+  });
+
+  it("plural copy for multiple referencing agents", () => {
+    assert.equal(
+      deleteHarnessConfirmMessage("My Harness", 3),
+      "3 agents use this harness and will stop launching. Delete My Harness?",
+    );
+  });
+});
@@ -41,3 +41,38 @@ export function customEntries(
 export function isEditableEntry(entry: AcpRuntimeCatalogEntry): boolean {
  return entry.source === "custom";
 }
+
+/**
+ * Count managed agents whose effective harness is the given definition id —
+ * either a direct record-level `runtime` pin, or inheritance from a linked
+ * persona whose `runtime` is that id.
+ *
+ * Drives the delete-confirmation copy: deleting a harness that agents still
+ * reference is allowed (blocking would turn cleanup into dependency
+ * untangling), but the user is told those agents will stop launching.
+ */
+export function countAgentsReferencingHarness(
+  harnessId: string,
+  agents: ReadonlyArray<{ runtime: string | null; personaId: string | null }>,
+  personas: ReadonlyArray<{ id: string; runtime: string | null }>,
+): number {
+  const personaRuntime = new Map(personas.map((p) => [p.id, p.runtime]));
+  return agents.filter((agent) => {
+    if (agent.runtime !== null) return agent.runtime === harnessId;
+    if (agent.personaId === null) return false;
+    return personaRuntime.get(agent.personaId) === harnessId;
+  }).length;
+}
+
+/**
+ * Confirmation copy for deleting a custom harness. Names the blast radius
+ * when agents still reference the definition; stays quiet when none do.
+ */
+export function deleteHarnessConfirmMessage(
+  label: string,
+  referencingAgents: number,
+): string {
+  if (referencingAgents === 0) return `Delete ${label}?`;
+  const noun = referencingAgents === 1 ? "agent uses" : "agents use";
+  return `${referencingAgents} ${noun} this harness and will stop launching. Delete ${label}?`;
+}
@@ -118,6 +118,8 @@ export type RawManagedAgent = {
  pubkey: string;
  name: string;
  persona_id: string | null;
+  // Optional: pre-feature fixtures may omit it. The record's harness/runtime id.
+  runtime?: string | null;
  team_id?: string | null;
  relay_url: string;
  acp_command: string;
@@ -693,6 +695,7 @@ export function fromRawManagedAgent(agent: RawManagedAgent): ManagedAgent {
    pubkey: agent.pubkey,
    name: agent.name,
    personaId: agent.persona_id,
+    runtime: agent.runtime ?? null,
    teamId: agent.team_id ?? null,
    relayUrl: agent.relay_url,
    acpCommand: agent.acp_command,
@@ -343,6 +343,12 @@ export type ManagedAgent = {
  pubkey: string;
  name: string;
  personaId: string | null;
+  /**
+   * The record's harness/runtime id (e.g. "goose", "my-custom-harness").
+   * `null` means the agent inherits its harness from the linked persona.
+   * Used to count agents referencing a harness definition (delete confirm).
+   */
+  runtime: string | null;
  teamId?: string | null;
  relayUrl: string;
  acpCommand: string;
@@ -75,6 +75,8 @@ type MockManagedAgentSeed = {
  name: string;
  avatarUrl?: string | null;
  personaId?: string | null;
+  /** Harness/runtime id pin; `null` = inherit from persona (native default). */
+  runtime?: string | null;
  status?: RawManagedAgent["status"];
  channelNames?: string[];
  channelIds?: string[];
@@ -708,6 +710,8 @@ type RawManagedAgent = {
  pubkey: string;
  name: string;
  persona_id: string | null;
+  /** Record-level harness/runtime pin (`null` when inheriting from the persona). */
+  runtime: string | null;
  relay_url: string;
  acp_command: string;
  agent_command: string;
@@ -1459,6 +1463,7 @@ function cloneManagedAgent(agent: MockManagedAgent): RawManagedAgent {
    pubkey: agent.pubkey,
    name: agent.name,
    persona_id: agent.persona_id,
+    runtime: agent.runtime ?? null,
    relay_url: agent.relay_url,
    acp_command: agent.acp_command,
    agent_command: agent.agent_command,
@@ -1992,6 +1997,9 @@ function buildSeededManagedAgent(seed: MockManagedAgentSeed): MockManagedAgent {
    pubkey: seed.pubkey,
    name: seed.name,
    persona_id: seed.personaId ?? null,
+    // Native serde always emits this key (`null` when unpinned) — the bridge
+    // must mirror the wire shape, not omit the key.
+    runtime: seed.runtime ?? null,
    relay_url: DEFAULT_RELAY_WS_URL,
    acp_command: "buzz-acp",
    agent_command: "goose",
@@ -7653,6 +7661,8 @@ async function handleCreateManagedAgent(
    pubkey,
    name,
    persona_id: args.input.personaId ?? null,
+    // Create never pins a harness id — the record inherits from the persona.
+    runtime: null,
    relay_url: args.input.relayUrl ?? DEFAULT_RELAY_WS_URL,
    acp_command: args.input.acpCommand ?? "buzz-acp",
    agent_command: agentCommand,