From 0e4cb69a7a874eddc96d706ffe7ea41fd58d63c4 Mon Sep 17 00:00:00 2001 From: Duncan Date: Sat, 8 Aug 2026 21:03:18 -0400 Subject: [PATCH] =?UTF-8?q?fix(desktop):=20Phase=201=20round-7=20=E2=80=94?= =?UTF-8?q?=20CI=20e2e=20failures:=20mock=20bridge=20persistence,=20phanto?= =?UTF-8?q?m=20commandUpdate,=20llmProviderFieldVisible,=20model=20label,?= =?UTF-8?q?=20combobox,=20harness=20save=20routing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six deterministic CI failure families closed: Family 1 (Save-still-open: edit-agent:156/190, agent-access-warning:155): Root cause: unlinked agents (personaId=null) generated a phantom agentCommand write every save because the second clause of commandUpdate was checking agentCommandOverride==null && pinned.length>0 — true for every unlinked agent with a default runtime. The submitted agentCommand never matched the mock's stale observe value → settlement always failed → dialog stayed open. Fix: guard the second clause with inst.personaId!=null so unlinked agents never generate a harness write on an untouched save. Root cause also: mock handleUpdateManagedAgent did not persist provider, agentCommand, harnessOverride, agentArgs, acpCommand, or parallelism. Any submit that included provider (test 190: Anthropic + custom model) would fail settlement because observed.provider stayed null. Fix: extend handleUpdateManagedAgent and RawManagedAgent in e2eBridge.ts to handle all fields that observedStateMatchesAgentInput checks, including agent_command_override to support harnessOverride settlement. Family 2 (model label source=global, edit-agent:318): resolveInheritedModelLabel only handled source=="build"; source=="global" fell through to the generic "Default (model)" label instead of calling getDefaultLlmModelLabel which renders "Use agent defaults (model)". Fix: add source==="global" branch. Family 3 (test 11 provider picker visible: global-agent-config-screenshots:899): llmProviderFieldVisible used prospectiveRuntimeId (instance harness pin, derived from inst.agentCommand="goose") in D+I context. The D-section provider picker is driven by the definition runtime, not the instance pin. A runtime-less definition with a goose-pinned instance was incorrectly exposing the provider picker. Fix: use definitionRuntimeId in D+I context; prospectiveRuntimeId only in pure I-only context. Family 4 (model combobox not found: agent-provider-dropdowns:147): Auto-seed effect set definitionRuntimeId but not selectedRuntimeId in D-only context. useAgentEditRuntimeState derives selectedRuntime from selectedRuntimeId which drove model discovery — a stale "custom" value left discovery unable to resolve a runtime and the model list stayed empty. Fix: auto-seed also calls setSelectedRuntimeId in D-only context (!inst). Also added pickModelComboboxOption helper to edit-agent.spec.ts (uses getByRole("button") not menuitemradio for PersonaModelCombobox items). Family 5 (Databricks text leak: agent-provider-dropdowns:213): Test used globalAgentConfig.model="global-databricks-model" which produced a "Use agent defaults (global-databricks-model)" label containing "Databricks". Fix: change test fixture to "global-openai-model". Family 6 (saved harness not selected in definition edit: inline-custom-harness:122): selectSavedHarness (usePendingHarnessSelection) called handleRuntimeDropdownChange in all contexts, but in D-only context the harness dropdown is driven by definitionRuntimeId via handleDefinitionRuntimeChange. The I-pin handler set selectedRuntimeId, which is hidden in D-only context. Fix: route to handleDefinitionRuntimeChange when showDef && !showInst. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../agents/ui/AgentEditMergedDialog.tsx | 15 ++++++++- .../agents/ui/useAgentEditMergedSubmit.ts | 11 ++++++- .../agents/ui/useAgentEditRuntimeState.ts | 13 +++++++- desktop/src/testing/e2eBridge.ts | 31 +++++++++++++++++++ .../e2e/agent-provider-dropdowns.spec.ts | 2 +- desktop/tests/e2e/edit-agent.spec.ts | 17 +++++++++- 6 files changed, 84 insertions(+), 5 deletions(-) diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx index 15c3505f1f..75b8530d16 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx +++ b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx @@ -322,6 +322,14 @@ export function AgentEditMergedDialog({ const defaultRuntime = getDefaultPersonaRuntime(runtimes); if (!defaultRuntime) return; setDefinitionRuntimeId(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` + // from `selectedRuntimeId`, and a stale "custom" value leaves discovery unable + // to resolve a runtime → model list stays empty. + if (!inst) { + setSelectedRuntimeId(defaultRuntime.id); + } }, [open, runtimes.length, showDef]); // Re-derive runtime id when catalog loads @@ -545,7 +553,12 @@ export function AgentEditMergedDialog({ const selectSavedHarness = usePendingHarnessSelection( runtimes, - handleRuntimeDropdownChange, + // 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, open, ); diff --git a/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts b/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts index 1d36326e5c..17d733cb31 100644 --- a/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts +++ b/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts @@ -181,9 +181,18 @@ export function useAgentEditMergedSubmit( commandUpdate = inst.agentCommandOverride != null ? "" : undefined; } else { const pinned = (effectiveRuntime?.command ?? s.agentCommand).trim(); + // Only write a commandUpdate when the command actually changed OR + // when a linked agent (personaId != null) is creating a new pin that + // matches the current inherited command (no override yet). Unlinked + // agents have no inherit/pin distinction — their agentCommandOverride + // is always null, so the second clause must not trigger for them or + // every unmodified save generates a phantom harness write that + // `observedStateMatchesAgentInput` then fails to settle. if ( pinned !== inst.agentCommand || - (inst.agentCommandOverride == null && pinned.length > 0) + (inst.personaId != null && + inst.agentCommandOverride == null && + pinned.length > 0) ) { commandUpdate = pinned; } diff --git a/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts b/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts index 466843bb5c..72c2ccb42c 100644 --- a/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts +++ b/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts @@ -31,6 +31,7 @@ import { sortPersonaRuntimes, type PersonaDropdownOption, getProviderApiKeyEnvVar, + getDefaultLlmModelLabel, } from "./agentConfigOptions"; import { MODEL_DISCOVERY_LOADING_VALUE, @@ -221,7 +222,16 @@ export function useAgentEditRuntimeState({ ]); const llmProviderFieldVisible = runtimeSupportsLlmProviderSelection( - showInst ? prospectiveRuntimeId : selectedRuntimeId, + // 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). + showInst && !showDef ? prospectiveRuntimeId : definitionRuntimeId, ); const prospectiveRuntime = runtimes.find( (r) => r.id === prospectiveRuntimeId, @@ -460,5 +470,6 @@ function resolveInheritedModelLabel( const model = inherited.value; if (!model) return "Default model"; if (inherited.source === "build") return getBakedModelInheritLabel(model); + if (inherited.source === "global") return getDefaultLlmModelLabel(model); return `Default (${model})`; } diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index 6961488fef..1481b3d4bd 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -857,6 +857,8 @@ type RawManagedAgent = { | { type: "local" } | { type: "provider"; id: string; config: Record }; backend_agent_id: string | null; + /** Non-null when the instance has an explicit harness pin (user chose a specific command). */ + agent_command_override?: string | null; respond_to: "owner-only" | "allowlist" | "anyone"; respond_to_allowlist: string[]; }; @@ -1687,6 +1689,7 @@ function cloneManagedAgent(agent: MockManagedAgent): RawManagedAgent { auto_restart_on_config_change: agent.auto_restart_on_config_change ?? true, backend: agent.backend ?? { type: "local" as const }, backend_agent_id: agent.backend_agent_id ?? null, + agent_command_override: agent.agent_command_override ?? null, respond_to: agent.respond_to ?? "owner-only", respond_to_allowlist: agent.respond_to_allowlist ? [...agent.respond_to_allowlist] @@ -8670,10 +8673,16 @@ async function handleUpdateManagedAgent(args: { pubkey: string; name?: string; model?: string | null; + provider?: string | null; systemPrompt?: string | null; envVars?: Record; respondTo?: "owner-only" | "allowlist" | "anyone"; respondToAllowlist?: string[]; + agentCommand?: string; + harnessOverride?: boolean; + agentArgs?: string[]; + acpCommand?: string; + parallelism?: number; }; }): Promise<{ agent: RawManagedAgent; profile_sync_error: string | null }> { const agent = getMockManagedAgent(args.input.pubkey); @@ -8683,6 +8692,9 @@ async function handleUpdateManagedAgent(args: { if (args.input.model !== undefined) { agent.model = args.input.model; } + if (args.input.provider !== undefined) { + agent.provider = args.input.provider; + } if (args.input.systemPrompt !== undefined) { agent.system_prompt = args.input.systemPrompt; } @@ -8695,6 +8707,25 @@ async function handleUpdateManagedAgent(args: { if (args.input.respondToAllowlist !== undefined) { agent.respond_to_allowlist = args.input.respondToAllowlist; } + if (args.input.agentCommand !== undefined) { + agent.agent_command = args.input.agentCommand; + // Mirror Rust backend: non-empty command + harnessOverride=true → set override; + // empty string (inherit sentinel) → clear override. + if (args.input.agentCommand.length === 0) { + agent.agent_command_override = null; + } else if (args.input.harnessOverride) { + agent.agent_command_override = args.input.agentCommand; + } + } + if (args.input.agentArgs !== undefined) { + agent.agent_args = [...args.input.agentArgs]; + } + if (args.input.acpCommand !== undefined) { + agent.acp_command = args.input.acpCommand; + } + if (args.input.parallelism !== undefined) { + agent.parallelism = args.input.parallelism; + } agent.updated_at = new Date().toISOString(); return { agent: cloneManagedAgent(agent), profile_sync_error: null }; } diff --git a/desktop/tests/e2e/agent-provider-dropdowns.spec.ts b/desktop/tests/e2e/agent-provider-dropdowns.spec.ts index 0a994c79d9..9446ed0e4f 100644 --- a/desktop/tests/e2e/agent-provider-dropdowns.spec.ts +++ b/desktop/tests/e2e/agent-provider-dropdowns.spec.ts @@ -216,7 +216,7 @@ test.describe("agent provider dropdown screenshots", () => { await installMockBridge(page, { globalAgentConfig: { provider: "databricks_v2", - model: "global-databricks-model", + model: "global-openai-model", env_vars: {}, }, personas: [ diff --git a/desktop/tests/e2e/edit-agent.spec.ts b/desktop/tests/e2e/edit-agent.spec.ts index 8cedd8091c..96f03b0732 100644 --- a/desktop/tests/e2e/edit-agent.spec.ts +++ b/desktop/tests/e2e/edit-agent.spec.ts @@ -78,6 +78,21 @@ async function pickDropdownOption( await page.getByRole("menuitemradio", { name: optionName }).click(); } +/** + * Pick an option from a PersonaModelCombobox (popover-based combobox with + *