Skip to content

feat(desktop): guard the single definition of agent identity - #6077

Open
mfethe1 wants to merge 7 commits into
block:mainfrom
mfethe1:feat/check-agent-identity-guard
Open

feat(desktop): guard the single definition of agent identity#6077
mfethe1 wants to merge 7 commits into
block:mainfrom
mfethe1:feat/check-agent-identity-guard

Conversation

@mfethe1

@mfethe1 mfethe1 commented Aug 16, 2026

Copy link
Copy Markdown

Stacks on #6013 (head 7466131). This branch contains #6013's three commits; the last two are this PR's. Take #6013 first — this guard cannot pass without it (proof under Verification).

Reviewable delta only (2 commits, 6 files): 7466131...1e99477

(GitHub's "Files changed" here shows all 19 files because the base must be block/buzz:main#6013's branch lives on a fork, so this PR cannot be retargeted at it.)

Why

The bug #6013 fixes was a drift, not a typo. Two surfaces answer "which agents exist" — @-mention autocomplete and the Agents library — and each hand-rolled its own key for "same agent?". #5202 moved autocomplete to the pubkey; nobody moved the library, which still grouped by personaId. Renaming an instance then made it disappear from the library entirely.

Nothing failed loudly. No test broke. A card simply stopped existing.

#6013 fixes that by convention — one shared module, src/features/agents/lib/agentIdentity.ts, and everything imports it. Convention doesn't stop it recurring, so this PR encodes the invariant as a check.

What it does

desktop/scripts/check-agent-identity.mjs flags a string or template literal that begins an identity namespace — `pubkey:` or `persona:` — anywhere under desktop/src outside the canonical module. Those two prefixes 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.

The match captures the namespace plus the first interpolation or word after it (check-agent-identity.mjs:46), not the bare prefix, so a violation report names one key rather than a whole file.

Wired into desktop/package.json's check chain, which is the single place these guards are enumerated — Justfile:118 runs pnpm check and .github/workflows/ci.yml:192 runs just desktop-check, so no CI mirror needs updating.

The allowlist is empty, on purpose

The first version of this guard carried four path:literal exceptions and matched only the bare prefix — so each entry exempted every occurrence of that prefix in the file, and the exempted files were the agent-adjacent ones. That is theatre over exactly the code most likely to drift. Reproduced before fixing: a second, unrelated `persona:${a.personaId}` added to an allowlisted file passed at exit 0.

The fix is not a tighter allowlist but removing the need for one. This is the product-code half of the diff, and it changes runtime string values:

  • desktop/src/features/agents/ui/PersonaCatalogDialog.tsx:60 — the dialog's radio-group selection token becomes catalog-persona: via a named constant, replacing four inline "persona:" literals. The token addresses a row in that dialog's own list, is produced and consumed only in that file, and is never persisted or sent over the wire.
  • desktop/src/features/profile/ui/UserProfilePanelUtils.ts:104 (profilePanelTargetKey) and desktop/src/features/profile/ui/UserProfilePanel.tsx:222 — the profile panel's render key becomes profile:. That key 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.

overrides at check-agent-identity.mjs:67 is now new Set([]), and documented as worth keeping that way.

Deliberately narrow

A guard that cries wolf gets disabled, so this one is scoped to the shape that caused the outage:

  • It does not try to catch every possible way of grouping agents.
  • Comment lines are skipped. Unrelated subsystems document their own scope keys — observedUnreadStorage.ts:295 and threadActivityStorage.ts:155 describe a "pubkey:normalizedRelayUrl" scope in prose — and flagging prose teaches people to silence the guard rather than read it.
  • Scans src/** for .ts/.tsx only, the same rule shape as desktop/scripts/check-pubkey-truncation.mjs:12-16. .test.mjs and desktop/tests/ are therefore out of scope.

Verification

Numbers below are from the head commit 1e99477.

  1. Passes on this branch. node desktop/scripts/check-agent-identity.mjs → exit 0, no output.
  2. Catches the real regression, not a synthetic one. Restoring the pre-fix(desktop): retain distinct agent instances in autocomplete #5202 `persona:${candidate.personaId}` into agentAutocompleteEligibility.ts → exit 1:
    Desktop agent-identity check failed:
    - src/features/agents/lib/agentAutocompleteEligibility.ts:295: `persona:${candidate.personaId}
    
  3. The stack order is a hard dependency. The same script run against main (f956e6f) → exit 1 with 7 violations, including src/features/agents/lib/agentAutocompleteEligibility.ts:297: \pubkey:${normalizePubkey(candidate.pubkey)}` — the line fix(desktop): stop the agents library hiding renamed agent instances #6013 deletes. Merging this before fix(desktop): stop the agents library hiding renamed agent instances #6013 turns CI red.
  4. pnpm check exits 0 with all four guards in the chain; desktop unit suite 4978/4978; tsc --noEmit clean; biome clean on the changed files.

check-file-sizes note: UserProfilePanel.tsx sits one line under the 1000-line ceiling, so this PR's change there is net 0 lines (1 added, 1 removed). A redundant comment was dropped to keep it there.

Out of scope

  • Desktop only. web/src has no agents feature, and git grep -n "personaId\|persona_id" -- mobile/lib returns nothing, so there is no mirror of this invariant to add and no shared scripts/*-core.mjs to extract.
  • No relay, event-kind, migration, or schema change — the diff touches desktop/ exclusively.
  • The guard does not govern the new catalog-persona: and profile: namespaces; they are single-file and single-purpose by construction.

@themiguelamador themiguelamador left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found two blocking issues:

  • The guard's path:matchedLiteral allowlist was file-wide in practice. Once a file was allowed for, say, a `persona:` prefix, any future hand-rolled identity key with that prefix in the same file was silently accepted. The four exceptions are adjacent to agent identity code, so this leaves exactly the high-risk files outside the invariant the guard claims to enforce.
  • Three commits in the stacked history contain AI Co-Authored-By trailers, which this repository's AGENTS.md explicitly prohibits. Those commits need to be rewritten before merge while retaining the human DCO signoffs.

I fixed both on Complear:review/pr-6077-fix; the tip is e84d14f17. The legitimate catalog-selection and profile-render tokens now use distinct catalog-persona: and profile: namespaces, so the identity guard has no exceptions at all. I also rewrote the fix-branch history to remove the prohibited trailers without changing any commit trees or human signoffs.

Verification:

  • node scripts/check-agent-identity.mjs
  • node scripts/check-file-sizes.mjs
  • focused UserProfilePanelUtils.test.mjs — 10 passed
  • tsc --noEmit
  • Biome on all five changed files
  • pre-commit desktop formatting and file-size hooks
  • git diff --check

mfethe1 pushed a commit to mfethe1/buzz that referenced this pull request Aug 16, 2026
…listing them

Review follow-up on PR block#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-block#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: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
mfethe1 pushed a commit to mfethe1/buzz that referenced this pull request Aug 16, 2026
…listing them


Review follow-up on PR block#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-block#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 <michael@jira-flow.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
@mfethe1
mfethe1 force-pushed the feat/check-agent-identity-guard branch from e9533fa to 1e99477 Compare August 16, 2026 22:38
mfethe1 pushed a commit to mfethe1/buzz that referenced this pull request Aug 17, 2026
…listing them


Review follow-up on PR block#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-block#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 <michael@jira-flow.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
@mfethe1
mfethe1 force-pushed the feat/check-agent-identity-guard branch from 1e99477 to 20e7cff Compare August 17, 2026 01:17
@mfethe1
mfethe1 marked this pull request as ready for review August 17, 2026 11:11
@mfethe1
mfethe1 requested a review from a team as a code owner August 17, 2026 11:11
@mfethe1

mfethe1 commented Aug 17, 2026

Copy link
Copy Markdown
Author

@themiguelamador — your review was filed against dad6a18, which is no longer in this branch's history. Both findings are addressed at head 20e7cff; requesting a re-review or dismissal.

1. The allowlist was file-wide — you were right, and I reproduced it before fixing. Keying on path:matchedLiteral where the literal was only the namespace prefix meant one entry exempted every occurrence of that prefix in the file, and the exempted files were the agent-adjacent ones. Planting a second, unrelated `persona:${a.personaId}|${a.name}` in an allowlisted file passed at exit 0.

Fixed the way you suggested — by removing the need for exceptions rather than tightening them. The two legitimate non-identity uses now carry their own namespaces: catalog-persona: for the persona catalog dialog's selection token (component-internal, never persisted) and profile: for the profile panel's render key (which exists precisely when there is no agent to identify). overrides is now empty. The match also widened from the bare prefix to the prefix plus what follows it, so a future entry would scope to one literal rather than a file.

Both attack cases re-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.

The tip commit also fixes a stale header comment that still advertised "allowlisted below with a reason each" — the first thing a re-reader would have hit.

2. Attribution trailers are removed across this branch's history; every commit carries only human Co-authored-by: / Signed-off-by:. Same good-faith note as on #6036: I couldn't locate that rule in the repo — AGENTS.md:132 covers only the DCO signoff, and main itself carries maintainer commits with agent co-author trailers. Pointer welcome if it lives somewhere I missed.

Ordering note for whoever merges: this PR stacks on #6013 and contains its commits. The guard run against origin/main alone exits 1, because one of the violations it reports is the exact line #6013 deletes. #6013 must merge first or CI goes red.

Not claimed as green: repository CI has never run on this PR — all runs are at action_required. Local pnpm check (all four guards) and the full desktop suite pass; those numbers are author-local.

mfethe1 pushed a commit to mfethe1/buzz that referenced this pull request Aug 17, 2026
…listing them


Review follow-up on PR block#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-block#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 <michael@jira-flow.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
@mfethe1
mfethe1 force-pushed the feat/check-agent-identity-guard branch from 20e7cff to 998e0af Compare August 17, 2026 19:20
mfethe1 pushed a commit to mfethe1/buzz that referenced this pull request Aug 17, 2026
…listing them


Review follow-up on PR block#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-block#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 <michael@jira-flow.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
@mfethe1
mfethe1 force-pushed the feat/check-agent-identity-guard branch from 998e0af to 9f987ae Compare August 17, 2026 20:11
Michael Feth added 7 commits August 18, 2026 10:21
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>
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?", block#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-block#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 <michael@jira-flow.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
…listing them


Review follow-up on PR block#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-block#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 <michael@jira-flow.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
… longer has

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 <michael@jira-flow.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
@mfethe1
mfethe1 force-pushed the feat/check-agent-identity-guard branch from 9f987ae to e6c6a5f Compare August 18, 2026 14:38
mfethe1 pushed a commit to mfethe1/buzz that referenced this pull request Aug 18, 2026
`normalizePubkey` is `trim().toLowerCase()`. Across `features/agents`, 20 call
sites reached for a bare `.toLowerCase()` instead. The two agree for every
well-formed pubkey and disagree the moment one carries surrounding whitespace,
so the divergence is invisible until a value arrives from somewhere untidy — a
pasted allowlist entry, a relay tag, a config file — and then one surface stops
matching another with nothing thrown and nothing logged.

That is the same failure shape the agent-identity work keeps turning up: two
places answer "is this the same agent?" differently, and a user reports that an
agent "isn't there".

One site was already asymmetric rather than merely inconsistent.
`RespondToField.handleRemove` compared `p.toLowerCase()` against a pubkey that
had been normalized, so removing an allowlist entry could fail to match the
entry it was given. That is a live bug, not a style point.

## Guard

`check-pubkey-normalization.mjs` matches any `.toLowerCase()` call and then
filters on the receiver, rather than anchoring the pubkey inside the pattern.
The first version did anchor it, and could not see
`agent?.pubkey.toLowerCase()` — the optional link sits between the identifier
and the segment being matched. Filtering on the receiver found three
optional-chained sites the initial sweep had skipped, two of which the anchored
pattern could never have reported.

Scoped to `src/features/agents` deliberately. Hand-rolled lowercasing is
repo-wide — 87 files under `desktop/src` against 113 using `normalizePubkey` —
and failing on all of them would make the guard unshippable. A guard that must
be disabled to land anything protects nothing. So it covers the surface whose
divergence caused an outage, and covers it completely: no allowlist. Widening it
is a follow-up that has to arrive with the call-site fixes, not a flag flip.

## Interaction with block#6077

Both this PR and block#6077 add a `check:*` script and extend the same `check` line
in `desktop/package.json`, so whichever lands second needs a one-line rebase
there. Nothing else overlaps: block#6077 guards how an identity KEY is minted, this
guards how the pubkey inside it is normalized.

Note that `main` recently removed `pnpm check:file-sizes` from that line (block#6187
made the ratchet a first-class root gate). Neither PR should put it back.

## Verification

`npx tsc --noEmit` clean. `pnpm check` clean. Desktop unit suite: 4993 passed,
0 failed.

Guard proven to fail, not just to pass: an injected `x.pubkey.toLowerCase()`
under `features/agents` is reported and exits 1; removing it exits 0.

Signed-off-by: Michael Feth <michael@jira-flow.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants