fix(desktop): discover shared managed agents - #5832
Conversation
Build the mention directory from owner-verified managed-agent policy and relay-signed channel membership instead of the unused agent-profile kind. Co-authored-by: diegorumo <diegorumo@gmail.com> Signed-off-by: diegorumo <diegorumo@gmail.com>
Preserve headless directory profiles, paginate relay reads, and fail closed across managed-policy collisions. Co-authored-by: diegorumo <diegorumo@gmail.com> Signed-off-by: diegorumo <diegorumo@gmail.com>
Chessing234
left a comment
There was a problem hiding this comment.
two things worth flagging.
agent_discovery.rs:1053 was the only non-test caller of nostr_convert::agents_from_events, so this removes desktop's only kind:10100 reader. an agent that does publish a valid 10100 profile now disappears from the directory entirely rather than just losing fields — and #5691 is adding exactly that publisher on the buzz-acp side, so the two land in direct conflict.
second, channels is hardcoded to Vec::new(). useManagedAgentActions.ts:107 only seeds the channel pills when ra.channels.length > 0 (pairing it index-wise with channelIds), and the kind:39002 fallback right below it is guarded to managed agents only, so shared agents end up with no channel pills even though channel_ids is now correct.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
The ownership verification is headed in the right direction, but the candidate-discovery boundary is attacker-controlled before verification.
managed_agent_pubkeys_from_events collects every syntactically valid d tag from every member-authored kind 30177 event, and list_relay_agents then issues one kind-0 query per 250 collected pubkeys. Kind 30177 currently requires only ordinary UsersWrite; ingest does not bind its d tag to an identity owned by the signer. Because it is parameterized-replaceable by (author, d), any community member can retain an arbitrary number of forged coordinates. Those records fail the later NIP-OA check, but only after forcing every Desktop client to download the unbounded 30177 set and fan it out into an unbounded number of profile queries on each directory refresh. A large enough set can also make one chunk fail, at which point this code returns an error and mention admission fails closed for the entire directory. Thus an unprivileged member can turn invalid records into community-wide read amplification and agent-mention denial of service.
Please bound the work from an authoritative candidate set before reading policy. The relay-signed kind 39002 membership heads already provide that set: collect channel-member bot pubkeys visible to the viewer, then query kind 30177 by bounded #d chunks (and kind 0 by the same bounded candidates), rather than scanning all owner-authored policy and trusting its d values as the query plan. At minimum, the implementation needs a hard total-work bound plus a regression test demonstrating that arbitrarily many forged 30177 coordinates cannot increase downstream profile-query fanout or suppress otherwise valid directory entries.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Two additional requirements before this is mergeable:
-
Please remove
docs/handovers/2026-08-13-shared-agent-picker.mdfrom the PR. It is a transient implementation handoff, repeats the PR description, and even leaves “open the upstream PR” as pending inside the upstream PR. That does not belong in the shipped product documentation. -
The converter unit tests are useful, but they do not test the feature boundary that was broken or the new multi-query orchestration in
list_relay_agents. We need regression coverage that fails onmainand proves the complete behavior: a non-owner viewer discovers an owner-authenticated managed agent from 30177 + the latest OA-bound kind 0 + relay-signed 39002 membership, sees it admitted in a shared channel, selects it, and sends kind 9 with the agentptag. The command/query layer also needs discriminating tests for pagination and partial failures, including:- permissive legacy 10100 plus authenticated-but-unparseable managed policy must fail closed rather than downgrade;
- forged/unverified 30177 records cannot expand downstream query work or suppress valid agents;
- latest/revoked OA profile and equal-timestamp canonical heads;
- 10100 compatibility and managed-policy precedence;
- relay-authority or membership-query failure behavior;
- more than one page of directory/membership results, including a dense tied timestamp boundary, with no duplicates, skips, or infinite cursor loop.
The existing mentions.spec.ts cases seed already-materialized relayAgents; they validate admission and outbound tagging after discovery, but they do not exercise this PR’s actual relay-event-to-directory path. Please add coverage at the lowest practical seam that runs list_relay_agents against controlled relay query responses, plus the user-visible mention assertion. Otherwise all the new pure converters can be green while the integration remains broken, which is precisely the sort of cheerful green trap this bug has already enjoyed.
wesbillman
left a comment
There was a problem hiding this comment.
Princess Donut, automated reviewer, commenting via Wes’s GitHub account.
CHANGES REQUESTED at exact PR head 2ae62afd1f3e6f2e8abd792aa66ec079c61ab73b.
I independently confirm the Court’s two security/correctness blockers:
-
Unbounded pre-authentication fanout / mention denial of service.
list_relay_agentsfully paginates every visible kind 30177, extracts every valid-lookingdvalue, and performs a kind-0 query for each 250 candidates before NIP-OA owner verification (agent_discovery.rs:1072-1101,nostr_convert.rs:503-512). Kind 30177 requires only ordinaryUsersWrite(crates/buzz-relay/src/handlers/ingest.rs:345-353) and replacement is per(kind, author, d), so one member can retain arbitrarily many forged coordinates. Later rejection does not undo the read amplification; one failed profile chunk fails the entire directory. Build the candidate set from bounded relay-authoritative membership before policy/profile lookup, or impose a hard total-work cap that cannot let forged coordinates amplify queries or suppress valid agents. Add a regression at the query-orchestration seam. -
Authenticated malformed/future policy downgrades to stale legacy permissions. The merge seeds kind 10100 entries first (
nostr_convert.rs:553-557), but an owner-authenticated 30177 whose content cannot parse is merely skipped (:592-604); only successfully parsed managed policy overwrites legacy (:558-560). Thus a stale legacyrespond_to: anyonerecord survives when the current cryptographically owned policy is corrupt or uses a future enum variant. Once a current 30177 coordinate matches the verified owner, reserve it before parsing; parse failure must omit/fail that agent rather than resurrect legacy authority. Add the permissive-10100 + valid OA profile + authenticated invalid-30177 regression.
Downstream scope is otherwise intentionally pinned: channel composers require authoritative channel membership, while community scope permits an explicitly allowlisted viewer without shared membership (agentAutocompleteEligibility.ts:12-41,66-79; test lines 136-165). Pagination’s (until,before_id) cursor matches relay storage ordering and predicate (buzz-db/src/event.rs:55-59,492-507,552-562), but only the cursor mutator—not multi-page orchestration/non-progress behavior—is tested.
Independent validation on a synthetic application of this PR onto current main 78cbffeb64c01220e705adf0aa9690fdbd0d7a37: 36/36 nostr_convert tests passed; the cursor unit test passed; git diff --check passed. The focused JS test could not run in this worktree because Desktop dependencies were not installed (typescript missing), so I do not claim that suite. GitHub exposes DCO only, not behavioral CI.
Also remove docs/handovers/2026-08-13-shared-agent-picker.md; it is transient handoff state and its “full suite green” language is not release documentation or proof of these boundaries.
What changed
Desktop now builds one shared-agent directory from both deployed agent shapes:
10100runtime profiles for headless and legacy agents30177policy for Desktop-managed agents0NIP-OA proof that binds each managed agent to its owner39002membership for channel IDsVerified kind
30177policy wins when the same agent also has a kind10100profile. A failed policy or owner-proof query cannot downgrade a managed agent to stale kind10100permissions.The old Desktop path only read kind
10100. Desktop-managed agents do not publish their policy there, so another allowed member could not select the agent. The outgoing message then had no agentptag.Safety and scale
30177d-tags are skipped before author queries010100agentuntilplusbefore_idcursor0reads use bounded 250-author chunks10100failure can fall back to verified managed records, but managed-policy and owner-proof failures stop the readThis keeps compatibility with headless seats while closing the unverified d-tag issue discussed on #4716.
Closes #5240. Related reports: #5363, #3776, #3277, #2349, #2950. Related implementation work: #4713, #4714, #4716, #5483, #5546, #5691.
Verification
Exact head:
2ae62afd1f3e6f2e8abd792aa66ec079c61ab73bjust desktop-tauri-fmt-check: exit 0just desktop-tauri-clippy: exit 010100compatibility, managed-policy precedence, forged suppression, canonical head ordering, and composite cursor constructionNo agent configuration rules changed. This patch only changes relay discovery.