From 61efb088e47252b4d1f5a1feae3b0217c34aa132 Mon Sep 17 00:00:00 2001 From: Wes Date: Wed, 8 Jul 2026 09:15:57 -0600 Subject: [PATCH] refactor(desktop): re-host instance edit as AgentInstanceEditDialog behind AgentDialog (Phase 1B.3b) Move EditAgentDialog.tsx to AgentInstanceEditDialog.tsx (git mv, body unchanged) and route it through the unified AgentDialog entry point as a new "instance-edit" mode, so all four agent intents (create definition, create+start, create instance, edit instance) share one dialog surface. - AgentDialog props become a discriminated union; the create paths are untouched and now live in an internal AgentCreateDialogRouter. The exported mode type for create callers is AgentDialogCreateMode. - UserProfilePanel keeps its persistent mount, editAgentOpen state, and the requestOpenEditAgent nudge-event subscription unchanged; only the mounted component name changes. - Pure modules (personaRuntimeModel.ts, useRequiredCredentialState.ts) are zero-diff; the [open, agent.pubkey] reset lifecycle travels inside the moved body untouched. - New agentInstanceEditPinning.test.mjs pins the inherit-toggle -> gate -> submit seam chain through the pure modules as the component wires them, plus the steady-state-inherit authoritative-edits contract. - All testids move with the body; edit-agent.spec.ts passes unmodified (zero e2e spec edits). Signed-off-by: Wes Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co> --- .../src/features/agents/ui/AgentDialog.tsx | 47 +++- ...Dialog.tsx => AgentInstanceEditDialog.tsx} | 2 +- desktop/src/features/agents/ui/AgentsView.tsx | 6 +- .../features/agents/ui/CreateAgentDialog.tsx | 2 +- .../ui/agentInstanceEditPinning.test.mjs | 251 ++++++++++++++++++ .../ui/editAgentProviderDiscovery.test.mjs | 6 +- .../agents/ui/personaProviderModelFields.tsx | 2 +- .../features/profile/ui/UserProfilePanel.tsx | 5 +- 8 files changed, 300 insertions(+), 21 deletions(-) rename desktop/src/features/agents/ui/{EditAgentDialog.tsx => AgentInstanceEditDialog.tsx} (99%) create mode 100644 desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs diff --git a/desktop/src/features/agents/ui/AgentDialog.tsx b/desktop/src/features/agents/ui/AgentDialog.tsx index 683a293d81..7d76d25012 100644 --- a/desktop/src/features/agents/ui/AgentDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDialog.tsx @@ -4,6 +4,7 @@ import type { AcpRuntimeCatalogEntry, CreateManagedAgentResponse, CreatePersonaInput, + ManagedAgent, UpdatePersonaInput, } from "@/shared/api/types"; import { Switch } from "@/shared/ui/switch"; @@ -12,14 +13,15 @@ import { intentForStartToggle, type AgentCreateIntent, } from "./agentCreateIntent"; +import { AgentInstanceEditDialog } from "./AgentInstanceEditDialog"; import { CreateAgentDialog } from "./CreateAgentDialog"; import { createPersonaDialogState } from "./personaDialogState"; import { AgentDefinitionDialog } from "./AgentDefinitionDialog"; -export type AgentDialogMode = "definition" | "instance"; +export type AgentDialogCreateMode = "definition" | "instance"; -type AgentDialogProps = { - mode: AgentDialogMode; +type AgentDialogCreateProps = { + mode: AgentDialogCreateMode; onOpenChange: (open: boolean) => void; definitionError: Error | null; isDefinitionPending: boolean; @@ -32,14 +34,39 @@ type AgentDialogProps = { onInstanceCreated: (result: CreateManagedAgentResponse) => void; }; +type AgentDialogInstanceEditProps = { + mode: "instance-edit"; + agent: ManagedAgent; + open: boolean; + onOpenChange: (open: boolean) => void; + onUpdated?: (agent: ManagedAgent) => void; +}; + +type AgentDialogProps = AgentDialogCreateProps | AgentDialogInstanceEditProps; + /** - * Unified create entry point (Phase 1B.2): routes a create intent to the - * form that owns it. The definition family renders AgentDefinitionDialog in - * create mode with a "start after create" toggle; the standalone-instance - * intent renders CreateAgentDialog unchanged. Physical consolidation of the - * two forms is Phase 1B.3. + * Unified entry point (Phase 1B.2/1B.3b): routes an intent to the form that + * owns it. The definition family renders AgentDefinitionDialog in create mode + * with a "start after create" toggle; the standalone-instance intent renders + * CreateAgentDialog unchanged; instance-edit renders AgentInstanceEditDialog + * (persistent mount + `open` toggle — its reset lifecycle is keyed on + * [open, agent.pubkey]). Physical consolidation of the forms is Phase 1B.3c+. */ -export function AgentDialog({ +export function AgentDialog(props: AgentDialogProps) { + if (props.mode === "instance-edit") { + return ( + + ); + } + return ; +} + +function AgentCreateDialogRouter({ mode, onOpenChange, definitionError, @@ -48,7 +75,7 @@ export function AgentDialog({ runtimesLoading, onSubmitDefinition, onInstanceCreated, -}: AgentDialogProps) { +}: AgentDialogCreateProps) { const [startAfterCreate, setStartAfterCreate] = React.useState(true); // Stable identity across toggle flips — AgentDefinitionDialog re-initializes its // fields whenever `initialValues` changes. diff --git a/desktop/src/features/agents/ui/EditAgentDialog.tsx b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx similarity index 99% rename from desktop/src/features/agents/ui/EditAgentDialog.tsx rename to desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx index 65d946d6a9..b1f40938ea 100644 --- a/desktop/src/features/agents/ui/EditAgentDialog.tsx +++ b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx @@ -66,7 +66,7 @@ const ADVANCED_FIELDS_MOTION_TRANSITION = { ease: [0.23, 1, 0.32, 1], } as const; -export function EditAgentDialog({ +export function AgentInstanceEditDialog({ agent, open, onOpenChange, diff --git a/desktop/src/features/agents/ui/AgentsView.tsx b/desktop/src/features/agents/ui/AgentsView.tsx index cb9a65d119..0a5ccf7b6d 100644 --- a/desktop/src/features/agents/ui/AgentsView.tsx +++ b/desktop/src/features/agents/ui/AgentsView.tsx @@ -5,7 +5,7 @@ import { } from "@/features/agents/openCreateAgentEvent"; import { AddAgentToChannelDialog } from "./AddAgentToChannelDialog"; import { AddTeamToChannelDialog } from "./AddTeamToChannelDialog"; -import { AgentDialog, type AgentDialogMode } from "./AgentDialog"; +import { AgentDialog, type AgentDialogCreateMode } from "./AgentDialog"; import { BatchImportDialog } from "./BatchImportDialog"; import { PersonaCatalogDialog } from "./PersonaCatalogDialog"; import { AgentDefinitionDialog } from "./AgentDefinitionDialog"; @@ -33,9 +33,9 @@ export function AgentsView() { // so the unified create dialog and the edit/dup/import AgentDefinitionDialog // mount never coexist. const [createDialogMode, setCreateDialogMode] = - React.useState(null); + React.useState(null); - function openUnifiedCreate(mode: AgentDialogMode) { + function openUnifiedCreate(mode: AgentDialogCreateMode) { if (mode === "definition") { personas.prepareCreate(); } diff --git a/desktop/src/features/agents/ui/CreateAgentDialog.tsx b/desktop/src/features/agents/ui/CreateAgentDialog.tsx index dedb72a8f2..4c630ac3b1 100644 --- a/desktop/src/features/agents/ui/CreateAgentDialog.tsx +++ b/desktop/src/features/agents/ui/CreateAgentDialog.tsx @@ -226,7 +226,7 @@ export function CreateAgentDialog({ [allRequiredEnvKeys, bakedSatisfiedEnvKeys, fileSatisfiedEnvKeys], ); - // Clear model when provider scope changes, mirroring EditAgentDialog. + // Clear model when provider scope changes, mirroring AgentInstanceEditDialog. React.useEffect(() => { if ( !open || diff --git a/desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs b/desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs new file mode 100644 index 0000000000..b48ba421e2 --- /dev/null +++ b/desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs @@ -0,0 +1,251 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + requiredCredentialEnvKeys, + runtimeSupportsLlmProviderSelection, +} from "./personaDialogPickers.tsx"; +import { + computeEditAgentFormValidity, + hasMissingRequiredEnvKey, + resolveAgentCommandUpdate, + resolveInheritedRuntimeSubmission, + resolveRuntimeProviderCapability, +} from "./personaRuntimeModel.ts"; + +// ── Phase 1B.3b re-host pinning: inherit-toggle → gate → submit ───────────── +// +// AgentInstanceEditDialog (re-homed from EditAgentDialog) wires three seams +// that must never disagree after the move: +// 1. TOGGLE: flipping "Inherit runtime from persona" re-resolves +// prospectiveRuntimeId from the LINKED PERSONA's runtime (not the stale +// override) and feeds resolveInheritedRuntimeSubmission. +// 2. GATE: useRequiredCredentialState validates the SAME inheritedSubmission +// provider/env the submit path will persist (gate ↔ record ↔ spawn). +// 3. SUBMIT: UpdateManagedAgentInput persists that same snapshot, with +// resolveAgentCommandUpdate + harnessOverride derived from the toggle. +// These tests chain the pure modules exactly as the component does, so a +// re-host that re-derives any seam independently fails here. + +const runtimes = [ + { id: "buzz-agent", command: "buzz-agent-cmd", defaultArgs: [] }, + { id: "claude", command: "claude-cmd", defaultArgs: [] }, +]; + +// Mirrors the component's prospectiveRuntimeId memo: when inheriting, resolve +// from the linked persona's runtime; fall back to the agentCommand dual-match. +function prospectiveRuntimeIdFor({ + inheritHarness, + selectedRuntimeId, + linkedPersonaRuntime, + agentCommand, +}) { + if (!inheritHarness) { + return selectedRuntimeId; + } + const personaRuntimeId = linkedPersonaRuntime?.trim(); + if (personaRuntimeId) { + return ( + runtimes.find((r) => r.id === personaRuntimeId)?.id ?? personaRuntimeId + ); + } + return ( + runtimes.find((r) => r.command?.trim() === agentCommand.trim())?.id ?? + runtimes.find((r) => r.id === agentCommand.trim())?.id ?? + "" + ); +} + +// A Claude-pinned agent linked to a buzz-agent/anthropic persona — the +// inherit-transition scenario that exercises every seam at once. +const pinnedAgent = { + name: "test-agent", + agentCommand: "claude-cmd", + agentCommandOverride: "claude-cmd", + acpCommand: "acp", + personaId: "p1", + provider: null, + model: null, + envVars: {}, +}; +const persona = { + runtime: "buzz-agent", + provider: "anthropic", + model: "claude-sonnet-4-5", + envVars: { ANTHROPIC_API_KEY: "sk-persona" }, +}; + +function inheritTransitionState() { + const inheritHarness = true; // user just checked the toggle + const prospectiveRuntimeId = prospectiveRuntimeIdFor({ + inheritHarness, + selectedRuntimeId: "claude", + linkedPersonaRuntime: persona.runtime, + agentCommand: pinnedAgent.agentCommand, + }); + const inheritedSubmission = resolveInheritedRuntimeSubmission({ + inheritHarness, + agentWasHarnessPinned: pinnedAgent.agentCommandOverride != null, + provider: "", // pinned Claude agent carries no provider + personaProvider: persona.provider, + model: "", + personaModel: persona.model, + envVars: {}, + personaEnvVars: persona.envVars, + }); + return { inheritHarness, prospectiveRuntimeId, inheritedSubmission }; +} + +test("rehost_toggle_resolvesProspectiveRuntimeFromPersona_notOverride", () => { + const { prospectiveRuntimeId } = inheritTransitionState(); + assert.equal( + prospectiveRuntimeId, + "buzz-agent", + "inherit-toggle must resolve the prospective runtime from the linked persona, not the still-present Claude pin", + ); +}); + +test("rehost_gate_validatesTheSubmissionSnapshot_sameValuesAsSubmit", () => { + const { prospectiveRuntimeId, inheritedSubmission } = + inheritTransitionState(); + + // Gate half: the credential requirement must be computed from the + // PROSPECTIVE runtime + the submission snapshot's provider/env. + const providerForRequiredKeys = runtimeSupportsLlmProviderSelection( + prospectiveRuntimeId, + ) + ? (inheritedSubmission.provider ?? "") + : ""; + const requiredKeys = requiredCredentialEnvKeys( + prospectiveRuntimeId, + providerForRequiredKeys, + ); + const missing = hasMissingRequiredEnvKey( + requiredKeys, + inheritedSubmission.envVars, + ); + + // The persona snapshot carries the credential, so the gate must clear — + // and it must clear because of the SAME env map submit will persist. + assert.equal(inheritedSubmission.provider, "anthropic"); + assert.deepEqual(inheritedSubmission.envVars, { + ANTHROPIC_API_KEY: "sk-persona", + }); + assert.equal( + missing, + false, + "gate must validate the submission snapshot (persona-layered env), not the agent's own empty env", + ); +}); + +test("rehost_gate_blocksSave_whenSubmissionSnapshotLacksCredential", () => { + const { prospectiveRuntimeId } = inheritTransitionState(); + // Same transition but the persona carries no credential: the submission + // snapshot is credential-less and the SAME gate must now block Save. + const bareSubmission = resolveInheritedRuntimeSubmission({ + inheritHarness: true, + agentWasHarnessPinned: true, + provider: "", + personaProvider: persona.provider, + model: "", + personaModel: persona.model, + envVars: {}, + personaEnvVars: {}, // no key anywhere + }); + const requiredKeys = requiredCredentialEnvKeys( + prospectiveRuntimeId, + bareSubmission.provider ?? "", + ); + const requiredEnvKeyMissing = hasMissingRequiredEnvKey( + requiredKeys, + bareSubmission.envVars, + ); + assert.equal(requiredEnvKeyMissing, true); + + const canSubmit = computeEditAgentFormValidity({ + name: pinnedAgent.name, + parallelism: "1", + turnTimeoutSeconds: "60", + agentAcpCommand: pinnedAgent.acpCommand, + acpCommand: pinnedAgent.acpCommand, + respondTo: "mentions", + respondToAllowlistLength: 0, + selectedRuntimeId: "claude", + inheritHarness: true, + agentCommand: pinnedAgent.agentCommand, + requiredEnvKeyMissing, + }); + assert.equal( + canSubmit, + false, + "missing credential in the submission snapshot must block Save through the same validity path", + ); +}); + +test("rehost_submit_persistsToggleAsCommandClear_andOmitsHarnessOverride", () => { + // Submit half of the toggle seam: inheriting with a persisted override must + // send the clear sentinel (""), and harnessOverride derives from the SAME + // agentCommandUpdate (omitted while inheriting — falsy per the component). + const agentCommandUpdate = resolveAgentCommandUpdate({ + inheritHarness: true, + agentCommand: pinnedAgent.agentCommand, + originalAgentCommand: pinnedAgent.agentCommand, + agentCommandOverride: pinnedAgent.agentCommandOverride, + }); + assert.equal( + agentCommandUpdate, + "", + "inherit with a persisted pin must persist the clear sentinel", + ); + const harnessOverride = + agentCommandUpdate != null ? !true /* inheritHarness */ : undefined; + assert.equal( + harnessOverride, + false, + "harnessOverride must derive from the shared agentCommandUpdate, signalling the cleared pin", + ); + + // And the provider tri-state must classify the PROSPECTIVE runtime — the + // same id the gate used — so submit persists what the gate validated. + const { prospectiveRuntimeId, inheritedSubmission } = + inheritTransitionState(); + const capability = resolveRuntimeProviderCapability( + prospectiveRuntimeId, + runtimeSupportsLlmProviderSelection(prospectiveRuntimeId), + ); + assert.equal(capability, "capable"); + const providerUpdate = + capability === "capable" + ? inheritedSubmission.provider !== (pinnedAgent.provider ?? null) + ? inheritedSubmission.provider + : undefined + : undefined; + assert.equal( + providerUpdate, + "anthropic", + "submit must persist the gate-validated submission provider for the prospective runtime", + ); +}); + +test("rehost_steadyStateInherit_localEditsStayAuthoritative", () => { + // Wipe-on-poll companion: an agent ALREADY inheriting at open (no pin) with + // deliberate local edits — the submission snapshot must pass them through + // untouched, not resurrect persona values (the pre-move contract). + const submission = resolveInheritedRuntimeSubmission({ + inheritHarness: true, + agentWasHarnessPinned: false, // steady state, not a transition + provider: "databricks", // deliberate local re-point + personaProvider: persona.provider, + model: "", + personaModel: persona.model, + envVars: { DATABRICKS_TOKEN: "tok" }, + personaEnvVars: persona.envVars, + }); + assert.equal(submission.provider, "databricks"); + assert.equal( + submission.model, + null, + "empty local model in steady state stays empty (runtime default), never backfilled from the persona", + ); + assert.deepEqual(submission.envVars, { DATABRICKS_TOKEN: "tok" }); +}); diff --git a/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs b/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs index 39c4cfe09a..efb84daf59 100644 --- a/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs +++ b/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs @@ -1186,7 +1186,7 @@ test("editAgent_findingE_capableBuzzAgentLoadedCatalog_preservedOnNoOpSave", () // to id-based matching when command-path matching misses — same id-fallback used // by effectiveRuntimeIdForSubmit. -// Helper mirrors the fixed seeding logic from EditAgentDialog.tsx open-effect / +// Helper mirrors the fixed seeding logic from AgentInstanceEditDialog.tsx open-effect / // catalog-arrival effect. function deriveSelectedRuntimeId(agentCommand, runtimes) { const matched = @@ -1390,7 +1390,7 @@ test("requiredCredentialEnvKeys: custom/unknown runtime → empty", () => { // ── Block-save gate: hasMissingRequiredEnvKey logic ──────────────────────── // -// The EditAgentDialog computes: +// The AgentInstanceEditDialog computes: // requiredEnvKeyMissing = hasMissingRequiredEnvKey(requiredEnvKeys, envVars) // and folds it into canSubmit (via computeEditAgentFormValidity). These tests // exercise the exported predicate directly. @@ -1472,7 +1472,7 @@ test("blockSave_buzzAgentDatabricksHostProvided_allowed", () => { // ── Block-save gate: isMissingRequiredDropdownField ──────────────────────── // -// The EditAgentDialog also gates on modelRequired / providerRequired. +// The AgentInstanceEditDialog also gates on modelRequired / providerRequired. // These tests guard the isMissingRequiredDropdownField predicate used for both. test("blockSave_missingRequiredModel_blocked", () => { diff --git a/desktop/src/features/agents/ui/personaProviderModelFields.tsx b/desktop/src/features/agents/ui/personaProviderModelFields.tsx index dd2fd5af59..9753d9490c 100644 --- a/desktop/src/features/agents/ui/personaProviderModelFields.tsx +++ b/desktop/src/features/agents/ui/personaProviderModelFields.tsx @@ -1,7 +1,7 @@ /** * Shared provider and model field components for agent dialogs. * - * Both CreateAgentDialog (local mode) and EditAgentDialog import these + * Both CreateAgentDialog (local mode) and AgentInstanceEditDialog import these * instead of duplicating the picker logic. */ import type * as React from "react"; diff --git a/desktop/src/features/profile/ui/UserProfilePanel.tsx b/desktop/src/features/profile/ui/UserProfilePanel.tsx index 24eb799bf1..9b078315c7 100644 --- a/desktop/src/features/profile/ui/UserProfilePanel.tsx +++ b/desktop/src/features/profile/ui/UserProfilePanel.tsx @@ -34,7 +34,7 @@ import { stopManagedAgentWithRules, } from "@/features/agents/lib/managedAgentControlActions"; import { describeLogFile } from "@/features/agents/ui/agentUi"; -import { EditAgentDialog } from "@/features/agents/ui/EditAgentDialog"; +import { AgentDialog } from "@/features/agents/ui/AgentDialog"; import { consumePendingOpenEditAgent, subscribeOpenEditAgent, @@ -918,8 +918,9 @@ export function UserProfilePanel({ ); const editAgentDialog = canEditAgent && managedAgent ? ( -