From 71efb2bb2399c53348fb9ca7a89fc75e39a22256 Mon Sep 17 00:00:00 2001 From: klopez4212 Date: Mon, 22 Jun 2026 10:48:02 +0100 Subject: [PATCH] Address agent import review feedback --- .../agents/ui/personaImportPlan.test.mjs | 26 +++++++- .../features/agents/ui/personaImportPlan.ts | 12 ++++ .../ui/personaImportUpdateInput.test.mjs | 60 +++++++++++++++++++ .../agents/ui/personaImportUpdateInput.ts | 47 +++++++++++++++ .../agents/ui/usePersonaImportActions.ts | 38 +++--------- .../avatars/gooseAppAvatarRefs.test.mjs | 30 +++++++++- .../src/shared/avatars/gooseAppAvatarRefs.ts | 27 +++++++-- 7 files changed, 200 insertions(+), 40 deletions(-) create mode 100644 desktop/src/features/agents/ui/personaImportUpdateInput.test.mjs create mode 100644 desktop/src/features/agents/ui/personaImportUpdateInput.ts diff --git a/desktop/src/features/agents/ui/personaImportPlan.test.mjs b/desktop/src/features/agents/ui/personaImportPlan.test.mjs index 02c04ae0b..96ccb0685 100644 --- 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 index fd01f167a..bbb682db1 100644 --- 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 new file mode 100644 index 000000000..bb1f5a2b5 --- /dev/null +++ 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 new file mode 100644 index 000000000..d38c7b37d --- /dev/null +++ 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; +}; + +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 index a6814aaa2..0f8e7c67a 100644 --- 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 index 9e4f04b94..fe1b10db0 100644 --- 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 index fa4486ac5..ab1f146c0 100644 --- 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,10 +33,12 @@ 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; @@ -39,8 +46,9 @@ export function parseGooseAppAvatarId( 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; }