From ec2417bebe7582cc9f459e37ac6021a01b72cd10 Mon Sep 17 00:00:00 2001 From: Logan Johnson Date: Fri, 2 Oct 2026 16:06:18 -0400 Subject: [PATCH] fix(desktop): check every signed recipient on the roster, after agent checks The publish pass now reads the destination's member list after agent authorization finishes, and requires every recipient it will sign to be on it, managed and relay agents included. A person or agent removed while an upload, invite or agent directory read is in flight is refused, and the draft is kept for retry. Agent policy and lifecycle checks still run first. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson --- .../forum/ui/ForumComposer.lifecycle.test.mjs | 127 +++++++++++++++++- .../messages/lib/agentMentionRevalidation.ts | 60 ++++----- .../ui/useMentionSendFlow.membership.test.mjs | 89 ++++++++---- 3 files changed, 219 insertions(+), 57 deletions(-) diff --git a/desktop/src/features/forum/ui/ForumComposer.lifecycle.test.mjs b/desktop/src/features/forum/ui/ForumComposer.lifecycle.test.mjs index c8d9bdd038..91c8b01157 100644 --- a/desktop/src/features/forum/ui/ForumComposer.lifecycle.test.mjs +++ b/desktop/src/features/forum/ui/ForumComposer.lifecycle.test.mjs @@ -317,11 +317,23 @@ async function setup(options = {}) { } if (command === "get_channels") return { hash: "h", channels: [], last_messages: {} }; + // Main readiness syncs a mentioned member agent into any live Huddle. + if (command === "sync_agents_to_active_huddle") + return { changed_channel_ids: [], error: null }; throw new Error(`unmocked ${command}`); }, }; const agentHooks = { - useManagedAgentsQuery: () => query(() => control.agents), + useManagedAgentsQuery: () => ({ + ...query(() => control.agents), + refetch: async () => { + calls.push(["agents-read"]); + // Hold only the final publish pass, not chat's earlier prepare. + if (control.agentRead && control.phase === "publish") + await control.agentRead.promise; + return { data: control.agents, error: null }; + }, + }), useRelayAgentsQuery: () => query(() => []), usePersonasQuery: () => ({ ...query(() => control.personas), @@ -332,7 +344,7 @@ async function setup(options = {}) { }), useTeamsQuery: () => query(() => control.teams), }; - const revalidation = await loadWithImports( + const productionRevalidation = await loadWithImports( "../../messages/lib/agentMentionRevalidation.ts", { "@/shared/api/tauriRelayAgents": { @@ -344,6 +356,22 @@ async function setup(options = {}) { }, }, ); + // Record the running pass so a test can hold only the publish pass. + const revalidation = { + ...productionRevalidation, + useAgentMentionRevalidation: (...args) => { + const revalidate = productionRevalidation.useAgentMentionRevalidation( + ...args, + ); + return React.useCallback( + (pubkeys, channelId, options = {}) => { + control.phase = options.phase; + return revalidate(pubkeys, channelId, options); + }, + [revalidate], + ); + }, + }; const productionMentions = await loadWithImports( "../../messages/lib/useMentions.ts", { @@ -1083,7 +1111,7 @@ for (const main of [false, true]) { .filter(([name]) => name === "error") .map(([, message]) => message), [ - "A mentioned person is no longer in this channel. Invite them or remove the mention, then retry.", + "Someone you mentioned is not in this channel now. Add them or remove the mention, then retry.", ], ); assert.equal( @@ -1094,3 +1122,96 @@ for (const main of [false, true]) { }); } } + +// Every signed recipient must be on the fresh roster, the managed agent +// included, and that roster read must follow agent authorization. Hold the +// managed-agent read, remove a recipient, then release it. +const SCOUT = { + pubkey: KEY, + role: "bot", + is_agent: true, + display_name: "Scout", +}; +for (const main of [false, true]) { + for (const [removed, when] of [ + ["agent", "after-selection"], + ["agent", "during-agent-read"], + ["person", "during-agent-read"], + ]) { + test(`mounted ${main ? "main" : "forum"} publish refuses the ${removed} removed ${when}`, async () => { + const s = await setup({ realMentions: true, main }); + s.control.cachedMembers = [ + { pubkey: HUMAN, role: "member", isAgent: false, displayName: "Pat" }, + { pubkey: KEY, role: "bot", isAgent: true, displayName: "Scout" }, + ]; + s.control.liveMembers = [PAT, SCOUT]; + s.navigate("b"); + await s.open(); + await s.choose("Pat"); + await s.open(); + await s.choose("Scout"); + assert.deepEqual( + [...s.mentions.extractMentionPubkeys(s.text)].sort(), + [KEY, HUMAN].sort(), + ); + const text = s.text; + const remove = () => { + const gone = removed === "agent" ? KEY : HUMAN; + s.control.liveMembers = s.control.liveMembers.filter( + (member) => member.pubkey !== gone, + ); + }; + if (when === "after-selection") remove(); + else s.control.agentRead = deferred(); + const reads = s.calls.length; + await s.submit(); + if (when === "during-agent-read") { + assert.equal(s.control.phase, "publish"); + assert.ok( + s.calls.slice(reads).some(([name]) => name === "agents-read"), + "publish is held at agent authorization", + ); + assert.equal(s.calls.filter(([name]) => name === "send").length, 0); + remove(); + await s.finish(s.control.agentRead); + await act(async () => {}); + } + await act(async () => {}); + assert.equal(s.calls.filter(([name]) => name === "send").length, 0); + assert.deepEqual( + s.calls + .filter(([name]) => name === "error") + .map(([, message]) => message), + [ + "Someone you mentioned is not in this channel now. Add them or remove the mention, then retry.", + ], + ); + assert.equal( + s.text.trim(), + text.trim(), + "the draft is retained for retry", + ); + }); + } +} + +for (const main of [false, true]) + test(`mounted ${main ? "main" : "forum"} publish keeps a person and agent who are both still members`, async () => { + const s = await setup({ realMentions: true, main }); + s.control.cachedMembers = [ + { pubkey: HUMAN, role: "member", isAgent: false, displayName: "Pat" }, + { pubkey: KEY, role: "bot", isAgent: true, displayName: "Scout" }, + ]; + s.control.liveMembers = [PAT, SCOUT]; + s.navigate("b"); + await s.open(); + await s.choose("Pat"); + await s.open(); + await s.choose("Scout"); + await s.submit(); + await act(async () => {}); + const sends = s.calls.filter(([name]) => name === "send"); + assert.equal(sends.length, 1); + assert.ok(sends[0].flat(2).includes(HUMAN)); + assert.ok(sends[0].flat(2).includes(KEY)); + }); diff --git a/desktop/src/features/messages/lib/agentMentionRevalidation.ts b/desktop/src/features/messages/lib/agentMentionRevalidation.ts index 82d36a0efa..8de9fd9db3 100644 --- a/desktop/src/features/messages/lib/agentMentionRevalidation.ts +++ b/desktop/src/features/messages/lib/agentMentionRevalidation.ts @@ -23,13 +23,13 @@ export class AgentMentionAuthorizationError extends Error { } } -/** A mentioned person is not a member of the destination at publication. */ +/** A signed recipient is not a member of the destination at publication. */ export class MentionMembershipChangedError extends Error { constructor(unverified = false) { super( unverified - ? "Could not check that the people you mentioned are still in this channel. Retry, or remove the mentions." - : "A mentioned person is no longer in this channel. Invite them or remove the mention, then retry.", + ? "Could not check that everyone you mentioned is still in this channel. Retry, or remove the mentions." + : "Someone you mentioned is not in this channel now. Add them or remove the mention, then retry.", ); this.name = "MentionMembershipChangedError"; } @@ -44,34 +44,30 @@ export function isMentionAuthorizationError(error: unknown): error is Error { } /** - * Publication signs a `p` tag for every person in `pubkeys`. Selection and - * the Invite prompt prove membership only at their own time, so read the - * destination's member list fresh and fail closed for any person who is not - * a member now (from the writer: a lagging replica can still list someone - * just removed). Send without inviting has already moved declined people to - * reference tags, and invited people are members by this point. + * Publication signs a `p` tag for every recipient in `pubkeys`, people and + * agents alike. Selection, Invite and agent attach prove membership only at + * their own time, so read the destination's member list fresh and fail + * closed for any recipient who is not a member now (from the writer: a + * lagging replica can still list someone just removed). Send without + * inviting has already moved declined people to reference tags. */ -export async function revalidateHumanMentionMembership({ +export async function revalidateRecipientMembership({ pubkeys, - agentPubkeys, channelId, fetchMembers = (id) => getChannelMembers(id, { readYourWrites: true }), }: { pubkeys: readonly string[]; - agentPubkeys: ReadonlySet; channelId: string; fetchMembers?: (channelId: string) => Promise<{ pubkey: string }[]>; }) { - const humans = [...new Set(pubkeys.map(normalizePubkey))].filter( - (pubkey) => !agentPubkeys.has(pubkey), - ); - if (humans.length === 0) return; + const recipients = [...new Set(pubkeys.map(normalizePubkey))]; + if (recipients.length === 0) return; const members = await fetchMembers(channelId).catch(() => null); if (!members) throw new MentionMembershipChangedError(true); const memberPubkeys = new Set( members.map((member) => normalizePubkey(member.pubkey)), ); - if (humans.some((pubkey) => !memberPubkeys.has(pubkey))) + if (recipients.some((pubkey) => !memberPubkeys.has(pubkey))) throw new MentionMembershipChangedError(); } @@ -184,18 +180,6 @@ export function useAgentMentionRevalidation({ ...getSelectedAgentPubkeys(), ...(options.intendedAgentPubkeys ?? []).map(normalizePubkey), ]); - // Only the explicit publish pass signs new recipients into a channel. - // DMs have fixed participants and no member list to change. - const humanMembership = - options.phase === "publish" && - destinationChannelId && - channelType !== "dm" - ? revalidateHumanMentionMembership({ - pubkeys, - agentPubkeys: knownAgentPubkeys, - channelId: destinationChannelId, - }) - : Promise.resolve(); const agents = revalidateAgentMentionPubkeys({ pubkeys, agentPubkeys: knownAgentPubkeys, @@ -210,7 +194,23 @@ export function useAgentMentionRevalidation({ "channelId" in scope ? (scope.channelId ?? undefined) : undefined, ), }); - return Promise.all([agents, humanMembership]).then(([result]) => result); + // Only the explicit publish pass signs new recipients into a channel. + // DMs have fixed participants and no member list to change. The roster + // read is the last awaited step: a removal during agent authorization + // must still be seen before signing. + if ( + options.phase !== "publish" || + !destinationChannelId || + channelType === "dm" + ) + return agents; + return agents.then(async (validated) => { + await revalidateRecipientMembership({ + pubkeys: validated, + channelId: destinationChannelId, + }); + return validated; + }); }, [ agentPubkeys, diff --git a/desktop/src/features/messages/ui/useMentionSendFlow.membership.test.mjs b/desktop/src/features/messages/ui/useMentionSendFlow.membership.test.mjs index 1be119722f..cda215b648 100644 --- a/desktop/src/features/messages/ui/useMentionSendFlow.membership.test.mjs +++ b/desktop/src/features/messages/ui/useMentionSendFlow.membership.test.mjs @@ -10,12 +10,15 @@ import { setup, deferred, KEY } from "./useMentionSendFlow.test-support.mjs"; const HUMAN = "d".repeat(64); const TEXT = "@Pat @RemoteScout hello"; const REMOVED = - "A mentioned person is no longer in this channel. Invite them or remove the mention, then retry."; + "Someone you mentioned is not in this channel now. Add them or remove the mention, then retry."; + +const AGENT_MEMBER = { pubkey: KEY, role: "bot", is_agent: true }; async function membershipSetup(delay) { const live = { members: [{ pubkey: HUMAN, role: "member", is_agent: false }], gate: null, + agentRead: null, }; window.__TAURI_INTERNALS__ = { invoke: async (command) => { @@ -34,10 +37,12 @@ async function membershipSetup(delay) { currentPubkey: "a".repeat(64), eligibilityScope: { type: "channel", channelId: "general" }, sharedChannelIds: new Set(), - refetchManagedAgents: async () => ({ - data: [{ pubkey: KEY }], - error: null, - }), + refetchManagedAgents: async () => { + // Hold only the final publish pass, not the earlier prepare pass. + if (live.agentRead && live.phase === "publish") + await live.agentRead.promise; + return { data: [{ pubkey: KEY }], error: null }; + }, }), ); const s = await setup(); @@ -49,6 +54,7 @@ async function membershipSetup(delay) { mentions.memberPubkeys = new Set([HUMAN]); mentions.revalidateMentionPubkeys = async (keys, channel, options) => { s.calls.push([options.phase, channel]); + live.phase = options.phase; return revalidate.current(keys, channel, options); }; if (delay === "upload") @@ -57,19 +63,28 @@ async function membershipSetup(delay) { return { s, live }; } +// Every signed recipient, the managed agent included, must be on the fresh +// roster. The agent joins through the Invite prompt, then can be removed. +const REMOVALS = { + none: (members) => members, + person: (members) => members.filter(({ pubkey }) => pubkey !== HUMAN), + agent: (members) => members.filter(({ pubkey }) => pubkey !== KEY), +}; + for (const delay of ["upload", "invite"]) { - for (const removed of [false, true]) { - test(`chat publish ${removed ? "refuses" : "keeps"} a person ${removed ? "removed" : "still present"} during the ${delay} delay`, async () => { + for (const removed of Object.keys(REMOVALS)) { + test(`chat publish ${removed === "none" ? "keeps everyone present" : `refuses the ${removed} removed`} during the ${delay} delay`, async () => { const { s, live } = await membershipSetup(delay); if (delay === "invite") s.control.add = deferred(); await s.prompt(TEXT); await s.invite(); assert.equal(s.events("add").length, 1); + live.members = [...live.members, AGENT_MEMBER]; if (delay === "upload") { assert.ok(s.control.uploadCallbacks); assert.equal(s.options.contentRef.current, "", "optimistic clear"); } - if (removed) live.members = []; + live.members = REMOVALS[removed](live.members); if (delay === "invite") await s.finish(s.control.add); else await s.act(async () => @@ -79,26 +94,52 @@ for (const delay of ["upload", "invite"]) { ), ); await s.flush(); - const sends = s.events("SEND"); - if (!removed) { - assert.equal(sends.length, 1); - assert.deepEqual(sends[0][2], [HUMAN, KEY]); - return; - } - assert.equal(sends.length, 0); - assert.deepEqual( - s.events("error").map(([, message]) => message), - [REMOVED], - ); - assert.equal( - s.options.contentRef.current, - TEXT, - "the draft is kept for retry", - ); + assertOutcome(s, removed !== "none"); }); } } +function assertOutcome(s, refused) { + const sends = s.events("SEND"); + if (!refused) { + assert.equal(sends.length, 1); + assert.deepEqual(sends[0][2], [HUMAN, KEY]); + return; + } + assert.equal(sends.length, 0); + assert.deepEqual( + s.events("error").map(([, message]) => message), + [REMOVED], + ); + assert.equal( + s.options.contentRef.current, + TEXT, + "the draft is kept for retry", + ); +} + +// The roster read must be the last check. A removal while the agent +// directory read is still pending has to be seen before signing. +for (const removed of ["person", "agent"]) { + test(`chat publish refuses the ${removed} removed while agent authorization is held`, async () => { + const { s, live } = await membershipSetup("invite"); + s.control.add = deferred(); + await s.prompt(TEXT); + await s.invite(); + live.members = [...live.members, AGENT_MEMBER]; + live.agentRead = deferred(); + await s.finish(s.control.add); + await s.flush(); + assert.equal(live.phase, "publish", "held in the final publish pass"); + assert.equal(s.events("SEND").length, 0); + assert.equal(s.events("error").length, 0, "not yet refused"); + live.members = REMOVALS[removed](live.members); + await s.finish(live.agentRead); + await s.flush(); + assertOutcome(s, true); + }); +} + test("Send without inviting still publishes: a declined person becomes a reference", async () => { const DECLINED = "e".repeat(64); const { s } = await membershipSetup("decline");