fix(desktop): stop the agents library hiding renamed agent instances - #6013
fix(desktop): stop the agents library hiding renamed agent instances#6013mfethe1 wants to merge 4 commits into
Conversation
56e004f to
df36be9
Compare
df36be9 to
7466131
Compare
751c6df to
59e0f06
Compare
Two surfaces answered "which agents exist" with two hand-rolled identity keys, and they had drifted. @-mention autocomplete keys agents by pubkey (block#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 <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
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 <michael@jira-flow.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
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 <michael@jira-flow.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
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:<name>' 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 <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
59e0f06 to
731fb7b
Compare
|
Rebased onto #6086 —
|
The bug
The desktop app has two surfaces that answer "which agents exist", and they used different, incompatible definitions of agent identity.
personaIdand rendered one card per group viapickProfileAgent.Nothing kept them in step, so the library silently dropped agents the autocomplete could see.
What that looks like with real data. An owner installs the built-in Welcome team and renames some instances. A persona group holding
Claude ×2andFizz ×2rendered one card, labelled"Fizz"(the persona's name) but wired to a Claude instance. SoFizzhad no card of its own, and the card that said "Fizz" opened something else. Four identities, one card, two agents unmanageable from the library. Renaming an agent — which keeps its built-inpersona_id— is all it takes.The fix
desktop/src/features/agents/lib/agentIdentity.tsbecomes the single shared definition, and the distinction it draws is the whole point:agentIdentityKey({pubkey})→pubkey:<hex>. The identity. The fix(desktop): retain distinct agent instances in autocomplete #5202 doctrine comment moved here; autocomplete now imports it instead of re-deriving it.agentDisplayGroupKey({personaId, name})→ presentation only: which agents may share one card. Documented as never a substitute for identity — a display group must retain every member identity.Why not one card per pubkey. That would be 18 cards for 11 agents and would revert a deliberate product decision pinned by an existing e2e (
agents.spec.ts: "duplicate instances move from the agents gallery into the agent profile", unchanged by this PR). #5202's argument doesn't transfer: a collapsed pubkey in autocomplete becomes unmentionable, whereas a gallery card enumerates every instance behind it.So collapsing stays, bounded by one rule: a card may only stand for instances whose label it truthfully shows. A persona whose instances all fold to one name → one card, byte-identical to today. Once a persona holds two or more distinct names, each name gets its own card — titled with the instance name, carrying the persona name on a second line so the persona stays findable.
Follow-through the split required:
useCanonicalManagedAgentProfilecollapsed any profile open ontopickProfileAgent(personaInstances), so a new "Fizz" card would have been visible but still opened Claude.pickCanonicalProfileAgentnow canonicalises within the requested instance's display group. Persona-level destructive actions (edit/delete/share) stay on exactly one card per persona, chosen status-independently so the Delete menu no longer migrates when an agent starts.Two degenerate inputs that reintroduce the same failure
Both were found by exercising
agentIdentity.tsdirectly, and both are fixed here:foldAgentDisplayNamelowercased without normalizing, so one name in NFC and NFD folded to two 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 got two cards with visually identical labels — a split that cannot be seen, explained, or fixed from the UI. Folding now normalizes to NFC first.agentDisplayGroupKeyjoined its segments with a literal|name:, but a display name is free text. Before the fix,{personaId:"a", name:"x|name:y"}and{personaId:"a|name:x", name:"y"}both producedpersona:a|name:x|name:y— two different agents on one card, one of them no longer openable. Segments are now length-prefixed.Contract changes reviewers should know about
data-testidmoved frompersona-agent-row-<personaId>topersona-agent-row-<personaId>::<foldedInstanceName>for split personas. Unsplit personas keep the old id exactly. The card's React key follows the same shape, deliberately derived from persona id + folded name rather than from runtime status, so a card no longer remounts (and refires its avatar query) when a sibling starts.<instance name> agent profilerather than<persona name> agent profile. Every existing spec that addresses a card by aria-label uses a single-instance fixture and is unaffected.profile.spec.tspreviously asserted that clicking the avatar on an older agent message opened the persona's running instance ("Stop"). It now asserts it opens the instance the message actually came from ("Start agent"). That is the point ofpickCanonicalProfileAgent: a message avatar must land where that instance's own card lands. The test was rewritten as a parity test rather than deleted — it still asserts the message and the library resolve to the identical profile contract.Testing
New unit coverage, all red-before-green:
agentIdentity.test.mjs(new, 6 tests) — identity is the pubkey and nothing else; renamed instances separate; NFC/NFD fold to one card; the group key cannot be forged by a name containing a separator. WithagentIdentity.tsreverted and the tests kept: 4 pass / 2 fail (the documentation test passes either way, which is correct).unifiedAgentGroups.test.mjs(new, 14 tests) — that module had no test file, because the one-card-per-persona decision lived inline in JSX where nothing could reach it. Covers: 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 and do not move when an instance starts; card keys stay unique and status-independent.pickProfileAgent.test.mjs(+94) — 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 unit tests alone could not give: the unit suite still passed with
UnifiedAgentsSection.tsxreverted. The newagents.spec.tscase 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 — revertingunifiedAgentGroups.ts+UnifiedAgentsSection.tsxand rebuilding makes it fail.agent-lifecycle-feedback.spec.tswas updated for the split-card labels.Measured results:
7466131): full desktop unit suite exit 0;tsc --noEmitclean; biome clean.192336f(the last commit to touch UI or e2e files;7466131is lib + unit-test only): Playwright smoke 1034 passed / 3 skipped / 0 failed,agents.spec.ts37/37,check:px-text/check:file-sizes/check:pubkey-truncationclean.Note for maintainers: repo CI has never run on this branch — every workflow run sits at
action_requiredpending approval, soDCO Checkis the only executed check.Scope
A persona with exactly one instance keeps the persona's name on its card, even if that instance was renamed. Nothing is hidden in that case — there is one card and one agent — so it is out of scope here, but it means the "card labelled Fizz opens Claude" mislabelling still exists for solo personas. Worth deciding separately.
No agent record is merged, renamed, or deleted — kind:5 tombstones propagate cross-device while the nsec does not, so a merge would strand agents on other machines.
Deliberately not included, though found while investigating: Windows sleep prevention is a silent no-op (
prevent_sleep.rsis macOS-only),useChannelAgentSessionslets a local record's status overwrite the relay's, andwelcomeGuide's relay-scoped match mints a fresh keypair per community. Each is a separate PR.