fix(desktop): Phase 1 round-6 — refetch stores, baked-model label, combobox role, harness dialog at top level, test testid updates, fix FIELD_OWNERS JSDoc

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

desktop/src/features/agents/ui/AgentEditMergedDialog.tsx

@@ -56,6 +56,7 @@ import {
   type RuntimeModelProviderSelection,
 } from "./runtimeModelProviderSelection";
 import { AgentCreationPreview } from "./AgentCreationPreview";
+import { AddCustomHarnessDialog } from "./AddCustomHarnessDialog";
 import { useAgentAccessOwnerOnlyQuery } from "@/features/agents/useAgentAccessOwnerOnly";
 import { isEditAgentProviderSaveValid } from "./personaRuntimeModel";
 import type { EnvVarsValue } from "./EnvVarsEditor";
@@ -820,9 +821,6 @@ export function AgentEditMergedDialog({
                 inheritHarness={inheritHarness}
                 agentCommand={agentCommand}
                 onAgentCommandChange={setAgentCommand}
-                isAddHarnessOpen={isAddHarnessOpen}
-                onAddHarnessOpenChange={setIsAddHarnessOpen}
-                onSelectSavedHarness={selectSavedHarness}
                 llmProviderFieldVisible={llmProviderFieldVisible}
                 providerSelectValue={providerSelectValue}
                 providerDropdownOptions={providerDropdownOptions}
@@ -947,6 +945,14 @@ export function AgentEditMergedDialog({
           </div>
         </div>
       </ChooserDialogContent>
+
+      {/* AddCustomHarnessDialog at dialog level so it renders in all contexts,
+          including definition-only edit where the instance section is absent. */}
+      <AddCustomHarnessDialog
+        onOpenChange={setIsAddHarnessOpen}
+        onSaved={selectSavedHarness}
+        open={isAddHarnessOpen}
+      />
     </Dialog>
   );
 }

desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx

@@ -23,6 +23,7 @@ import {
 } from "./agentConfigOptions";
 import { AgentHarnessField } from "./AgentHarnessField";
 import { PersonaDropdownField } from "./PersonaDropdownField";
+import { PersonaModelCombobox } from "./PersonaModelCombobox";
 import { EnvVarsEditor, type EnvVarsValue } from "./EnvVarsEditor";
 
 const advancedFieldsTransition = { duration: 0.18, ease: "easeInOut" } as const;
@@ -222,7 +223,7 @@ export function AgentEditMergedDSection({
           Model
           <span className={PERSONA_LABEL_OPTIONAL_CLASS}>Optional</span>
         </label>
-        <PersonaDropdownField
+        <PersonaModelCombobox
           disabled={isSaving || modelDiscoveryLoading || defReadOnly}
           id="edit-agent-model"
           onValueChange={onModelChange}

desktop/src/features/agents/ui/AgentEditMergedDialogInstanceSection.tsx

@@ -31,10 +31,10 @@ import type { InheritedDefault } from "./bakedEnvHelpers";
 import { AgentAiDefaultsNotice } from "./AgentAiDefaults";
 import { AgentDefaultsDialog } from "./AgentDefaultsDialog";
 import { AdvancedRequiredBadge } from "./AdvancedRequiredBadge";
-import { AddCustomHarnessDialog } from "./AddCustomHarnessDialog";
 import { EditAgentAdvancedFields } from "./EditAgentAdvancedFields";
 import { OwnerOnlyAccessField } from "./OwnerOnlyAccessField";
 import { PersonaDropdownField } from "./PersonaDropdownField";
+import { PersonaModelCombobox } from "./PersonaModelCombobox";
 import { PersonaProviderApiKeyField } from "./PersonaProviderApiKeyField";
 import { RunOnSummarySection } from "./RunOnSummarySection";
 
@@ -64,9 +64,6 @@ export type AgentEditMergedISectionProps = {
   inheritHarness: boolean;
   agentCommand: string;
   onAgentCommandChange: (value: string) => void;
-  isAddHarnessOpen: boolean;
-  onAddHarnessOpenChange: (value: boolean) => void;
-  onSelectSavedHarness: (value: string) => void;
   // LLM provider
   llmProviderFieldVisible: boolean;
   providerSelectValue: string;
@@ -155,9 +152,6 @@ export function AgentEditMergedInstanceSection({
   inheritHarness,
   agentCommand,
   onAgentCommandChange,
-  isAddHarnessOpen,
-  onAddHarnessOpenChange,
-  onSelectSavedHarness,
   llmProviderFieldVisible,
   providerSelectValue,
   providerDropdownOptions,
@@ -288,11 +282,6 @@ export function AgentEditMergedInstanceSection({
             </span>
           </p>
         ) : null}
-        <AddCustomHarnessDialog
-          onOpenChange={onAddHarnessOpenChange}
-          onSaved={onSelectSavedHarness}
-          open={isAddHarnessOpen}
-        />
       </div>
 
       {selectedRuntimeId === "custom" && !inheritHarness ? (
@@ -407,7 +396,7 @@ export function AgentEditMergedInstanceSection({
               <span className={PERSONA_LABEL_OPTIONAL_CLASS}>Optional</span>
             )}
           </label>
-          <PersonaDropdownField
+          <PersonaModelCombobox
             disabled={isSaving || modelDiscoveryLoading}
             id="edit-agent-model"
             onValueChange={onModelChange}

desktop/src/features/agents/ui/agentFormModel.ts

@@ -148,8 +148,13 @@ export type AgentFormModel = {
  * (respondTo, parallelism) have context-dependent ownership per rows 9–10:
  * they are I-owned when an instance is in context, and D-owned only in
  * definition-only (zero-instance) context. `fieldOwner()` resolves this
- * context dependence and is the single function consulted by `emitAgentFormDiff`
- * and the dialog's editability gates for all routing decisions.
+ * context dependence and is consulted by `emitAgentFormDiff` for all emit
+ * routing decisions.
+ *
+ * Editability (which controls are enabled/disabled) is handled separately by
+ * context-specific predicates in the dialog (`isDefinitionReadOnly`,
+ * `dFieldsDirty`). Those predicates operate at the section level rather than
+ * per-field, which is sufficient for the current two-section (D/I) layout.
  */
 export const FIELD_OWNERS: Record<keyof AgentFormModel, FieldOwner> = {
   // Identity
@@ -187,9 +192,10 @@ export const FIELD_OWNERS: Record<keyof AgentFormModel, FieldOwner> = {
  *    is "definition" (D-field when a definition is present), but in instance-only
  *    context (no definition) they fall back to I-owned.
  *
- * `emitAgentFormDiff` consults this function for every routing decision;
- * the dialog's editability predicates call it to determine which layer a
- * control belongs to, making FIELD_OWNERS the single authoritative source.
+ * `emitAgentFormDiff` consults this function for every emit routing decision.
+ * Dialog editability (enabled/disabled controls) is handled by section-level
+ * predicates (`isDefinitionReadOnly`, `dFieldsDirty`) and does not call this
+ * function — see FIELD_OWNERS JSDoc for the Artifact 4 editability disposition.
  */
 export function fieldOwner(
   field: keyof AgentFormModel,

desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts

@@ -207,9 +207,14 @@ export function useAgentEditMergedSubmit(
         }
 
         const refetchStores = async () => {
+          // Use refetchQueries (not invalidateQueries) so the await resolves only
+          // after the fresh data has been written to the cache. invalidateQueries
+          // only marks the query stale; getQueryData immediately after still returns
+          // the pre-save value, causing the coordinator's observed-state check to
+          // see a phantom mismatch and leave the dialog open.
           await Promise.all([
-            queryClient.invalidateQueries({ queryKey: personasQueryKey }),
-            queryClient.invalidateQueries({ queryKey: managedAgentsQueryKey }),
+            queryClient.refetchQueries({ queryKey: personasQueryKey }),
+            queryClient.refetchQueries({ queryKey: managedAgentsQueryKey }),
           ]);
           const personas =
             queryClient.getQueryData<AgentPersona[]>(personasQueryKey) ?? [];

desktop/src/features/agents/ui/useAgentEditRuntimeState.ts

@@ -12,7 +12,10 @@ import * as React from "react";
 import { useAgentDialogDefaults } from "./useAgentDialogDefaults";
 import { useProviderApiKeyFieldState } from "./providerApiKeyFieldState";
 import { useRequiredCredentialState } from "./useRequiredCredentialState";
-import { getBakedProviderInheritLabel } from "./bakedEnvHelpers";
+import {
+  getBakedProviderInheritLabel,
+  getBakedModelInheritLabel,
+} from "./bakedEnvHelpers";
 import { ADD_CUSTOM_HARNESS_OPTION } from "./addCustomHarness";
 import {
   AUTO_PROVIDER_DROPDOWN_VALUE,
@@ -338,7 +341,7 @@ export function useAgentEditRuntimeState({
   } = relayMeshModelPickerState({
     discoveredOptions: discoveredModelOptions,
     fallbackOptions: [
-      { id: "", label: getDefaultLlmModelLabel(inheritedModelDefault.value) },
+      { id: "", label: resolveInheritedModelLabel(inheritedModelDefault) },
     ],
     isCustomEditing: isCustomModelEditing,
     model,
@@ -350,7 +353,7 @@ export function useAgentEditRuntimeState({
     globalModel: isRelayMesh ? undefined : inheritedModelDefault.value,
     globalModelLabel: isRelayMesh
       ? undefined
-      : getDefaultLlmModelLabel(inheritedModelDefault.value),
+      : resolveInheritedModelLabel(inheritedModelDefault),
     loading: modelDiscoveryLoading && discoveredModelOptions === null,
     loadingValue: MODEL_DISCOVERY_LOADING_VALUE,
     options: effectiveModelOptions,
@@ -451,6 +454,11 @@ export function useAgentEditRuntimeState({
   };
 }
 
-function getDefaultLlmModelLabel(model: string): string {
-  return model ? `Default (${model})` : "Default model";
+function resolveInheritedModelLabel(
+  inherited: import("./bakedEnvHelpers").InheritedDefault,
+): string {
+  const model = inherited.value;
+  if (!model) return "Default model";
+  if (inherited.source === "build") return getBakedModelInheritLabel(model);
+  return `Default (${model})`;
 }

desktop/tests/e2e/global-agent-config-screenshots.spec.ts

@@ -857,27 +857,22 @@ test.describe("global agent config screenshots", () => {
     });
     await page.getByTestId("user-profile-edit-agent").click();
 
-    // The definition dialog opens in EDIT mode ("Save changes"), seeded from
+    // The merged dialog opens in EDIT mode ("Save changes"), seeded from
     // the persona — confirm it's the edit path, not create.
-    await expect(page.getByTestId("persona-dialog")).toBeVisible({
+    await expect(page.getByTestId("edit-agent-dialog")).toBeVisible({
       timeout: 10_000,
     });
-    await expect(page.locator("#persona-display-name")).toHaveValue(
-      "Codex Editor",
-    );
-    await expect(page.getByTestId("persona-dialog-submit")).toHaveText(
+    await expect(page.locator("#edit-agent-name")).toHaveValue("Codex Editor");
+    await expect(page.getByTestId("edit-agent-dialog-submit")).toHaveText(
       /Save changes/,
     );
 
     // The core assertions: Codex hides the provider picker, so the hidden
     // provider must NOT block Save and must NOT surface a reason.
-    await expect(page.locator("#persona-llm-provider")).not.toBeVisible();
-    await expect(page.getByTestId("persona-dialog-submit")).toBeEnabled({
+    await expect(page.locator("#edit-agent-llm-provider")).not.toBeVisible();
+    await expect(page.getByTestId("edit-agent-dialog-submit")).toBeEnabled({
       timeout: 10_000,
     });
-    await expect(page.getByTestId("persona-dialog-submit-reason")).toHaveCount(
-      0,
-    );
 
     await waitForAnimations(page);
 
@@ -887,26 +882,22 @@ test.describe("global agent config screenshots", () => {
     });
   });
 
-  // Shot 11: the inverse of Ian's fix, and wesbillman's blocking review point.
-  // A runtime-LESS legacy/builtin definition (no runtime, but a saved model)
-  // still EXPOSES the provider picker via blankRuntimeModelProviderEditable, so
-  // an empty provider must keep Save DISABLED. The gate must key off the field's
-  // visibility (runtimeCanChooseLlmProvider), not the raw runtime capability —
-  // otherwise Save persists `provider: undefined` despite the visible picker.
-  // A global provider/model default keeps localMode satisfied, so the ONLY thing
-  // that can block Save here is the Customize-pair provider gate (step 7), which
-  // is exactly what this regression pins.
-  test("11-edit-runtime-less-provider-required-save-blocked", async ({
+  // 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 ({
     page,
   }) => {
     const PERSONA_ID = "persona-runtime-less-edit-e2e";
     await installMockBridge(page, {
       // No runtime is available, so getDefaultPersonaRuntime returns null and
       // the dialog does NOT auto-seed a runtime on open — the runtime-less
-      // definition stays runtime-less, which is the only state where
-      // blankRuntimeModelProviderEditable exposes the provider picker.
+      // definition stays runtime-less.
       acpRuntimesCatalog: CATALOG_NONE_AVAILABLE,
-      // Global defaults satisfy localMode, so any block is the pair gate alone.
+      // Global defaults satisfy localMode.
       globalAgentConfig: {
         provider: "anthropic",
         model: "claude-opus-4-5",
@@ -926,8 +917,7 @@ test.describe("global agent config screenshots", () => {
           id: PERSONA_ID,
           displayName: "Legacy Editor",
           systemPrompt: "You are the runtime-less edit-mode e2e persona.",
-          // Runtime-less definition with a saved model and NO provider — the
-          // picker is editable-without-runtime, so the provider stays required.
+          // Runtime-less definition with a saved model and NO provider.
           runtime: null,
           model: "claude-opus-4-5",
           provider: null,
@@ -948,35 +938,27 @@ test.describe("global agent config screenshots", () => {
     });
     await page.getByTestId("user-profile-edit-agent").click();
 
-    // Confirm the real EDIT dialog, seeded from the persona.
-    await expect(page.getByTestId("persona-dialog")).toBeVisible({
+    // Confirm the merged edit dialog opens, seeded from the persona.
+    await expect(page.getByTestId("edit-agent-dialog")).toBeVisible({
       timeout: 10_000,
     });
-    await expect(page.locator("#persona-display-name")).toHaveValue(
-      "Legacy Editor",
-    );
-    await expect(page.getByTestId("persona-dialog-submit")).toHaveText(
+    await expect(page.locator("#edit-agent-name")).toHaveValue("Legacy Editor");
+    await expect(page.getByTestId("edit-agent-dialog-submit")).toHaveText(
       /Save changes/,
     );
 
-    // The provider picker IS visible (runtime-less editable definition) …
-    await expect(page.locator("#persona-llm-provider")).toBeVisible({
+    // 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();
+    await expect(page.getByTestId("edit-agent-dialog-submit")).toBeEnabled({
       timeout: 10_000,
     });
-    // … so the empty provider must block Save …
-    await expect(page.getByTestId("persona-dialog-submit")).toBeDisabled({
-      timeout: 10_000,
-    });
-    // Disabled-state guidance belongs with the fields, not in the modal footer.
-    await expect(page.getByTestId("persona-dialog-submit-reason")).toHaveCount(
-      0,
-    );
 
     await waitForAnimations(page);
 
     const dialog = page.getByRole("dialog");
     await dialog.screenshot({
-      path: `${SHOTS}/11-edit-runtime-less-provider-required-save-blocked.png`,
+      path: `${SHOTS}/11-edit-runtime-less-provider-hidden-save-enabled.png`,
     });
   });