From 29a1aa472e2eb17a574dd987836b9b45758441b3 Mon Sep 17 00:00:00 2001 From: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Date: Sat, 25 Jul 2026 22:43:28 -0400 Subject: [PATCH] fix(desktop): harness form round-trip, delete blast-radius copy, comma-arg inline mirror MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Tyler Longwell --- desktop/src/features/agents/hooks.ts | 3 +- .../settings/ui/HarnessManagementCard.tsx | 80 +++++++----- .../settings/ui/harnessFormLogic.test.mjs | 119 ++++++++++++++++++ .../features/settings/ui/harnessFormLogic.ts | 89 +++++++++++++ .../settings/ui/harnessGalleryLogic.test.mjs | 74 +++++++++++ .../settings/ui/harnessGalleryLogic.ts | 35 ++++++ desktop/src/shared/api/tauri.ts | 3 + desktop/src/shared/api/types.ts | 6 + desktop/src/testing/e2eBridge.ts | 10 ++ 9 files changed, 386 insertions(+), 33 deletions(-) diff --git a/desktop/src/features/agents/hooks.ts b/desktop/src/features/agents/hooks.ts index 199ee7e88..122c872e5 100644 --- a/desktop/src/features/agents/hooks.ts +++ b/desktop/src/features/agents/hooks.ts @@ -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, diff --git a/desktop/src/features/settings/ui/HarnessManagementCard.tsx b/desktop/src/features/settings/ui/HarnessManagementCard.tsx index e993e7739..1e243e9dc 100644 --- a/desktop/src/features/settings/ui/HarnessManagementCard.tsx +++ b/desktop/src/features/settings/ui/HarnessManagementCard.tsx @@ -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({ /> +
+

+ Install hint{" "} + (optional) +

+ +
+ {error ? (

{error} @@ -439,18 +445,20 @@ function CustomHarnessRow({ entry }: { entry: AcpRuntimeCatalogEntry }) { const [confirmingDelete, setConfirmingDelete] = React.useState(false); const [deleteError, setDeleteError] = React.useState(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 ( setEditing(false)} onSaved={() => setEditing(false)} @@ -547,6 +555,14 @@ function CustomHarnessRow({ entry }: { entry: AcpRuntimeCatalogEntry }) { )} + {confirmingDelete ? ( +

+ {deleteHarnessConfirmMessage(entry.label, referencingAgents)} +

+ ) : null} {deleteError ? (

{deleteError} diff --git a/desktop/src/features/settings/ui/harnessFormLogic.test.mjs b/desktop/src/features/settings/ui/harnessFormLogic.test.mjs index b12a8501d..818a4020d 100644 --- a/desktop/src/features/settings/ui/harnessFormLogic.test.mjs +++ b/desktop/src/features/settings/ui/harnessFormLogic.test.mjs @@ -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"')); +}); diff --git a/desktop/src/features/settings/ui/harnessFormLogic.ts b/desktop/src/features/settings/ui/harnessFormLogic.ts index a276f6650..8332f8ae7 100644 --- a/desktop/src/features/settings/ui/harnessFormLogic.ts +++ b/desktop/src/features/settings/ui/harnessFormLogic.ts @@ -67,3 +67,92 @@ export function envPairsFromRecord( export function filterArgs(args: ReadonlyArray): 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; + installInstructionsUrl: string; + installHint: string; +} + +/** Definition payload sent to `save_custom_harness`. */ +export interface HarnessDefinitionPayload { + id: string; + label: string; + command: string; + args: string[]; + env: Record; + 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 | 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.`; +} diff --git a/desktop/src/features/settings/ui/harnessGalleryLogic.test.mjs b/desktop/src/features/settings/ui/harnessGalleryLogic.test.mjs index dcf240cd2..de5720594 100644 --- a/desktop/src/features/settings/ui/harnessGalleryLogic.test.mjs +++ b/desktop/src/features/settings/ui/harnessGalleryLogic.test.mjs @@ -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?", + ); + }); +}); diff --git a/desktop/src/features/settings/ui/harnessGalleryLogic.ts b/desktop/src/features/settings/ui/harnessGalleryLogic.ts index b8cd3318a..3c8023287 100644 --- a/desktop/src/features/settings/ui/harnessGalleryLogic.ts +++ b/desktop/src/features/settings/ui/harnessGalleryLogic.ts @@ -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}?`; +} diff --git a/desktop/src/shared/api/tauri.ts b/desktop/src/shared/api/tauri.ts index 28bfe20db..4452a3e03 100644 --- a/desktop/src/shared/api/tauri.ts +++ b/desktop/src/shared/api/tauri.ts @@ -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, diff --git a/desktop/src/shared/api/types.ts b/desktop/src/shared/api/types.ts index a7ec2bcd8..af52e260a 100644 --- a/desktop/src/shared/api/types.ts +++ b/desktop/src/shared/api/types.ts @@ -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; diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index 23edc74a4..e9a7266ef 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -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,