From f0c5c33f181f6b701099f20c22511c55cbf62252 Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Sat, 15 Aug 2026 22:36:08 -0400 Subject: [PATCH 1/5] fix(desktop): stop the agents library hiding renamed agent instances MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two surfaces answered "which agents exist" with two hand-rolled identity keys, and they had drifted. @-mention autocomplete keys agents by pubkey (#5202). The Agents library grouped by `personaId` and then rendered ONE card per group via `pickProfileAgent`, so every instance past the first had no card at all. That is invisible while a persona's instances all share a name. It stops being invisible the moment an agent is renamed but keeps its builtin persona id — the owner's `managed-agents.json` has `builtin:fizz` holding two "Claude" and two "Fizz" instances, and `builtin:honey` holding two "Cody" and two "Honey". The library rendered exactly one card for each of those personas, labelled with the persona name and wired to whichever instance `pickProfileAgent` returned. Two of eleven agents were invisible and unmanageable. One identity definition, shared ------------------------------- `agents/lib/agentIdentity.ts` now owns the doctrine that used to live as a comment inside `agentAutocompleteEligibility.ts`: agentIdentityKey() pubkey — THE identity, used by autocomplete coalescing and by the library agentDisplayGroupKey() persona + folded name — presentation only: which agents may share ONE card. Never a substitute for identity; a display group keeps every member identity and callers must keep them all reachable. Autocomplete now imports `agentIdentityKey` instead of re-deriving it, so the two surfaces cannot answer this question differently again. Why not one card per instance ----------------------------- Exploding to a card per pubkey would have produced 18 agent cards from 11 agents, and it would have reverted a deliberate product decision: same- named instances of one persona already collapse onto the persona's card and stay reachable through that card's profile panel (pinned by the e2e "duplicate instances move from the agents gallery into the agent profile"). The autocomplete argument for never collapsing does not transfer — there, collapsing makes a pubkey unmentionable; here, the card lists every instance behind it. So the collapse stays, bounded by one rule: a card may only stand for instances whose label it truthfully shows. All-same-name persona group → one card, labelled with the persona name, exactly as before. Once the owner has renamed an instance, the persona name can no longer stand for all of them, so each surviving name gets its own card. The owner's data goes from 9 cards hiding 9 agents to 11 cards hiding none. Opening a split card must open that instance, so `pickCanonicalProfileAgent` canonicalises within the requested instance's display group rather than across the whole persona. Same-named instances still collapse onto one profile target; a renamed one opens itself instead of silently redirecting to its persona sibling. Tests ----- `unifiedAgentGroups.ts` had no test file. It has one now, plus `agentIdentity.test.mjs`, covering: a renamed instance gets a card; no identity is dropped by persona grouping; the library and autocomplete agree on the identity set; same-name instances still share one card; persona actions stay on exactly one card per persona. No agent records are merged, renamed, or deleted — deletes propagate cross-device as kind:5 tombstones while the nsec does not, so a merge would strand agents on other machines. Co-authored-by: Michael Feth Signed-off-by: Michael Feth --- .../lib/agentAutocompleteEligibility.ts | 20 +- .../agents/lib/agentIdentity.test.mjs | 74 +++++++ .../src/features/agents/lib/agentIdentity.ts | 83 ++++++++ .../agents/lib/pickProfileAgent.test.mjs | 125 ++++++++++- .../features/agents/lib/pickProfileAgent.ts | 46 +++- .../agents/ui/UnifiedAgentsSection.tsx | 68 +++--- .../UnifiedAgentsSectionCardTarget.test.mjs | 12 +- .../agents/ui/unifiedAgentGroups.test.mjs | 200 ++++++++++++++++++ .../features/agents/ui/unifiedAgentGroups.ts | 104 ++++++++- .../lib/useCanonicalManagedAgentProfile.ts | 14 +- 10 files changed, 694 insertions(+), 52 deletions(-) create mode 100644 desktop/src/features/agents/lib/agentIdentity.test.mjs create mode 100644 desktop/src/features/agents/lib/agentIdentity.ts diff --git a/desktop/src/features/agents/lib/agentAutocompleteEligibility.ts b/desktop/src/features/agents/lib/agentAutocompleteEligibility.ts index a4b235fa04c..c3f266b7061 100644 --- a/desktop/src/features/agents/lib/agentAutocompleteEligibility.ts +++ b/desktop/src/features/agents/lib/agentAutocompleteEligibility.ts @@ -1,3 +1,4 @@ +import { agentIdentityKey } from "@/features/agents/lib/agentIdentity"; import type { Channel, RelayAgent } from "@/shared/api/types"; import { normalizePubkey } from "@/shared/lib/pubkey"; @@ -285,16 +286,13 @@ type AgentAutocompleteCandidate = { personaId?: string | null; }; -function agentIdentityKey(candidate: T) { - if (candidate.isAgent !== true || !candidate.pubkey) { - return null; - } - - // Pubkeys—not persona metadata or a display name—are agent identities. - // A persona may be installed more than once, and an owner may intentionally - // create multiple same-named agents. Collapsing either case makes one agent - // impossible to choose from autocomplete. - return `pubkey:${normalizePubkey(candidate.pubkey)}`; +function agentAutocompleteIdentityKey( + candidate: T, +) { + // Only agents coalesce; two humans may legitimately share every other field. + // The identity itself comes from `agentIdentityKey` so this surface and the + // Agents library cannot drift into two different answers for "same agent?". + return candidate.isAgent === true ? agentIdentityKey(candidate) : null; } function agentCandidateRank( @@ -369,7 +367,7 @@ export function coalesceAgentAutocompleteCandidates< const indexesByKey = new Map(); for (const candidate of candidates) { - const key = agentIdentityKey(candidate); + const key = agentAutocompleteIdentityKey(candidate); if (!key) { output.push(candidate); continue; diff --git a/desktop/src/features/agents/lib/agentIdentity.test.mjs b/desktop/src/features/agents/lib/agentIdentity.test.mjs new file mode 100644 index 00000000000..47b67b356fc --- /dev/null +++ b/desktop/src/features/agents/lib/agentIdentity.test.mjs @@ -0,0 +1,74 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + agentDisplayGroupKey, + agentIdentityKey, + groupAgentsForDisplay, +} from "./agentIdentity.ts"; + +const PUBKEY_A = "a".repeat(64); +const PUBKEY_B = "b".repeat(64); + +test("agent identity is the pubkey, never persona metadata or a name", () => { + const left = { + pubkey: PUBKEY_A, + name: "Bumble", + personaId: "builtin:bumble", + }; + const right = { + pubkey: PUBKEY_B, + name: "Bumble", + personaId: "builtin:bumble", + }; + + assert.notEqual(agentIdentityKey(left), agentIdentityKey(right)); + assert.equal( + agentIdentityKey({ pubkey: ` ${PUBKEY_A.toUpperCase()} ` }), + agentIdentityKey({ pubkey: PUBKEY_A, personaId: "something-else" }), + ); + assert.equal(agentIdentityKey({ pubkey: null }), null); + assert.equal(agentIdentityKey({}), null); +}); + +test("the display group key separates renamed instances of one persona", () => { + const claude = { + pubkey: PUBKEY_A, + name: "Claude", + personaId: "builtin:fizz", + }; + const fizz = { pubkey: PUBKEY_B, name: "Fizz", personaId: "builtin:fizz" }; + + assert.notEqual(agentDisplayGroupKey(claude), agentDisplayGroupKey(fizz)); + assert.equal( + agentDisplayGroupKey(claude), + agentDisplayGroupKey({ ...claude, pubkey: PUBKEY_B, name: " claude " }), + ); + assert.notEqual( + agentDisplayGroupKey(claude), + agentDisplayGroupKey({ ...claude, personaId: "builtin:honey" }), + ); +}); + +test("display grouping keeps every distinct identity and drops repeats", () => { + const agents = [ + { pubkey: PUBKEY_A, name: "Claude", personaId: "builtin:fizz" }, + { pubkey: PUBKEY_B, name: "Fizz", personaId: "builtin:fizz" }, + { pubkey: PUBKEY_A, name: "Claude", personaId: "builtin:fizz" }, + ]; + + const groups = groupAgentsForDisplay(agents); + + assert.deepEqual( + groups.map((group) => group.name), + ["Claude", "Fizz"], + ); + assert.deepEqual( + new Set(groups.flatMap((group) => group.agents).map(agentIdentityKey)), + new Set([ + agentIdentityKey({ pubkey: PUBKEY_A }), + agentIdentityKey(agents[1]), + ]), + ); + assert.equal(groups[0].agents.length, 1); +}); diff --git a/desktop/src/features/agents/lib/agentIdentity.ts b/desktop/src/features/agents/lib/agentIdentity.ts new file mode 100644 index 00000000000..45b58b14ffc --- /dev/null +++ b/desktop/src/features/agents/lib/agentIdentity.ts @@ -0,0 +1,83 @@ +import { normalizePubkey } from "@/shared/lib/pubkey"; + +/** + * THE definition of agent identity, shared by every surface that answers + * "which agents exist" (@-mention autocomplete, the Agents library, the + * profile panel). Two surfaces that hand-roll this drift apart, and the drift + * is invisible until an agent becomes unreachable on one of them. + * + * Pubkeys—not persona metadata or a display name—are agent identities. A + * persona may be installed more than once, and an owner may intentionally + * create multiple same-named agents. Collapsing either case makes one agent + * impossible to choose from autocomplete, and impossible to manage from the + * Agents library. + */ +export type AgentIdentityInput = { pubkey?: string | null }; + +export function agentIdentityKey(candidate: AgentIdentityInput): string | null { + const pubkey = candidate.pubkey?.trim(); + return pubkey ? `pubkey:${normalizePubkey(pubkey)}` : null; +} + +export type AgentDisplayInput = AgentIdentityInput & { + name?: string | null; + personaId?: string | null; +}; + +/** + * Presentation-only key: which agents may legitimately share ONE card in the + * Agents library. This is NOT an identity — it is a statement about what a + * single label can truthfully stand for. Instances of one persona that all + * carry the same name are interchangeable on a card (the card's profile panel + * lists every instance behind it); an instance the owner renamed is not, and + * must get a card of its own or it disappears from the library. + * + * Every caller must keep the full identity list of a display group reachable — + * grouping may never drop an `agentIdentityKey`. + */ +export function agentDisplayGroupKey(agent: AgentDisplayInput): string { + const personaId = agent.personaId?.trim() ?? ""; + const name = agent.name?.trim().toLowerCase() ?? ""; + return `persona:${personaId}|name:${name}`; +} + +export type AgentDisplayGroup = { + key: string; + /** Trimmed display name shared by every member, empty when unnamed. */ + name: string; + agents: T[]; +}; + +/** + * Split agents into display groups, in first-seen order, dropping repeated + * records of the same identity (the same pubkey read from two sources) but + * never dropping a distinct identity. + */ +export function groupAgentsForDisplay( + agents: readonly T[], +): AgentDisplayGroup[] { + const groups: AgentDisplayGroup[] = []; + const groupsByKey = new Map>(); + const seenIdentities = new Set(); + + for (const agent of agents) { + const identity = agentIdentityKey(agent); + if (identity) { + if (seenIdentities.has(identity)) continue; + seenIdentities.add(identity); + } + + const key = agentDisplayGroupKey(agent); + const existing = groupsByKey.get(key); + if (existing) { + existing.agents.push(agent); + continue; + } + + const group = { key, name: agent.name?.trim() ?? "", agents: [agent] }; + groupsByKey.set(key, group); + groups.push(group); + } + + return groups; +} diff --git a/desktop/src/features/agents/lib/pickProfileAgent.test.mjs b/desktop/src/features/agents/lib/pickProfileAgent.test.mjs index 710b5fc4be8..39da6937d5c 100644 --- a/desktop/src/features/agents/lib/pickProfileAgent.test.mjs +++ b/desktop/src/features/agents/lib/pickProfileAgent.test.mjs @@ -2,6 +2,7 @@ import assert from "node:assert/strict"; import test from "node:test"; import { + pickCanonicalProfileAgent, pickDirectProfileAgent, pickProfileAgent, } from "./pickProfileAgent.ts"; @@ -69,6 +70,98 @@ test("a fail-open predicate keeps every instance eligible while loading", () => assert.equal(pickProfileAgent([stopped, running], NONE_ARCHIVED), running); }); +test("opening a renamed instance opens that instance, not the persona's", () => { + const claude = { + name: "Claude", + personaId: "builtin:fizz", + pubkey: "a".repeat(64), + status: "running", + }; + const fizz = { + name: "Fizz", + personaId: "builtin:fizz", + pubkey: "b".repeat(64), + status: "stopped", + }; + const instances = [claude, fizz]; + + assert.equal( + pickCanonicalProfileAgent(instances, fizz, NONE_ARCHIVED), + fizz, + ); + assert.equal( + pickCanonicalProfileAgent(instances, claude, NONE_ARCHIVED), + claude, + ); +}); + +test("same-named instances still canonicalise onto one profile target", () => { + const stopped = { + name: "Bumble", + personaId: "builtin:bumble", + pubkey: "a".repeat(64), + status: "stopped", + }; + const running = { + name: "Bumble", + personaId: "builtin:bumble", + pubkey: "b".repeat(64), + status: "running", + }; + const instances = [stopped, running]; + + assert.equal( + pickCanonicalProfileAgent(instances, stopped, NONE_ARCHIVED), + running, + ); + assert.equal( + pickCanonicalProfileAgent(instances, undefined, NONE_ARCHIVED), + running, + ); +}); + +test("a fully archived display group falls back to the persona's live instance", () => { + // Scoping runs before archive filtering, so a group whose every instance is + // archived must not resolve to nothing where the unscoped selector would + // still have found a live sibling. + const archivedFizz = { + name: "Fizz", + personaId: "builtin:fizz", + pubkey: "a".repeat(64), + status: "stopped", + }; + const liveClaude = { + name: "Claude", + personaId: "builtin:fizz", + pubkey: "b".repeat(64), + status: "running", + }; + const isArchived = (pubkey) => pubkey === archivedFizz.pubkey; + + assert.equal( + pickCanonicalProfileAgent( + [archivedFizz, liveClaude], + archivedFizz, + isArchived, + ), + liveClaude, + ); +}); + +test("a requested instance survives when every candidate is archived", () => { + const requested = { + name: "Fizz", + personaId: "builtin:fizz", + pubkey: "a".repeat(64), + status: "stopped", + }; + + assert.equal( + pickCanonicalProfileAgent([requested], requested, () => true), + requested, + ); +}); + test("a direct-opened active instance is never redirected to a sibling", () => { // "Alpha Sibling" sorts before "Tyler Agent"; without the direct guard an // access edit on Tyler would target the sibling. @@ -90,13 +183,18 @@ test("a direct-opened active instance is never redirected to a sibling", () => { }); test("a direct-opened inactive instance redirects to the active sibling", () => { + // The avatar on an old message points at a retired instance. Both carry the + // label the Agents library shows, so the retired one is not a card of its + // own and redirecting to the live instance matches what the library renders. const historical = { - name: "Earlier Parity Agent", + name: "Parity Agent", + personaId: "builtin:parity", pubkey: "a".repeat(64), status: "stopped", }; const current = { - name: "Current Parity Agent", + name: "Parity Agent", + personaId: "builtin:parity", pubkey: "b".repeat(64), status: "running", }; @@ -107,6 +205,29 @@ test("a direct-opened inactive instance redirects to the active sibling", () => ); }); +test("a direct-opened inactive instance never redirects across a rename", () => { + // The owner renamed one instance, so the library shows two cards. Redirecting + // the retired card to the differently named live instance would reopen the + // bug where a card refuses to open the agent it names. + const renamed = { + name: "Fizz", + personaId: "builtin:fizz", + pubkey: "a".repeat(64), + status: "stopped", + }; + const current = { + name: "Claude", + personaId: "builtin:fizz", + pubkey: "b".repeat(64), + status: "running", + }; + + assert.equal( + pickDirectProfileAgent(renamed, [renamed, current], NONE_ARCHIVED), + renamed, + ); +}); + test("a direct-opened inactive instance with no active sibling stays put", () => { const clicked = { name: "Only Instance", diff --git a/desktop/src/features/agents/lib/pickProfileAgent.ts b/desktop/src/features/agents/lib/pickProfileAgent.ts index dc2437c86ea..fe63a8fec73 100644 --- a/desktop/src/features/agents/lib/pickProfileAgent.ts +++ b/desktop/src/features/agents/lib/pickProfileAgent.ts @@ -1,3 +1,4 @@ +import { agentDisplayGroupKey } from "@/features/agents/lib/agentIdentity"; import { isManagedAgentActive } from "@/features/agents/lib/managedAgentControlActions"; import type { ManagedAgent } from "@/shared/api/types"; @@ -29,6 +30,39 @@ export function pickProfileAgent( })[0]; } +/** + * Pick the instance a profile request should actually land on. + * + * Collapsing onto the persona's representative instance is only correct while + * the instances are interchangeable presentations of that persona. Once the + * owner has renamed one, opening its card must open *it* — otherwise the + * Agents library shows a card the profile panel refuses to open. + * + * Scoping happens before archive filtering rather than after: the display group + * decides *which* siblings are candidates, and `pickProfileAgent` then drops the + * archived ones. A group whose every instance is archived falls back to the + * persona-wide list, so this never resolves to nothing where the unscoped + * selector would have found a live instance. + */ +export function pickCanonicalProfileAgent( + personaInstances: readonly ManagedAgent[], + requested: ManagedAgent | undefined, + isArchived: (pubkey: string) => boolean, +) { + if (!requested) return pickProfileAgent(personaInstances, isArchived); + + const key = agentDisplayGroupKey(requested); + const sameLabel = personaInstances.filter( + (instance) => agentDisplayGroupKey(instance) === key, + ); + + return ( + pickProfileAgent(sameLabel, isArchived) ?? + pickProfileAgent(personaInstances, isArchived) ?? + requested + ); +} + /** * Resolve which instance a profile panel opened for `directAgent` should * show, given every instance of the same persona. @@ -40,6 +74,12 @@ export function pickProfileAgent( * an old message from a retired instance), redirect to the active one so the * panel matches the Agents library. The `isArchived` predicate keeps that * redirect from ever landing on an archived sibling. + * + * The redirect resolves through `pickCanonicalProfileAgent`, so it also stays + * inside the clicked instance's display group: a retired instance the owner + * renamed falls back to an active sibling carrying *that* name, never to a + * differently named one. Redirecting across names would reopen the bug that + * made a renamed agent unreachable from its own card. */ export function pickDirectProfileAgent( directAgent: ManagedAgent, @@ -47,6 +87,10 @@ export function pickDirectProfileAgent( isArchived: (pubkey: string) => boolean, ) { if (isManagedAgentActive(directAgent)) return directAgent; - const canonical = pickProfileAgent(personaInstances, isArchived); + const canonical = pickCanonicalProfileAgent( + personaInstances, + directAgent, + isArchived, + ); return canonical && isManagedAgentActive(canonical) ? canonical : directAgent; } diff --git a/desktop/src/features/agents/ui/UnifiedAgentsSection.tsx b/desktop/src/features/agents/ui/UnifiedAgentsSection.tsx index d0ff2e2738a..5ffb8ed6a19 100644 --- a/desktop/src/features/agents/ui/UnifiedAgentsSection.tsx +++ b/desktop/src/features/agents/ui/UnifiedAgentsSection.tsx @@ -8,7 +8,6 @@ import { import { resolveAgentCardModelLabel } from "@/features/agents/lib/agentCardModelLabel"; import { friendlyAgentLastError } from "@/features/agents/lib/friendlyAgentLastError"; import { isManagedAgentActive } from "@/features/agents/lib/managedAgentControlActions"; -import { pickProfileAgent } from "@/features/agents/lib/pickProfileAgent"; import { useIsArchivedPredicate } from "@/features/identity-archive/hooks"; import { useUserProfileQuery } from "@/features/profile/hooks"; import type { AgentPersona, ManagedAgent } from "@/shared/api/types"; @@ -130,42 +129,51 @@ export function UnifiedAgentsSection(props: UnifiedAgentsSectionProps) { disabled={isPersonasPending} onClick={onOpenCatalog} /> - {groups.map((group) => { - const profileAgent = pickProfileAgent(group.agents, isArchived); - return ( + {groups.flatMap((group) => + group.cards.map((card) => ( ( - - onSharePersona(persona, linkedAgent, effectiveAvatarUrl) - } - /> - )} - agent={profileAgent} + actions={ + card.ownsPersonaActions + ? (effectiveAvatarUrl, isEffectiveAvatarLoading) => ( + + onSharePersona( + persona, + linkedAgent, + effectiveAvatarUrl, + ) + } + /> + ) + : undefined + } + agent={card.agent} defaultModel={defaultModel} - key={group.persona.id} - persona={group.persona} + key={card.key} + label={card.label} + persona={card.persona} restartingAgentPubkey={restartingAgentPubkey} startingAgentPubkey={startingAgentPubkey} startingPersonaIds={startingPersonaIds} + testId={`persona-agent-row-${card.key}`} onOpenAgentProfile={onOpenAgentProfile} onOpenPersonaProfile={onOpenPersonaProfile} onRestartAgent={onRestartAgent} onStartAgent={onStartAgent} onStartPersona={onStartPersona} /> - ); - })} + )), + )} {unknown.length > 0 ? ( @@ -223,10 +231,12 @@ function AgentPersonaCard({ actions, agent, defaultModel, + label, persona, restartingAgentPubkey, startingAgentPubkey, startingPersonaIds, + testId, onOpenAgentProfile, onOpenPersonaProfile, onRestartAgent, @@ -239,10 +249,12 @@ function AgentPersonaCard({ ) => React.ReactNode; agent: ManagedAgent | undefined; defaultModel: string; + label: string; persona: AgentPersona; restartingAgentPubkey: string | null; startingAgentPubkey: string | null; startingPersonaIds: ReadonlySet; + testId: string; onOpenAgentProfile: ( pubkey: string, options?: ProfilePanelOpenOptions, @@ -252,7 +264,7 @@ function AgentPersonaCard({ onStartAgent: (pubkey: string) => void; onStartPersona: (persona: AgentPersona) => void; }) { - const title = persona.displayName; + const title = label; const modelLabel = resolveAgentCardModelLabel({ agent, personaModel: persona.model, @@ -310,7 +322,7 @@ function AgentPersonaCard({ ) } avatarUrl={avatarUrl} - dataTestId={`persona-agent-row-${persona.id}`} + dataTestId={testId} label={title} modelLabel={modelLabel} onClick={() => { diff --git a/desktop/src/features/agents/ui/UnifiedAgentsSectionCardTarget.test.mjs b/desktop/src/features/agents/ui/UnifiedAgentsSectionCardTarget.test.mjs index 690a921040e..9390fe96997 100644 --- a/desktop/src/features/agents/ui/UnifiedAgentsSectionCardTarget.test.mjs +++ b/desktop/src/features/agents/ui/UnifiedAgentsSectionCardTarget.test.mjs @@ -200,12 +200,14 @@ test("persona card main click records a persona target, never an explicit pubkey recordedPersona = persona; }; - // Archived sibling sorts first by name, so under fail-open pickProfileAgent - // selects it — the card displays the archived identity at click time. A - // durable pubkey target would strand the panel there after hydration. + // Both siblings carry the persona's name, so they stay on its single card. + // The archived one is first and ranks equal by name, so under fail-open + // pickProfileAgent selects it — the card displays the archived identity at + // click time. A durable pubkey target would strand the panel there after + // hydration. const agents = [ - agent({ pubkey: ARCHIVED_PK, name: "Archived Sibling" }), - agent({ pubkey: LIVE_PK, name: "Zed Sibling" }), + agent({ pubkey: ARCHIVED_PK, name: "Fizz Prime" }), + agent({ pubkey: LIVE_PK, name: "Fizz Prime" }), ]; await act(async () => { diff --git a/desktop/src/features/agents/ui/unifiedAgentGroups.test.mjs b/desktop/src/features/agents/ui/unifiedAgentGroups.test.mjs index b3ade7f229b..ca28be26d6a 100644 --- a/desktop/src/features/agents/ui/unifiedAgentGroups.test.mjs +++ b/desktop/src/features/agents/ui/unifiedAgentGroups.test.mjs @@ -1,6 +1,8 @@ import assert from "node:assert/strict"; import test from "node:test"; +import { coalesceAgentAutocompleteCandidates } from "../lib/agentAutocompleteEligibility.ts"; +import { agentIdentityKey } from "../lib/agentIdentity.ts"; import { buildUnifiedGroups } from "./unifiedAgentGroups.ts"; const NONE_ARCHIVED = () => false; @@ -19,6 +21,37 @@ function persona(overrides = {}) { return { id: "persona-1", displayName: "Persona", ...overrides }; } +/** Positional builders for the rename fixtures, where name is the subject. */ +function namedAgent(pubkey, name, personaId, status = "stopped") { + return { pubkey, name, personaId, status }; +} + +function namedPersona(id, displayName) { + return { id, displayName }; +} + +/** + * The owner's real shape: one builtin persona whose instances were renamed, so + * the group carries two distinct names across four distinct pubkeys. + */ +const FIZZ_PERSONA = namedPersona("builtin:fizz", "Fizz"); +const PUBKEYS = { + claudeOne: "1".repeat(64), + claudeTwo: "2".repeat(64), + fizzOne: "3".repeat(64), + fizzTwo: "4".repeat(64), +}; +const FIZZ_AGENTS = [ + namedAgent(PUBKEYS.claudeOne, "Claude", "builtin:fizz"), + namedAgent(PUBKEYS.claudeTwo, "Claude", "builtin:fizz"), + namedAgent(PUBKEYS.fizzOne, "Fizz", "builtin:fizz"), + namedAgent(PUBKEYS.fizzTwo, "Fizz", "builtin:fizz"), +]; + +function cardsOf(result) { + return result.groups.flatMap((group) => group.cards); +} + test("archived standalone custom agents are omitted while live peers remain", () => { const archived = agent({ pubkey: "a".repeat(64), personaId: null }); const live = agent({ pubkey: "b".repeat(64), personaId: null }); @@ -75,3 +108,170 @@ test("a fail-open predicate keeps every standalone agent discoverable", () => { assert.equal(ungrouped.length, 2); }); + +test("a renamed instance still gets a card in the agents library", () => { + const { groups } = buildUnifiedGroups( + [FIZZ_PERSONA], + FIZZ_AGENTS, + NONE_ARCHIVED, + ); + + assert.equal(groups.length, 1); + assert.deepEqual( + groups[0].cards.map((card) => card.label), + ["Claude", "Fizz"], + ); + assert.deepEqual( + groups[0].cards.map((card) => card.agent.pubkey), + [PUBKEYS.claudeOne, PUBKEYS.fizzOne], + ); +}); + +test("a fully archived name gets no card of its own", () => { + // Archiving every "Claude" leaves only "Fizz" as a clickable card — an + // archived identity must never become a library card in its own right. + const isArchived = (pubkey) => + pubkey === PUBKEYS.claudeOne || pubkey === PUBKEYS.claudeTwo; + + const { groups } = buildUnifiedGroups( + [FIZZ_PERSONA], + FIZZ_AGENTS, + isArchived, + ); + + assert.deepEqual( + groups[0].cards.map((card) => card.label), + ["Fizz"], + ); + assert.equal(groups[0].cards[0].ownsPersonaActions, true); +}); + +test("a split persona with every instance archived keeps one persona-only card", () => { + const { groups } = buildUnifiedGroups([FIZZ_PERSONA], FIZZ_AGENTS, () => true); + + assert.equal(groups[0].cards.length, 1); + assert.equal(groups[0].cards[0].key, FIZZ_PERSONA.id); + assert.equal(groups[0].cards[0].label, FIZZ_PERSONA.displayName); + assert.equal(groups[0].cards[0].agent, undefined); + assert.equal(groups[0].cards[0].ownsPersonaActions, true); +}); + +test("no managed agent is dropped by persona grouping", () => { + const { groups } = buildUnifiedGroups( + [FIZZ_PERSONA], + FIZZ_AGENTS, + NONE_ARCHIVED, + ); + const reachable = cardsOf({ groups }) + .flatMap((card) => card.agents) + .map(agentIdentityKey); + + assert.deepEqual( + new Set(reachable), + new Set(FIZZ_AGENTS.map(agentIdentityKey)), + ); + assert.equal(reachable.length, FIZZ_AGENTS.length); +}); + +test("the agents library and @-mention autocomplete agree on which agents exist", () => { + const records = [ + ...FIZZ_AGENTS, + namedAgent("5".repeat(64), "Solo", "custom:solo", "running"), + ]; + const { groups, ungrouped, unknown } = buildUnifiedGroups( + [FIZZ_PERSONA, namedPersona("custom:solo", "Solo")], + records, + NONE_ARCHIVED, + ); + + const libraryIdentities = new Set( + [ + ...cardsOf({ groups }).flatMap((card) => card.agents), + ...ungrouped, + ...unknown, + ].map(agentIdentityKey), + ); + const autocompleteIdentities = new Set( + coalesceAgentAutocompleteCandidates( + records.map((record) => ({ ...record, isAgent: true })), + { getLabel: (candidate) => candidate.name }, + ).map(agentIdentityKey), + ); + + assert.deepEqual(libraryIdentities, autocompleteIdentities); +}); + +test("same-named instances of one persona stay on the persona's single card", () => { + const duplicates = [ + namedAgent( + PUBKEYS.claudeOne, + "Duplicate Auditor", + "custom:duplicate", + "running", + ), + namedAgent(PUBKEYS.claudeTwo, "Duplicate Auditor", "custom:duplicate"), + ]; + + const { groups } = buildUnifiedGroups( + [namedPersona("custom:duplicate", "Duplicate Auditor")], + duplicates, + NONE_ARCHIVED, + ); + + assert.equal(groups[0].cards.length, 1); + assert.equal(groups[0].cards[0].key, "custom:duplicate"); + assert.equal(groups[0].cards[0].label, "Duplicate Auditor"); + assert.equal(groups[0].cards[0].agents.length, 2); + assert.equal(groups[0].cards[0].ownsPersonaActions, true); +}); + +test("persona-level actions live on exactly one card per persona", () => { + const { groups } = buildUnifiedGroups( + [FIZZ_PERSONA], + FIZZ_AGENTS, + NONE_ARCHIVED, + ); + + assert.equal( + groups[0].cards.filter((card) => card.ownsPersonaActions).length, + 1, + ); +}); + +test("card keys stay unique so cards cannot overwrite each other", () => { + const { groups } = buildUnifiedGroups( + [FIZZ_PERSONA], + FIZZ_AGENTS, + NONE_ARCHIVED, + ); + const keys = groups[0].cards.map((card) => card.key); + + assert.equal(new Set(keys).size, keys.length); +}); + +test("a persona with no instances keeps its single unchanged card", () => { + const { groups } = buildUnifiedGroups( + [namedPersona("custom:idle", "Idle")], + [], + NONE_ARCHIVED, + ); + + assert.equal(groups[0].cards.length, 1); + assert.equal(groups[0].cards[0].key, "custom:idle"); + assert.equal(groups[0].cards[0].label, "Idle"); + assert.equal(groups[0].cards[0].agent, undefined); +}); + +test("agents without a persona, and personas that vanished, are unchanged", () => { + const orphan = namedAgent("6".repeat(64), "Orphan", "custom:gone"); + const custom = namedAgent("7".repeat(64), "Custom", null); + + const { ungrouped, unknown } = buildUnifiedGroups( + [], + [orphan, custom], + NONE_ARCHIVED, + ); + + assert.deepEqual(ungrouped, [custom]); + assert.deepEqual(unknown, [orphan]); +}); diff --git a/desktop/src/features/agents/ui/unifiedAgentGroups.ts b/desktop/src/features/agents/ui/unifiedAgentGroups.ts index 2ddf34d8402..263c2338286 100644 --- a/desktop/src/features/agents/ui/unifiedAgentGroups.ts +++ b/desktop/src/features/agents/ui/unifiedAgentGroups.ts @@ -1,6 +1,101 @@ +import { groupAgentsForDisplay } from "@/features/agents/lib/agentIdentity"; +import { pickProfileAgent } from "@/features/agents/lib/pickProfileAgent"; import type { AgentPersona, ManagedAgent } from "@/shared/api/types"; -type PersonaGroup = { persona: AgentPersona; agents: ManagedAgent[] }; +/** + * One card in the Agents library. + * + * A card stands for every instance in `agents` and opens `agent` — never for + * an instance it does not list. Persona grouping decides which cards exist; + * it may never decide which agents exist (see `agentDisplayGroupKey`). + */ +export type UnifiedAgentCard = { + /** Stable card key, also used for the card's `data-testid`. */ + key: string; + label: string; + persona: AgentPersona; + /** Instance the card opens; `undefined` for a persona with no instances. */ + agent: ManagedAgent | undefined; + /** Every instance the card stands for. */ + agents: ManagedAgent[]; + /** Persona-level actions (edit/share/delete) live on exactly one card. */ + ownsPersonaActions: boolean; +}; + +export type PersonaGroup = { + persona: AgentPersona; + agents: ManagedAgent[]; + cards: UnifiedAgentCard[]; +}; + +/** + * Cards for one persona. + * + * All of a persona's instances carrying the same name stay collapsed onto the + * persona's single card — that is the established gallery behaviour, and the + * card's profile panel lists every instance behind it. Once the owner renames + * an instance, the persona's name can no longer truthfully stand for all of + * them, so each surviving name gets its own card. Without this, a rename made + * an agent vanish from the library entirely. + * + * A split card only exists for a name that still has a live instance, so an + * archived identity never becomes a clickable card of its own. When archiving + * leaves no live instance under any name, the persona collapses back to a + * single persona-only card rather than disappearing from the library. + */ +function buildPersonaCards( + persona: AgentPersona, + agents: ManagedAgent[], + isArchived: (pubkey: string) => boolean, +): UnifiedAgentCard[] { + const displayGroups = groupAgentsForDisplay(agents); + const personaOnlyCard = (members: ManagedAgent[]): UnifiedAgentCard => ({ + key: persona.id, + label: persona.displayName, + persona, + agent: pickProfileAgent(members, isArchived), + agents: members, + ownsPersonaActions: true, + }); + + if (displayGroups.length <= 1) { + return [personaOnlyCard(displayGroups[0]?.agents ?? [])]; + } + + const liveGroups = displayGroups + .map((group) => ({ + group, + primary: pickProfileAgent(group.agents, isArchived), + })) + .filter( + (entry): entry is { group: (typeof displayGroups)[number]; primary: ManagedAgent } => + entry.primary !== undefined, + ); + + if (liveGroups.length === 0) return [personaOnlyCard(agents)]; + + const personaPrimary = pickProfileAgent(agents, isArchived); + const cards = liveGroups.map(({ group, primary }) => ({ + key: `${persona.id}-${primary.pubkey}`, + label: group.name || persona.displayName, + persona, + agent: primary, + agents: group.agents, + ownsPersonaActions: personaPrimary + ? group.agents.includes(personaPrimary) + : false, + })); + + // Persona actions must exist on exactly one card. `personaPrimary` is live so + // its own group survived the filter, but instance de-duplication can drop the + // very record it points at; fall back to the first card rather than stranding + // edit/share/delete on none of them. + if (!cards.some((card) => card.ownsPersonaActions)) { + cards[0].ownsPersonaActions = true; + } + + return cards; +} /** * Group managed agents under their personas for the Agents library. @@ -34,7 +129,12 @@ export function buildUnifiedGroups( const matched = new Set(); const groups: PersonaGroup[] = personas.map((persona) => { matched.add(persona.id); - return { persona, agents: byPersonaId.get(persona.id) ?? [] }; + const personaAgents = byPersonaId.get(persona.id) ?? []; + return { + persona, + agents: personaAgents, + cards: buildPersonaCards(persona, personaAgents, isArchived), + }; }); const unknown: ManagedAgent[] = []; diff --git a/desktop/src/features/profile/lib/useCanonicalManagedAgentProfile.ts b/desktop/src/features/profile/lib/useCanonicalManagedAgentProfile.ts index 0393a795ce3..664c7e4211e 100644 --- a/desktop/src/features/profile/lib/useCanonicalManagedAgentProfile.ts +++ b/desktop/src/features/profile/lib/useCanonicalManagedAgentProfile.ts @@ -1,8 +1,8 @@ import * as React from "react"; import { + pickCanonicalProfileAgent, pickDirectProfileAgent, - pickProfileAgent, } from "@/features/agents/lib/pickProfileAgent"; import { useIsArchivedPredicate } from "@/features/identity-archive/hooks"; import { useUserProfileQuery } from "@/features/profile/hooks"; @@ -28,7 +28,9 @@ import { normalizePubkey } from "@/shared/lib/pubkey"; * opened active instance exact so an access edit targets it, only redirecting * an inactive click to a live sibling — see `pickDirectProfileAgent`. * - Otherwise persona-target and non-archived historical navigation resolve - * through the shared archive-aware selector: all instances archived yields + * through the shared archive-aware selector, scoped to the requested + * instance's display group so a renamed instance resolves to itself rather + * than to a differently named sibling: all candidates archived yields * `undefined` (persona-only mode), else the canonical live instance. */ export function resolveCanonicalManagedAgent(input: { @@ -60,7 +62,13 @@ export function resolveCanonicalManagedAgent(input: { isArchived, ); } - return pickProfileAgent(personaInstances, isArchived) ?? directManagedAgent; + return ( + pickCanonicalProfileAgent( + personaInstances, + directManagedAgent, + isArchived, + ) ?? directManagedAgent + ); } /** From e7728dbaa274dede1abbda2c14b6987315c8ae4f Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Sun, 16 Aug 2026 01:10:14 -0400 Subject: [PATCH 2/5] fix(desktop): give a split persona card a name and a stable identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the previous commit, applying an adversarial review of it. The first pass made renamed instances visible; these are the consequences it left undecided. - A split card now titles itself with the INSTANCE name and carries the persona name on a second line (`subtitle`, `text-2xs`), so a renamed instance is disambiguated without the persona becoming unfindable. Unsplit personas pass `personaLabel: null` and render byte-identically to before. - Persona-level destructive actions (edit / delete / share) were owned by `pickProfileAgent`, which is active-first — so the Delete-persona menu moved between cards when an agent started or stopped. `pickPersonaActionsIndex()` now picks the card whose folded name matches the persona, else index 0, which is status-independent. - The React key was `${persona.id}-${pickProfileAgent(...).pubkey}` and so moved with runtime status too. It is now `${persona.id}::${foldedName}`, folded through `foldAgentDisplayName()` in `agentIdentity.ts` rather than lowercased inline, so the card key and the group key cannot disagree. - `pickCanonicalProfileAgent` gains the two cases that genuinely lacked coverage: canonicalising within a display group holding both a stopped and a running member, and a requested instance absent from the persona list. Render-layer coverage, which was the review's substantive finding: the previous commit's unit tests all passed with `UnifiedAgentsSection.tsx` reverted, because the one-card-per-persona decision lived in JSX where nothing could reach it. The new `agents.spec.ts` case drives the real gallery with four instances under two names and asserts two cards, the persona name surviving as a second line, one actions menu on the correctly-named card, and each card opening its own instance. Verified as a genuine guard: reverting `unifiedAgentGroups.ts` + `UnifiedAgentsSection.tsx` to d8281b9 and rebuilding makes it fail. `profile.spec.ts` was rewritten as a parity test rather than deleted, and `agent-lifecycle-feedback.spec.ts` updated for the split-card labels. Verified: unit 4974/4975 (the one failure is a pre-existing timing flake in `useDocumentVisible.test.mjs`, which this branch does not touch, reproduced on base by stashing); Playwright smoke 1034 passed / 3 skipped / 0 failed; `agents.spec.ts` 37/37; `tsc --noEmit`, biome, check:px-text, check-file-sizes and check-pubkey-truncation all clean. Co-authored-by: Michael Feth Signed-off-by: Michael Feth --- .../src/features/agents/lib/agentIdentity.ts | 21 ++- .../agents/lib/pickProfileAgent.test.mjs | 66 ++++++++- .../features/agents/lib/pickProfileAgent.ts | 21 ++- .../features/agents/ui/AgentIdentityCard.tsx | 8 ++ .../agents/ui/UnifiedAgentsSection.tsx | 4 + .../agents/ui/unifiedAgentGroups.test.mjs | 131 +++++++++++++++++- .../features/agents/ui/unifiedAgentGroups.ts | 79 +++++++---- .../e2e/agent-lifecycle-feedback.spec.ts | 27 +++- desktop/tests/e2e/agents.spec.ts | 68 +++++++++ desktop/tests/e2e/profile.spec.ts | 36 +++-- 10 files changed, 401 insertions(+), 60 deletions(-) diff --git a/desktop/src/features/agents/lib/agentIdentity.ts b/desktop/src/features/agents/lib/agentIdentity.ts index 45b58b14ffc..62c54be9045 100644 --- a/desktop/src/features/agents/lib/agentIdentity.ts +++ b/desktop/src/features/agents/lib/agentIdentity.ts @@ -37,14 +37,24 @@ export type AgentDisplayInput = AgentIdentityInput & { */ export function agentDisplayGroupKey(agent: AgentDisplayInput): string { const personaId = agent.personaId?.trim() ?? ""; - const name = agent.name?.trim().toLowerCase() ?? ""; - return `persona:${personaId}|name:${name}`; + return `persona:${personaId}|name:${foldAgentDisplayName(agent.name)}`; +} + +/** + * The one place a display name is folded for comparison. Callers that need a + * name-scoped key of their own (React keys, `data-testid`s) must fold through + * this rather than lowercasing inline, or their key and the group's disagree. + */ +export function foldAgentDisplayName(name: string | null | undefined): string { + return name?.trim().toLowerCase() ?? ""; } export type AgentDisplayGroup = { key: string; /** Trimmed display name shared by every member, empty when unnamed. */ name: string; + /** `name` folded for comparison — the group's identity within a persona. */ + foldedName: string; agents: T[]; }; @@ -74,7 +84,12 @@ export function groupAgentsForDisplay( continue; } - const group = { key, name: agent.name?.trim() ?? "", agents: [agent] }; + const group = { + key, + name: agent.name?.trim() ?? "", + foldedName: foldAgentDisplayName(agent.name), + agents: [agent], + }; groupsByKey.set(key, group); groups.push(group); } diff --git a/desktop/src/features/agents/lib/pickProfileAgent.test.mjs b/desktop/src/features/agents/lib/pickProfileAgent.test.mjs index 39da6937d5c..6be35cc5c9d 100644 --- a/desktop/src/features/agents/lib/pickProfileAgent.test.mjs +++ b/desktop/src/features/agents/lib/pickProfileAgent.test.mjs @@ -85,10 +85,7 @@ test("opening a renamed instance opens that instance, not the persona's", () => }; const instances = [claude, fizz]; - assert.equal( - pickCanonicalProfileAgent(instances, fizz, NONE_ARCHIVED), - fizz, - ); + assert.equal(pickCanonicalProfileAgent(instances, fizz, NONE_ARCHIVED), fizz); assert.equal( pickCanonicalProfileAgent(instances, claude, NONE_ARCHIVED), claude, @@ -162,6 +159,67 @@ test("a requested instance survives when every candidate is archived", () => { ); }); +test("a renamed instance canonicalises within its own name, not the persona", () => { + // The message-avatar path: `builtin:fizz` holds two "Claude" and one "Fizz". + // Clicking an old "Claude" message must land on the running Claude, never on + // the persona-wide winner, or the profile panel and the library card that + // now exists for "Claude" disagree. + const claudeStopped = { + name: "Claude", + personaId: "builtin:fizz", + pubkey: "a".repeat(64), + status: "stopped", + }; + const claudeRunning = { + name: "Claude", + personaId: "builtin:fizz", + pubkey: "b".repeat(64), + status: "running", + }; + const fizzRunning = { + name: "Fizz", + personaId: "builtin:fizz", + pubkey: "c".repeat(64), + status: "running", + }; + const instances = [claudeStopped, claudeRunning, fizzRunning]; + + assert.equal( + pickCanonicalProfileAgent(instances, claudeStopped, NONE_ARCHIVED), + claudeRunning, + ); + assert.equal( + pickCanonicalProfileAgent(instances, fizzRunning, NONE_ARCHIVED), + fizzRunning, + ); +}); + +test("an instance missing from the persona list still resolves", () => { + // A historical agent read off an old message may no longer be in the + // persona's instance list; the request must not resolve to nothing. + const current = { + name: "Current", + personaId: "builtin:fizz", + pubkey: "a".repeat(64), + status: "running", + }; + const historical = { + name: "Retired", + personaId: "builtin:fizz", + pubkey: "b".repeat(64), + status: "stopped", + }; + + assert.equal( + pickCanonicalProfileAgent([current], historical, NONE_ARCHIVED), + current, + ); + assert.equal( + pickCanonicalProfileAgent([], historical, NONE_ARCHIVED), + historical, + ); +}); + test("a direct-opened active instance is never redirected to a sibling", () => { // "Alpha Sibling" sorts before "Tyler Agent"; without the direct guard an // access edit on Tyler would target the sibling. diff --git a/desktop/src/features/agents/lib/pickProfileAgent.ts b/desktop/src/features/agents/lib/pickProfileAgent.ts index fe63a8fec73..6cd0a716221 100644 --- a/desktop/src/features/agents/lib/pickProfileAgent.ts +++ b/desktop/src/features/agents/lib/pickProfileAgent.ts @@ -3,11 +3,13 @@ import { isManagedAgentActive } from "@/features/agents/lib/managedAgentControlA import type { ManagedAgent } from "@/shared/api/types"; /** - * Pick the instance that represents a persona throughout the UI. + * Pick the instance that represents a *set of interchangeable instances* — + * a display group, or a whole persona when nothing has been renamed. * - * A persona can have several historical agent instances. Keeping this rule in - * one place prevents an avatar click on an older message from opening a - * different detail surface than the card in the Agents library. + * Active-first, then by name, so the answer is stable for a given set. Callers + * choose the set; do not call this with a whole persona when the caller is + * resolving a specific instance — use `pickCanonicalProfileAgent`, which scopes + * the set to the requested instance's display group first. * * Relay-archived instances are never eligible, so an archived record early in * file order can't hijack the persona target. Returns `undefined` when every @@ -31,12 +33,17 @@ export function pickProfileAgent( } /** - * Pick the instance a profile request should actually land on. + * Pick the instance a profile request should actually land on. This is the + * rule that keeps every profile entry point — Agents library card, message + * avatar, mention, deep link, Inbox — agreeing with each other. * * Collapsing onto the persona's representative instance is only correct while * the instances are interchangeable presentations of that persona. Once the - * owner has renamed one, opening its card must open *it* — otherwise the - * Agents library shows a card the profile panel refuses to open. + * owner has renamed one, opening it must open *it*: the Agents library now + * shows the renamed instance its own card, and a message avatar must reach + * the same place that card does. So the requested instance's display group, + * not the whole persona, is the set we canonicalise over. Same-named siblings + * still collapse onto the active one, exactly as before. * * Scoping happens before archive filtering rather than after: the display group * decides *which* siblings are candidates, and `pickProfileAgent` then drops the diff --git a/desktop/src/features/agents/ui/AgentIdentityCard.tsx b/desktop/src/features/agents/ui/AgentIdentityCard.tsx index b0668616aeb..5dca2f2482c 100644 --- a/desktop/src/features/agents/ui/AgentIdentityCard.tsx +++ b/desktop/src/features/agents/ui/AgentIdentityCard.tsx @@ -15,6 +15,8 @@ type AgentIdentityCardProps = { onClick: () => void; /** Optional badge rendered below the label (e.g. "Restart required"). */ statusBadge?: ReactNode; + /** Optional line under the label (e.g. the persona a split card belongs to). */ + subtitle?: string | null; }; export function AgentIdentityCard({ @@ -27,6 +29,7 @@ export function AgentIdentityCard({ modelLabel, onClick, statusBadge, + subtitle, }: AgentIdentityCardProps) { const trimmedAvatarUrl = avatarUrl?.trim() || null; @@ -72,6 +75,11 @@ export function AgentIdentityCard({ {label} + {subtitle ? ( + + {subtitle} + + ) : null} {modelLabel ? ( {modelLabel} diff --git a/desktop/src/features/agents/ui/UnifiedAgentsSection.tsx b/desktop/src/features/agents/ui/UnifiedAgentsSection.tsx index 5ffb8ed6a19..1877a8f432d 100644 --- a/desktop/src/features/agents/ui/UnifiedAgentsSection.tsx +++ b/desktop/src/features/agents/ui/UnifiedAgentsSection.tsx @@ -165,6 +165,7 @@ export function UnifiedAgentsSection(props: UnifiedAgentsSectionProps) { restartingAgentPubkey={restartingAgentPubkey} startingAgentPubkey={startingAgentPubkey} startingPersonaIds={startingPersonaIds} + subtitle={card.personaLabel} testId={`persona-agent-row-${card.key}`} onOpenAgentProfile={onOpenAgentProfile} onOpenPersonaProfile={onOpenPersonaProfile} @@ -236,6 +237,7 @@ function AgentPersonaCard({ restartingAgentPubkey, startingAgentPubkey, startingPersonaIds, + subtitle, testId, onOpenAgentProfile, onOpenPersonaProfile, @@ -254,6 +256,7 @@ function AgentPersonaCard({ restartingAgentPubkey: string | null; startingAgentPubkey: string | null; startingPersonaIds: ReadonlySet; + subtitle: string | null; testId: string; onOpenAgentProfile: ( pubkey: string, @@ -345,6 +348,7 @@ function AgentPersonaCard({ ) : null } + subtitle={subtitle} /> ); } diff --git a/desktop/src/features/agents/ui/unifiedAgentGroups.test.mjs b/desktop/src/features/agents/ui/unifiedAgentGroups.test.mjs index ca28be26d6a..fd134ce97ea 100644 --- a/desktop/src/features/agents/ui/unifiedAgentGroups.test.mjs +++ b/desktop/src/features/agents/ui/unifiedAgentGroups.test.mjs @@ -147,7 +147,11 @@ test("a fully archived name gets no card of its own", () => { }); test("a split persona with every instance archived keeps one persona-only card", () => { - const { groups } = buildUnifiedGroups([FIZZ_PERSONA], FIZZ_AGENTS, () => true); + const { groups } = buildUnifiedGroups( + [FIZZ_PERSONA], + FIZZ_AGENTS, + () => true, + ); assert.equal(groups[0].cards.length, 1); assert.equal(groups[0].cards[0].key, FIZZ_PERSONA.id); @@ -238,6 +242,82 @@ test("persona-level actions live on exactly one card per persona", () => { ); }); +test("persona actions stay put when an instance starts or stops", () => { + // Deriving the owner from an active-first pick relocated the only route to + // editing or deleting a persona the moment an agent started. + const ownerOf = (agents) => + buildUnifiedGroups( + [FIZZ_PERSONA], + agents, + NONE_ARCHIVED, + ).groups[0].cards.find((card) => card.ownsPersonaActions).label; + + assert.equal(ownerOf(FIZZ_AGENTS), "Fizz"); + assert.equal( + ownerOf([ + namedAgent(PUBKEYS.claudeOne, "Claude", "builtin:fizz", "running"), + namedAgent(PUBKEYS.claudeTwo, "Claude", "builtin:fizz"), + namedAgent(PUBKEYS.fizzOne, "Fizz", "builtin:fizz"), + namedAgent(PUBKEYS.fizzTwo, "Fizz", "builtin:fizz"), + ]), + "Fizz", + ); + assert.equal( + ownerOf([ + namedAgent(PUBKEYS.claudeOne, "Claude", "builtin:fizz"), + namedAgent(PUBKEYS.claudeTwo, "Claude", "builtin:fizz"), + namedAgent(PUBKEYS.fizzOne, "Fizz", "builtin:fizz", "running"), + namedAgent(PUBKEYS.fizzTwo, "Fizz", "builtin:fizz"), + ]), + "Fizz", + ); +}); + +test("persona actions fall back to the first card when every name was changed", () => { + const renamed = [ + namedAgent(PUBKEYS.claudeOne, "Cascade Instance A", "custom:cascade"), + namedAgent( + PUBKEYS.claudeTwo, + "Cascade Instance B", + "custom:cascade", + "running", + ), + ]; + const { groups } = buildUnifiedGroups( + [namedPersona("custom:cascade", "Cascade Test Agent")], + renamed, + NONE_ARCHIVED, + ); + + assert.deepEqual( + groups[0].cards.map((card) => card.ownsPersonaActions), + [true, false], + ); +}); + +test("a split persona keeps its own name on every card", () => { + const { groups } = buildUnifiedGroups( + [FIZZ_PERSONA], + FIZZ_AGENTS, + NONE_ARCHIVED, + ); + + assert.deepEqual( + groups[0].cards.map((card) => card.personaLabel), + ["Fizz", "Fizz"], + ); +}); + +test("an unsplit persona has no second line to add", () => { + const { groups } = buildUnifiedGroups( + [namedPersona("custom:solo", "Solo")], + [namedAgent(PUBKEYS.claudeOne, "Solo", "custom:solo")], + NONE_ARCHIVED, + ); + + assert.equal(groups[0].cards[0].personaLabel, null); +}); + test("card keys stay unique so cards cannot overwrite each other", () => { const { groups } = buildUnifiedGroups( [FIZZ_PERSONA], @@ -247,6 +327,55 @@ test("card keys stay unique so cards cannot overwrite each other", () => { const keys = groups[0].cards.map((card) => card.key); assert.equal(new Set(keys).size, keys.length); + // Pinned: e2e specs address split cards by `persona-agent-row-`. + assert.deepEqual(keys, ["builtin:fizz::claude", "builtin:fizz::fizz"]); +}); + +test("card keys do not move when an instance starts, stops, or is reordered", () => { + // A key derived from the current active-first winner remounts the card and + // refires its avatar query whenever a sibling's status changes. + const keysFor = (agents) => + buildUnifiedGroups( + [FIZZ_PERSONA], + agents, + NONE_ARCHIVED, + ).groups[0].cards.map((card) => card.key); + const baseline = keysFor(FIZZ_AGENTS); + + assert.deepEqual( + keysFor([ + namedAgent(PUBKEYS.claudeOne, "Claude", "builtin:fizz"), + namedAgent(PUBKEYS.claudeTwo, "Claude", "builtin:fizz", "running"), + namedAgent(PUBKEYS.fizzOne, "Fizz", "builtin:fizz"), + namedAgent(PUBKEYS.fizzTwo, "Fizz", "builtin:fizz", "running"), + ]), + baseline, + ); + assert.deepEqual( + keysFor([ + namedAgent(PUBKEYS.claudeTwo, "Claude", "builtin:fizz"), + namedAgent(PUBKEYS.claudeOne, "Claude", "builtin:fizz"), + namedAgent(PUBKEYS.fizzTwo, "Fizz", "builtin:fizz"), + namedAgent(PUBKEYS.fizzOne, "Fizz", "builtin:fizz"), + ]), + baseline, + ); +}); + +test("every distinct name is openable from a card the library actually renders", () => { + // `card.agent` is what the card's click handler opens; `card.agents` is the + // full set behind it. The render layer reads `agent`, so assert on it too — + // an invariant held only by a field no component reads proves nothing. + const { groups } = buildUnifiedGroups( + [FIZZ_PERSONA], + FIZZ_AGENTS, + NONE_ARCHIVED, + ); + + assert.deepEqual( + groups[0].cards.map((card) => card.agent.name), + ["Claude", "Fizz"], + ); }); test("a persona with no instances keeps its single unchanged card", () => { diff --git a/desktop/src/features/agents/ui/unifiedAgentGroups.ts b/desktop/src/features/agents/ui/unifiedAgentGroups.ts index 263c2338286..3a29c3bb614 100644 --- a/desktop/src/features/agents/ui/unifiedAgentGroups.ts +++ b/desktop/src/features/agents/ui/unifiedAgentGroups.ts @@ -1,4 +1,7 @@ -import { groupAgentsForDisplay } from "@/features/agents/lib/agentIdentity"; +import { + foldAgentDisplayName, + groupAgentsForDisplay, +} from "@/features/agents/lib/agentIdentity"; import { pickProfileAgent } from "@/features/agents/lib/pickProfileAgent"; import type { AgentPersona, ManagedAgent } from "@/shared/api/types"; @@ -10,9 +13,23 @@ import type { AgentPersona, ManagedAgent } from "@/shared/api/types"; * it may never decide which agents exist (see `agentDisplayGroupKey`). */ export type UnifiedAgentCard = { - /** Stable card key, also used for the card's `data-testid`. */ + /** + * Stable card key, also used for the card's React key and `data-testid`. + * + * Derived only from the persona and the display group, never from runtime + * status or array position: a key that moves when an agent starts remounts + * the card and refires its avatar query. + */ key: string; label: string; + /** + * Persona name, rendered as the card's second line, and only when the + * persona has split into several cards — otherwise `label` already is the + * persona name. Without it a split persona's own name appears nowhere in + * the library, which is the same disappearing act this module exists to + * prevent. + */ + personaLabel: string | null; persona: AgentPersona; /** Instance the card opens; `undefined` for a persona with no instances. */ agent: ManagedAgent | undefined; @@ -52,6 +69,7 @@ function buildPersonaCards( const personaOnlyCard = (members: ManagedAgent[]): UnifiedAgentCard => ({ key: persona.id, label: persona.displayName, + personaLabel: null, persona, agent: pickProfileAgent(members, isArchived), agents: members, @@ -62,39 +80,44 @@ function buildPersonaCards( return [personaOnlyCard(displayGroups[0]?.agents ?? [])]; } - const liveGroups = displayGroups - .map((group) => ({ - group, - primary: pickProfileAgent(group.agents, isArchived), - })) - .filter( - (entry): entry is { group: (typeof displayGroups)[number]; primary: ManagedAgent } => - entry.primary !== undefined, - ); - + const liveGroups = displayGroups.filter( + (group) => pickProfileAgent(group.agents, isArchived) !== undefined, + ); if (liveGroups.length === 0) return [personaOnlyCard(agents)]; - const personaPrimary = pickProfileAgent(agents, isArchived); - const cards = liveGroups.map(({ group, primary }) => ({ - key: `${persona.id}-${primary.pubkey}`, + const ownerIndex = pickPersonaActionsIndex(persona, liveGroups); + return liveGroups.map((group, index) => ({ + key: `${persona.id}::${group.foldedName}`, label: group.name || persona.displayName, + personaLabel: persona.displayName, persona, - agent: primary, + agent: pickProfileAgent(group.agents, isArchived), agents: group.agents, - ownsPersonaActions: personaPrimary - ? group.agents.includes(personaPrimary) - : false, + ownsPersonaActions: index === ownerIndex, })); +} - // Persona actions must exist on exactly one card. `personaPrimary` is live so - // its own group survived the filter, but instance de-duplication can drop the - // very record it points at; fall back to the first card rather than stranding - // edit/share/delete on none of them. - if (!cards.some((card) => card.ownsPersonaActions)) { - cards[0].ownsPersonaActions = true; - } - - return cards; +/** + * Which split card carries the persona menu — Edit / Duplicate / Share / + * Deactivate / Delete persona. + * + * Deliberately independent of runtime status. Deriving it from + * `pickProfileAgent` (active-first) relocated the only route to editing or + * deleting a persona whenever an instance started, leaving the card the owner + * was looking at with no menu and no hint where it went. The card that still + * carries the persona's own name is the natural home; if the owner renamed + * every instance — or archiving retired the card that held the persona's own + * name — the first surviving card gets it. + */ +function pickPersonaActionsIndex( + persona: AgentPersona, + displayGroups: readonly { foldedName: string }[], +): number { + const personaName = foldAgentDisplayName(persona.displayName); + const named = displayGroups.findIndex( + (group) => group.foldedName === personaName, + ); + return named === -1 ? 0 : named; } /** diff --git a/desktop/tests/e2e/agent-lifecycle-feedback.spec.ts b/desktop/tests/e2e/agent-lifecycle-feedback.spec.ts index de94bb230f7..a20cdb5715b 100644 --- a/desktop/tests/e2e/agent-lifecycle-feedback.spec.ts +++ b/desktop/tests/e2e/agent-lifecycle-feedback.spec.ts @@ -106,16 +106,29 @@ test.describe("agent lifecycle feedback screenshots", () => { await openAgentsView(page); - // The custom persona card appears in the library. + // Both instances were renamed, so the persona splits into one card per + // name rather than hiding an instance behind a single persona card. await expect( - page.getByText("Cascade Test Agent", { exact: true }), + page.getByText("Cascade Instance A", { exact: true }), ).toBeVisible({ timeout: 10_000 }); + await expect( + page.getByText("Cascade Instance B", { exact: true }), + ).toBeVisible(); - // Open the actions menu for the custom persona. The trigger button carries - // an aria-label derived from the persona displayName. - await page - .getByRole("button", { name: "Open actions for Cascade Test Agent" }) - .click(); + // The persona's own name stays visible as each split card's second line, + // so a split persona is still findable in the library. + await expect( + page.getByText("Cascade Test Agent", { exact: true }), + ).toHaveCount(2); + + // Persona actions live on exactly one card — deterministically the first, + // since no instance kept the persona's name. The trigger button carries an + // aria-label derived from the persona displayName. + const personaActions = page.getByRole("button", { + name: "Open actions for Cascade Test Agent", + }); + await expect(personaActions).toHaveCount(1); + await personaActions.click(); // For a custom (non-builtin) persona, Delete opens PersonaDeleteDialog. await page.getByRole("menuitem", { name: "Delete" }).click(); diff --git a/desktop/tests/e2e/agents.spec.ts b/desktop/tests/e2e/agents.spec.ts index e191d293045..286beb52a5e 100644 --- a/desktop/tests/e2e/agents.spec.ts +++ b/desktop/tests/e2e/agents.spec.ts @@ -2700,3 +2700,71 @@ test("duplicate instances move from the agents gallery into the agent profile", page.getByTestId(`user-profile-agent-delete-${additionalPubkey}`), ).toHaveCount(0); }); + +test("renamed instances of one persona each get their own gallery card", async ({ + page, +}) => { + // The regression this pins lived in JSX: the gallery rendered exactly one + // card per persona, so an instance the owner renamed had no card at all and + // was unreachable from Settings. Model-layer tests cannot catch a revert of + // the render layer; this can. + const personaId = "builtin:fizz"; + const claudePubkey = TEST_IDENTITIES.alice.pubkey; + const fizzPubkey = TEST_IDENTITIES.charlie.pubkey; + await installMockBridge(page, { + personas: [ + { + id: personaId, + displayName: "Fizz", + systemPrompt: "You are Fizz.", + }, + ], + managedAgents: [ + { pubkey: claudePubkey, name: "Claude", personaId, status: "running" }, + { pubkey: "1".repeat(64), name: "Claude", personaId, status: "stopped" }, + { pubkey: fizzPubkey, name: "Fizz", personaId, status: "stopped" }, + { pubkey: "2".repeat(64), name: "Fizz", personaId, status: "stopped" }, + ], + }); + await gotoApp(page); + await page.getByTestId("open-agents-view").click(); + + // Four instances, two names, two cards — not one card hiding "Fizz", and not + // four cards exploding same-named duplicates apart. + const claudeCard = page.getByTestId(`persona-agent-row-${personaId}::claude`); + const fizzCard = page.getByTestId(`persona-agent-row-${personaId}::fizz`); + await expect(claudeCard).toBeVisible(); + await expect(fizzCard).toBeVisible(); + await expect( + page.locator(`[data-testid^="persona-agent-row-${personaId}"]`), + ).toHaveCount(2); + await expect(claudeCard).toContainText("Claude"); + + // The persona's own name survives the split as each card's second line, so a + // split persona stays findable in the library. + await expect(claudeCard.getByText("Fizz", { exact: true })).toBeVisible(); + + // Persona actions live on exactly one card — the one still carrying the + // persona's name — rather than migrating to whichever instance is running. + await expect( + page.getByRole("button", { name: "Open actions for Fizz" }), + ).toHaveCount(1); + await expect( + fizzCard.getByRole("button", { name: "Open actions for Fizz" }), + ).toBeVisible(); + + // The renamed card opens its own instance, not the persona's active winner: + // the stopped "Fizz" offers Start, where running "Claude" would offer Stop. + await fizzCard.click(); + await expect(page.getByTestId("user-profile-panel")).toBeVisible(); + await expect( + page.getByTestId("user-profile-agent-primary-action"), + ).toHaveAttribute("aria-label", "Start agent"); + + // ...and the "Claude" card opens the running instance. + await page.getByTestId("auxiliary-panel-close").click(); + await claudeCard.click(); + await expect( + page.getByTestId("user-profile-agent-primary-action"), + ).toHaveAttribute("aria-label", "Stop"); +}); diff --git a/desktop/tests/e2e/profile.spec.ts b/desktop/tests/e2e/profile.spec.ts index 6c88b628ce8..4493eaec645 100644 --- a/desktop/tests/e2e/profile.spec.ts +++ b/desktop/tests/e2e/profile.spec.ts @@ -1871,24 +1871,40 @@ test("an older agent message opens the same persona instance as the Agents libra await page.goto("/"); await page.getByTestId("open-agents-view").click(); - await page.getByTestId(`persona-agent-row-${personaId}`).click(); + + // The two instances carry different names, so the persona shows one card per + // name — neither is hidden — and each card opens its own instance. The + // persona's own name stays on both cards as a second line. + const earlierCard = page.getByTestId( + `persona-agent-row-${personaId}::earlier parity agent`, + ); + const currentCard = page.getByTestId( + `persona-agent-row-${personaId}::current parity agent`, + ); + await expect(earlierCard).toBeVisible(); + await expect(currentCard).toBeVisible(); + await expect(page.getByText("Parity Agent", { exact: true })).toHaveCount(2); + + // The running instance's card opens the running instance. + await currentCard.click(); + await expectHashSearchParam(page, "profile", currentPubkey); await expect( page.getByTestId("user-profile-agent-primary-action"), ).toHaveAttribute("aria-label", "Stop"); - const agentsLibraryContract = await readOwnedAgentProfileContract(page); + await page.getByTestId("auxiliary-panel-close").click(); - await page.getByTestId("user-profile-tab-runtime").click(); - await page.getByTestId("user-profile-instances").click(); - await page.getByTestId(`user-profile-instance-${historicalPubkey}`).click(); + // The renamed, stopped instance's card opens *that* instance rather than + // silently redirecting to its running sibling. + await earlierCard.click(); await expectHashSearchParam(page, "profile", historicalPubkey); - await expectHashSearchParam(page, "profileTab", "runtime"); await expect( page.getByTestId("user-profile-agent-primary-action"), ).toHaveAttribute("aria-label", "Start agent"); - await expect( - page.getByTestId(`user-profile-instance-${historicalPubkey}`), - ).toContainText("Current"); + const agentsLibraryContract = await readOwnedAgentProfileContract(page); + // Parity, restated for the new card model: an avatar click on an older + // message from the renamed instance must land on the same contract its card + // does — not on the persona's running instance. await page.getByTestId("auxiliary-panel-close").click(); await page.getByTestId("channel-agents").click(); const historicalMessage = page @@ -1898,7 +1914,7 @@ test("an older agent message opens the same persona instance as the Agents libra await historicalMessage.locator("button").first().click(); await expect( page.getByTestId("user-profile-agent-primary-action"), - ).toHaveAttribute("aria-label", "Stop"); + ).toHaveAttribute("aria-label", "Start agent"); const messageContract = await readOwnedAgentProfileContract(page); expect(messageContract).toEqual(agentsLibraryContract); From 5174b0970d3bedee78977f257d97d86e5d581a68 Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Sun, 16 Aug 2026 09:59:26 -0400 Subject: [PATCH 3/5] fix(desktop): normalize unicode and length-prefix the display group key Two edge cases found by exercising agentIdentity.ts directly with degenerate input, both of which reintroduce the failure this module exists to prevent. Unicode: foldAgentDisplayName lowercased without normalizing, so the same name in NFC and NFD folded to two different keys. macOS input methods and file systems commonly emit NFD while Windows emits NFC, so one owner naming an agent on a Mac and on a Windows box produced two cards with visually identical labels -- a split that cannot be seen, explained, or fixed from the UI. Folding now normalizes to NFC first. Separator: agentDisplayGroupKey joined its segments with a literal `|name:`, but a display name is free text and may contain any separator. Verified before the fix: agentDisplayGroupKey({personaId: "a", name: "x|name:y"}) agentDisplayGroupKey({personaId: "a|name:x", name: "y"}) -> both "persona:a|name:x|name:y" Two different agents sharing one card, one of them no longer openable. Segments are now length-prefixed, so no name can forge another key. This does not change the card's React key or data-testid, which derive from persona.id and the folded name rather than from this key. Also adds a test asserting that unnamed instances of one persona share a card. That was already true and stays true -- they remain reachable through the card's profile panel -- but it was implicit in the fold rather than stated, so a future change to name handling now has to decide it deliberately. Red-before-green: with agentIdentity.ts reverted and the tests kept, the two behavioural tests fail (4 pass / 2 fail); the documentation test passes either way, which is correct. Verified: full desktop unit suite exit 0, tsc --noEmit clean, biome clean. Co-authored-by: Michael Feth Signed-off-by: Michael Feth --- .../agents/lib/agentIdentity.test.mjs | 64 +++++++++++++++++++ .../src/features/agents/lib/agentIdentity.ts | 16 ++++- 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/desktop/src/features/agents/lib/agentIdentity.test.mjs b/desktop/src/features/agents/lib/agentIdentity.test.mjs index 47b67b356fc..59715f0d2ce 100644 --- a/desktop/src/features/agents/lib/agentIdentity.test.mjs +++ b/desktop/src/features/agents/lib/agentIdentity.test.mjs @@ -50,6 +50,70 @@ test("the display group key separates renamed instances of one persona", () => { ); }); +test("a name is folded to NFC, so one fleet does not split on encoding", () => { + // macOS input methods and file systems commonly emit NFD, Windows emits NFC. + // The same name typed on two machines must land on one card. + const precomposed = "José"; // é as U+00E9 + const decomposed = "José"; // e + U+0301 combining acute + + assert.notEqual(precomposed, decomposed, "the inputs really do differ"); + assert.equal( + agentDisplayGroupKey({ personaId: "builtin:fizz", name: precomposed }), + agentDisplayGroupKey({ personaId: "builtin:fizz", name: decomposed }), + ); + + const groups = groupAgentsForDisplay([ + { pubkey: PUBKEY_A, name: precomposed, personaId: "builtin:fizz" }, + { pubkey: PUBKEY_B, name: decomposed, personaId: "builtin:fizz" }, + ]); + + assert.equal( + groups.length, + 1, + "two encodings of one name must not render two identical-looking cards", + ); + assert.equal(groups[0].agents.length, 2); +}); + +test("the group key cannot be forged by a name containing a separator", () => { + // Segments are length-prefixed. With a plain `|` join both of these render + // `persona:a|name:x|name:y`, silently merging two different agents onto one + // card and leaving one of them unopenable. + assert.notEqual( + agentDisplayGroupKey({ personaId: "a", name: "x|name:y" }), + agentDisplayGroupKey({ personaId: "a|name:x", name: "y" }), + ); + assert.notEqual( + agentDisplayGroupKey({ personaId: "builtin:fizz", name: "a:b" }), + agentDisplayGroupKey({ personaId: "builtin:fizz:a", name: "b" }), + ); + // A separator in a name is still just a name — same input, same key. + assert.equal( + agentDisplayGroupKey({ personaId: "a", name: "x|name:y" }), + agentDisplayGroupKey({ personaId: "a", name: " X|NAME:Y " }), + ); +}); + +test("unnamed instances of one persona share a card — documented, not accidental", () => { + // "", " ", null and undefined all fold to the same empty name, so several + // unnamed instances of one persona collapse onto the persona's card. They + // stay reachable through that card's profile panel, which lists every + // instance behind it. Asserted so a future change to the fold has to decide + // this deliberately rather than discover it. + const groups = groupAgentsForDisplay([ + { pubkey: PUBKEY_A, name: "", personaId: "builtin:fizz" }, + { pubkey: PUBKEY_B, name: " ", personaId: "builtin:fizz" }, + { pubkey: "c".repeat(64), name: null, personaId: "builtin:fizz" }, + { pubkey: "d".repeat(64), name: "Fizz", personaId: "builtin:fizz" }, + ]); + + assert.deepEqual( + groups.map((group) => group.name), + ["", "Fizz"], + ); + assert.equal(groups[0].agents.length, 3, "no unnamed instance is dropped"); +}); + test("display grouping keeps every distinct identity and drops repeats", () => { const agents = [ { pubkey: PUBKEY_A, name: "Claude", personaId: "builtin:fizz" }, diff --git a/desktop/src/features/agents/lib/agentIdentity.ts b/desktop/src/features/agents/lib/agentIdentity.ts index 62c54be9045..aab8d20e329 100644 --- a/desktop/src/features/agents/lib/agentIdentity.ts +++ b/desktop/src/features/agents/lib/agentIdentity.ts @@ -37,16 +37,28 @@ export type AgentDisplayInput = AgentIdentityInput & { */ export function agentDisplayGroupKey(agent: AgentDisplayInput): string { const personaId = agent.personaId?.trim() ?? ""; - return `persona:${personaId}|name:${foldAgentDisplayName(agent.name)}`; + // Length-prefixed segments, not a delimiter. A display name is free text and + // may contain any separator we could pick: with a plain `|` join, + // {personaId:"a", name:"x|name:y"} and {personaId:"a|name:x", name:"y"} both + // render `persona:a|name:x|name:y`, so two different agents share one card + // and one of them stops being openable. + const foldedName = foldAgentDisplayName(agent.name); + return `persona:${personaId.length}:${personaId}|name:${foldedName.length}:${foldedName}`; } /** * The one place a display name is folded for comparison. Callers that need a * name-scoped key of their own (React keys, `data-testid`s) must fold through * this rather than lowercasing inline, or their key and the group's disagree. + * + * Unicode is normalized to NFC before folding. macOS input methods and file + * systems commonly produce NFD while Windows produces NFC, so without this the + * same name typed on two machines in one fleet folds to two different keys and + * the library renders two cards with visually identical labels — a split the + * owner cannot see, explain, or fix from the UI. */ export function foldAgentDisplayName(name: string | null | undefined): string { - return name?.trim().toLowerCase() ?? ""; + return name?.normalize("NFC").trim().toLowerCase() ?? ""; } export type AgentDisplayGroup = { From 731fb7b2f296e37a2de184edeca46f01046e01a7 Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Sun, 16 Aug 2026 21:15:03 -0400 Subject: [PATCH 4/5] docs(desktop): record why the card key's plain separator is safe The tip commit length-prefixed the segments of agentDisplayGroupKey because a free-text agent name can forge a delimiter, and its message noted that the card's React key and data-testid were left on the plain `::` join. That reads as an oversight; it is a deliberate asymmetry and now says so. The join is unambiguous because the LEFT segment cannot contain the separator. A persona id is a slugify() output (every non-alphanumeric becomes '-'), a v4 UUID, or a 'builtin:' literal carrying a single colon. Traced all three sources: util.rs:28-43, personas/snapshot/import.rs:559, and the builtin literals. None can emit '::', so a forged separator in the free-text right segment cannot shift the boundary. Length-prefixing this key too would be defensive against an unreachable input and would churn four e2e literals plus a unit pin for it, so the invariant is documented rather than enforced. Co-authored-by: Michael Feth Signed-off-by: Michael Feth --- desktop/src/features/agents/ui/unifiedAgentGroups.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/desktop/src/features/agents/ui/unifiedAgentGroups.ts b/desktop/src/features/agents/ui/unifiedAgentGroups.ts index 3a29c3bb614..5dc8a0ae776 100644 --- a/desktop/src/features/agents/ui/unifiedAgentGroups.ts +++ b/desktop/src/features/agents/ui/unifiedAgentGroups.ts @@ -87,6 +87,14 @@ function buildPersonaCards( const ownerIndex = pickPersonaActionsIndex(persona, liveGroups); return liveGroups.map((group, index) => ({ + // Plain `::` join rather than the length-prefixed form `agentDisplayGroupKey` + // uses, because this key is also the card's `data-testid` and e2e specs read + // it. That is safe only because the left segment cannot contain the + // separator: a persona id is either a `slugify()` output (every + // non-alphanumeric becomes `-`), a v4 UUID, or a `builtin:` literal + // with a single colon — never `::`. The right segment is free text, but a + // forged separator there cannot shift the boundary when the left one is + // constrained. Revisit if persona ids ever become user-supplied. key: `${persona.id}::${group.foldedName}`, label: group.name || persona.displayName, personaLabel: persona.displayName, From 79f54b6dd4cdd1b4d0323c8472755cf95b4bbac5 Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Tue, 18 Aug 2026 13:00:10 -0400 Subject: [PATCH 5/5] docs(desktop): note the store-namespaced persona id in the card-key invariant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `::` card-key separator is safe because the left segment cannot contain it. That comment enumerated three persona-id shapes; #5904 introduced a fourth, `:`, when it migrated team membership onto namespaced ids. The invariant still holds — `slugify` maps every non-alphanumeric to `-`, so neither half of that form can carry a colon and the whole id has exactly one — but a reader checking the claim against current `main` would find a shape the comment does not mention and reasonably doubt the rest of it. Stating the rule (at most one colon, because every shape is slug-constrained) rather than listing cases also stops the comment going stale on the fifth shape. Signed-off-by: Michael Feth --- desktop/src/features/agents/ui/unifiedAgentGroups.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/desktop/src/features/agents/ui/unifiedAgentGroups.ts b/desktop/src/features/agents/ui/unifiedAgentGroups.ts index 5dc8a0ae776..1cffae6dab5 100644 --- a/desktop/src/features/agents/ui/unifiedAgentGroups.ts +++ b/desktop/src/features/agents/ui/unifiedAgentGroups.ts @@ -90,11 +90,13 @@ function buildPersonaCards( // Plain `::` join rather than the length-prefixed form `agentDisplayGroupKey` // uses, because this key is also the card's `data-testid` and e2e specs read // it. That is safe only because the left segment cannot contain the - // separator: a persona id is either a `slugify()` output (every - // non-alphanumeric becomes `-`), a v4 UUID, or a `builtin:` literal - // with a single colon — never `::`. The right segment is free text, but a - // forged separator there cannot shift the boundary when the left one is - // constrained. Revisit if persona ids ever become user-supplied. + // separator. Every shape a persona id takes carries at most one colon: + // a `slugify()` output (which maps every non-alphanumeric to `-`, so it + // cannot contain one at all), a v4 UUID, a `builtin:` literal, or the + // store-namespaced `:` form — both halves of that + // last one are slugs, so it is a single colon too. The right segment is free + // text, but a forged separator there cannot shift the boundary when the left + // one is constrained. Revisit if persona ids ever become user-supplied. key: `${persona.id}::${group.foldedName}`, label: group.name || persona.displayName, personaLabel: persona.displayName,