From c238c3404bb55bad0e7957bc90feaa07059d8793 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Sun, 25 Oct 2026 18:21:27 +0800 Subject: [PATCH] Stop reporting a share to a Bot the other home removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A friend's Bot that B removed from its home leaves no trace on A's side: the relationship stays `active`, so the row stayed, the click posted `grant_bot_to_friend` and the panel printed "Shared Chief with selftestb2." over a grant nothing can receive (#1128). The evidence was already on screen: `list_remote_bots`, the roster the panel paints from. `removedOnRoster` reads it: a row whose home is listed with Bots but whose Bot appears under neither its Bot id nor its handle is marked `removed` with the reason, and the host refuses the grant with `bot/removed-bot` before it is sent. A home the cache does not hold, a roster read that failed, an empty roster, or a handle hit under another id decides nothing — each of those three has its own test, so breaking one guard reds one test. Fixes #1128 --- changelog.d/1128-share-removed-bot.fixed.md | 1 + docs/subsystems/social.md | 4 + plugins/social/client.js | 12 +- plugins/social/src/social.js | 37 ++++- plugins/social/test/social.test.js | 171 ++++++++++++++++++++ 5 files changed, 223 insertions(+), 2 deletions(-) create mode 100644 changelog.d/1128-share-removed-bot.fixed.md diff --git a/changelog.d/1128-share-removed-bot.fixed.md b/changelog.d/1128-share-removed-bot.fixed.md new file mode 100644 index 00000000..1ce8ad77 --- /dev/null +++ b/changelog.d/1128-share-removed-bot.fixed.md @@ -0,0 +1 @@ +**Share with a friend** used to report success for a Bot the other home no longer runs. A row whose Bot is gone from the roster that home is still listed in now says so — ` was removed from that home. Sharing with it cannot reach anyone.` — and its **Share** is disabled; a click that still reaches the host is refused with `bot/removed-bot` and sends no grant, so the list stops claiming a share that would reach nobody. The row is judged on both the peer id and the handle (`list_remote_bots` entries carry both), and a home missing from the cached roster, or a roster read that failed or came back empty, decides nothing — it shares as before rather than guessing (#1128). diff --git a/docs/subsystems/social.md b/docs/subsystems/social.md index 9aea0688..b7c492ba 100644 --- a/docs/subsystems/social.md +++ b/docs/subsystems/social.md @@ -157,6 +157,10 @@ Creating a group opens its Session with zero turns, and the host draws no sessio Settings → **Bot Social** lists friends and shares once the server is connected (`state: 'connected'`). Until then the page is only the server form: a missing, connecting, or failed server draws neither section. **Friends** reads `social/overview` and shows each accepted friend (name, handle), each incoming request (accept it in Add friend), and each outgoing request that is still waiting. A name is one line: controls and angle brackets are stripped and the text is capped at 80, so a value that skipped the host's `cleanText` still cannot break the row. A failed read says so and is not drawn as an empty list. **Add friend** closes Settings and opens the sidebar panel the **+** menu already opens (`openAddFriend`); that **+** row stays. **Sharing** lists this home's Bots from `social/status` (the roster's own Bots, never a friend's, when that read fails or comes back empty) with the Main Bot first. Each **People with access** button opens that Bot's settings (`openSettings('bots', id)`) and scrolls to `ManageAccess`, where Revoke stays; opening Bot settings any other way does not scroll. Tests: `plugins/social/test/social.test.js` (`#1074`). +### A share to a Bot the other home removed (#1128) + +**Share with a friend** reads the roster it already paints from (`remoteBots()`), and a row whose Bot is no longer in it stays drawn with `removed: true` and the reason ` was removed from that home. Sharing with it cannot reach anyone.`; its **Share** is disabled, and a click that still reaches `social/share-with-friend` is refused with `bot/removed-bot` before any grant is sent (a `bot/` code is the only shape the channel passes through unchanged, `core/src/host/channel.js:24`, so the refusal reaches the panel as a code and not as `bot/refused`), so the panel never reports a share that reaches nobody. `removedOnRoster` decides it: both the row's `peerId` and its `handle` must be known (a home missing from the cached roster, or an empty `bots` list, decides nothing and shares as before rather than guessing), and a hit on either the id or the handle means the Bot is still there, because one id alone cannot tell an old server's renumbering from a removal. No extra round trip — it reads the roster the panel already paints from. Tests: `plugins/social/test/social.test.js` (`#1128`). + ### Add friend invite input (#310) `social/join-invite` runs the box text through `parseInviteInput` (`social.js`) before `join_invite`. It takes `https://host/i/CODE`, the same without the scheme, a link whose `/i/` is missing (host plus one bare token), a bare code, or the whole copied invite text (the first token with `/i/` or a URL wins), and sends only the code: Bot Social's `join_invite` looks up a code (it strips just a leading `…/i/`), so no host is ever built or added. Copy invite link hands out the server's `invite.url` through `inviteLink`, which rebuilds `origin/i/CODE` if a server ever answered a url without `/i/`; Bot Social itself always emits `/i/CODE` (`inviteUrl`) and serves only that route, so a link without `/i/` comes from hand-trimming or a client that dropped it. A pasted host that is not `config.server` is kept as `host`; a not-found then says the link may come from another server. `mapJoinInviteError` turns `invite_not_found`, expired / revoked / used invites, an own invite, `blocked` and `rate_limited` into one short sentence each ("This invite link wasn't found. It may be incomplete, expired, or already used. Ask your friend for a new link."); anything else passes through unchanged. The client (`plugins/social/client.js`) shows those sentences as written (#347): `errorText` relabels a message as "This invite was already used" only for the raw server words (`invite_unavailable`, `invite is used`, `invite_used`), never for the not-found sentence, which itself mentions "already used" as one possible cause; `run()` keeps its "… failed:" prefix for every other message and drops it only for the "already used" sentence; the neutral already-have sentence shows without error styling (#496). diff --git a/plugins/social/client.js b/plugins/social/client.js index fb98fd4e..690e684a 100644 --- a/plugins/social/client.js +++ b/plugins/social/client.js @@ -3464,6 +3464,8 @@ window.__ModuleLoader__.load({ // its busy flag, and what a fallback refresh removes. A friend without a Bot id keys on the person. const friendKey = friend => friend?.peerBotId || friend?.handle || friend?.home || friend?.ownerId || 'share' const share = async (friend) => { + // #1128: the button is disabled; a click that still arrives says why instead of posting a grant. + if (friend.removed === true) { setNote({ tone: 'error', text: friend.reason || 'That Bot was removed from that home.' }); return } setBusy(friendKey(friend)) setNote(null) try { @@ -3471,6 +3473,9 @@ window.__ModuleLoader__.load({ botId: bot.id, home: friend.home, ...(friend.peerBotId ? { friendBotId: friend.peerBotId } : {}), + // #1128: the row's handle goes with the click, so the host can tell "this Bot is gone from + // that home" (absent from the roster under both names) from an ordinary share. + ...(friend.handle ? { friendHandle: friend.handle } : {}), // #994: the receipt says who got it; the label the row already shows goes with the click. ...(friend.name ? { friendName: friend.name } : {}), ...(friend.ownerId ? { ownerId: friend.ownerId } : {}), @@ -3498,11 +3503,16 @@ window.__ModuleLoader__.load({ h('span', { className: 'fs-name' }, friend.name || friend.handle || friend.home), friend.handle || friend.home ? h('span', { className: 'fs-sub' }, friend.handle || friend.home) + : null, + friend.removed === true + ? h('span', { className: 'fs-sub', 'data-flybot': 'share-friend-removed' }, friend.reason || 'That Bot was removed from that home.') : null), h('button', { type: 'button', className: 'fs-btn fs-btn-sm', - disabled: busy === friendKey(friend), + // #1128: a row whose Bot the friend's home no longer lists stays visible, with the reason, and + // is not clickable: the grant would reach nobody. + disabled: busy === friendKey(friend) || friend.removed === true, 'data-flybot': 'share-friend', 'aria-label': `Share ${bot.name || 'this Bot'} with ${friend.name || friend.home}`, onClick: () => { void share(friend) }, diff --git a/plugins/social/src/social.js b/plugins/social/src/social.js index ddca1992..88dfea96 100644 --- a/plugins/social/src/social.js +++ b/plugins/social/src/social.js @@ -3118,6 +3118,24 @@ export function createSocial(service, config = {}, { client: given, api: givenAp // #500: friends who do not yet have this Bot — for Sharing → "Share with a friend". // Uses the active friendships (list_friends) so a friend we invited (and who has not // shared anything back) still appears. Homes already in People with access are omitted. + // #1128: the row names a Bot the friend's home no longer has — they removed it there — so a grant + // would name a Bot nothing can receive while the panel still printed "Shared …". The evidence is the + // roster this panel already reads (\`list_remote_bots\`): the Bot is absent from a home the cache holds, + // under both names we know (its Bot id and its handle). A home the cache does not hold, or a roster + // with no ids, is not evidence, so nothing here refuses on a guess. + function removedOnRoster(roster, homeKey, { peerId, handle } = {}) { + // Both names, or no answer: one id alone cannot be told from an older server that numbered its Bots + // differently, so a click without the row's handle is shared as before rather than refused. + if (!homeKey || !peerId || !handle) return false + const entry = (roster?.homes ?? []).find(item => item.home === norm(homeKey)) + if (!entry) return false + const bots = entry.bots ?? [] + if (!bots.length) return false + if (peerId && bots.some(bot => bot.botId && String(bot.botId) === String(peerId))) return false + if (handle && bots.some(bot => bot.handle && norm(bot.handle) === norm(handle))) return false + return true + } + async function shareFriendsForBot(botId) { if (!home) throw new Error('Bot Social is not connected: there is no FlyBot home yet') const bot = typeof botId === 'string' && botId ? flybot.bot.get(botId) : undefined @@ -3149,6 +3167,10 @@ export function createSocial(service, config = {}, { client: given, api: givenAp grantsByHome.set(key, entry) } const overview = await friendsOverview() + // #1128: what the friends' homes list right now; \`remoteBots()\` is the panel's own cached read, so a + // warm cache is no round trip and a cold one is the fetch the panel needed anyway. + const roster = await remoteBots().catch(error => { warn('roster not read for the share list', error); return null }) + const rowGone = (homeKey, item) => removedOnRoster(roster, homeKey, { peerId: item.peerBotId, handle: item.handle }) const friends = [] const seen = new Set() for (const item of overview.friends ?? []) { @@ -3171,9 +3193,13 @@ export function createSocial(service, config = {}, { client: given, api: givenAp const label = botName && ownerName ? `${botName} (${ownerName})` : botName || ownerName || String(homeKey).replace(/^@/, '').replace(/\.home$/i, '') || homeKey + // One roster read for the whole list, and only when a row has a Bot to look up: it is the same + // cached lookup the roster draws from, so an ordinary share costs no round trip (#1128). + const gone = rowGone(homeKey, item) friends.push({ home: homeKey, name: label, + ...(gone ? { removed: true, reason: `${label || 'That Bot'} was removed from that home. Sharing with it cannot reach anyone.` } : {}), ...(item.handle ? { handle: item.handle } : {}), ...(item.peerBotId ? { peerBotId: item.peerBotId } : {}), ...(item.ownerId ? { ownerId: item.ownerId } : {}), @@ -3220,7 +3246,7 @@ export function createSocial(service, config = {}, { client: given, api: givenAp // #500: grant this Bot to an existing friend (Bot Social grant_bot_to_friend). Does not // replace invite links. After a successful share, People with access re-syncs this Bot. - disposers.push(flybot.api.handle('social/share-with-friend', async ({ botId, home: friendHome, ownerId, friendOwnerId, friendBotId, peerBotId, friendName } = {}) => { + disposers.push(flybot.api.handle('social/share-with-friend', async ({ botId, home: friendHome, ownerId, friendOwnerId, friendBotId, peerBotId, friendName, friendHandle } = {}) => { if (!home) throw new Error('Bot Social is not connected: there is no FlyBot home yet') const bot = typeof botId === 'string' && botId ? flybot.bot.get(botId) : undefined if (!bot) throw new Error('Choose one of your Bots to share') @@ -3232,6 +3258,15 @@ export function createSocial(service, config = {}, { client: given, api: givenAp // never listed for a friend. const peerId = friendBotId || peerBotId if (!homeKey && !oid && !peerId) throw new Error('Choose a friend to share with') + // #1128: a Bot the friend's home no longer lists cannot receive this grant. Refuse instead of printing + // a success line over a grant nothing can use, and name what happened rather than "try again". + if (peerId) { + const roster = await remoteBots().catch(error => { warn('roster not read before sharing', error); return null }) + if (removedOnRoster(roster, homeKey, { peerId, handle: friendHandle })) { + const who = cleanText(friendName) || 'That Bot' + throw Object.assign(new Error(`${who} was removed from that home, so there is nothing there to share with. Ask them to add it back, then share again.`), { code: 'bot/removed-bot' }) + } + } if (typeof api.grantBotToFriend !== 'function') { throw Object.assign(new Error('Bot Social needs an update to share with a friend. Invite links still work.'), { code: 'unavailable' }) } diff --git a/plugins/social/test/social.test.js b/plugins/social/test/social.test.js index d3edc39e..10dfefc7 100644 --- a/plugins/social/test/social.test.js +++ b/plugins/social/test/social.test.js @@ -13569,3 +13569,174 @@ test('#1024: a pending friendship does not lend its id to the grant', async (t) }) assert.deepEqual(granted.map(a => a.friend_bot_id), ['pb-coder'], 'only the active friendship') }) + +// #1128: B removes a Bot from its home; A's relationship with it stays `active`, so the row is still +// drawn. A click used to print "Shared Chief with selftestb2." over a grant that names a Bot nothing can +// receive. The row is now judged against the roster the panel already reads (no extra round trip), so a +// Bot the other home still lists is still shared — the fix never guesses a removal. +test('#1128: a share to a Bot the other home still lists goes through as before', async (t) => { + const { ctx, home, call, stop } = await team() + t.after(stop) + // The roster's own answer: @bob.home lists x2 @writer.bob.home, so the row is not a removed Bot. + const server = mockServer() + const granted = [] + server.tools.list_friends = () => ({ + bot: { handle: '@danyang.home' }, + friends: [{ relationship_id: 'r-writer', status: 'active', peer: { bot_id: 'x2', handle: '@writer.bob.home', display_name: 'Writer', owner_display_name: 'Bob' } }], + pending_incoming: [], + pending_outgoing: [], + }) + server.tools.list_bot_grants = ({ bot_id }) => ({ bot: { bot_id }, grants: [] }) + server.tools.grant_bot_to_friend = (a) => { + granted.push(a) + return { grant_id: 'gr_1', status: 'active', friend: { handle: '@writer.bob.home', display_name: 'Writer', owner_display_name: 'Bob' } } + } + const social = await startSocial(ctx.flybot, { + server: 'https://social.test', credentialsPath: join(home, 'flybot-social.json'), + fetch: server.fetch, inboxPollMs: 0, + }) + t.after(() => social.dispose()) + await settle() + const chief = ctx.flybot.bot.list().find(b => b.isMain) || ctx.flybot.bot.list()[0] + await call('flybot/social/create-invite', { botId: chief.id }) + const listing = (await call('flybot/social/share-friends', { botId: chief.id })).value + assert.equal(listing.friends.length, 1) + assert.equal(listing.friends[0].removed, undefined, 'a Bot the other home still lists is not marked removed') + const shared = (await call('flybot/social/share-with-friend', { + botId: chief.id, home: '@bob.home', peerBotId: 'x2', friendHandle: '@writer.bob.home', friendName: 'Writer (Bob)', + })).value + assert.equal(shared.status, 'active') + assert.equal(granted.length, 1, 'the share went through') + assert.equal(granted[0].friend_bot_id, 'x2') +}) + +// #1128: the home is still listed, but the Bot the relationship names is gone from its roster — the friend +// removed it there. The row says so instead of reporting a share, and the host refuses the grant from the +// same cached roster, so an old row cannot post a grant that reaches nobody. +test('#1128: a Bot the other home no longer lists is not shared, and the row says why', async (t) => { + const { ctx, home, call, stop } = await team() + t.after(stop) + const server = mockServer() + const granted = [] + server.tools.list_friends = () => ({ + bot: { handle: '@danyang.home' }, + friends: [{ relationship_id: 'r-writer', status: 'active', peer: { bot_id: 'pb-writer', handle: '@writer.bob.home', display_name: 'Writer', owner_display_name: 'Bob' } }], + pending_incoming: [], + pending_outgoing: [], + }) + // The home is on the roster and lists another Bot, but not the one the relationship names. + server.tools.list_remote_bots = () => ({ + homes: [{ + relationship_id: 'r-writer', + home: { bot_id: 'h2', handle: '@bob.home', display_name: 'Bob', owner_display_name: 'Bob' }, + bots: [{ bot_id: 'x1', handle: '@coder.bob.home', display_name: 'Coder' }], + }], + }) + server.tools.list_bot_grants = ({ bot_id }) => ({ bot: { bot_id }, grants: [] }) + server.tools.grant_bot_to_friend = (a) => { + granted.push(a) + return { grant_id: 'gr_1', status: 'active' } + } + const social = await startSocial(ctx.flybot, { + server: 'https://social.test', credentialsPath: join(home, 'flybot-social.json'), + fetch: server.fetch, inboxPollMs: 0, + }) + t.after(() => social.dispose()) + await settle() + const chief = ctx.flybot.bot.list().find(b => b.isMain) || ctx.flybot.bot.list()[0] + await call('flybot/social/create-invite', { botId: chief.id }) + const listing = (await call('flybot/social/share-friends', { botId: chief.id })).value + const row = listing.friends.find(f => f.peerBotId === 'pb-writer') + assert.ok(row, 'the relationship is still active, so the row is still drawn') + assert.equal(row.removed, true, 'the row is marked as removed from that home') + assert.match(row.reason, /removed from that home/, 'the row carries the reason it cannot be shared') + const result = await call('flybot/social/share-with-friend', { + botId: chief.id, home: '@bob.home', peerBotId: 'pb-writer', friendHandle: '@writer.bob.home', friendName: 'Writer (Bob)', + }) + assert.equal(result.ok, false, 'the share is refused, not reported as done') + assert.match(String(result.error?.message || result.error), /removed from that home, so there is nothing there to share with/) + // The client branches on the code, so the code is part of the API contract, not just the sentence (#1139). + assert.equal(result.error?.code, 'bot/removed-bot', 'the refusal carries a real code, not bot/refused') + assert.equal(granted.length, 0, 'no grant is sent to a Bot that is gone') +}) + +// #1128: the row keeps its place in the panel with the reason and a disabled button. A click that reaches +// it anyway (an old row, a keyboard focus) says why instead of posting a share. +test('#1128: a row for a Bot the other home removed is shown with the reason and cannot be shared', async () => { + const calls = [] + const friends = [ + { home: '@bob.home', handle: '@coder.bob.home', name: 'Coder (Bob)', peerBotId: 'pb-coder' }, + { home: '@bob.home', handle: '@writer.bob.home', name: 'Writer (Bob)', peerBotId: 'pb-writer', removed: true, reason: 'Writer (Bob) was removed from that home. Sharing with it cannot reach anyone.' }, + ] + const { plugin, all, text, setters } = clientPlugin([friends, '', null], { + call: async (endpoint, payload) => { + calls.push([endpoint, payload]) + if (endpoint === 'social/share-friends') return { botId: payload.botId, friends } + if (endpoint === 'social/share-with-friend') return { message: 'Shared Writer with Writer (Bob).', status: 'active' } + return {} + }, + }) + const nodes = all(plugin.ShareWithFriend({ bot: { id: 'b', name: 'Writer' } })) + assert.equal(nodes.filter(n => n.props?.['data-flybot'] === 'share-friend-row').length, 2, 'the removed row stays visible') + const removed = nodes.find(n => n.props?.['data-flybot'] === 'share-friend-removed') + assert.ok(removed, 'the row says why it cannot be shared') + assert.match(text(removed), /was removed from that home/) + const buttons = nodes.filter(n => n.props?.['data-flybot'] === 'share-friend') + assert.equal(buttons[1].props.disabled, true, 'the removed row cannot be shared') + assert.equal(buttons[0].props.disabled, false, 'the row beside it still works') + // A click that arrives anyway says why instead of going quiet, and never posts the grant. + await buttons[1].props.onClick() + await new Promise(resolve => setTimeout(resolve, 0)) + assert.equal(calls.some(([endpoint]) => endpoint === 'social/share-with-friend'), false, 'no grant was requested') + const note = setters.map(([, value]) => value).find(value => value && typeof value === 'object' && /removed from that home/.test(String(value.text))) + assert.ok(note, 'a click that arrives anyway says why') + assert.equal(note.tone, 'error') +}) + +// #1128: the fix must never guess a removal. Each of the three inputs that only look like evidence — +// the roster read failing, a home listed with no Bots, a Bot whose handle is there under another id — +// has to leave the row shareable, because marking it removed disables Share and makes the host refuse +// with bot_removed: a worse bug than the one this fixes. One test per input, so the knife on each guard +// reds exactly one of them (Reviewer2 on #1139). +for (const [name, tools, expectation] of [ + ['the roster read fails', () => { throw new Error('roster down') }, 'a failed roster read is not evidence'], + ['the home is listed with no Bots', () => ({ homes: [{ relationship_id: 'r-writer', home: { bot_id: 'h2', handle: '@bob.home', display_name: 'Bob' }, bots: [] }] }), 'an empty roster is not evidence'], + ['the Bot is listed under a different id', () => ({ homes: [{ relationship_id: 'r-writer', home: { bot_id: 'h2', handle: '@bob.home', display_name: 'Bob' }, bots: [{ bot_id: 'x9', handle: '@writer.bob.home', display_name: 'Writer' }] }] }), 'a handle hit is still a hit'], +]) { + test(`#1128: shares as before when ${name} — the row is not marked removed`, async (t) => { + const { ctx, home, call, stop } = await team() + t.after(stop) + const server = mockServer() + const granted = [] + server.tools.list_friends = () => ({ + bot: { handle: '@danyang.home' }, + friends: [{ relationship_id: 'r-writer', status: 'active', peer: { bot_id: 'pb-writer', handle: '@writer.bob.home', display_name: 'Writer', owner_display_name: 'Bob' } }], + pending_incoming: [], + pending_outgoing: [], + }) + server.tools.list_remote_bots = tools + server.tools.list_bot_grants = ({ bot_id }) => ({ bot: { bot_id }, grants: [] }) + server.tools.grant_bot_to_friend = (a) => { + granted.push(a) + return { grant_id: 'gr_1', status: 'active', friend: { handle: '@writer.bob.home', display_name: 'Writer', owner_display_name: 'Bob' } } + } + const social = await startSocial(ctx.flybot, { + server: 'https://social.test', credentialsPath: join(home, 'flybot-social.json'), + fetch: server.fetch, inboxPollMs: 0, + }) + t.after(() => social.dispose()) + await settle() + const chief = ctx.flybot.bot.list().find(b => b.isMain) || ctx.flybot.bot.list()[0] + await call('flybot/social/create-invite', { botId: chief.id }) + const listing = (await call('flybot/social/share-friends', { botId: chief.id })).value + const row = listing.friends.find(f => f.peerBotId === 'pb-writer') + assert.ok(row, 'the relationship is active, so the row is drawn') + assert.equal(row.removed, undefined, expectation) + assert.equal(row.reason, undefined, 'no reason line without evidence') + const shared = (await call('flybot/social/share-with-friend', { + botId: chief.id, home: '@bob.home', peerBotId: 'pb-writer', friendHandle: '@writer.bob.home', friendName: 'Writer (Bob)', + })).value + assert.equal(shared.status, 'active', 'the share went through instead of refusing on a guess') + assert.equal(granted.length, 1, 'exactly one grant was sent') + }) +}