From 47cc90626d5093146e7a351cc40fb34ff1ca0621 Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Sat, 15 Aug 2026 22:36:08 -0400 Subject: [PATCH 1/7] 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 516520e2ca3..7755d541349 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"; @@ -299,16 +300,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( @@ -383,7 +381,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 3acf014ab3647b510ed660cbabee8de66fb23bd0 Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Sun, 16 Aug 2026 01:10:14 -0400 Subject: [PATCH 2/7] 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 93c21ca541b706cf04d7ef74ed560f4353c3a985 Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Sun, 16 Aug 2026 09:59:26 -0400 Subject: [PATCH 3/7] 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 896ce38a55bf1fe8ad9ddd07e4dda2e071ee0aa6 Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Sun, 16 Aug 2026 21:15:03 -0400 Subject: [PATCH 4/7] 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 165f5b3776bbf8832e34ef370e79e0164607c90a Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Sun, 16 Aug 2026 15:37:23 -0400 Subject: [PATCH 5/7] feat(desktop): guard the single definition of agent identity The bug the parent commits fix was a drift, not a typo: @-mention autocomplete and the Agents library each hand-rolled a key for "same agent?", #5202 moved one of them to the pubkey, and nobody moved the other. Renaming an instance then made it disappear from the library entirely. Nothing failed loudly -- a card simply stopped existing. Convention alone does not stop that recurring, so this encodes the invariant: an agent identity or display-group key is minted in exactly one module, src/features/agents/lib/agentIdentity.ts, and everywhere else imports it. The guard flags a string or template literal that begins an identity namespace -- `pubkey:` or `persona:` -- anywhere outside that module. Those are the wire formats agentIdentityKey and agentDisplayGroupKey produce, so a literal starting with one is either a second implementation or one edit away from becoming one. Verified it catches the real thing, not just a synthetic case: restoring the pre-#5202 `persona:${candidate.personaId}` into agentAutocompleteEligibility.ts fails the check and names that file and line. Restoring the fix passes. Deliberately narrow, because a guard that cries wolf gets disabled: - It does not try to catch every way of grouping agents, only the shape that caused the outage. - Comment lines are skipped. Unrelated subsystems document their own scope keys (channel storage uses "pubkey:normalizedRelayUrl"), and flagging prose teaches people to silence the guard rather than read it. - Four genuine non-identity uses are allowlisted by `path:literal` with a reason each: the persona catalog dialog's selection token, and the profile panel's render key for a persona that has no agent instance yet. Both are adjacent to the real thing, so each entry says what would make it a violation. Wired into `pnpm check` alongside check-file-sizes, check-px-text and check-pubkey-truncation, matching their structure (single-purpose script, `path:matchedLiteral` allowlist, failure message that names the fix). Verified: pnpm check exits 0 with the new guard in the chain; biome clean; package.json parses. Stacks on the agent-identity fix -- the invariant has no canonical module to point at without it. Co-authored-by: Michael Feth Signed-off-by: Michael Feth --- desktop/package.json | 3 +- desktop/scripts/check-agent-identity.mjs | 147 +++++++++++++++++++++++ 2 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 desktop/scripts/check-agent-identity.mjs diff --git a/desktop/package.json b/desktop/package.json index 073a764a1ba..4f6af51756c 100644 --- a/desktop/package.json +++ b/desktop/package.json @@ -11,8 +11,9 @@ "check:file-sizes": "node ./scripts/check-file-sizes.mjs", "check:px-text": "node ./scripts/check-px-text.mjs", "check:pubkey-truncation": "node ./scripts/check-pubkey-truncation.mjs", + "check:agent-identity": "node ./scripts/check-agent-identity.mjs", "lint": "biome lint .", - "check": "biome check . && pnpm check:px-text && pnpm check:pubkey-truncation", + "check": "biome check . && pnpm check:px-text && pnpm check:pubkey-truncation && pnpm check:agent-identity", "format": "biome format --write .", "test": "node --import ./test-loader.mjs --experimental-strip-types --test \"src/**/*.test.mjs\"", "preview": "vite preview", diff --git a/desktop/scripts/check-agent-identity.mjs b/desktop/scripts/check-agent-identity.mjs new file mode 100644 index 00000000000..f93de9df0ec --- /dev/null +++ b/desktop/scripts/check-agent-identity.mjs @@ -0,0 +1,147 @@ +import { promises as fs } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +/** + * Guards the single definition of agent identity. + * + * Two surfaces answer "which agents exist" — @-mention autocomplete and the + * Agents library — and each once hand-rolled its own key. They drifted, and the + * drift was invisible until an agent became unreachable on one of them: + * autocomplete keyed on the pubkey (#5202) while the library still grouped by + * `personaId`, so renaming an instance made it vanish from the library + * entirely. Nothing failed loudly; a card simply stopped existing. + * + * The invariant that prevents a third answer appearing: an agent identity or + * display-group key is minted in exactly ONE module, + * `src/features/agents/lib/agentIdentity.ts`. Everywhere else imports it. + * + * So this flags a string or template literal that *begins* an identity + * namespace — `pubkey:` or `persona:` — anywhere outside that module. Those + * two prefixes are the wire format `agentIdentityKey` and + * `agentDisplayGroupKey` produce; a literal starting with one is either a + * second implementation or something close enough to become one. + * + * Deliberately narrow. A guard that cries wolf gets disabled, so it does not + * try to catch every possible way of grouping agents — only the shape that + * actually caused the outage. Non-identity uses of the same prefix (a dialog's + * selection token, a render key for a panel with no pubkey) are allowlisted + * below with a reason each. + */ + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const projectRoot = path.resolve(__dirname, ".."); + +/** The one module allowed to mint these keys. */ +const CANONICAL_MODULE = "src/features/agents/lib/agentIdentity.ts"; + +const SCAN_ROOT = "src"; +const EXTENSIONS = new Set([".ts", ".tsx"]); + +// A quote or backtick immediately followed by an identity namespace. Matching +// the opening delimiter is what keeps `builtin:fizz`, `"persona"`, or a +// sentence containing "persona:" from tripping it. +const IDENTITY_KEY_RE = /[`"'](?:pubkey|persona):/g; + +// Allowlisted `relativePath:matchedLiteral` pairs. Matching the literal rather +// than a line number keeps these stable when unrelated edits move code. +const overrides = new Set([ + // A radio-group selection token for the persona catalog dialog: it addresses + // a row in that dialog's own list, is never compared against an agent, and + // never leaves the component. + 'src/features/agents/ui/PersonaCatalogDialog.tsx:"persona:', + "src/features/agents/ui/PersonaCatalogDialog.tsx:`persona:", + // React render key for the profile panel when there is no pubkey to key on + // (an uninstantiated persona has no agent identity yet). Adjacent to the + // real thing — if this ever starts being compared against an agent key, + // route it through `agentIdentity` instead of widening this exception. + "src/features/profile/ui/UserProfilePanel.tsx:`persona:", + "src/features/profile/ui/UserProfilePanelUtils.ts:`persona:", +]); + +/** + * Whether a line is wholly a comment. Deliberately a heuristic over the common + * shapes (`//`, `/*`, and the `*` continuation of a block comment) rather than + * a parser: a false negative here just means the allowlist earns an entry, + * while a parser would be far more machinery than this guard is worth. + */ +function isCommentLine(line) { + const trimmed = line.trimStart(); + return ( + trimmed.startsWith("//") || + trimmed.startsWith("/*") || + trimmed.startsWith("*") + ); +} + +async function walkFiles(directory) { + const entries = await fs.readdir(directory, { withFileTypes: true }); + const files = await Promise.all( + entries.map(async (entry) => { + const fullPath = path.join(directory, entry.name); + return entry.isDirectory() ? walkFiles(fullPath) : [fullPath]; + }), + ); + return files.flat(); +} + +const scanDirectory = path.join(projectRoot, SCAN_ROOT); +const candidateFiles = await fs + .access(scanDirectory) + .then(() => walkFiles(scanDirectory)) + .catch(() => []); + +const violations = []; + +for (const filePath of candidateFiles) { + // Override keys and the canonical path are authored with `/`, but + // path.relative yields `\` on Windows — compare in posix form or this + // silently matches nothing. + const relativePath = path + .relative(projectRoot, filePath) + .split(path.sep) + .join("/"); + + if (!EXTENSIONS.has(path.extname(relativePath))) { + continue; + } + if (relativePath === CANONICAL_MODULE) { + continue; + } + + const content = await fs.readFile(filePath, "utf8"); + content.split(/\r?\n/).forEach((line, index) => { + // Prose describing a key format is not a second implementation of one. + // Unrelated subsystems document their own scope keys (e.g. the channel + // storage scope `"pubkey:normalizedRelayUrl"`), and flagging a comment + // teaches people to silence the guard rather than read it. + if (isCommentLine(line)) { + return; + } + for (const match of line.match(IDENTITY_KEY_RE) ?? []) { + if (!overrides.has(`${relativePath}:${match}`)) { + violations.push({ relativePath, lineNumber: index + 1, match }); + } + } + }); +} + +if (violations.length > 0) { + console.error("Desktop agent-identity check failed:"); + for (const violation of violations) { + console.error( + `- ${violation.relativePath}:${violation.lineNumber}: ${violation.match}`, + ); + } + console.error( + `Agent identity is minted in one place: \`${CANONICAL_MODULE}\`. ` + + "Import `agentIdentityKey` (identity — the pubkey) or " + + "`agentDisplayGroupKey` (presentation — which agents may share one card) " + + "instead of building the key here. Two surfaces that answer " + + '"which agents exist" differently is how a renamed agent silently ' + + "disappeared from the Agents library. If this literal is genuinely not " + + "an agent identity, add a narrowly scoped `relativePath:matchedLiteral` " + + "exception, with a reason, in `desktop/scripts/check-agent-identity.mjs`.", + ); + process.exit(1); +} From d534393cbab8e39d3a18d8f7f17b23402cafeb81 Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Sun, 16 Aug 2026 17:15:45 -0400 Subject: [PATCH 6/7] fix(desktop): remove the identity guard's exceptions instead of allowlisting them Review follow-up on PR #6077, and the reviewer is right. The guard's allowlist was keyed on `path:matchedLiteral` where the literal was only the namespace prefix, so one entry exempted EVERY occurrence of that prefix in the file. The four exempted files were the agent-adjacent ones, so the guard was theatre over precisely the code most likely to drift. Reproduced before fixing: adding a second, unrelated `persona:${a.personaId}|${a.name}` to an allowlisted file passed at exit 0. The fix is not a tighter allowlist but removing the need for one. The two legitimate non-identity uses get namespaces of their own: - `catalog-persona:` for the persona catalog dialog's radio-group selection token, now a named constant. It addresses a row in that dialog's own list, is produced and consumed only in that file, and is never persisted. - `profile:` for the profile panel's render key, which exists precisely when there is NO agent to identify (an uninstantiated persona has no pubkey), so looking like an agent identity was the wrong signal to send. The allowlist is now empty, and documented as worth keeping that way. The match also widened from the bare prefix to the prefix plus the first interpolation or word after it, so if an entry ever does become unavoidable it scopes to one literal rather than the whole file. Both attack cases verified after the change: the second key in a formerly-exempt file is now caught, and reintroducing the historical pre-#5202 `persona:${candidate.personaId}` into agentAutocompleteEligibility.ts still fails with the file and line named. One thing worth noting for reviewers: `check-file-sizes` caught a 2-line comment I had added to UserProfilePanel.tsx, which sits exactly on the 1000-line ceiling (1000 -> 1002). The comment was redundant with the doc on `profilePanelTargetKey` and is gone; the file is unchanged in length. Verified: pnpm check exits 0 with all four guards; desktop unit suite 4978/4978; tsc --noEmit clean. Co-authored-by: Michael Feth Signed-off-by: Michael Feth --- desktop/scripts/check-agent-identity.mjs | 45 +++++++++++-------- .../agents/ui/PersonaCatalogDialog.tsx | 20 +++++++-- .../features/profile/ui/UserProfilePanel.tsx | 2 +- .../profile/ui/UserProfilePanelUtils.test.mjs | 4 +- .../profile/ui/UserProfilePanelUtils.ts | 11 ++++- 5 files changed, 56 insertions(+), 26 deletions(-) diff --git a/desktop/scripts/check-agent-identity.mjs b/desktop/scripts/check-agent-identity.mjs index f93de9df0ec..abf6624af07 100644 --- a/desktop/scripts/check-agent-identity.mjs +++ b/desktop/scripts/check-agent-identity.mjs @@ -38,26 +38,33 @@ const CANONICAL_MODULE = "src/features/agents/lib/agentIdentity.ts"; const SCAN_ROOT = "src"; const EXTENSIONS = new Set([".ts", ".tsx"]); -// A quote or backtick immediately followed by an identity namespace. Matching -// the opening delimiter is what keeps `builtin:fizz`, `"persona"`, or a -// sentence containing "persona:" from tripping it. -const IDENTITY_KEY_RE = /[`"'](?:pubkey|persona):/g; +// A quote or backtick immediately followed by an identity namespace, plus the +// first interpolation or word after it. Matching the opening delimiter keeps +// `builtin:fizz`, `"persona"`, and prose containing "persona:" from tripping +// it; capturing what FOLLOWS the namespace is what makes an allowlist entry +// specific to one key rather than to the whole file (see `overrides`). +const IDENTITY_KEY_RE = /[`"'](?:pubkey|persona):(?:\$\{[^}]*\}|[\w.-]*)/g; -// Allowlisted `relativePath:matchedLiteral` pairs. Matching the literal rather -// than a line number keeps these stable when unrelated edits move code. -const overrides = new Set([ - // A radio-group selection token for the persona catalog dialog: it addresses - // a row in that dialog's own list, is never compared against an agent, and - // never leaves the component. - 'src/features/agents/ui/PersonaCatalogDialog.tsx:"persona:', - "src/features/agents/ui/PersonaCatalogDialog.tsx:`persona:", - // React render key for the profile panel when there is no pubkey to key on - // (an uninstantiated persona has no agent identity yet). Adjacent to the - // real thing — if this ever starts being compared against an agent key, - // route it through `agentIdentity` instead of widening this exception. - "src/features/profile/ui/UserProfilePanel.tsx:`persona:", - "src/features/profile/ui/UserProfilePanelUtils.ts:`persona:", -]); +/** + * Allowlisted `relativePath:matchedLiteral` pairs. + * + * **Empty on purpose, and worth keeping that way.** The first version of this + * guard carried four entries and matched only the bare `persona:` prefix, so + * each entry exempted *every* occurrence of that prefix in the file — and the + * exempted files were the agent-adjacent ones, i.e. exactly the code most + * likely to drift. The guard was theatre over its highest-risk surface. + * + * The fix was not a tighter allowlist but removing the need for one: the two + * legitimate non-identity uses now carry their own namespaces + * (`catalog-persona:` for the persona catalog dialog's selection token, + * `profile:` for the profile panel's render key), so neither looks like an + * agent identity to this guard or to a reader. + * + * Prefer that route. If an entry is ever genuinely unavoidable, note that the + * key now includes the text after the namespace, so it scopes to one literal — + * but a namespace of its own is still the better answer. + */ +const overrides = new Set([]); /** * Whether a line is wholly a comment. Deliberately a heuristic over the common diff --git a/desktop/src/features/agents/ui/PersonaCatalogDialog.tsx b/desktop/src/features/agents/ui/PersonaCatalogDialog.tsx index f78f9d327ef..8ad916530b0 100644 --- a/desktop/src/features/agents/ui/PersonaCatalogDialog.tsx +++ b/desktop/src/features/agents/ui/PersonaCatalogDialog.tsx @@ -48,6 +48,16 @@ type PersonaCatalogDialogProps = { type PendingNavigation = | { type: "close" } | { type: "selection"; selection: string }; + +/** + * Prefix for this dialog's radio-group selection token, alongside the bare + * `"create"` and `"import"` values. Deliberately NOT `persona:` — that + * namespace belongs to `agentIdentity`, and a token that merely addresses a row + * in this list must not look like an agent identity to a reader or to + * `check-agent-identity`. Produced and consumed only in this file; never + * persisted or sent over the wire. + */ +const CATALOG_PERSONA_PREFIX = "catalog-persona:"; export function PersonaCatalogDialog({ createContent, error, @@ -70,8 +80,8 @@ export function PersonaCatalogDialog({ const [pendingNavigation, setPendingNavigation] = React.useState(null); const [selection, setSelection] = React.useState("create"); - const selectedPersonaId = selection.startsWith("persona:") - ? selection.slice("persona:".length) + const selectedPersonaId = selection.startsWith(CATALOG_PERSONA_PREFIX) + ? selection.slice(CATALOG_PERSONA_PREFIX.length) : null; const selectedPersona = React.useMemo(() => { if (!selectedPersonaId) { @@ -380,7 +390,9 @@ function PersonaCatalogChooser({ data-testid={`persona-catalog-list-item-${persona.id}`} key={persona.id} onClick={() => { - onSelectionChange(`persona:${persona.id}`); + onSelectionChange( + `${CATALOG_PERSONA_PREFIX}${persona.id}`, + ); }} type="button" > @@ -438,7 +450,7 @@ function PersonaCatalogChooser({ ) : null} - {selection.startsWith("persona:") && isLoading ? ( + {selection.startsWith(CATALOG_PERSONA_PREFIX) && isLoading ? (
diff --git a/desktop/src/features/profile/ui/UserProfilePanel.tsx b/desktop/src/features/profile/ui/UserProfilePanel.tsx index f02b5aee097..72556ff452d 100644 --- a/desktop/src/features/profile/ui/UserProfilePanel.tsx +++ b/desktop/src/features/profile/ui/UserProfilePanel.tsx @@ -219,7 +219,7 @@ export function UserProfilePanel({ ); }, [linkedPersonaId, managedAgent?.personaId, persona, personasQuery.data]); const profileIdentityKey = - managedAgent?.pubkey ?? pubkey ?? `persona:${persona?.id ?? "unknown"}`; + managedAgent?.pubkey ?? pubkey ?? `profile:${persona?.id ?? "unknown"}`; const resolvedPersona = useRetainedPersona( resolvedPersonaFromSource, profileIdentityKey, diff --git a/desktop/src/features/profile/ui/UserProfilePanelUtils.test.mjs b/desktop/src/features/profile/ui/UserProfilePanelUtils.test.mjs index 0c983fa3e84..14825cff495 100644 --- a/desktop/src/features/profile/ui/UserProfilePanelUtils.test.mjs +++ b/desktop/src/features/profile/ui/UserProfilePanelUtils.test.mjs @@ -241,6 +241,8 @@ test("profile target identity stays stable while a requested pubkey is canonical ); assert.equal( profilePanelTargetKey(undefined, "requested-persona"), - "persona:requested-persona", + // `profile:`, never `persona:` — that namespace belongs to agentIdentity, + // and this key exists precisely when there is no agent to identify. + "profile:requested-persona", ); }); diff --git a/desktop/src/features/profile/ui/UserProfilePanelUtils.ts b/desktop/src/features/profile/ui/UserProfilePanelUtils.ts index be1a57c112e..195b239ed19 100644 --- a/desktop/src/features/profile/ui/UserProfilePanelUtils.ts +++ b/desktop/src/features/profile/ui/UserProfilePanelUtils.ts @@ -88,11 +88,20 @@ export function profilePanelTabFromSearch(value: unknown): ProfilePanelTab { return parseProfilePanelTab(value) ?? "info"; } +/** + * Which profile the panel is showing — a render/reset key, not an identity. + * + * The persona fallback is namespaced `profile:` rather than `persona:` on + * purpose. `persona:` belongs to `agentIdentity`, and this value addresses a + * *panel*: it exists precisely when there is no agent yet (an uninstantiated + * persona has no pubkey), so it must never be mistaken for, or compared + * against, an agent identity key. + */ export function profilePanelTargetKey( pubkey: string | undefined, personaId: string | undefined, ): string { - return pubkey ?? `persona:${personaId ?? "unknown"}`; + return pubkey ?? `profile:${personaId ?? "unknown"}`; } export type UserProfilePanelProps = { From e6c6a5f0b542c28bf6574745c42e943b2b6b6cf9 Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Sun, 16 Aug 2026 21:16:58 -0400 Subject: [PATCH 7/7] docs(desktop): stop the identity guard advertising an allowlist it no longer has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The header still told the reader that non-identity uses "are allowlisted below with a reason each" while `overrides` is `new Set([])` — the tip commit removed every entry by giving those uses their own namespaces instead. That stale paragraph is the first thing anyone re-reading this file sees, and it describes precisely the design the review rejected. Header now states the actual design: no exceptions, and a namespace of its own is the preferred route if one is ever needed. Co-authored-by: Michael Feth Signed-off-by: Michael Feth --- desktop/scripts/check-agent-identity.mjs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/desktop/scripts/check-agent-identity.mjs b/desktop/scripts/check-agent-identity.mjs index abf6624af07..f29667b3a89 100644 --- a/desktop/scripts/check-agent-identity.mjs +++ b/desktop/scripts/check-agent-identity.mjs @@ -24,9 +24,13 @@ import { fileURLToPath } from "node:url"; * * Deliberately narrow. A guard that cries wolf gets disabled, so it does not * try to catch every possible way of grouping agents — only the shape that - * actually caused the outage. Non-identity uses of the same prefix (a dialog's - * selection token, a render key for a panel with no pubkey) are allowlisted - * below with a reason each. + * actually caused the outage. + * + * It has no exceptions. The two legitimate non-identity uses of a similar + * prefix carry namespaces of their own instead — `catalog-persona:` for the + * persona catalog dialog's selection token, `profile:` for the profile panel's + * render key — so neither looks like an agent identity to this guard or to a + * reader. Prefer that route over an allowlist entry; see `overrides`. */ const __dirname = path.dirname(fileURLToPath(import.meta.url));