Address agent import review feedback

klopez4212 · klopez4212 · commit 71efb2bb2399 · 2026-06-22T15:45:53.000+01:00
diff --git a/desktop/src/features/agents/ui/personaImportPlan.test.mjs b/desktop/src/features/agents/ui/personaImportPlan.test.mjs
@@ -14,6 +14,7 @@ function createPersona(overrides = {}) {
     systemPrompt: "Be helpful.",
     runtime: null,
     model: null,
+    provider: null,
     namePool: [],
     isBuiltIn: false,
     isActive: true,
@@ -31,6 +32,7 @@ function createPreview(overrides = {}) {
     avatarRef: null,
     runtime: null,
     model: null,
+    provider: null,
     namePool: [],
     sourceFile: "alice.persona.json",
     ...overrides,
@@ -118,6 +120,19 @@ test("buildPersonaImportPlan detects model change", () => {
   assert.equal(plan.fields[0]?.label, "Preferred model");
 });
 
+test("buildPersonaImportPlan detects provider change", () => {
+  const plan = buildPersonaImportPlan({
+    persona: createPersona({ provider: "anthropic" }),
+    preview: createPreview({ provider: "databricks" }),
+  });
+
+  assert.equal(plan.fields.length, 1);
+  assert.equal(plan.fields[0]?.field, "provider");
+  assert.equal(plan.fields[0]?.label, "LLM provider");
+  assert.equal(plan.fields[0]?.existingValue, "anthropic");
+  assert.equal(plan.fields[0]?.importedValue, "databricks");
+});
+
 test("buildPersonaImportPlan detects name pool change", () => {
   const plan = buildPersonaImportPlan({
     persona: createPersona({ namePool: ["Birch", "Compass"] }),
@@ -135,17 +150,24 @@ test("buildPersonaImportPlan detects multiple field changes", () => {
       displayName: "Alice",
       systemPrompt: "Old prompt",
       model: "gpt-4o",
+      provider: "openai",
     }),
     preview: createPreview({
       displayName: "Alicia",
       systemPrompt: "New prompt",
       model: "claude-sonnet-4-20250514",
+      provider: "anthropic",
     }),
   });
 
-  assert.equal(plan.fields.length, 3);
+  assert.equal(plan.fields.length, 4);
   const fieldNames = plan.fields.map((f) => f.field);
-  assert.deepEqual(fieldNames, ["displayName", "systemPrompt", "model"]);
+  assert.deepEqual(fieldNames, [
+    "displayName",
+    "systemPrompt",
+    "model",
+    "provider",
+  ]);
 });
 
 test("hasAnyPersonaImportChanges returns false for empty plan", () => {
diff --git a/desktop/src/features/agents/ui/personaImportPlan.ts b/desktop/src/features/agents/ui/personaImportPlan.ts
@@ -177,6 +177,18 @@ export function buildPersonaImportPlan({
     });
   }
 
+  const existingProvider = normalizeOptionalText(persona.provider);
+  const importedProvider = normalizeOptionalText(preview.provider);
+  if (existingProvider !== importedProvider) {
+    fields.push({
+      field: "provider",
+      label: "LLM provider",
+      existingValue: existingProvider,
+      importedValue: importedProvider,
+      ...singleLineChanges(existingProvider, importedProvider),
+    });
+  }
+
   const existingNamePool = namePoolToString(persona.namePool);
   const importedNamePool = namePoolToString(preview.namePool);
   if (existingNamePool !== importedNamePool) {
diff --git a/desktop/src/features/agents/ui/personaImportUpdateInput.test.mjs b/desktop/src/features/agents/ui/personaImportUpdateInput.test.mjs
@@ -0,0 +1,60 @@
+import assert from "node:assert/strict";
+import test from "node:test";
+
+import { buildPersonaImportUpdateInput } from "./personaImportUpdateInput.ts";
+
+function createPersona(overrides = {}) {
+  return {
+    id: "persona-1",
+    displayName: "Alice",
+    avatarUrl: null,
+    systemPrompt: "Be helpful.",
+    runtime: "goose",
+    model: "claude-sonnet-4",
+    provider: "anthropic",
+    namePool: [],
+    isBuiltIn: false,
+    isActive: true,
+    envVars: {},
+    createdAt: "2026-01-01T00:00:00Z",
+    updatedAt: "2026-01-01T00:00:00Z",
+    ...overrides,
+  };
+}
+
+function createPreview(overrides = {}) {
+  return {
+    displayName: "Alice",
+    systemPrompt: "Be helpful.",
+    avatarDataUrl: null,
+    avatarRef: null,
+    runtime: "goose",
+    model: "gpt-5",
+    provider: "databricks",
+    namePool: [],
+    sourceFile: "alice.md",
+    ...overrides,
+  };
+}
+
+test("buildPersonaImportUpdateInput applies selected model and provider updates", () => {
+  const input = buildPersonaImportUpdateInput({
+    existing: createPersona(),
+    preview: createPreview(),
+    selectedFields: ["model", "provider"],
+  });
+
+  assert.equal(input.model, "gpt-5");
+  assert.equal(input.provider, "databricks");
+});
+
+test("buildPersonaImportUpdateInput preserves provider when provider is not selected", () => {
+  const input = buildPersonaImportUpdateInput({
+    existing: createPersona(),
+    preview: createPreview(),
+    selectedFields: ["model"],
+  });
+
+  assert.equal(input.model, "gpt-5");
+  assert.equal(input.provider, "anthropic");
+});
diff --git a/desktop/src/features/agents/ui/personaImportUpdateInput.ts b/desktop/src/features/agents/ui/personaImportUpdateInput.ts
@@ -0,0 +1,47 @@
+import type { ParsedPersonaPreview } from "@/shared/api/tauriPersonas";
+import type { AgentPersona, UpdatePersonaInput } from "@/shared/api/types";
+import { resolveImportedPersonaAvatarUrl } from "@/shared/avatars/gooseAppAvatarRefs";
+
+type BuildPersonaImportUpdateInputArgs = {
+  existing: AgentPersona;
+  preview: ParsedPersonaPreview;
+  selectedFields: Iterable<string>;
+};
+
+export function buildPersonaImportUpdateInput({
+  existing,
+  preview,
+  selectedFields,
+}: BuildPersonaImportUpdateInputArgs): UpdatePersonaInput {
+  const selectedFieldSet = new Set(selectedFields);
+  const previewAvatarUrl = resolveImportedPersonaAvatarUrl(preview);
+
+  return {
+    id: existing.id,
+    displayName: selectedFieldSet.has("displayName")
+      ? preview.displayName
+      : existing.displayName,
+    systemPrompt: selectedFieldSet.has("systemPrompt")
+      ? preview.systemPrompt
+      : existing.systemPrompt,
+    avatarUrl: selectedFieldSet.has("avatarUrl")
+      ? (previewAvatarUrl ?? undefined)
+      : (existing.avatarUrl ?? undefined),
+    runtime: selectedFieldSet.has("runtime")
+      ? (preview.runtime ?? undefined)
+      : (existing.runtime ?? undefined),
+    model: selectedFieldSet.has("model")
+      ? (preview.model ?? undefined)
+      : (existing.model ?? undefined),
+    provider: selectedFieldSet.has("provider")
+      ? (preview.provider ?? undefined)
+      : (existing.provider ?? undefined),
+    namePool: selectedFieldSet.has("namePool")
+      ? preview.namePool.length > 0
+        ? preview.namePool
+        : undefined
+      : existing.namePool.length > 0
+        ? [...existing.namePool]
+        : undefined,
+  };
+}
diff --git a/desktop/src/features/agents/ui/usePersonaImportActions.ts b/desktop/src/features/agents/ui/usePersonaImportActions.ts
@@ -7,9 +7,9 @@ import {
   updatePersona as updatePersonaApi,
   type ParsedPersonaPreview,
 } from "@/shared/api/tauriPersonas";
-import { resolveImportedPersonaAvatarUrl } from "@/shared/avatars/gooseAppAvatarRefs";
-import type { AgentPersona, UpdatePersonaInput } from "@/shared/api/types";
+import type { AgentPersona } from "@/shared/api/types";
 import { buildPersonaImportPlan } from "./personaImportPlan";
+import { buildPersonaImportUpdateInput } from "./personaImportUpdateInput";
 import {
   editPersonaDialogState,
   type PersonaDialogState,
@@ -106,42 +106,20 @@ export function usePersonaImportActions(
       preview: personaImportTargetPreview.preview,
     });
 
-    const selectedFieldSet = new Set(selectedFields);
     const preview = personaImportTargetPreview.preview;
     const existing = personaImportTarget;
-    const previewAvatarUrl = resolveImportedPersonaAvatarUrl(preview);
 
     try {
-      const updateInput: UpdatePersonaInput = {
-        id: existing.id,
-        displayName: selectedFieldSet.has("displayName")
-          ? preview.displayName
-          : existing.displayName,
-        systemPrompt: selectedFieldSet.has("systemPrompt")
-          ? preview.systemPrompt
-          : existing.systemPrompt,
-        avatarUrl: selectedFieldSet.has("avatarUrl")
-          ? (previewAvatarUrl ?? undefined)
-          : (existing.avatarUrl ?? undefined),
-        runtime: selectedFieldSet.has("runtime")
-          ? (preview.runtime ?? undefined)
-          : (existing.runtime ?? undefined),
-        model: selectedFieldSet.has("model")
-          ? (preview.model ?? undefined)
-          : (existing.model ?? undefined),
-        namePool: selectedFieldSet.has("namePool")
-          ? preview.namePool.length > 0
-            ? preview.namePool
-            : undefined
-          : existing.namePool.length > 0
-            ? [...existing.namePool]
-            : undefined,
-      };
+      const updateInput = buildPersonaImportUpdateInput({
+        existing,
+        preview,
+        selectedFields,
+      });
 
       await updatePersonaApi(updateInput);
 
       const updatedFieldCount = plan.fields.filter((field) =>
-        selectedFieldSet.has(field.field),
+        selectedFields.includes(field.field),
       ).length;
 
       feedback.setPersonaNoticeMessage(
diff --git a/desktop/src/shared/avatars/gooseAppAvatarRefs.test.mjs b/desktop/src/shared/avatars/gooseAppAvatarRefs.test.mjs
@@ -13,9 +13,15 @@ test("toGooseAppAvatarRef canonicalizes app-avatar refs", () => {
   );
 });
 
-test("toGooseAppAvatarRef detects Goose avatar ids in paths", () => {
+test("toGooseAppAvatarRef ignores Goose-looking paths by default", () => {
+  assert.equal(toGooseAppAvatarRef("./avatars/pollies_2.png"), null);
+});
+
+test("toGooseAppAvatarRef detects Goose avatar ids in paths during import", () => {
   assert.equal(
-    toGooseAppAvatarRef("./avatars/pollies_2.png"),
+    toGooseAppAvatarRef("./avatars/pollies_2.png", {
+      allowFilenameFallback: true,
+    }),
     "app-avatar:pollies-2",
   );
 });
@@ -40,6 +46,16 @@ test("resolveImportedPersonaAvatarUrl preserves ordinary image URLs", () => {
   );
 });
 
+test("resolveImportedPersonaAvatarUrl does not rewrite Goose-looking remote URLs", () => {
+  assert.equal(
+    resolveImportedPersonaAvatarUrl({
+      avatarDataUrl: "https://cdn.example.com/avatars/pollies_2.png",
+      avatarRef: null,
+    }),
+    "https://cdn.example.com/avatars/pollies_2.png",
+  );
+});
+
 test("resolveImportedPersonaAvatarUrl preserves URL avatar refs", () => {
   assert.equal(
     resolveImportedPersonaAvatarUrl({
@@ -49,3 +65,13 @@ test("resolveImportedPersonaAvatarUrl preserves URL avatar refs", () => {
     "https://example.com/persona-avatar.png",
   );
 });
+
+test("resolveImportedPersonaAvatarUrl preserves Goose-looking URL avatar refs", () => {
+  assert.equal(
+    resolveImportedPersonaAvatarUrl({
+      avatarDataUrl: null,
+      avatarRef: " https://cdn.example.com/avatars/pollies_2.png ",
+    }),
+    "https://cdn.example.com/avatars/pollies_2.png",
+  );
+});
diff --git a/desktop/src/shared/avatars/gooseAppAvatarRefs.ts b/desktop/src/shared/avatars/gooseAppAvatarRefs.ts
@@ -3,6 +3,10 @@ export const GOOSE_APP_AVATAR_REF_PREFIX = "app-avatar:" as const;
 const APP_AVATAR_ID_PATTERN = /^[a-z0-9][a-z0-9_-]{0,63}$/;
 const GOOSE_COLLECTION_ID_PATTERN = /^(fuzzies|gloopies|pollies)[-_](\d+)$/;
 
+type ParseGooseAppAvatarOptions = {
+  allowFilenameFallback?: boolean;
+};
+
 function cleanAvatarCandidate(value: string): string {
   const basename = value
     .trim()
@@ -15,6 +19,7 @@ function cleanAvatarCandidate(value: string): string {
 
 export function parseGooseAppAvatarId(
   value: string | null | undefined,
+  options: ParseGooseAppAvatarOptions = {},
 ): string | null {
   const trimmed = value?.trim();
   if (!trimmed) {
@@ -28,19 +33,22 @@ export function parseGooseAppAvatarId(
     return APP_AVATAR_ID_PATTERN.test(id) ? id : null;
   }
 
-  const candidate = cleanAvatarCandidate(trimmed);
-  const collectionMatch = GOOSE_COLLECTION_ID_PATTERN.exec(candidate);
-  if (collectionMatch) {
-    return `${collectionMatch[1]}-${collectionMatch[2]}`;
+  if (options.allowFilenameFallback) {
+    const candidate = cleanAvatarCandidate(trimmed);
+    const collectionMatch = GOOSE_COLLECTION_ID_PATTERN.exec(candidate);
+    if (collectionMatch) {
+      return `${collectionMatch[1]}-${collectionMatch[2]}`;
+    }
   }
 
   return null;
 }
 
 export function toGooseAppAvatarRef(
   value: string | null | undefined,
+  options: ParseGooseAppAvatarOptions = {},
 ): string | null {
-  const id = parseGooseAppAvatarId(value);
+  const id = parseGooseAppAvatarId(value, options);
   return id ? `${GOOSE_APP_AVATAR_REF_PREFIX}${id}` : null;
 }
 
@@ -59,8 +67,15 @@ export function resolveImportedPersonaAvatarUrl({
   avatarDataUrl?: string | null;
   avatarRef?: string | null;
 }): string | null {
+  const trimmedAvatarRef = avatarRef?.trim();
+  const avatarRefFileFallback =
+    trimmedAvatarRef && !isPersistableAvatarUrl(trimmedAvatarRef)
+      ? toGooseAppAvatarRef(trimmedAvatarRef, { allowFilenameFallback: true })
+      : null;
   const gooseRef =
-    toGooseAppAvatarRef(avatarRef) ?? toGooseAppAvatarRef(avatarDataUrl);
+    toGooseAppAvatarRef(avatarRef) ??
+    avatarRefFileFallback ??
+    toGooseAppAvatarRef(avatarDataUrl);
   if (gooseRef) {
     return gooseRef;
   }
