From 3139d1aaa3e19650b2a33496d3655629bdd2f2a2 Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 12 Aug 2026 21:49:59 -0400 Subject: [PATCH] feat(desktop): restore definition-context tuning and API-key fields to merged dialog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every field reachable in main's AgentDefinitionDialog edit mode was not carried into the merged dialog's definition (D) section: the numeric tuning knobs and buzz-agent effort knob, the provider API-key field, the parallelism cap hint, and the full EnvVarsEditor prop set (required-key highlighting, file-satisfied indicators, mint-key annotation, hidden structured keys). A definition-backed or definition-only edit therefore lost controls it had on main. These are additive-only restorations to reach field parity. The instance-side credential state was gated on open && showInst, starving the D-section in definition-only context. useAgentEditRuntimeState now derives a DSectionAdvancedState bundle from the definition runtime, provider, and env — independent of the instance harness pin — mirroring AgentDefinitionDialog. Bundling it as one prop keeps AgentEditMergedDialog under the desktop line-size gate. Tuning and API-key edits write the definition env layer, so emitAgentFormDiff routes them to personaInput and never the instance overlay. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../agents/ui/AgentEditMergedDialog.tsx | 2 + .../ui/AgentEditMergedDialogDSection.tsx | 142 +++++- .../ui/agentEditMergedDSectionParity.test.mjs | 473 ++++++++++++++++++ .../agents/ui/agentFormModel.test.mjs | 73 +++ .../agents/ui/useAgentEditRuntimeState.ts | 83 +++ 5 files changed, 772 insertions(+), 1 deletion(-) create mode 100644 desktop/src/features/agents/ui/agentEditMergedDSectionParity.test.mjs diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx index baebbb69c6..1521102d2a 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx +++ b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx @@ -422,6 +422,7 @@ export function AgentEditMergedDialog({ apiKeyIsRequired, topLevelSecretEnvVar, advancedRequiredEnvKeys, + dAdvanced, } = useAgentEditRuntimeState({ open, showDef, @@ -826,6 +827,7 @@ export function AgentEditMergedDialog({ agentAccessOwnerOnly={agentAccessOwnerOnly} parallelism={parallelism} onParallelismChange={setParallelism} + dAdvanced={dAdvanced} /> ) : null} diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx index 8f7df5ea4e..3bddcfb72e 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx +++ b/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx @@ -20,6 +20,8 @@ import { Button } from "@/shared/ui/button"; import type { RespondToMode } from "@/shared/api/types"; import { + CARD_MINT_KEY_ANNOTATIONS, + getProviderApiKeyLabel, PERSONA_FIELD_CONTROL_CLASS, PERSONA_FIELD_SHELL_CLASS, PERSONA_LABEL_OPTIONAL_CLASS, @@ -28,9 +30,24 @@ import { import { AgentHarnessField } from "./AgentHarnessField"; import { PersonaDropdownField } from "./PersonaDropdownField"; import { PersonaModelCombobox } from "./PersonaModelCombobox"; +import { PersonaProviderApiKeyField } from "./PersonaProviderApiKeyField"; import { EnvVarsEditor, type EnvVarsValue } from "./EnvVarsEditor"; import { OwnerOnlyAccessField } from "./OwnerOnlyAccessField"; +import { + BuzzAgentModelTuningFields, + NumericTuningFields, +} from "./buzzAgentModelTuningFields"; +import { + isBuzzAgentRuntime, + BUZZ_AGENT_THINKING_EFFORT, +} from "./buzzAgentConfig"; +import { + deriveNumericDescriptors, + structuredEnvKeys, +} from "../lib/agentConfigCore"; +import { parallelismCapHint } from "../lib/agentParallelism"; import type { AgentFormModel } from "./agentFormModel"; +import type { DSectionAdvancedState } from "./useAgentEditRuntimeState"; const advancedFieldsTransition = { duration: 0.18, ease: "easeInOut" } as const; @@ -95,6 +112,12 @@ export type AgentEditMergedDSectionProps = { agentAccessOwnerOnly: boolean | undefined; parallelism: string; onParallelismChange: (value: string) => void; + /** + * Definition-context advanced state (tuning knobs, env-key highlighting, + * provider API-key field) derived from the definition runtime/provider/env. + * Consumed by the Advanced section to match main's PersonaAdvancedFields. + */ + dAdvanced: DSectionAdvancedState; }; // ── Component ───────────────────────────────────────────────────────────────── @@ -138,9 +161,71 @@ export function AgentEditMergedDSection({ agentAccessOwnerOnly, parallelism, onParallelismChange, + dAdvanced, }: AgentEditMergedDSectionProps) { const [showAdvanced, setShowAdvanced] = React.useState(false); + // Numeric tuning descriptors — gate on catalog status so loading/error never + // collapses to "no controls": keys stay visible as generic rows. Derived from + // the DEFINITION runtime (dAdvanced.selectedRuntime), matching main's + // PersonaAdvancedFields. + const numericDescriptors = React.useMemo( + () => + runtimeCatalogStatus === "ready" + ? deriveNumericDescriptors(dAdvanced.selectedRuntime) + : [], + [runtimeCatalogStatus, dAdvanced.selectedRuntime], + ); + + // Effective hidden keys: the provider secret (rendered as the API-key field) + // + the buzz-agent effort key (rendered by BuzzAgentModelTuningFields) + the + // structured numeric keys — none of these should appear as generic env rows. + const effectiveHiddenKeys = React.useMemo( + () => [ + ...(dAdvanced.topLevelSecretEnvVar + ? [dAdvanced.topLevelSecretEnvVar] + : []), + ...(isBuzzAgentRuntime(dAdvanced.runtimeId) + ? [BUZZ_AGENT_THINKING_EFFORT] + : []), + ...structuredEnvKeys(numericDescriptors), + ], + [dAdvanced.topLevelSecretEnvVar, dAdvanced.runtimeId, numericDescriptors], + ); + + // Parallelism cap hint: the definition keeps a portable requested value; when + // the selected harness has a cap and the draft exceeds it, explain the agent + // will run at the cap — without clamping the stored value. + const parallelismHint = React.useMemo(() => { + const runtime = dAdvanced.selectedRuntime; + if (runtime?.maxParallelism === undefined || parallelism === "") { + return null; + } + const requested = parseInt(parallelism, 10); + if (Number.isNaN(requested)) return null; + return parallelismCapHint(runtime.label, runtime.maxParallelism, requested); + }, [dAdvanced.selectedRuntime, parallelism]); + + // Env-var mutation shared by both tuning-field groups: delete on empty so a + // cleared knob reverts to the inherited placeholder rather than persisting "". + const onTuningEnvVarChange = React.useCallback( + (key: string, value: string) => { + const next = { ...envVars }; + if (value === "") { + delete next[key]; + } else { + next[key] = value; + } + onEnvVarsChange(next); + }, + [envVars, onEnvVarsChange], + ); + + // Bind the definition secret key to a local so the truthy guard narrows it + // to `string` inside the API-key field's onValueChange closure (a property + // access on `dAdvanced` would not narrow through the closure). + const defSecretEnvVar = dAdvanced.topLevelSecretEnvVar; + return ( <> {/* Agent name */} @@ -252,6 +337,26 @@ export function AgentEditMergedDSection({ ) : null} + {/* Provider API key — D-field. Matches AgentDefinitionDialog edit mode: + rendered when the provider is custom and exposes a top-level secret. + Writes to the definition env layer (envVars → personaInput.envVars). */} + {llmProviderFieldVisible && defSecretEnvVar ? ( + { + onEnvVarsChange({ ...envVars, [defSecretEnvVar]: next }); + }} + value={dAdvanced.apiKeyValue} + /> + ) : null} + {/* Model — D-field */}
+ {parallelismHint !== null ? ( +

+ {parallelismHint} +

+ ) : null} ) : null} @@ -389,7 +499,10 @@ export function AgentEditMergedDSection({

- {/* Definition env vars — D-field (row 8) */} + {/* Definition env vars — D-field (row 8). Full prop set matches + main's PersonaAdvancedFields: required-key highlighting, + file-satisfied indicators, mint-key annotation, and hidden + structured keys (secret + effort + numeric descriptors). */}

Environment variables @@ -397,10 +510,37 @@ export function AgentEditMergedDSection({

+ + {/* Descriptor-driven numeric tuning knobs — shown when the catalog + has settled and the definition runtime exposes numeric env-var + fields. Writes route to the definition env layer. */} + {numericDescriptors.length > 0 ? ( + + ) : null} + + {/* Effort-tuning knob — only shown for buzz-agent runtimes. */} + {isBuzzAgentRuntime(dAdvanced.runtimeId) ? ( + + ) : null} ) : null} diff --git a/desktop/src/features/agents/ui/agentEditMergedDSectionParity.test.mjs b/desktop/src/features/agents/ui/agentEditMergedDSectionParity.test.mjs new file mode 100644 index 0000000000..eda8329ee5 --- /dev/null +++ b/desktop/src/features/agents/ui/agentEditMergedDSectionParity.test.mjs @@ -0,0 +1,473 @@ +/** + * D-section field-parity mount tests. + * + * PR #5328 merges main's two edit dialogs (AgentDefinitionDialog + + * AgentInstanceEditDialog) into one AgentEditMergedDialog. Four D-section + * (definition-field) capabilities from main's PersonaAdvancedFields / + * AgentDefinitionDialog edit mode were dropped in the merge and restored here: + * + * Fix2 parallelism cap hint (amber "runs at most N" warning) + * Fix4 provider API-key field (custom provider + top-level secret) + * ---- descriptor-driven numeric tuning knobs + buzz-agent effort knob + * ---- env-editor structured-key hiding + mint-key annotation + * + * These mount the REAL dialog in a definition-only context (react-dom + jsdom) + * so the restored controls are genuinely rendered, the seed/derive effects run, + * and the D-section reads the definition runtime/provider/env — not the + * instance overlay. The Tauri boundary is stubbed at + * window.__TAURI_INTERNALS__.invoke; the query cache is pre-seeded so the + * dialog paints on first settle without live IPC. + * + * Emit-layer routing (a D-env edit lands in personaInput, never agentInput) is + * covered by the pure-logic tests in agentFormModel.test.mjs; these assert the + * rendered surface. + */ + +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", +}); + +/** QueryClients created per test, cleared in afterEach so no gc timer lingers. */ +const liveClients = []; +/** + * Radix's dismissable-layer / focus-scope schedule global setTimeout/setInterval + * that outlive the render. jsdom's own timers delegate to the global, so we + * track every timer we hand out and clear the survivors in teardown — otherwise + * the process hangs after the tests 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(); +}); + +after(() => { + globalThis.setTimeout = nativeSetTimeout; + globalThis.setInterval = nativeSetInterval; + globalThis.clearTimeout = nativeClearTimeout; + globalThis.clearInterval = nativeClearInterval; + dom.window.close(); +}); + +/** + * A buzz-agent runtime catalog entry with the numeric + thinking env vars set + * (so descriptors derive) and a parallelism cap (so the cap hint can fire). + * + * `availability` is deliberately NOT "available": an available runtime is the + * sole trigger for the asynchronous model-discovery IPC, whose settle resolves + * after the mount's act window and logs act(...) warnings. Model discovery is + * orthogonal to D-section field parity — every field under test (tuning knobs, + * effort, API key, cap hint, env-key hiding) derives from the catalog entry and + * provider, not from availability, exactly as main's PersonaAdvancedFields does. + * Suppressing discovery keeps these mounts deterministic without weakening the + * parity claim. + */ +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, +}; + +/** + * Build a definition-only edit context. `envVars`, `parallelism`, and + * `provider` default to values that exercise the restored controls. + */ +function definitionCtx(overrides = {}) { + return { + kind: "definition-only", + definition: { + id: "def-parity", + 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: 8, + createdAt: "2025-01-01T00:00:00Z", + updatedAt: "2025-01-01T00:00:00Z", + ...overrides, + }, + }; +} + +/** 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"], []); + 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; +} + +/** Stub the Tauri boundary — the dialog only reads; nothing here mutates. */ +function installInvoke() { + dom.window.__TAURI_INTERNALS__ = { + invoke: async (cmd) => { + if (cmd === "list_personas") return []; + if (cmd === "list_managed_agents") return []; + if (cmd === "get_runtime_file_config") { + return { provider: null, model: null, satisfiedEnvKeys: [] }; + } + 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 = 40) { + const { act } = await import("react"); + await act(async () => { + await new Promise((r) => setTimeout(r, ms)); + }); +} + +/** Click the D-section "Advanced" disclosure to reveal env + tuning fields. */ +async function openAdvanced() { + const { fireEvent, screen, act } = 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)); + }); + void act; +} + +// ── Fix4: provider API-key field in a definition-only edit ─────────────────── + +test("test_definition_only_dsection_renders_provider_api_key_field_for_custom_provider", async () => { + // Main's AgentDefinitionDialog edit mode renders PersonaProviderApiKeyField + // when the provider is custom and exposes a top-level secret. A definition + // whose provider is anthropic must therefore expose the ANTHROPIC_API_KEY + // field in the merged D-section — the field the merge dropped from def-only. + const qc = await makeSeededClient(); + installInvoke(); + await renderDialog(qc, definitionCtx()); + await settle(); + + const { screen } = await import("@testing-library/react"); + const apiKey = screen.getByTestId("persona-provider-api-key"); + assert.ok( + apiKey, + "the provider API-key input must render for a custom provider", + ); + + // The env var name is shown as a monospace hint, and the label is provider-derived. + assert.match( + dom.window.document.body.textContent ?? "", + /ANTHROPIC_API_KEY/, + "the API-key field must show its ANTHROPIC_API_KEY env-var hint", + ); + assert.match( + dom.window.document.body.textContent ?? "", + /Anthropic API Key/, + "the API-key field label must be the provider-specific label", + ); +}); + +test("test_definition_only_dsection_hides_provider_api_key_field_when_provider_has_no_secret", async () => { + // Databricks uses OAuth PKCE — no typed secret credential. Main renders no + // API-key field for it, so the merged D-section must not either (guards + // against adding a field main never had). + const qc = await makeSeededClient(); + installInvoke(); + await renderDialog( + qc, + definitionCtx({ provider: "databricks", model: null }), + ); + await settle(); + + const { screen } = await import("@testing-library/react"); + assert.equal( + screen.queryByTestId("persona-provider-api-key"), + null, + "a provider with no typed secret must not render an API-key field", + ); +}); + +// ── Fix2: parallelism cap hint ─────────────────────────────────────────────── + +test("test_definition_only_dsection_shows_parallelism_cap_hint_when_requested_exceeds_runtime_cap", async () => { + // The definition requests parallelism 8; the buzz-agent runtime caps at 2. + // Main's PersonaAdvancedFields shows an amber hint explaining the agent will + // run at the cap. The merged D-section must reproduce it without clamping. + const qc = await makeSeededClient(); + installInvoke(); + await renderDialog(qc, definitionCtx({ parallelism: 8 })); + await settle(); + + assert.match( + dom.window.document.body.textContent ?? "", + /runs at most 2 parallel conversations/, + "the amber cap hint must appear when requested parallelism exceeds the runtime cap", + ); +}); + +test("test_definition_only_dsection_hides_parallelism_cap_hint_when_within_cap", async () => { + // At-or-below the cap, no hint — the value is honored as requested. + const qc = await makeSeededClient(); + installInvoke(); + await renderDialog(qc, definitionCtx({ parallelism: 1 })); + await settle(); + + assert.doesNotMatch( + dom.window.document.body.textContent ?? "", + /runs at most/, + "no cap hint when the requested parallelism is within the runtime cap", + ); +}); + +// ── Numeric tuning + effort knobs (restored from PersonaAdvancedFields) ────── + +test("test_definition_only_dsection_advanced_renders_numeric_and_effort_tuning_knobs", async () => { + // buzz-agent exposes three numeric env-var fields plus the thinking-effort + // knob. All four render under the D-section Advanced disclosure, reading the + // definition env layer — a value already set on the definition shows in its + // input rather than as a generic env row. + const qc = await makeSeededClient(); + installInvoke(); + await renderDialog( + qc, + definitionCtx({ envVars: { BUZZ_AGENT_MAX_OUTPUT_TOKENS: "4096" } }), + ); + await settle(); + await openAdvanced(); + + const { screen } = await import("@testing-library/react"); + const maxTokens = screen.getByTestId("numeric-max-output-tokens-input"); + assert.ok(maxTokens, "max output tokens input must render"); + assert.equal( + maxTokens.value, + "4096", + "the numeric input must read the definition's stored env value", + ); + assert.ok( + screen.getByTestId("numeric-context-limit-input"), + "context limit input must render", + ); + assert.ok( + screen.getByTestId("numeric-max-rounds-input"), + "max rounds input must render", + ); + assert.ok( + screen.getByTestId("ba-thinking-effort-select"), + "the buzz-agent thinking-effort knob must render", + ); +}); + +// ── Env editor: structured-key hiding + mint-key annotation ────────────────── + +test("test_definition_only_dsection_env_editor_hides_structured_keys_and_annotates_mint_key", async () => { + // The D-section env editor must hide keys owned by structured controls (the + // provider secret, the numeric descriptors, the effort key) so they are never + // double-edited, while a plain user key stays a generic row and OPENAI_API_KEY + // carries its mint-key annotation. + const qc = await makeSeededClient(); + installInvoke(); + await renderDialog( + qc, + definitionCtx({ + envVars: { + BUZZ_AGENT_MAX_OUTPUT_TOKENS: "4096", // structured → hidden + ANTHROPIC_API_KEY: "sk-live", // secret → hidden (renders as API-key field) + OPENAI_API_KEY: "sk-openai", // annotated generic row + MY_PLAIN_VAR: "hello", // plain generic row + }, + }), + ); + await settle(); + await openAdvanced(); + + const { screen } = await import("@testing-library/react"); + const genericKeys = screen + .queryAllByTestId("env-vars-key") + .map((el) => el.value); + + assert.ok( + genericKeys.includes("MY_PLAIN_VAR"), + "a plain env key must remain an editable generic row", + ); + assert.ok( + !genericKeys.includes("BUZZ_AGENT_MAX_OUTPUT_TOKENS"), + "a numeric structured key must be hidden from the generic env rows", + ); + assert.ok( + !genericKeys.includes("ANTHROPIC_API_KEY"), + "the provider secret must be hidden from the generic env rows", + ); + assert.ok( + genericKeys.includes("OPENAI_API_KEY"), + "OPENAI_API_KEY is not structured for this provider — stays a generic row", + ); + const annotations = screen + .queryAllByTestId("env-vars-key-annotation") + .map((el) => el.textContent ?? ""); + assert.ok( + annotations.some((t) => /minting agent trading cards/.test(t)), + "the OPENAI_API_KEY mint-key annotation must render", + ); +}); diff --git a/desktop/src/features/agents/ui/agentFormModel.test.mjs b/desktop/src/features/agents/ui/agentFormModel.test.mjs index b10b1fa21a..d2d03f1fab 100644 --- a/desktop/src/features/agents/ui/agentFormModel.test.mjs +++ b/desktop/src/features/agents/ui/agentFormModel.test.mjs @@ -598,3 +598,76 @@ test("test_emitAgentFormDiff_routes_respondTo_change_to_personaInput_via_fieldOw "agentInput must be null — no instance in definition-only context", ); }); + +// ── D-section tuning/API-key env writes route to the DEFINITION layer ───────── +// +// The restored D-section Advanced knobs (numeric tuning, effort, provider API +// key) all mutate the same `envVars` model field — the definition env layer. +// Paul's parity mandate: definition fields → personaInput, instance fields → +// agentInput. These probe that a D-env edit lands in personaInput.envVars and +// never leaks into the instance overlay (agentInput.envVars), in both contexts +// where the D-section renders (linked and definition-only). + +test("test_definition_tuning_env_edit_routes_to_personaInput_not_instance_overlay", () => { + // Linked agent: the D-section edits the definition env, while the instance + // keeps its own overlay. A tuning-knob write must reach personaInput.envVars + // and leave the instance overlay (and agentInput) untouched. + const definition = makeDefinition({ runtime: "buzz-agent", envVars: {} }); + const instance = makeInstance({ envVars: { PER_INSTANCE: "keep" } }); + const ctx = { kind: "instance-with-definition", definition, instance }; + const seed = seedAgentFormModel(ctx); + + // User sets a numeric tuning knob (writes to the D-env layer, `envVars`). + const next = { + ...seed, + envVars: { ...seed.envVars, GOOSE_MAX_OUTPUT_TOKENS: "8192" }, + }; + const emit = emitAgentFormDiff(seed, next, ctx); + + assert.ok(emit.personaInput, "a D-env tuning edit must emit a personaInput"); + assert.equal( + emit.personaInput.envVars?.GOOSE_MAX_OUTPUT_TOKENS, + "8192", + "the tuning value must land in personaInput.envVars (definition layer)", + ); + assert.equal( + emit.agentInput, + null, + "the instance overlay is untouched, so no agentInput is emitted", + ); +}); + +test("test_definition_api_key_env_edit_routes_to_personaInput_in_definition_only", () => { + // Definition-only edit: the restored provider API-key field writes the secret + // into the definition env. It must travel as personaInput.envVars — there is + // no instance layer to leak into. + const definition = makeDefinition({ + runtime: "buzz-agent", + provider: "anthropic", + envVars: {}, + }); + const ctx = { kind: "definition-only", definition }; + const seed = seedAgentFormModel(ctx); + + // User types the provider API key (writes ANTHROPIC_API_KEY into `envVars`). + const next = { + ...seed, + envVars: { ...seed.envVars, ANTHROPIC_API_KEY: "sk-live-xyz" }, + }; + const emit = emitAgentFormDiff(seed, next, ctx); + + assert.ok( + emit.personaInput, + "a definition-only API-key edit must emit a personaInput", + ); + assert.equal( + emit.personaInput.envVars?.ANTHROPIC_API_KEY, + "sk-live-xyz", + "the API key must land in personaInput.envVars (definition layer)", + ); + assert.equal( + emit.agentInput, + null, + "definition-only context has no instance, so agentInput must be null", + ); +}); diff --git a/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts b/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts index 30632e24e9..231da91786 100644 --- a/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts +++ b/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts @@ -49,6 +49,42 @@ import type { AgentPersona } from "@/shared/api/types"; // ── Types ───────────────────────────────────────────────────────────────────── +/** + * Definition-context advanced state for the D-section (parity with main's + * `AgentDefinitionDialog` edit mode + `PersonaAdvancedFields`). + * + * Every field here is derived from the DEFINITION's runtime/provider/env + * (`definitionRuntimeId`, the D-layer provider, and the D-env `envVars`) — + * independent of the instance harness pin. In linked context the instance + * overlay (I-section) keeps using the prospective-runtime state; the D-section + * uses this bundle so its tuning knobs, env-key highlighting, and API-key field + * reflect the definition, not the instance. + * + * Bundled into a single object so the parent threads one prop into the + * D-section rather than a dozen — the merged dialog file sits against the + * desktop line-size gate. + */ +export type DSectionAdvancedState = { + /** Definition runtime id — drives the buzz-agent effort-tuning gate. */ + runtimeId: string; + /** Catalog entry for the definition runtime — drives numeric descriptors and the parallelism cap hint. */ + selectedRuntime: AcpRuntimeCatalogEntry | undefined; + /** Effective definition provider (agent value → global fallback) — for tuning-field filtering and the API-key label. */ + effectiveProvider: string; + /** Inherited env layer for tuning-field placeholders (global / template defaults). */ + inheritedEnvVars: Record; + /** Required non-secret env keys highlighted in the env editor (secret key excluded — it renders as the API-key field). */ + advancedRequiredEnvKeys: readonly string[]; + /** Required keys already satisfied by the runtime file layer (shown as info rows). */ + fileSatisfiedEnvKeys: readonly string[]; + /** Definition-level provider secret env var, or null when the provider has none. */ + topLevelSecretEnvVar: string | null; + apiKeyValue: string; + apiKeyIsInherited: boolean; + apiKeyInheritedLabel: string; + apiKeyIsRequired: boolean; +}; + export type AgentEditRuntimeStateInputs = { open: boolean; showDef: boolean; @@ -120,6 +156,12 @@ export type AgentEditRuntimeStateResult = { * Matches AgentDefinitionDialog's blankRuntimeModelProviderEditable contract. */ blankRuntimeModelProviderEditable: boolean; + /** + * Definition-context advanced state consumed by the D-section (tuning knobs, + * env-key highlighting, provider API-key field). Derived from the definition + * runtime/provider/env so the D-section matches main's PersonaAdvancedFields. + */ + dAdvanced: DSectionAdvancedState; }; // ── Hook ────────────────────────────────────────────────────────────────────── @@ -323,6 +365,46 @@ export function useAgentEditRuntimeState({ { enabled: open }, ); + // ── Definition-context advanced state (D-section parity) ─────────────────── + // The instance-side credential/api-key state above is gated on `showInst`, so + // in definition-only context it starves the D-section. Compute the definition + // layer independently from `definitionRuntimeId` + the D provider + the D env, + // mirroring main's AgentDefinitionDialog. Runs in every context (the query is + // `enabled: open`); the D-section only consumes it when `showDef`. + const defSelectedRuntime = runtimes.find((r) => r.id === definitionRuntimeId); + const defEffectiveProvider = + provider.trim() || inheritedProviderDefault.value; + const defRequired = useRequiredCredentialState({ + open, + prospectiveRuntimeId: definitionRuntimeId, + provider: defEffectiveProvider, + globalProvider: inheritedProviderDefault.value, + envVars, + globalEnvVars: globalConfig.env_vars, + }); + const defApiKeyState = useProviderApiKeyFieldState({ + bakedEnvKeys, + effectiveEnvVars: envVars, + envVars, + fileSatisfiedEnvKeys: defRequired.fileSatisfiedEnvKeys, + globalEnvVars: globalConfig.env_vars, + provider: defEffectiveProvider, + requiredEnvKeys: defRequired.requiredEnvKeys, + }); + const dAdvanced: DSectionAdvancedState = { + runtimeId: definitionRuntimeId, + selectedRuntime: defSelectedRuntime, + effectiveProvider: defEffectiveProvider, + inheritedEnvVars: inheritedEnvVarsForAdvanced, + advancedRequiredEnvKeys: defApiKeyState.advancedRequiredEnvKeys, + fileSatisfiedEnvKeys: defRequired.fileSatisfiedEnvKeys, + topLevelSecretEnvVar: defApiKeyState.secretEnvVar, + apiKeyValue: defApiKeyState.value, + apiKeyIsInherited: defApiKeyState.isInherited, + apiKeyInheritedLabel: defApiKeyState.inheritedLabel, + apiKeyIsRequired: defApiKeyState.isRequired, + }; + const effectiveProvider = (inheritedSubmission.provider ?? "").trim() || inheritedProviderDefault.value; @@ -501,6 +583,7 @@ export function useAgentEditRuntimeState({ definitionRuntimeId === "custom" && ((linkedPersona?.model ?? def?.model ?? "").trim().length > 0 || (linkedPersona?.provider ?? def?.provider ?? "").trim().length > 0), + dAdvanced, }; }