From 376a7f8b5ffb13391acf1f6d45940b9bfe90b4e9 Mon Sep 17 00:00:00 2001 From: Duncan Date: Thu, 13 Aug 2026 11:57:59 -0400 Subject: [PATCH] fix(desktop): make merged D-section provider/model, inheritance, and credential gate follow the definition layer The parity restoration exposed four behavioral gaps in the merged agent-edit dialog's definition (D) section, all rooted in the runtime hook multiplexing D- and I-layer selection on `showInst`: - Provider options, model discovery, and model-scope clearing switched on `showInst`, so in linked context they followed the instance harness pin instead of the definition runtime whose picker is rendered. Collapsed to a single `activeRuntimeId`/`activeRuntime` layer choice (definition whenever `showDef`, instance pin only for unlinked instance-only). - The D inherited-env layer reused the persona-inclusive overlay, so clearing a definition tuning value echoed its own just-deleted value as the inherited placeholder. Derive a D-only layer from global/build defaults with no persona env, matching main's AgentDefinitionDialog. - The definition's missing-required-credential signal was computed but dropped, so Save was never gated and the collapsed Advanced badge never showed. Carry it, gate Save whenever `showDef && !defReadOnly`, and render the shared AdvancedRequiredBadge; team-managed definitions never block instance edits. Adds linked-runtime, clear/reopen, save-gate, and seam-interaction regression tests that fail when any restored write callback is disconnected. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../agents/ui/AgentEditMergedDialog.tsx | 6 + .../ui/AgentEditMergedDialogDSection.tsx | 6 + ...gentEditMergedDSectionCorrectness.test.mjs | 542 ++++++++++++++++++ .../agentEditMergedDSectionWriteSeam.test.mjs | 438 ++++++++++++++ .../agents/ui/useAgentEditRuntimeState.ts | 78 ++- 5 files changed, 1042 insertions(+), 28 deletions(-) create mode 100644 desktop/src/features/agents/ui/agentEditMergedDSectionCorrectness.test.mjs create mode 100644 desktop/src/features/agents/ui/agentEditMergedDSectionWriteSeam.test.mjs diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx index 1521102d2a..c59fc46f65 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx +++ b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx @@ -542,6 +542,12 @@ export function AgentEditMergedDialog({ !isAvatarUploadPending && displayName.trim().length > 0 && providerValid && + // D-side credential gate: whenever the definition is editable, its required + // credential keys must be satisfied — independent of the instance gate, so a + // linked instance pin can't unblock a definition that main would reject. + // Team-managed definitions (defReadOnly) never block: the user can't change + // their credentials, so their readiness is not this form's concern. + (!showDef || defReadOnly || !dAdvanced.requiredEnvKeyMissing) && (showInst ? !requiredEnvKeyMissing && (parsedParallelism > 0 || parallelism === "") && diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx index 3bddcfb72e..04740b386b 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx +++ b/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx @@ -31,6 +31,7 @@ import { AgentHarnessField } from "./AgentHarnessField"; import { PersonaDropdownField } from "./PersonaDropdownField"; import { PersonaModelCombobox } from "./PersonaModelCombobox"; import { PersonaProviderApiKeyField } from "./PersonaProviderApiKeyField"; +import { AdvancedRequiredBadge } from "./AdvancedRequiredBadge"; import { EnvVarsEditor, type EnvVarsValue } from "./EnvVarsEditor"; import { OwnerOnlyAccessField } from "./OwnerOnlyAccessField"; import { @@ -455,12 +456,17 @@ export function AgentEditMergedDSection({
{showAdvanced ? ( diff --git a/desktop/src/features/agents/ui/agentEditMergedDSectionCorrectness.test.mjs b/desktop/src/features/agents/ui/agentEditMergedDSectionCorrectness.test.mjs new file mode 100644 index 0000000000..9f5d321313 --- /dev/null +++ b/desktop/src/features/agents/ui/agentEditMergedDSectionCorrectness.test.mjs @@ -0,0 +1,542 @@ +/** + * D-section correctness regressions (Thufir pass-2 findings F1–F3). + * + * The parity/write-seam tests cover the four RESTORED controls. These three + * lock the three behavioral gaps Thufir's control-by-control trace exposed — + * each mounts the REAL dialog and asserts the observable a broken fix would + * change: + * + * F1 — linked D provider/model machinery follows the DEFINITION runtime, not + * the instance harness pin. Observable: the discover_agent_models IPC + * agentCommand. A definition on buzz-agent linked to a goose-pinned + * instance must discover models against buzz-agent. + * F2 — a cleared D tuning value falls back to the global/build default, never + * to the definition's own just-deleted value. Observable: the numeric + * input's "Inherit (…)" placeholder after a clear. + * F3 — a missing required D credential gates Save and marks collapsed + * Advanced. Observables: the Save button disabled state and the + * "Required" badge. + * + * Deleting any one of the three fixes turns its test red (verified by mutation + * in the fix-round report). + */ + +import assert from "node:assert/strict"; +import { after, afterEach, before, test } from "node:test"; +import { JSDOM } from "jsdom"; + +const dom = new JSDOM("", { + url: "http://localhost", +}); + +/** Records every Tauri command the dialog invokes. */ +let invokeCalls = []; +/** QueryClients created per test, cleared in afterEach so no gc timer lingers. */ +const liveClients = []; +/** + * Radix's dismissable-layer / focus-scope schedule global timers that outlive + * the render; jsdom's timers delegate to the global, so track every timer we + * hand out and clear the survivors in teardown or the process hangs after pass. + */ +const pendingTimers = new Set(); +const nativeSetTimeout = globalThis.setTimeout; +const nativeSetInterval = globalThis.setInterval; +const nativeClearTimeout = globalThis.clearTimeout; +const nativeClearInterval = globalThis.clearInterval; + +before(() => { + globalThis.setTimeout = (fn, ms, ...args) => { + const id = nativeSetTimeout( + (...a) => { + pendingTimers.delete(id); + return fn(...a); + }, + ms, + ...args, + ); + pendingTimers.add(id); + return id; + }; + globalThis.setInterval = (fn, ms, ...args) => { + const id = nativeSetInterval(fn, ms, ...args); + pendingTimers.add(id); + return id; + }; + globalThis.clearTimeout = (id) => { + pendingTimers.delete(id); + return nativeClearTimeout(id); + }; + globalThis.clearInterval = (id) => { + pendingTimers.delete(id); + return nativeClearInterval(id); + }; + + for (const key of Object.getOwnPropertyNames(dom.window)) { + if (key in globalThis) continue; + try { + globalThis[key] = dom.window[key]; + } catch { + /* getter-only global — skip */ + } + } + Object.assign(globalThis, { + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + Node: dom.window.Node, + IS_REACT_ACT_ENVIRONMENT: true, + window: dom.window, + getComputedStyle: dom.window.getComputedStyle.bind(dom.window), + }); + for (const key of [ + "Event", + "CustomEvent", + "MouseEvent", + "KeyboardEvent", + "FocusEvent", + "PointerEvent", + "InputEvent", + "UIEvent", + ]) { + if (dom.window[key]) globalThis[key] = dom.window[key]; + } + dom.window.ResizeObserver = class { + observe() {} + unobserve() {} + disconnect() {} + }; + globalThis.ResizeObserver = dom.window.ResizeObserver; + dom.window.HTMLElement.prototype.scrollIntoView = () => {}; + dom.window.HTMLElement.prototype.hasPointerCapture = () => false; + dom.window.HTMLElement.prototype.releasePointerCapture = () => {}; + dom.window.HTMLElement.prototype.setPointerCapture = () => {}; + dom.window.matchMedia = () => ({ + matches: false, + addEventListener() {}, + removeEventListener() {}, + }); +}); + +afterEach(async () => { + const { cleanup } = await import("@testing-library/react"); + cleanup(); + for (const qc of liveClients.splice(0)) { + qc.clear(); + qc.unmount(); + } + for (const id of pendingTimers) nativeClearTimeout(id); + pendingTimers.clear(); + invokeCalls = []; +}); + +after(() => { + globalThis.setTimeout = nativeSetTimeout; + globalThis.setInterval = nativeSetInterval; + globalThis.clearTimeout = nativeClearTimeout; + globalThis.clearInterval = nativeClearInterval; + dom.window.close(); +}); + +/** + * Two available runtimes with DISTINCT discovery commands so the F1 observable + * (which command discovery runs against) unambiguously identifies the layer. + */ +const BUZZ_AGENT_RUNTIME = { + id: "buzz-agent", + label: "Buzz Agent", + avatarUrl: "", + availability: "available", + command: "buzz-agent-cmd", + binaryPath: "buzz-agent-cmd", + defaultArgs: [], + mcpCommand: null, + modelEnvVar: "BUZZ_AGENT_MODEL", + providerEnvVar: "BUZZ_AGENT_PROVIDER", + thinkingEnvVar: "BUZZ_AGENT_THINKING_EFFORT", + maxTokensEnvVar: "BUZZ_AGENT_MAX_OUTPUT_TOKENS", + contextLimitEnvVar: "BUZZ_AGENT_MAX_CONTEXT_TOKENS", + maxRoundsEnvVar: "BUZZ_AGENT_MAX_ROUNDS", + installHint: "", + installInstructionsUrl: "", + canAutoInstall: false, + requiresExternalCli: false, + underlyingCliPath: null, + nodeRequired: false, + authStatus: { status: "not_applicable" }, + loginHint: null, + source: "builtin", + maxParallelism: 2, +}; + +const GOOSE_RUNTIME = { + ...BUZZ_AGENT_RUNTIME, + id: "goose", + label: "Goose", + command: "goose-cmd", + binaryPath: "goose-cmd", + modelEnvVar: null, + providerEnvVar: null, + thinkingEnvVar: null, + maxTokensEnvVar: null, + contextLimitEnvVar: null, + maxRoundsEnvVar: null, + maxParallelism: 4, +}; + +function DEFINITION(overrides = {}) { + return { + id: "def-correctness", + displayName: "Brain", + avatarUrl: null, + systemPrompt: "You are helpful.", + runtime: "buzz-agent", + model: "claude-sonnet-4-5", + provider: "anthropic", + namePool: [], + isBuiltIn: false, + isActive: true, + shared: false, + sourceTeam: null, + catalogSource: null, + envVars: { ANTHROPIC_API_KEY: "sk-seed" }, + respondTo: null, + respondToAllowlist: [], + parallelism: null, + createdAt: "2025-01-01T00:00:00Z", + updatedAt: "2025-01-01T00:00:00Z", + ...overrides, + }; +} + +/** + * A ManagedAgent whose harness is pinned to goose (agentCommandOverride set, + * so inheritHarness is false and prospectiveRuntimeId resolves to goose). Its + * personaId links it to the definition, making the context linked. + */ +function INSTANCE(overrides = {}) { + return { + pubkey: "pk-correctness", + name: "Brain", + personaId: "def-correctness", + runtime: "goose", + teamId: null, + relayUrl: "wss://relay.test", + acpCommand: "", + agentCommand: "goose-cmd", + agentCommandOverride: "goose-cmd", + agentArgs: [], + mcpCommand: "", + turnTimeoutSeconds: 0, + idleTimeoutSeconds: null, + maxTurnDurationSeconds: null, + parallelism: 1, + systemPrompt: "You are helpful.", + avatarUrl: null, + model: null, + modelSource: "definition", + provider: null, + personaOutOfDate: false, + personaOrphaned: false, + needsRestart: false, + restartDiff: [], + envVars: {}, + status: "stopped", + pid: null, + createdAt: "2025-01-01T00:00:00Z", + updatedAt: "2025-01-01T00:00:00Z", + lastStartedAt: null, + lastStoppedAt: null, + lastExitCode: null, + lastError: null, + lastErrorCode: null, + logPath: "", + startOnAppLaunch: false, + autoRestartOnConfigChange: false, + backend: "local", + backendAgentId: null, + respondTo: "anyone", + respondToAllowlist: [], + ...overrides, + }; +} + +/** + * Seed a QueryClient with every catalog the dialog reads on open. `globalEnv` + * seeds the global agent config env (used by F2 to prove the D inherited layer + * is the global default, not the definition's own env). + */ +async function makeSeededClient({ runtimes, definition, globalEnv = {} } = {}) { + const { QueryClient } = await import("@tanstack/react-query"); + const qc = new QueryClient({ + defaultOptions: { + queries: { retry: false, gcTime: 0, staleTime: Number.POSITIVE_INFINITY }, + }, + }); + qc.setQueryData(["acp-runtimes"], runtimes ?? [BUZZ_AGENT_RUNTIME]); + qc.setQueryData(["personas"], definition ? [definition] : []); + qc.setQueryData(["managed-agents"], []); + qc.setQueryData(["teams"], []); + qc.setQueryData(["baked-build-env-keys"], []); + qc.setQueryData(["agent-access-owner-only"], false); + qc.setQueryData(["globalAgentConfig"], { + env_vars: globalEnv, + provider: null, + model: null, + preferred_runtime: null, + }); + qc.setQueryData(["runtime-file-config", "buzz-agent"], { + provider: null, + model: null, + satisfiedEnvKeys: [], + }); + qc.setQueryData(["runtime-file-config", "goose"], { + provider: null, + model: null, + satisfiedEnvKeys: [], + }); + liveClients.push(qc); + return qc; +} + +/** Records every discover_agent_models agentCommand the dialog requests. */ +function installInvoke({ definitionRaw, onDiscover } = {}) { + dom.window.__TAURI_INTERNALS__ = { + invoke: async (cmd, args) => { + invokeCalls.push({ cmd, args }); + if (cmd === "list_personas") return definitionRaw ? [definitionRaw] : []; + if (cmd === "list_managed_agents") return []; + if (cmd === "get_runtime_file_config") { + return { provider: null, model: null, satisfiedEnvKeys: [] }; + } + if (cmd === "discover_agent_models") { + onDiscover?.(args?.input); + return { + agentName: "buzz-agent", + agentVersion: "1.0.0", + models: [], + agentDefaultModel: null, + selectedModel: null, + supportsSwitching: false, + }; + } + return null; + }, + transformCallback: (cb) => cb, + unregisterCallback() {}, + convertFileSrc: (p) => p, + }; +} + +async function renderDialog(qc, ctx) { + const { createElement } = await import("react"); + const { render } = await import("@testing-library/react"); + const { QueryClientProvider } = await import("@tanstack/react-query"); + const { ThemeProvider } = await import("@/shared/theme/ThemeProvider"); + const { AgentEditMergedDialog } = await import("./AgentEditMergedDialog.tsx"); + + return render( + createElement( + QueryClientProvider, + { client: qc }, + createElement( + ThemeProvider, + { defaultTheme: "buzz" }, + createElement(AgentEditMergedDialog, { + open: true, + onOpenChange: () => {}, + ctx, + }), + ), + ), + ); +} + +async function settle(ms = 60) { + const { act } = await import("react"); + await act(async () => { + await new Promise((r) => setTimeout(r, ms)); + }); +} + +async function openAdvanced() { + const { fireEvent, screen } = await import("@testing-library/react"); + const reactAct = (await import("react")).act; + const advanced = screen.getByTestId("persona-advanced-toggle"); + await reactAct(async () => { + fireEvent.click(advanced); + await new Promise((r) => setTimeout(r, 30)); + }); +} + +async function fireChange(el, value) { + const { fireEvent } = await import("@testing-library/react"); + const reactAct = (await import("react")).act; + await reactAct(async () => { + fireEvent.change(el, { target: { value } }); + await new Promise((r) => setTimeout(r, 10)); + }); +} + +// ── F1: linked D provider/model machinery follows the definition runtime ───── + +test("test_linked_dsection_model_discovery_follows_definition_runtime_not_instance_pin", async () => { + // Definition runs buzz-agent; the instance is harness-pinned to goose. The + // D-section provider/model picker is the definition's, so model discovery + // must run against the DEFINITION runtime command (buzz-agent-cmd), never the + // instance pin (goose-cmd). Before F1, the discovery selectedRuntime switched + // on `showInst` and would have discovered against goose. + const definition = DEFINITION(); + const discoverCommands = []; + const qc = await makeSeededClient({ + runtimes: [BUZZ_AGENT_RUNTIME, GOOSE_RUNTIME], + definition, + }); + installInvoke({ + onDiscover: (input) => discoverCommands.push(input?.agentCommand), + }); + await renderDialog(qc, { + kind: "instance-with-definition", + definition, + instance: INSTANCE(), + }); + // anthropic requires an explicit model → discovery is debounced 250ms. + await settle(400); + + assert.ok( + discoverCommands.length > 0, + "model discovery must run for the buzz-agent definition runtime", + ); + assert.ok( + discoverCommands.every((c) => c === "buzz-agent-cmd"), + `discovery must target the definition runtime command; saw ${JSON.stringify(discoverCommands)}`, + ); + assert.equal( + discoverCommands.includes("goose-cmd"), + false, + "discovery must never run against the instance harness pin (goose)", + ); +}); + +// ── F2: a cleared D tuning value inherits the global default, not its own ──── + +test("test_linked_dsection_cleared_tuning_inherits_global_default_not_definition_value", async () => { + // The definition sets BUZZ_AGENT_MAX_OUTPUT_TOKENS=4096; the global default is + // 2048. When the user clears the definition's value, the numeric input's + // inherited placeholder must show the GLOBAL fallback (2048) — not the + // definition's just-deleted 4096. Before F2, the D inherited layer reused the + // persona-inclusive overlay and echoed the definition's own value back. + const definition = DEFINITION({ + envVars: { + ANTHROPIC_API_KEY: "sk-seed", + BUZZ_AGENT_MAX_OUTPUT_TOKENS: "4096", + }, + }); + const qc = await makeSeededClient({ + runtimes: [BUZZ_AGENT_RUNTIME, GOOSE_RUNTIME], + definition, + globalEnv: { BUZZ_AGENT_MAX_OUTPUT_TOKENS: "2048" }, + }); + installInvoke(); + await renderDialog(qc, { + kind: "instance-with-definition", + definition, + instance: INSTANCE(), + }); + await settle(); + await openAdvanced(); + + const { screen } = await import("@testing-library/react"); + const maxTokens = screen.getByTestId("numeric-max-output-tokens-input"); + assert.equal( + maxTokens.value, + "4096", + "the definition's stored value shows in the input before clearing", + ); + + // Clear the value — onTuningEnvVarChange deletes the key, reverting to inherit. + await fireChange(maxTokens, ""); + + assert.equal( + maxTokens.value, + "", + "the input is empty after the clear (value reverts to inherited)", + ); + assert.equal( + maxTokens.getAttribute("placeholder"), + "Inherit (2048)", + "the inherited placeholder must be the GLOBAL default, not the definition's just-deleted 4096", + ); +}); + +// ── F3: a missing required D credential gates Save + marks Advanced ────────── + +test("test_definition_only_missing_required_secret_disables_save", async () => { + // A buzz-agent/anthropic definition with no ANTHROPIC_API_KEY has an unmet + // required credential. Save must be disabled until it is satisfied — main's + // localModeSatisfied gate. Before F3, requiredEnvKeyMissing was dropped from + // the D bundle and Save stayed enabled in definition-only context. + const definition = DEFINITION({ envVars: {} }); + const qc = await makeSeededClient({ definition }); + installInvoke(); + await renderDialog(qc, { kind: "definition-only", definition }); + await settle(); + + const { screen } = await import("@testing-library/react"); + const save = screen.getByTestId("edit-agent-dialog-submit"); + assert.equal( + save.disabled, + true, + "Save must be blocked while the required ANTHROPIC_API_KEY is unset", + ); + + // Typing the key satisfies the gate and re-enables Save. + const apiKey = screen.getByTestId("persona-provider-api-key"); + await fireChange(apiKey, "sk-live-xyz"); + assert.equal( + save.disabled, + false, + "Save re-enables once the required credential is provided", + ); +}); + +test("test_definition_only_missing_nonsecret_required_key_marks_collapsed_advanced", async () => { + // Databricks requires DATABRICKS_HOST (a non-secret required key rendered as a + // generic Advanced row, not an API-key field). When it is missing, the + // collapsed Advanced disclosure must carry the "Required" badge so the user + // sees configuration is incomplete without expanding. Before F3 the badge was + // absent from the merged D-section. + const definition = DEFINITION({ + provider: "databricks", + model: null, + envVars: {}, + }); + const qc = await makeSeededClient({ definition }); + installInvoke(); + await renderDialog(qc, { kind: "definition-only", definition }); + await settle(); + + const { screen } = await import("@testing-library/react"); + // Advanced is collapsed on open; the badge must be visible without expanding. + assert.ok( + screen.getByTestId("persona-advanced-required-badge"), + "collapsed Advanced must show the Required badge for a missing non-secret required key", + ); +}); + +test("test_team_managed_definition_does_not_block_save_on_missing_credential", async () => { + // A team-managed (read-only) definition's credentials are not the user's to + // change, so a missing required key must NOT block Save — F3's defReadOnly + // carve-out. Mounted definition-only with a sourceTeam and no key. + const definition = DEFINITION({ sourceTeam: "team-acme", envVars: {} }); + const qc = await makeSeededClient({ definition }); + installInvoke(); + await renderDialog(qc, { kind: "definition-only", definition }); + await settle(); + + const { screen } = await import("@testing-library/react"); + const save = screen.getByTestId("edit-agent-dialog-submit"); + assert.equal( + save.disabled, + false, + "a team-managed definition must not block Save on a credential the user cannot edit", + ); +}); diff --git a/desktop/src/features/agents/ui/agentEditMergedDSectionWriteSeam.test.mjs b/desktop/src/features/agents/ui/agentEditMergedDSectionWriteSeam.test.mjs new file mode 100644 index 0000000000..9fb7ed8fff --- /dev/null +++ b/desktop/src/features/agents/ui/agentEditMergedDSectionWriteSeam.test.mjs @@ -0,0 +1,438 @@ +/** + * D-section write-seam tests. + * + * The parity mount tests (agentEditMergedDSectionParity.test.mjs) assert the + * restored controls RENDER and read the definition layer; the pure-logic tests + * (agentFormModel.test.mjs) assert a changed `envVars` object routes to + * personaInput. Neither exercises the seam between the two: the restored + * control's own `onChange` → the D-env write → the coordinator's update_persona + * payload. A disconnected callback (onEnvVarChange / onValueChange → no-op) + * passes every one of those tests while silently dropping the edit. + * + * These mount the REAL dialog in a definition-only context, interact with each + * restored control (numeric tuning input, buzz-agent effort select, provider + * API-key input), click Save, and assert the typed value reaches + * update_persona.envVars — and that no instance write (update_managed_agent) + * fires, since definition-only context has no instance overlay. Deleting any + * one of the three production callbacks turns its test red. + */ + +import assert from "node:assert/strict"; +import { after, afterEach, before, test } from "node:test"; +import { JSDOM } from "jsdom"; + +const dom = new JSDOM("", { + url: "http://localhost", +}); + +/** Records every Tauri command the dialog invokes. */ +let invokeCalls = []; +/** QueryClients created per test, cleared in afterEach so no gc timer lingers. */ +const liveClients = []; +/** + * Radix's dismissable-layer / focus-scope schedule global timers that outlive + * the render; jsdom's timers delegate to the global, so track every timer we + * hand out and clear the survivors in teardown or the process hangs after pass. + */ +const pendingTimers = new Set(); +const nativeSetTimeout = globalThis.setTimeout; +const nativeSetInterval = globalThis.setInterval; +const nativeClearTimeout = globalThis.clearTimeout; +const nativeClearInterval = globalThis.clearInterval; + +before(() => { + globalThis.setTimeout = (fn, ms, ...args) => { + const id = nativeSetTimeout( + (...a) => { + pendingTimers.delete(id); + return fn(...a); + }, + ms, + ...args, + ); + pendingTimers.add(id); + return id; + }; + globalThis.setInterval = (fn, ms, ...args) => { + const id = nativeSetInterval(fn, ms, ...args); + pendingTimers.add(id); + return id; + }; + globalThis.clearTimeout = (id) => { + pendingTimers.delete(id); + return nativeClearTimeout(id); + }; + globalThis.clearInterval = (id) => { + pendingTimers.delete(id); + return nativeClearInterval(id); + }; + + for (const key of Object.getOwnPropertyNames(dom.window)) { + if (key in globalThis) continue; + try { + globalThis[key] = dom.window[key]; + } catch { + /* getter-only global — skip */ + } + } + Object.assign(globalThis, { + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + Node: dom.window.Node, + IS_REACT_ACT_ENVIRONMENT: true, + window: dom.window, + getComputedStyle: dom.window.getComputedStyle.bind(dom.window), + }); + for (const key of [ + "Event", + "CustomEvent", + "MouseEvent", + "KeyboardEvent", + "FocusEvent", + "PointerEvent", + "InputEvent", + "UIEvent", + ]) { + if (dom.window[key]) globalThis[key] = dom.window[key]; + } + dom.window.ResizeObserver = class { + observe() {} + unobserve() {} + disconnect() {} + }; + globalThis.ResizeObserver = dom.window.ResizeObserver; + dom.window.HTMLElement.prototype.scrollIntoView = () => {}; + dom.window.HTMLElement.prototype.hasPointerCapture = () => false; + dom.window.HTMLElement.prototype.releasePointerCapture = () => {}; + dom.window.HTMLElement.prototype.setPointerCapture = () => {}; + dom.window.matchMedia = () => ({ + matches: false, + addEventListener() {}, + removeEventListener() {}, + }); +}); + +afterEach(async () => { + const { cleanup } = await import("@testing-library/react"); + cleanup(); + for (const qc of liveClients.splice(0)) { + qc.clear(); + qc.unmount(); + } + for (const id of pendingTimers) nativeClearTimeout(id); + pendingTimers.clear(); + invokeCalls = []; +}); + +after(() => { + globalThis.setTimeout = nativeSetTimeout; + globalThis.setInterval = nativeSetInterval; + globalThis.clearTimeout = nativeClearTimeout; + globalThis.clearInterval = nativeClearInterval; + dom.window.close(); +}); + +const BUZZ_AGENT_RUNTIME = { + id: "buzz-agent", + label: "Buzz Agent", + avatarUrl: "", + availability: "not_installed", + command: "buzz-agent", + binaryPath: "buzz-agent", + defaultArgs: [], + mcpCommand: null, + modelEnvVar: "BUZZ_AGENT_MODEL", + providerEnvVar: "BUZZ_AGENT_PROVIDER", + thinkingEnvVar: "BUZZ_AGENT_THINKING_EFFORT", + maxTokensEnvVar: "BUZZ_AGENT_MAX_OUTPUT_TOKENS", + contextLimitEnvVar: "BUZZ_AGENT_MAX_CONTEXT_TOKENS", + maxRoundsEnvVar: "BUZZ_AGENT_MAX_ROUNDS", + installHint: "", + installInstructionsUrl: "", + canAutoInstall: false, + requiresExternalCli: false, + underlyingCliPath: null, + nodeRequired: false, + authStatus: { status: "not_applicable" }, + loginHint: null, + source: "builtin", + maxParallelism: 2, +}; + +const DEFINITION = { + id: "def-seam", + displayName: "Brain", + avatarUrl: null, + systemPrompt: "You are helpful.", + runtime: "buzz-agent", + model: "claude-sonnet-4-5", + provider: "anthropic", + namePool: [], + isBuiltIn: false, + isActive: true, + shared: false, + sourceTeam: null, + catalogSource: null, + envVars: {}, + respondTo: null, + respondToAllowlist: [], + parallelism: null, + createdAt: "2025-01-01T00:00:00Z", + updatedAt: "2025-01-01T00:00:00Z", +}; + +function DEFINITION_RAW() { + return { + id: DEFINITION.id, + display_name: DEFINITION.displayName, + avatar_url: null, + system_prompt: DEFINITION.systemPrompt, + runtime: "buzz-agent", + model: "claude-sonnet-4-5", + provider: "anthropic", + name_pool: [], + is_builtin: false, + is_active: true, + shared: false, + source_team: null, + env_vars: {}, + respond_to: null, + respond_to_allowlist: [], + parallelism: null, + created_at: DEFINITION.createdAt, + updated_at: DEFINITION.updatedAt, + }; +} + +/** Seed a QueryClient with every catalog the dialog reads on open. */ +async function makeSeededClient() { + const { QueryClient } = await import("@tanstack/react-query"); + const qc = new QueryClient({ + defaultOptions: { + queries: { retry: false, gcTime: 0, staleTime: Number.POSITIVE_INFINITY }, + }, + }); + qc.setQueryData(["acp-runtimes"], [BUZZ_AGENT_RUNTIME]); + qc.setQueryData(["personas"], [DEFINITION]); + qc.setQueryData(["managed-agents"], []); + qc.setQueryData(["teams"], []); + qc.setQueryData(["baked-build-env-keys"], []); + qc.setQueryData(["agent-access-owner-only"], false); + qc.setQueryData(["globalAgentConfig"], { + env_vars: {}, + provider: null, + model: null, + preferred_runtime: null, + }); + qc.setQueryData(["runtime-file-config", "buzz-agent"], { + provider: null, + model: null, + satisfiedEnvKeys: [], + }); + liveClients.push(qc); + return qc; +} + +/** + * Install a Tauri invoke stub that records the update_persona payload. The + * persisted persona becomes the settlement re-fetch result so the coordinator + * observes the write and closes the dialog. + */ +function installInvoke(onUpdatePersona) { + let currentRaw = DEFINITION_RAW(); + dom.window.__TAURI_INTERNALS__ = { + invoke: async (cmd, args) => { + invokeCalls.push({ cmd, args }); + if (cmd === "list_personas") return [currentRaw]; + if (cmd === "list_managed_agents") return []; + if (cmd === "get_runtime_file_config") { + return { provider: null, model: null, satisfiedEnvKeys: [] }; + } + if (cmd === "update_persona") { + currentRaw = onUpdatePersona(args?.input) ?? currentRaw; + return currentRaw; + } + return null; + }, + transformCallback: (cb) => cb, + unregisterCallback() {}, + convertFileSrc: (p) => p, + }; +} + +async function renderDialog(qc, ctx) { + const { createElement } = await import("react"); + const { render } = await import("@testing-library/react"); + const { QueryClientProvider } = await import("@tanstack/react-query"); + const { ThemeProvider } = await import("@/shared/theme/ThemeProvider"); + const { AgentEditMergedDialog } = await import("./AgentEditMergedDialog.tsx"); + + const closed = { value: false }; + const result = render( + createElement( + QueryClientProvider, + { client: qc }, + createElement( + ThemeProvider, + { defaultTheme: "buzz" }, + createElement(AgentEditMergedDialog, { + open: true, + onOpenChange: (open) => { + if (!open) closed.value = true; + }, + ctx, + }), + ), + ), + ); + return { ...result, closed }; +} + +async function settle(ms = 40) { + const { act } = await import("react"); + await act(async () => { + await new Promise((r) => setTimeout(r, ms)); + }); +} + +async function openAdvanced() { + const { fireEvent, screen } = await import("@testing-library/react"); + const reactAct = (await import("react")).act; + const advanced = screen.getByRole("button", { name: /Advanced/ }); + await reactAct(async () => { + fireEvent.click(advanced); + await new Promise((r) => setTimeout(r, 30)); + }); +} + +async function fireChange(el, value) { + const { fireEvent } = await import("@testing-library/react"); + const reactAct = (await import("react")).act; + await reactAct(async () => { + fireEvent.change(el, { target: { value } }); + await new Promise((r) => setTimeout(r, 10)); + }); +} + +async function clickSave() { + const { fireEvent, screen } = await import("@testing-library/react"); + const reactAct = (await import("react")).act; + const save = screen.getByTestId("edit-agent-dialog-submit"); + await reactAct(async () => { + fireEvent.click(save); + await new Promise((r) => setTimeout(r, 60)); + }); +} + +const definitionCtx = { kind: "definition-only", definition: DEFINITION }; + +// ── Numeric tuning knob write reaches personaInput.envVars ─────────────────── + +test("test_numeric_tuning_edit_reaches_update_persona_env_and_not_instance", async () => { + const qc = await makeSeededClient(); + let captured = null; + installInvoke((input) => { + captured = input; + return { ...DEFINITION_RAW(), env_vars: input.envVars ?? {} }; + }); + // Seed the required provider secret so the D credential gate does not block + // Save — the subject under test is the numeric-knob write, not readiness. + await renderDialog(qc, { + kind: "definition-only", + definition: { ...DEFINITION, envVars: { ANTHROPIC_API_KEY: "sk-live" } }, + }); + await settle(); + await openAdvanced(); + + const { screen } = await import("@testing-library/react"); + const maxTokens = screen.getByTestId("numeric-max-output-tokens-input"); + await fireChange(maxTokens, "8192"); + await clickSave(); + + assert.ok( + captured, + "a numeric tuning edit must reach update_persona (personaInput emitted)", + ); + assert.equal( + captured.envVars?.BUZZ_AGENT_MAX_OUTPUT_TOKENS, + "8192", + "the numeric value must land in update_persona.envVars (definition layer)", + ); + assert.equal( + invokeCalls.some((c) => c.cmd === "update_managed_agent"), + false, + "definition-only context has no instance overlay — no update_managed_agent", + ); +}); + +// ── buzz-agent effort select write reaches personaInput.envVars ────────────── + +test("test_effort_knob_edit_reaches_update_persona_env_and_not_instance", async () => { + const qc = await makeSeededClient(); + let captured = null; + installInvoke((input) => { + captured = input; + return { ...DEFINITION_RAW(), env_vars: input.envVars ?? {} }; + }); + await renderDialog(qc, { + kind: "definition-only", + definition: { ...DEFINITION, envVars: { ANTHROPIC_API_KEY: "sk-live" } }, + }); + await settle(); + await openAdvanced(); + + const { screen } = await import("@testing-library/react"); + const effort = screen.getByTestId("ba-thinking-effort-select"); + // "high" is a valid effort for claude-sonnet-4-5 (anthropic adaptive table). + await fireChange(effort, "high"); + await clickSave(); + + assert.ok( + captured, + "an effort edit must reach update_persona (personaInput emitted)", + ); + assert.equal( + captured.envVars?.BUZZ_AGENT_THINKING_EFFORT, + "high", + "the effort value must land in update_persona.envVars (definition layer)", + ); + assert.equal( + invokeCalls.some((c) => c.cmd === "update_managed_agent"), + false, + "definition-only context has no instance overlay — no update_managed_agent", + ); +}); + +// ── provider API-key write reaches personaInput.envVars ────────────────────── + +test("test_api_key_edit_reaches_update_persona_env_and_not_instance", async () => { + const qc = await makeSeededClient(); + let captured = null; + installInvoke((input) => { + captured = input; + return { ...DEFINITION_RAW(), env_vars: input.envVars ?? {} }; + }); + await renderDialog(qc, definitionCtx); + await settle(); + + // The API-key field is a top-level D control (not under Advanced). + const { screen } = await import("@testing-library/react"); + const apiKey = screen.getByTestId("persona-provider-api-key"); + await fireChange(apiKey, "sk-live-xyz"); + await clickSave(); + + assert.ok( + captured, + "an API-key edit must reach update_persona (personaInput emitted)", + ); + assert.equal( + captured.envVars?.ANTHROPIC_API_KEY, + "sk-live-xyz", + "the API key must land in update_persona.envVars (definition layer)", + ); + assert.equal( + invokeCalls.some((c) => c.cmd === "update_managed_agent"), + false, + "definition-only context has no instance overlay — no update_managed_agent", + ); +}); diff --git a/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts b/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts index 231da91786..bde97ded8e 100644 --- a/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts +++ b/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts @@ -42,7 +42,10 @@ import { relayMeshModelPickerState, } from "./relayMeshModelPicker"; import { resolveModelFieldStatusMessage } from "./agentConfigControls"; -import { resolveInheritedRuntimeSubmission } from "./personaRuntimeModel"; +import { + resolveInheritedRuntimeSubmission, + hasMissingRequiredEnvKey, +} from "./personaRuntimeModel"; import { useRuntimeFileConfigQuery } from "../hooks"; import type { AcpRuntimeCatalogEntry, ManagedAgent } from "@/shared/api/types"; import type { AgentPersona } from "@/shared/api/types"; @@ -83,6 +86,18 @@ export type DSectionAdvancedState = { apiKeyIsInherited: boolean; apiKeyInheritedLabel: string; apiKeyIsRequired: boolean; + /** + * Any required credential key (secret or non-secret) is still unset for the + * definition runtime/provider. Gates Save whenever the definition is editable, + * mirroring main's `localModeSatisfied` credential arm. + */ + requiredEnvKeyMissing: boolean; + /** + * A non-secret required env key (rendered as a generic Advanced row, not the + * API-key field) is still missing. Drives the collapsed-Advanced "Required" + * badge, matching main's `missingEnvKeys ∩ advancedRequiredEnvKeys`. + */ + advancedRequiredEnvKeyMissing: boolean; }; export type AgentEditRuntimeStateInputs = { @@ -271,19 +286,17 @@ export function useAgentEditRuntimeState({ inst, ]); - 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 - // here would make the D-section picker visible whenever the instance's - // agentCommand matches a provider-supporting runtime, even if the definition - // has no runtime configured — a phantom provider field exposure. - // 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). - const activeRuntimeId = - showInst && !showDef ? prospectiveRuntimeId : definitionRuntimeId; + // The active provider/model layer, chosen ONCE for every provider/model + // derivation below (visibility, provider options, model discovery, scope + // clearing): the DEFINITION runtime whenever a definition is present (linked + // or definition-only — its picker is the one rendered), and the instance + // harness pin only for unlinked instance-only agents. Main splits this across + // two dialogs; the merged hook selects the layer here so the D-section picker + // follows the definition and the I-section follows the instance pin, instead + // of both multiplexing on `showInst` per call site. + const activeRuntimeId = showDef ? definitionRuntimeId : prospectiveRuntimeId; + const llmProviderFieldVisible = React.useMemo(() => { if (runtimeSupportsLlmProviderSelection(activeRuntimeId)) return true; // blankRuntimeModelProviderEditable: for an edit context (not create), when the @@ -302,17 +315,15 @@ export function useAgentEditRuntimeState({ } return false; - }, [ - showInst, - showDef, - prospectiveRuntimeId, - definitionRuntimeId, - linkedPersona, - def, - ]); + }, [activeRuntimeId, showDef, linkedPersona, def]); const prospectiveRuntime = runtimes.find( (r) => r.id === prospectiveRuntimeId, ); + // The catalog entry for the active provider/model layer (definition when a + // definition is present, instance harness pin otherwise). Every provider/model + // derivation below reads this so the D-section follows the definition runtime + // and the I-section follows the instance pin — never cross-contaminated. + const activeRuntime = runtimes.find((r) => r.id === activeRuntimeId); const inheritedEnvVars = linkedPersona?.envVars ?? {}; const inheritedSubmission = React.useMemo(() => { @@ -374,6 +385,14 @@ export function useAgentEditRuntimeState({ const defSelectedRuntime = runtimes.find((r) => r.id === definitionRuntimeId); const defEffectiveProvider = provider.trim() || inheritedProviderDefault.value; + // D-only inherited env layer: global + build defaults with NO persona env. + // Main's AgentDefinitionDialog calls useAgentDialogDefaults({ open }) with no + // persona env, so a definition tuning value that is cleared falls back to the + // global/build default — never to the definition's own just-deleted value. + // The instance overlay keeps the persona-inclusive `inheritedEnvVarsForAdvanced`. + const { inheritedEnvVars: dInheritedEnvVars } = useAgentDialogDefaults({ + open, + }); const defRequired = useRequiredCredentialState({ open, prospectiveRuntimeId: definitionRuntimeId, @@ -395,7 +414,7 @@ export function useAgentEditRuntimeState({ runtimeId: definitionRuntimeId, selectedRuntime: defSelectedRuntime, effectiveProvider: defEffectiveProvider, - inheritedEnvVars: inheritedEnvVarsForAdvanced, + inheritedEnvVars: dInheritedEnvVars, advancedRequiredEnvKeys: defApiKeyState.advancedRequiredEnvKeys, fileSatisfiedEnvKeys: defRequired.fileSatisfiedEnvKeys, topLevelSecretEnvVar: defApiKeyState.secretEnvVar, @@ -403,6 +422,11 @@ export function useAgentEditRuntimeState({ apiKeyIsInherited: defApiKeyState.isInherited, apiKeyInheritedLabel: defApiKeyState.inheritedLabel, apiKeyIsRequired: defApiKeyState.isRequired, + requiredEnvKeyMissing: defRequired.requiredEnvKeyMissing, + advancedRequiredEnvKeyMissing: hasMissingRequiredEnvKey( + defApiKeyState.advancedRequiredEnvKeys, + envVars, + ), }; const effectiveProvider = @@ -424,7 +448,7 @@ export function useAgentEditRuntimeState({ modelFieldVisible: true, open, provider: providerForDiscovery, - selectedRuntime: showInst ? prospectiveRuntime : selectedRuntime, + selectedRuntime: activeRuntime, }); const hideProviderIds = React.useMemo( @@ -437,7 +461,7 @@ export function useAgentEditRuntimeState({ const providerOptions = getPersonaProviderOptions( provider.trim(), - showInst ? (selectedRuntime?.id ?? "") : selectedRuntimeId, + activeRuntimeId, inheritedProviderDefault.source === "global" ? inheritedProviderDefault.value : "", @@ -524,7 +548,7 @@ export function useAgentEditRuntimeState({ !shouldClearKnownModelForSelectionScope({ model, provider: providerForDiscovery, - runtime: showInst ? prospectiveRuntimeId : selectedRuntimeId, + runtime: activeRuntimeId, }) ) return; @@ -535,9 +559,7 @@ export function useAgentEditRuntimeState({ model, open, providerForDiscovery, - selectedRuntimeId, - prospectiveRuntimeId, - showInst, + activeRuntimeId, setModel, setIsCustomModelEditing, ]);