diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx index 75b8530d16..acbbe28c70 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx +++ b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx @@ -20,6 +20,7 @@ import { useUpdateManagedAgentMutation, useUpdatePersonaMutation, } from "@/features/agents/hooks"; +import { useTeamsQuery } from "@/features/agents/teamHooks"; import { useUpdatePersonaAndPublishMutation } from "@/features/agents/lib/usePersonaCatalogRelay"; import type { ManagedAgent, RespondToMode } from "@/shared/api/types"; import type { EditAgentFocusTarget } from "@/features/agents/openEditAgentEvent"; @@ -40,21 +41,10 @@ import { useAgentEditMergedSubmit } from "./useAgentEditMergedSubmit"; import { AgentEditMergedDSection } from "./AgentEditMergedDialogDSection"; import { AgentEditMergedInstanceSection } from "./AgentEditMergedDialogInstanceSection"; import { - runtimeDropdownAction, - usePendingHarnessSelection, -} from "./addCustomHarness"; -import { - AUTO_PROVIDER_DROPDOWN_VALUE, getDefaultPersonaRuntime, isMissingRequiredDropdownField, - runtimeSupportsLlmProviderSelection, } from "./agentConfigOptions"; -import { - selectionOnModelDropdownChange, - selectionOnProviderDropdownChange, - selectionOnRuntimeChange, - type RuntimeModelProviderSelection, -} from "./runtimeModelProviderSelection"; +import { useAgentEditRuntimeHandlers } from "./useAgentEditRuntimeHandlers"; import { AgentCreationPreview } from "./AgentCreationPreview"; import { AddCustomHarnessDialog } from "./AddCustomHarnessDialog"; import { useAgentAccessOwnerOnlyQuery } from "@/features/agents/useAgentAccessOwnerOnly"; @@ -156,10 +146,18 @@ export function AgentEditMergedDialog({ const { data: bakedEnvKeys } = useBakedBuildEnvKeysQuery({ enabled: open }); const { data: agentAccessOwnerOnly } = useAgentAccessOwnerOnlyQuery({ - enabled: open && showInst, + // Query in both instance and definition-only contexts: rows 9–10 say + // access/parallelism are D-owned in definition-only (shown on the form). + enabled: open && (showInst || ctx.kind === "definition-only"), }); const personasQuery = usePersonasQuery(); + const teamsQuery = useTeamsQuery(); + const sourceTeamName = React.useMemo(() => { + if (!def?.sourceTeam) return null; + const team = teamsQuery.data?.find((t) => t.id === def.sourceTeam); + return team?.name ?? def.sourceTeam; + }, [def?.sourceTeam, teamsQuery.data]); const linkedPersona = React.useMemo( () => inst?.personaId @@ -177,20 +175,43 @@ export function AgentEditMergedDialog({ // Definition runtime (D-field) — separate from instance harness pin (I-field) const [definitionRuntimeId, setDefinitionRuntimeId] = React.useState("custom"); + // Track whether the D-runtime was auto-seeded (not a deliberate user choice). + // Prevents a no-op save from persisting the app default as the definition runtime. + const autoSeededDefinitionRuntimeRef = React.useRef(null); // I-fields — harness pin state (I-section only, independent of D-runtime) const [selectedRuntimeId, setSelectedRuntimeId] = React.useState("custom"); - // LLM model/provider (shared between D and I sections; which store they target is context-dependent) - const [model, setModel] = React.useState(""); - const [provider, setProvider] = React.useState(""); - const [isCustomModelEditing, setIsCustomModelEditing] = React.useState(false); - const [isCustomProviderEditing, setIsCustomProviderEditing] = + // D-section LLM model/provider — for the definition (D-owned) + const [dModel, setDModel] = React.useState(""); + const [dProvider, setDProvider] = React.useState(""); + const [dIsCustomModelEditing, setDIsCustomModelEditing] = + React.useState(false); + const [dIsCustomProviderEditing, setDIsCustomProviderEditing] = + React.useState(false); + // I-section LLM model/provider — for unlinked agents (I-owned, shown only when !showDef) + const [iModel, setIModel] = React.useState(""); + const [iProvider, setIProvider] = React.useState(""); + const [iIsCustomModelEditing, setIIsCustomModelEditing] = + React.useState(false); + const [iIsCustomProviderEditing, setIIsCustomProviderEditing] = React.useState(false); const runtimeTouched = React.useRef(false); + // Convenience: the active model/provider — D-state when a definition is present, + // I-state when instance-only (no definition). Used for the D-section and for + // useAgentEditRuntimeState, which is always driven by the relevant layer. + const model = showDef ? dModel : iModel; + const provider = showDef ? dProvider : iProvider; + const isCustomModelEditing = showDef + ? dIsCustomModelEditing + : iIsCustomModelEditing; + const isCustomProviderEditing = showDef + ? dIsCustomProviderEditing + : iIsCustomProviderEditing; + // I-fields const [instanceName, setInstanceName] = React.useState(""); - const [respondTo, setRespondTo] = React.useState( - inst?.respondTo ?? "anyone", + const [respondTo, setRespondTo] = React.useState( + inst?.respondTo ?? null, ); const [respondToAllowlist, setRespondToAllowlist] = React.useState( inst?.respondToAllowlist ?? [], @@ -247,13 +268,23 @@ export function AgentEditMergedDialog({ setAvatarUrl(seed.avatarUrl); setSystemPrompt(seed.systemPrompt); setNamePoolText(formatPersonaNamePoolText(seed.namePool ?? [])); - setModel(seed.model ?? ""); - setProvider(seed.provider ?? ""); - setIsCustomModelEditing(false); - setIsCustomProviderEditing(false); - setRespondTo((seed.respondTo ?? "anyone") as RespondToMode); + // D-section model/provider — seed from definition (or instance for I-only) + setDModel(seed.model ?? ""); + setDProvider(seed.provider ?? ""); + setDIsCustomModelEditing(false); + setDIsCustomProviderEditing(false); + // I-section model/provider — only relevant for instance-only context + setIModel(seed.model ?? ""); + setIProvider(seed.provider ?? ""); + setIIsCustomModelEditing(false); + setIIsCustomProviderEditing(false); + // Rows 9–10: seed respondTo/allowlist/parallelism from the canonical seed. + // Preserve null from the seed — do NOT coerce null→"anyone" here; the + // seed already handles the instance-vs-definition context. Coercing here + // would cause phantom D-writes when an unset definition behavior opens. + setRespondTo((seed.respondTo as RespondToMode | null) ?? null); setRespondToAllowlist(seed.respondToAllowlist); - setParallelism(String(seed.parallelism ?? 1)); + setParallelism(seed.parallelism != null ? String(seed.parallelism) : ""); setEnvVars(seed.envVars); setInstanceEnvVars(seed.instanceEnvVars ?? {}); setInstanceName(seed.instanceName ?? ""); @@ -262,6 +293,7 @@ export function AgentEditMergedDialog({ setShowAdvancedFields(false); setIsAvatarUploadPending(false); runtimeTouched.current = false; + autoSeededDefinitionRuntimeRef.current = null; // Seed definition runtime (D-field) from definition record. const defRuntimeId = def?.runtime?.trim() ?? ""; @@ -292,16 +324,22 @@ export function AgentEditMergedDialog({ setDisplayName(initialValueOverrides.displayName); if (initialValueOverrides.systemPrompt !== undefined) setSystemPrompt(initialValueOverrides.systemPrompt); - if (initialValueOverrides.model !== undefined) - setModel(initialValueOverrides.model ?? ""); - if (initialValueOverrides.provider !== undefined) - setProvider(initialValueOverrides.provider ?? ""); + if (initialValueOverrides.model !== undefined) { + setDModel(initialValueOverrides.model ?? ""); + setIModel(initialValueOverrides.model ?? ""); + } + if (initialValueOverrides.provider !== undefined) { + setDProvider(initialValueOverrides.provider ?? ""); + setIProvider(initialValueOverrides.provider ?? ""); + } if (initialValueOverrides.runtime !== undefined) setDefinitionRuntimeId(initialValueOverrides.runtime ?? "custom"); - // R6: carry agent-requested respondTo into definition-only form (IMPORTANT-4) + // R6: carry agent-requested respondTo into definition-only form. + // This makes the requested change VISIBLE in the review dialog — + // the owner sees the requested value and can accept (Save) or change it. if (initialValueOverrides.respondTo !== undefined) setRespondTo( - (initialValueOverrides.respondTo ?? "anyone") as RespondToMode, + (initialValueOverrides.respondTo as RespondToMode | null) ?? null, ); } setDFieldsDirty(false); @@ -322,6 +360,10 @@ export function AgentEditMergedDialog({ const defaultRuntime = getDefaultPersonaRuntime(runtimes); if (!defaultRuntime) return; setDefinitionRuntimeId(defaultRuntime.id); + // Record that this runtime was auto-seeded, not chosen by the user. + // useAgentEditMergedSubmit uses this to prevent persisting the auto-seed + // as a definition runtime change when no other D-fields were modified. + autoSeededDefinitionRuntimeRef.current = defaultRuntime.id; // In definition-only context, selectedRuntimeId is the same conceptual field // as definitionRuntimeId (there is no separate I-harness pin). Sync it so // that model discovery runs: useAgentEditRuntimeState derives `selectedRuntime` @@ -390,11 +432,12 @@ export function AgentEditMergedDialog({ instanceEnvVars, inheritHarness, inst, + def, linkedPersona, bakedEnvKeys, originalAgentCommand, - setModel, - setIsCustomModelEditing, + setModel: setDModel, + setIsCustomModelEditing: setDIsCustomModelEditing, }); // ── Submit validity ──────────────────────────────────────────────────────── @@ -432,8 +475,8 @@ export function AgentEditMergedDialog({ avatarUrl, systemPrompt, namePoolText, - model, - provider, + model: showDef ? dModel : iModel, + provider: showDef ? dProvider : iProvider, respondTo, respondToAllowlist, parallelism, @@ -444,6 +487,7 @@ export function AgentEditMergedDialog({ autoRestartOnConfigChange, startOnAppLaunch: showInst ? startOnAppLaunch : undefined, definitionRuntimeId, + autoSeededDefinitionRuntimeRef, selectedRuntimeId, inheritHarness, agentCommand, @@ -476,121 +520,49 @@ export function AgentEditMergedDialog({ agentCommand.trim().length > 0) : true); - // ── selection helpers ────────────────────────────────────────────────────── - const selection: RuntimeModelProviderSelection = { - provider, - model, - isCustomProviderEditing, - isCustomModelEditing, - envVars: showInst ? instanceEnvVars : envVars, - }; - - function applySelection(next: RuntimeModelProviderSelection) { - setProvider(next.provider); - setModel(next.model); - setIsCustomProviderEditing(next.isCustomProviderEditing); - setIsCustomModelEditing(next.isCustomModelEditing); - if (showInst) { - setInstanceEnvVars(next.envVars); - } else { - setEnvVars(next.envVars); - } - } - - // D-section runtime change (definition runtime, D-field) - function handleDefinitionRuntimeChange(nextValue: string) { - const action = runtimeDropdownAction(nextValue); - if (action.kind === "add-custom-harness") { - setIsAddHarnessOpen(true); - return; - } - const nextRuntimeId = action.runtimeId; - const previousRuntimeId = definitionRuntimeId; - setDefinitionRuntimeId(nextRuntimeId || "custom"); - applySelection( - selectionOnRuntimeChange(selection, { - previousRuntime: previousRuntimeId, - nextRuntime: nextRuntimeId, - nextRuntimeCanChooseProvider: - runtimeSupportsLlmProviderSelection(nextRuntimeId), - lockedRuntimeReset: "full", - }), - ); - } - - // I-section runtime change (instance harness pin, I-field) - function handleRuntimeDropdownChange(nextValue: string) { - const action = runtimeDropdownAction(nextValue); - if (action.kind === "add-custom-harness") { - setIsAddHarnessOpen(true); - return; - } - const nextRuntimeId = action.runtimeId; - const previousRuntimeId = selectedRuntimeId; - - runtimeTouched.current = true; - const nextRuntime = runtimes.find((r) => r.id === nextRuntimeId); - const resolvedRuntimeId = nextRuntimeId || "custom"; - setSelectedRuntimeId(resolvedRuntimeId); - if (resolvedRuntimeId === "custom" || nextRuntime?.command) { - setInheritHarness(false); - } - if (nextRuntime?.command) { - setAgentCommand(nextRuntime.command); - setAgentArgs(nextRuntime.defaultArgs.join(",")); - } - - applySelection( - selectionOnRuntimeChange(selection, { - previousRuntime: previousRuntimeId, - nextRuntime: nextRuntimeId, - nextRuntimeCanChooseProvider: - runtimeSupportsLlmProviderSelection(nextRuntimeId), - lockedRuntimeReset: "full", - }), - ); - } - - const selectSavedHarness = usePendingHarnessSelection( + // ── Runtime/model/provider handlers (extracted to useAgentEditRuntimeHandlers) ── + const { + handleDefinitionRuntimeChange, + handleRuntimeDropdownChange, + handleProviderDropdownChange, + handleModelDropdownChange, + selectSavedHarness, + } = useAgentEditRuntimeHandlers({ + showDef, + showInst, + dProvider, + dModel, + dIsCustomProviderEditing, + dIsCustomModelEditing, + setDProvider, + setDModel, + setDIsCustomProviderEditing, + setDIsCustomModelEditing, + envVars, + setEnvVars, + definitionRuntimeId, + setDefinitionRuntimeId, + iProvider, + iModel, + iIsCustomProviderEditing, + iIsCustomModelEditing, + setIProvider, + setIModel, + setIIsCustomProviderEditing, + setIIsCustomModelEditing, + instanceEnvVars, + setInstanceEnvVars, + selectedRuntimeId, + setSelectedRuntimeId, + setInheritHarness, + setAgentCommand, + setAgentArgs, + runtimeTouched, + setIsAddHarnessOpen, runtimes, - // In definition-only context the harness dropdown drives `definitionRuntimeId` - // (via handleDefinitionRuntimeChange), not the I-harness pin. Route to the - // correct handler so the saved harness appears in the D-section dropdown. - showDef && !showInst - ? handleDefinitionRuntimeChange - : handleRuntimeDropdownChange, + selectedRuntime, open, - ); - - function handleProviderDropdownChange(nextValue: string) { - const nextProvider = - nextValue === AUTO_PROVIDER_DROPDOWN_VALUE ? "" : nextValue; - if (nextProvider === "relay-mesh" && selectedRuntimeId !== "buzz-agent") { - handleRuntimeDropdownChange("buzz-agent"); - } - const nextSelection = selectionOnProviderDropdownChange(selection, { - runtime: - nextProvider === "relay-mesh" - ? "buzz-agent" - : (selectedRuntime?.id ?? selectedRuntimeId), - nextValue, - clearModelWhenApiKeyMissing: !showInst, - }); - applySelection({ - ...nextSelection, - model: nextProvider === "relay-mesh" ? "auto" : nextSelection.model, - }); - } - - function handleModelDropdownChange(nextValue: string) { - applySelection( - selectionOnModelDropdownChange(selection, { - nextValue, - clearKnownModelOnCustomEntry: !showInst, - isModelCustom: false, - }), - ); - } + }); // ── Focus target (R2) ────────────────────────────────────────────────────── const normalizedFieldFocusFiredRef = React.useRef(false); @@ -740,8 +712,8 @@ export function AgentEditMergedDialog({ className="text-sm text-muted-foreground" data-testid="team-managed-notice" > - Managed by team{def?.sourceTeam ? ` ${def.sourceTeam}` : ""}. - Its configuration cannot be edited here. + Managed by team{sourceTeamName ? ` ${sourceTeamName}` : ""}. Its + configuration cannot be edited here.

) : null} @@ -755,8 +727,10 @@ export function AgentEditMergedDialog({

) : null} - {/* Identity-archived flair (14b) — fields remain editable; Save does not unarchive */} - {isIdentityArchived && showDef ? ( + {/* Identity-archived flair (14b) — per agent pubkey, shown for any + archived agent regardless of definition linkage. Fields remain + editable; Save does not unarchive. */} + {isIdentityArchived ? (

{ @@ -806,8 +780,25 @@ export function AgentEditMergedDialog({ modelDiscoveryLoading={modelDiscoveryLoading} showCustomModelInput={showCustomModelInput} model={model} - onModelTextChange={dChange(setModel)} + onModelTextChange={dChange(setDModel)} modelStatusMessage={modelStatusMessage} + showInstancePresent={showInst} + respondTo={respondTo} + respondToAllowlist={respondToAllowlist} + onRespondToChange={(v) => { + setRespondTo(v); + setDFieldsDirty(true); + }} + onAllowlistChange={(v) => { + setRespondToAllowlist(v); + setDFieldsDirty(true); + }} + agentAccessOwnerOnly={agentAccessOwnerOnly} + parallelism={parallelism} + onParallelismChange={(v) => { + setParallelism(v); + setDFieldsDirty(true); + }} /> ) : null} @@ -821,7 +812,7 @@ export function AgentEditMergedDialog({ runtimeCatalogStatus={runtimeCatalogStatus} instanceName={instanceName} onInstanceNameChange={setInstanceName} - respondTo={respondTo} + respondTo={respondTo ?? "anyone"} respondToAllowlist={respondToAllowlist} onRespondToChange={setRespondTo} onAllowlistChange={setRespondToAllowlist} @@ -840,7 +831,7 @@ export function AgentEditMergedDialog({ onProviderChange={handleProviderDropdownChange} isCustomProviderEditing={isCustomProviderEditing} provider={provider} - onProviderTextChange={setProvider} + onProviderTextChange={setIProvider} providerRequired={providerRequired} topLevelSecretEnvVar={topLevelSecretEnvVar} apiKeyValue={apiKeyValue} @@ -857,7 +848,7 @@ export function AgentEditMergedDialog({ modelDiscoveryLoading={modelDiscoveryLoading} showCustomModelInput={showCustomModelInput} model={model} - onModelTextChange={setModel} + onModelTextChange={setIModel} modelStatusMessage={modelStatusMessage} modelRequired={modelRequired} inheritedSubmission={inheritedSubmission} diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx index 0719d41398..943be52af9 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx +++ b/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx @@ -4,6 +4,9 @@ * Renders agent name, system prompt, name pool, D-env vars, and (for all definition * contexts) runtime, LLM provider, and model. Team-managed fields render read-only. * + * When showInstancePresent is false (definition-only context), also renders the + * D-owned access/allowlist and parallelism controls (rows 9–10, Artifact 4). + * * Extracted from AgentEditMergedDialog to satisfy the desktop file-size gate. */ @@ -14,6 +17,7 @@ import { cn } from "@/shared/lib/cn"; import { Input } from "@/shared/ui/input"; import { Textarea } from "@/shared/ui/textarea"; import { Button } from "@/shared/ui/button"; +import type { RespondToMode } from "@/shared/api/types"; import { PERSONA_FIELD_CONTROL_CLASS, @@ -25,6 +29,7 @@ import { AgentHarnessField } from "./AgentHarnessField"; import { PersonaDropdownField } from "./PersonaDropdownField"; import { PersonaModelCombobox } from "./PersonaModelCombobox"; import { EnvVarsEditor, type EnvVarsValue } from "./EnvVarsEditor"; +import { OwnerOnlyAccessField } from "./OwnerOnlyAccessField"; const advancedFieldsTransition = { duration: 0.18, ease: "easeInOut" } as const; @@ -69,6 +74,21 @@ export type AgentEditMergedDSectionProps = { model: string; onModelTextChange: (value: string) => void; modelStatusMessage: string | null; + /** + * When false (definition-only context, no instance), renders the D-owned + * access/allowlist and parallelism controls (rows 9–10, Artifact 4). + * When true (instance is present), access/parallelism are I-owned and + * rendered in AgentEditMergedDialogInstanceSection. + */ + showInstancePresent: boolean; + // D-owned access/parallelism (shown when !showInstancePresent) + respondTo: RespondToMode | null; + respondToAllowlist: string[]; + onRespondToChange: (value: RespondToMode) => void; + onAllowlistChange: (value: string[]) => void; + agentAccessOwnerOnly: boolean | undefined; + parallelism: string; + onParallelismChange: (value: string) => void; }; // ── Component ───────────────────────────────────────────────────────────────── @@ -104,6 +124,14 @@ export function AgentEditMergedDSection({ model, onModelTextChange, modelStatusMessage, + showInstancePresent, + respondTo, + respondToAllowlist, + onRespondToChange, + onAllowlistChange, + agentAccessOwnerOnly, + parallelism, + onParallelismChange, }: AgentEditMergedDSectionProps) { const [showAdvanced, setShowAdvanced] = React.useState(false); @@ -258,6 +286,46 @@ export function AgentEditMergedDSection({ ) : null} + {/* D-owned access/parallelism (rows 9–10, Artifact 4) — rendered only in + definition-only context (no instance). When an instance is present, + access/parallelism are I-owned and rendered in the I-section. */} + {!showInstancePresent ? ( + <> + +

+ +
+ onParallelismChange(e.target.value)} + placeholder="Default (1)" + type="number" + value={parallelism} + /> +
+
+ + ) : null} + {/* Advanced section toggle (name pool + D-env vars) */}
- {/* Respond-to (D-field when linked; I-field when unlinked) */} + {/* Respond-to / access (I-owned field — editable even when definition is team-managed) */} { "instance write must NOT be attempted when D-write did not persist", ); }); + +// ── R6 route-level behavioral tests ────────────────────────────────────────── +// +// These tests exercise the full chain that R6 (owner-review dialog) exercises +// in production: +// 1. reviewOverrides.respondTo arrives from useAgentManagement +// 2. Dialog seed effect applies it as initialValueOverrides.respondTo +// 3. emitAgentFormDiff routes the change to personaInput.behavior.respondTo +// 4. runAgentSaveCoordinator calls updatePersona with the correct behavior +// 5. onValidate is checked (permission gate) +// 6. Settlement confirms persistence +// +// This is a coordinator-level behavioral test: it calls emitAgentFormDiff to +// produce inputs and then passes them through runAgentSaveCoordinator, proving +// the full pipeline from override → emission → coordinator → save. + +test("test_r6_respondTo_override_visible_and_delivered_through_coordinator", async () => { + // Scenario: owner-review dialog for a definition that currently has no + // respondTo (null = owner-only by backend default). Agent requested + // respondTo: "owner-only" explicitly. The dialog pre-fills it via + // initialValueOverrides.respondTo = "owner-only". + + const definition = makeDefinition({ respondTo: null }); + const ctx = { kind: "definition-only", definition }; + + // Step 1: seed form model (mirrors AgentEditMergedDialog's seed effect). + const seed = seedAgentFormModel(ctx); + assert.equal( + seed.respondTo, + null, + "definition-only seed preserves null respondTo", + ); + + // Step 2: apply initialValueOverrides.respondTo (mirrors the dialog's + // useEffect block that sets respondTo from initialValueOverrides). + const formState = { ...seed, respondTo: "owner-only" }; + + // Step 3: emit the diff — proves the override is visible in the form state + // and routes to personaInput (D-owned in definition-only context). + const { personaInput, agentInput } = emitAgentFormDiff(seed, formState, ctx); + + assert.ok( + personaInput, + "R6 respondTo override must produce a personaInput diff", + ); + assert.equal( + personaInput.behavior?.respondTo, + "owner-only", + "personaInput.behavior.respondTo must carry the agent-requested value", + ); + assert.equal(agentInput, null, "definition-only: no agentInput"); + + // Step 4+5: run coordinator to verify coordinator receives and + // persists a well-formed personaInput from the emission pipeline. + // (onValidate is called by useAgentEditMergedSubmit before the coordinator; + // we verify here that the coordinator delivers the respondTo to updatePersona.) + let capturedPersonaInput = null; + let doneCalled = false; + + const updatedDefinition = makeDefinition({ + respondTo: "owner-only", + respondToAllowlist: [], + parallelism: null, + }); + + const result = await runAgentSaveCoordinator({ + ctx, + personaInput, + agentInput: null, + policySets: [], + updatePersona: async (input) => { + capturedPersonaInput = input; + }, + updatePersonaAndPublish: async () => ({ publicationStatus: "published" }), + updateManagedAgent: async () => { + throw new Error("no instance in definition-only context"); + }, + setAutoRestart: async () => {}, + setStartOnAppLaunch: async () => {}, + refetchStores: async () => ({ + persona: updatedDefinition, + agent: null, + }), + onDone: () => { + doneCalled = true; + }, + }); + + assert.equal(result, true, "coordinator must succeed with approved save"); + assert.ok(capturedPersonaInput, "updatePersona must be called"); + assert.equal( + capturedPersonaInput.behavior?.respondTo, + "owner-only", + "updatePersona must receive the agent-requested respondTo", + ); + assert.equal(doneCalled, true, "onDone must be called on success"); +}); + +test("test_r6_respondTo_override_settlement_fails_if_persona_not_updated", async () => { + // Negative case: coordinator emits the correct personaInput but the + // re-fetched persona still has the old value → settlement failure → onDone NOT called. + const definition = makeDefinition({ respondTo: null }); + const ctx = { kind: "definition-only", definition }; + const seed = seedAgentFormModel(ctx); + const formState = { ...seed, respondTo: "owner-only" }; + const { personaInput } = emitAgentFormDiff(seed, formState, ctx); + + let doneCalled = false; + + // Settlement returns STALE persona (respondTo still null). + const stalePersona = makeDefinition({ respondTo: null }); + + const result = await runAgentSaveCoordinator({ + ctx, + personaInput, + agentInput: null, + policySets: [], + updatePersona: async () => {}, + updatePersonaAndPublish: async () => ({ publicationStatus: "published" }), + updateManagedAgent: async () => { + throw new Error("no instance in definition-only context"); + }, + setAutoRestart: async () => {}, + setStartOnAppLaunch: async () => {}, + refetchStores: async () => ({ + persona: stalePersona, + agent: null, + }), + onDone: () => { + doneCalled = true; + }, + }); + + assert.equal(result, false, "stale observed state must return false"); + assert.equal( + doneCalled, + false, + "onDone must NOT be called when respondTo did not persist", + ); +}); diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.ts b/desktop/src/features/agents/ui/agentSaveCoordinator.ts index 463b1e408f..9f79d47eab 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.ts +++ b/desktop/src/features/agents/ui/agentSaveCoordinator.ts @@ -150,15 +150,15 @@ export async function runAgentSaveCoordinator( firstError = err instanceof Error ? err.message : "Failed to save agent profile."; } - // Settle after definition write — verify the write persisted before advancing. - if (!firstError) { - const { persona: settled } = await refetchStores(); - if ( - settled === null || - !observedStateMatchesPersonaInput(settled, personaInput) - ) { - firstError = "Agent profile did not persist — reopen to retry."; - } + // Settle after definition write — re-fetch regardless of throw so that + // observed persistence (not the command result) decides whether it failed. + const { persona: settled } = await refetchStores(); + if ( + !firstError && + (settled === null || + !observedStateMatchesPersonaInput(settled, personaInput)) + ) { + firstError = "Agent profile did not persist — reopen to retry."; } } @@ -172,15 +172,13 @@ export async function runAgentSaveCoordinator( firstError = err instanceof Error ? err.message : "Failed to save agent settings."; } - // Settle after instance write. - if (!firstError) { - const { agent: settled } = await refetchStores(); - if ( - settled === null || - !observedStateMatchesAgentInput(settled, agentInput) - ) { - firstError = "Agent settings did not persist — reopen to retry."; - } + // Settle after instance write — re-fetch regardless of throw. + const { agent: settled } = await refetchStores(); + if ( + !firstError && + (settled === null || !observedStateMatchesAgentInput(settled, agentInput)) + ) { + firstError = "Agent settings did not persist — reopen to retry."; } } @@ -345,11 +343,12 @@ function observedStateMatchesPersonaInput( (observed.avatarUrl ?? "") !== (submitted.avatarUrl ?? "") ) return false; - if ( - submitted.runtime !== undefined && - (observed.runtime ?? null) !== (submitted.runtime ?? null) - ) - return false; + // Runtime: UpdatePersonaRequest is a full write — omitted/undefined runtime + // means "clear this field to null". Compare against observed null when + // submitted.runtime is undefined (clear semantics). If submitted.runtime is + // a non-empty string, compare against that value. + const submittedRuntime = submitted.runtime ?? null; + if ((observed.runtime ?? null) !== submittedRuntime) return false; if ((observed.model ?? null) !== (submitted.model ?? null)) return false; if ((observed.provider ?? null) !== (submitted.provider ?? null)) return false; @@ -433,11 +432,17 @@ function observedStateMatchesAgentInput( return false; } // Harness-pin fields (Artifact 3 / Thufir pass-2 CRITICAL-3) - if ( - submitted.agentCommand !== undefined && - (submitted.agentCommand ?? "") !== (observed.agentCommand ?? "") - ) { - return false; + if (submitted.agentCommand !== undefined) { + if (submitted.agentCommand === "") { + // "" is the sentinel for "inherit/unpin" — the backend clears + // agentCommandOverride. Settle against agentCommandOverride === null, + // NOT against agentCommand (which carries the resolved effective command). + if ((observed.agentCommandOverride ?? null) !== null) return false; + } else { + // Explicit pin — compare the stored command. + if ((submitted.agentCommand ?? "") !== (observed.agentCommand ?? "")) + return false; + } } if (submitted.harnessOverride !== undefined) { // harnessOverride=true means agentCommand is a pin (agentCommandOverride non-null). diff --git a/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts b/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts index 17d733cb31..14b172d117 100644 --- a/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts +++ b/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts @@ -43,7 +43,7 @@ export type AgentEditSubmitState = { namePoolText: string; model: string; provider: string; - respondTo: string; + respondTo: string | null; respondToAllowlist: string[]; parallelism: string; parsedParallelism: number; @@ -54,6 +54,12 @@ export type AgentEditSubmitState = { startOnAppLaunch: boolean | undefined; /** D-field: definition runtime id (independent of I-harness pin). */ definitionRuntimeId: string; + /** + * Ref tracking the auto-seeded definition runtime ID. When non-null, the + * auto-seed was not a user choice — skip persisting runtime as a D-change + * if no other D-field was actually modified. + */ + autoSeededDefinitionRuntimeRef: React.RefObject; /** I-field: harness pin runtime id (instance only). */ selectedRuntimeId: string; inheritHarness: boolean; @@ -135,11 +141,17 @@ export function useAgentEditMergedSubmit( systemPrompt: s.systemPrompt.trim(), respondTo: s.respondTo as typeof seed.respondTo, respondToAllowlist: s.respondToAllowlist, - // D-field: use definitionRuntimeId (independent of I-harness pin) + // D-field: use definitionRuntimeId (independent of I-harness pin). + // If the runtime was auto-seeded (not a user choice), use the seed's + // original runtime so a no-op save doesn't persist the app default + // as a new definition runtime. runtime: - s.definitionRuntimeId === "custom" - ? undefined - : s.definitionRuntimeId, + s.autoSeededDefinitionRuntimeRef.current !== null && + s.autoSeededDefinitionRuntimeRef.current === s.definitionRuntimeId + ? (seed.runtime ?? undefined) // preserve original (undefined = no runtime) + : s.definitionRuntimeId === "custom" + ? undefined + : s.definitionRuntimeId, model: normalizedModel, provider: normalizedProvider, // D-field env: use the definition env from the form state (not the diff --git a/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.ts b/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.ts new file mode 100644 index 0000000000..f2c8d51948 --- /dev/null +++ b/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.ts @@ -0,0 +1,250 @@ +/** + * useAgentEditRuntimeHandlers — runtime/model/provider selection handlers for + * AgentEditMergedDialog. + * + * Extracted to keep the dialog under the 1000-line file-size gate. Owns the + * D/I selection objects, apply helpers, and the four handler callbacks. + */ + +import type * as React from "react"; + +import { + runtimeDropdownAction, + usePendingHarnessSelection, +} from "./addCustomHarness"; +import { + AUTO_PROVIDER_DROPDOWN_VALUE, + runtimeSupportsLlmProviderSelection, +} from "./agentConfigOptions"; +import { + selectionOnModelDropdownChange, + selectionOnProviderDropdownChange, + selectionOnRuntimeChange, + type RuntimeModelProviderSelection, +} from "./runtimeModelProviderSelection"; +import type { AcpRuntimeCatalogEntry } from "@/shared/api/types"; + +export type RuntimeHandlersInput = { + showDef: boolean; + showInst: boolean; + // D-state + dProvider: string; + dModel: string; + dIsCustomProviderEditing: boolean; + dIsCustomModelEditing: boolean; + setDProvider: (v: string) => void; + setDModel: (v: string) => void; + setDIsCustomProviderEditing: (v: boolean) => void; + setDIsCustomModelEditing: (v: boolean) => void; + envVars: Record; + setEnvVars: (v: Record) => void; + definitionRuntimeId: string; + setDefinitionRuntimeId: (v: string) => void; + // I-state + iProvider: string; + iModel: string; + iIsCustomProviderEditing: boolean; + iIsCustomModelEditing: boolean; + setIProvider: (v: string) => void; + setIModel: (v: string) => void; + setIIsCustomProviderEditing: (v: boolean) => void; + setIIsCustomModelEditing: (v: boolean) => void; + instanceEnvVars: Record; + setInstanceEnvVars: (v: Record) => void; + selectedRuntimeId: string; + setSelectedRuntimeId: (v: string) => void; + setInheritHarness: (v: boolean) => void; + setAgentCommand: (v: string) => void; + setAgentArgs: (v: string) => void; + runtimeTouched: React.RefObject; + setIsAddHarnessOpen: (v: boolean) => void; + runtimes: readonly AcpRuntimeCatalogEntry[]; + selectedRuntime: AcpRuntimeCatalogEntry | undefined; + open: boolean; +}; + +/** + * Returns the four runtime/model/provider change handlers and selectSavedHarness. + * + * D-owned handlers write to D state only (via applyDSelection). + * I-owned handlers write to I state only (via applyISelection). + * In definition-only context (showInst=false), D and I state are mirrored. + */ +export function useAgentEditRuntimeHandlers(input: RuntimeHandlersInput) { + const { + showDef, + showInst, + dProvider, + dModel, + dIsCustomProviderEditing, + dIsCustomModelEditing, + setDProvider, + setDModel, + setDIsCustomProviderEditing, + setDIsCustomModelEditing, + envVars, + setEnvVars, + definitionRuntimeId, + setDefinitionRuntimeId, + iProvider, + iModel, + iIsCustomProviderEditing, + iIsCustomModelEditing, + setIProvider, + setIModel, + setIIsCustomProviderEditing, + setIIsCustomModelEditing, + instanceEnvVars, + setInstanceEnvVars, + selectedRuntimeId, + setSelectedRuntimeId, + setInheritHarness, + setAgentCommand, + setAgentArgs, + runtimeTouched, + setIsAddHarnessOpen, + runtimes, + selectedRuntime, + open, + } = input; + + const dSelection: RuntimeModelProviderSelection = { + provider: dProvider, + model: dModel, + isCustomProviderEditing: dIsCustomProviderEditing, + isCustomModelEditing: dIsCustomModelEditing, + envVars, + }; + + const iSelection: RuntimeModelProviderSelection = { + provider: iProvider, + model: iModel, + isCustomProviderEditing: iIsCustomProviderEditing, + isCustomModelEditing: iIsCustomModelEditing, + envVars: instanceEnvVars, + }; + + /** Writes D model/provider/env. In D-only context, mirrors to I as well. */ + function applyDSelection(next: RuntimeModelProviderSelection) { + setDProvider(next.provider); + setDModel(next.model); + setDIsCustomProviderEditing(next.isCustomProviderEditing); + setDIsCustomModelEditing(next.isCustomModelEditing); + setEnvVars(next.envVars); + if (!showInst) { + setIProvider(next.provider); + setIModel(next.model); + setIIsCustomProviderEditing(next.isCustomProviderEditing); + setIIsCustomModelEditing(next.isCustomModelEditing); + } + } + + /** Writes I model/provider/env only. Never touches D state. */ + function applyISelection(next: RuntimeModelProviderSelection) { + setIProvider(next.provider); + setIModel(next.model); + setIIsCustomProviderEditing(next.isCustomProviderEditing); + setIIsCustomModelEditing(next.isCustomModelEditing); + setInstanceEnvVars(next.envVars); + } + + function handleDefinitionRuntimeChange(nextValue: string) { + const action = runtimeDropdownAction(nextValue); + if (action.kind === "add-custom-harness") { + setIsAddHarnessOpen(true); + return; + } + const nextRuntimeId = action.runtimeId; + const previousRuntimeId = definitionRuntimeId; + setDefinitionRuntimeId(nextRuntimeId || "custom"); + applyDSelection( + selectionOnRuntimeChange(dSelection, { + previousRuntime: previousRuntimeId, + nextRuntime: nextRuntimeId, + nextRuntimeCanChooseProvider: + runtimeSupportsLlmProviderSelection(nextRuntimeId), + lockedRuntimeReset: "full", + }), + ); + } + + function handleRuntimeDropdownChange(nextValue: string) { + const action = runtimeDropdownAction(nextValue); + if (action.kind === "add-custom-harness") { + setIsAddHarnessOpen(true); + return; + } + const nextRuntimeId = action.runtimeId; + const previousRuntimeId = selectedRuntimeId; + runtimeTouched.current = true; + const nextRuntime = runtimes.find((r) => r.id === nextRuntimeId); + const resolvedRuntimeId = nextRuntimeId || "custom"; + setSelectedRuntimeId(resolvedRuntimeId); + if (resolvedRuntimeId === "custom" || nextRuntime?.command) { + setInheritHarness(false); + } + if (nextRuntime?.command) { + setAgentCommand(nextRuntime.command); + setAgentArgs(nextRuntime.defaultArgs.join(",")); + } + applyISelection( + selectionOnRuntimeChange(iSelection, { + previousRuntime: previousRuntimeId, + nextRuntime: nextRuntimeId, + nextRuntimeCanChooseProvider: + runtimeSupportsLlmProviderSelection(nextRuntimeId), + lockedRuntimeReset: "full", + }), + ); + } + + const selectSavedHarness = usePendingHarnessSelection( + runtimes, + showDef && !showInst + ? handleDefinitionRuntimeChange + : handleRuntimeDropdownChange, + open, + ); + + function handleProviderDropdownChange(nextValue: string) { + const nextProvider = + nextValue === AUTO_PROVIDER_DROPDOWN_VALUE ? "" : nextValue; + if (nextProvider === "relay-mesh" && selectedRuntimeId !== "buzz-agent") { + handleRuntimeDropdownChange("buzz-agent"); + } + const sel = showDef ? dSelection : iSelection; + const applyFn = showDef ? applyDSelection : applyISelection; + const nextSelection = selectionOnProviderDropdownChange(sel, { + runtime: + nextProvider === "relay-mesh" + ? "buzz-agent" + : (selectedRuntime?.id ?? selectedRuntimeId), + nextValue, + clearModelWhenApiKeyMissing: !showInst, + }); + applyFn({ + ...nextSelection, + model: nextProvider === "relay-mesh" ? "auto" : nextSelection.model, + }); + } + + function handleModelDropdownChange(nextValue: string) { + const sel = showDef ? dSelection : iSelection; + const applyFn = showDef ? applyDSelection : applyISelection; + applyFn( + selectionOnModelDropdownChange(sel, { + nextValue, + clearKnownModelOnCustomEntry: !showInst, + isModelCustom: false, + }), + ); + } + + return { + handleDefinitionRuntimeChange, + handleRuntimeDropdownChange, + handleProviderDropdownChange, + handleModelDropdownChange, + selectSavedHarness, + }; +} diff --git a/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts b/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts index 72c2ccb42c..070c4d8d0a 100644 --- a/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts +++ b/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts @@ -65,6 +65,7 @@ export type AgentEditRuntimeStateInputs = { instanceEnvVars: Record; inheritHarness: boolean; inst: ManagedAgent | null; + def: AgentPersona | null; linkedPersona: AgentPersona | null; bakedEnvKeys: string[] | undefined; originalAgentCommand: string; @@ -113,6 +114,12 @@ export type AgentEditRuntimeStateResult = { advancedRequiredEnvKeys: readonly string[]; /** True while model discovery is in progress (drives model dropdown disabled state). */ modelDiscoveryLoading: boolean; + /** + * True when the definition has no runtime but has a saved model/provider, + * making the provider/model picker editable without a runtime selection. + * Matches AgentDefinitionDialog's blankRuntimeModelProviderEditable contract. + */ + blankRuntimeModelProviderEditable: boolean; }; // ── Hook ────────────────────────────────────────────────────────────────────── @@ -133,6 +140,7 @@ export function useAgentEditRuntimeState({ instanceEnvVars, inheritHarness, inst, + def, linkedPersona, bakedEnvKeys, originalAgentCommand, @@ -221,7 +229,7 @@ export function useAgentEditRuntimeState({ inst, ]); - const llmProviderFieldVisible = runtimeSupportsLlmProviderSelection( + const llmProviderFieldVisible = React.useMemo(() => { // In linked context (showDef && showInst), the D-section provider/model // picker is driven by the definition's runtime (definitionRuntimeId), not by // the instance's harness pin (prospectiveRuntimeId). Using prospectiveRuntimeId @@ -231,8 +239,37 @@ export function useAgentEditRuntimeState({ // Instance-only context (showInst && !showDef) correctly uses prospectiveRuntimeId // because the instance's harness is the only runtime in scope. // D-only context falls through to definitionRuntimeId via selectedRuntimeId (they equal). - showInst && !showDef ? prospectiveRuntimeId : definitionRuntimeId, - ); + const activeRuntimeId = + showInst && !showDef ? prospectiveRuntimeId : definitionRuntimeId; + + if (runtimeSupportsLlmProviderSelection(activeRuntimeId)) return true; + + // blankRuntimeModelProviderEditable: for an edit context (not create), when the + // definition has no runtime configured but already has a saved model or provider, + // expose the provider/model picker so the user can edit the existing values. + // This matches AgentDefinitionDialog's `initialModelProviderEditableWithoutRuntime` + // contract introduced in commit 87dc4dccba — restoring the capability that was + // accidentally dropped when test-11 was rewritten. + if (showDef && activeRuntimeId === "custom") { + const defRuntime = ( + activeRuntimeId === "custom" ? "" : activeRuntimeId + ).trim(); + const isRuntimeless = defRuntime.length === 0; + const hasSavedModelOrProvider = + (linkedPersona?.model ?? def?.model ?? "").trim().length > 0 || + (linkedPersona?.provider ?? def?.provider ?? "").trim().length > 0; + if (isRuntimeless && hasSavedModelOrProvider) return true; + } + + return false; + }, [ + showInst, + showDef, + prospectiveRuntimeId, + definitionRuntimeId, + linkedPersona, + def, + ]); const prospectiveRuntime = runtimes.find( (r) => r.id === prospectiveRuntimeId, ); @@ -461,6 +498,11 @@ export function useAgentEditRuntimeState({ topLevelSecretEnvVar, advancedRequiredEnvKeys, modelDiscoveryLoading, + blankRuntimeModelProviderEditable: + showDef && + definitionRuntimeId === "custom" && + ((linkedPersona?.model ?? def?.model ?? "").trim().length > 0 || + (linkedPersona?.provider ?? def?.provider ?? "").trim().length > 0), }; } diff --git a/desktop/tests/e2e/edit-agent.spec.ts b/desktop/tests/e2e/edit-agent.spec.ts index 96f03b0732..3882012c20 100644 --- a/desktop/tests/e2e/edit-agent.spec.ts +++ b/desktop/tests/e2e/edit-agent.spec.ts @@ -168,6 +168,53 @@ test.describe("edit agent dialog", () => { await expect(page.getByTestId("agent-respond-to")).toBeVisible(); }); + test("definition-only context renders access control and parallelism (rows 9–10 D-owned)", async ({ + page, + }) => { + // Rows 9–10: in definition-only context (zero-instance definition), access/parallelism + // are D-owned and must be rendered. This test proves Artifact 4 access-field + // completeness for the definition-only route (R5 — definition-only edit from library). + const DEFONLY_PERSONA_ID = "persona-defonly-access-e2e"; + await installMockBridge(page, { + bakedBuildEnv: BAKED_DEFAULTS, + personas: [ + { + id: DEFONLY_PERSONA_ID, + displayName: "Definition Only Agent", + systemPrompt: "No instances here.", + respondTo: "owner-only", + }, + ], + }); + + await page.goto("/"); + await page.getByTestId("open-agents-view").click(); + + // Navigate to the definition-edit route via the definitions library button. + const defButton = page.getByRole("button", { + name: "Definition Only Agent agent profile", + }); + await expect(defButton).toBeVisible({ timeout: 10_000 }); + await defButton.click(); + await expect(page.getByTestId("user-profile-panel")).toBeVisible({ + timeout: 10_000, + }); + await page.getByTestId("user-profile-edit-agent").click(); + + await expect(page.getByTestId("edit-agent-dialog")).toBeVisible({ + timeout: 10_000, + }); + + // Rows 9–10 Artifact 4: access control IS visible in definition-only context. + await expect(page.getByTestId("agent-respond-to")).toBeVisible({ + timeout: 10_000, + }); + // D-owned parallelism field rendered. + await expect(page.locator("#edit-agent-parallelism")).toBeVisible({ + timeout: 10_000, + }); + }); + test("edits the agent name and persists it across a dialog reopen", async ({ page, }) => { diff --git a/desktop/tests/e2e/global-agent-config-screenshots.spec.ts b/desktop/tests/e2e/global-agent-config-screenshots.spec.ts index dbe7aa25b7..1486351599 100644 --- a/desktop/tests/e2e/global-agent-config-screenshots.spec.ts +++ b/desktop/tests/e2e/global-agent-config-screenshots.spec.ts @@ -883,12 +883,11 @@ test.describe("global agent config screenshots", () => { }); // Shot 11: the merged surface's equivalent of the "provider gate" regression pin. - // In the merged surface, a runtime-less definition-with-instance does NOT expose - // the provider picker (llmProviderFieldVisible=false when no runtime supports LLM - // selection). This pins the correct observable behavior: provider picker hidden - // and save enabled. Compared to AgentDefinitionDialog's blankRuntimeModelProviderEditable - // — that dialog's own gate behavior is pinned separately in its own e2e path. - test("11-edit-runtime-less-provider-hidden-save-enabled", async ({ + // A runtime-less definition-with-instance that has a saved model (but no provider) + // exposes the provider picker (blankRuntimeModelProviderEditable — restoring the + // contract from AgentDefinitionDialog commit 87dc4dccba). The global provider + // "anthropic" satisfies the provider gate, so Save is enabled with the picker visible. + test("11-edit-runtime-less-provider-visible-save-enabled", async ({ page, }) => { const PERSONA_ID = "persona-runtime-less-edit-e2e"; @@ -947,9 +946,13 @@ test.describe("global agent config screenshots", () => { /Save changes/, ); - // In the merged surface, runtime-less definition-with-instance: no runtime - // supports LLM selection → provider picker NOT shown → save is NOT blocked. - await expect(page.locator("#edit-agent-llm-provider")).not.toBeVisible(); + // blankRuntimeModelProviderEditable: runtime-less definition with a saved model + // exposes the provider picker so the user can edit the existing model/provider values. + // The global provider "anthropic" satisfies isEditAgentProviderSaveValid, so Save + // is not blocked even though the form provider field is empty. + await expect(page.locator("#edit-agent-llm-provider")).toBeVisible({ + timeout: 10_000, + }); await expect(page.getByTestId("edit-agent-dialog-submit")).toBeEnabled({ timeout: 10_000, }); @@ -958,7 +961,7 @@ test.describe("global agent config screenshots", () => { const dialog = page.getByRole("dialog"); await dialog.screenshot({ - path: `${SHOTS}/11-edit-runtime-less-provider-hidden-save-enabled.png`, + path: `${SHOTS}/11-edit-runtime-less-provider-visible-save-enabled.png`, }); });