diff --git a/changelog.d/666-unfriended-send.fixed.md b/changelog.d/666-unfriended-send.fixed.md new file mode 100644 index 000000000..f07434b65 --- /dev/null +++ b/changelog.d/666-unfriended-send.fixed.md @@ -0,0 +1 @@ +A message to a friend's Bot right after that friendship was removed is now refused at once with the usual *not a Bot a friend shared with you* reason, and the Bot leaves the list, instead of being held for approval and then lost when the approval is clicked: the send policy asks Bot Social about the friendship itself rather than trusting the cached roster, and a removal event drops that cache ([#666](https://github.com/ThinkFlowLab/FlyBot/issues/666)). The same check runs again when a held card is approved, so a line whose friendship was removed while the card waited fails with a reason instead of being sent into a refusal. diff --git a/docs/subsystems/social.md b/docs/subsystems/social.md index 4f1ffdd7c..4e7b78d0c 100644 --- a/docs/subsystems/social.md +++ b/docs/subsystems/social.md @@ -130,6 +130,7 @@ pins the list. The authority for the list is Bot Social's README and `docs/FACE_ - **#496:** re-accepting an invite for a Bot this home already has is a result, not an error. On `409 relationship_exists` `social/join-invite` answers `{ alreadyHave: true, status: 'active', bot, botName, ownerName, restored: [{ handle, name, owner }] }`. Bot Social's 409 names no Bot, so the plugin reads the public invite info (`GET /v1/invites/{code}`, `inviteInfo` in `mcp-client.js`, sent without a token) for the invite's Bot and owner. `restored` lists only Bots that were locally hidden or declined and are listed again after the unhide. The panel shows neutral text `You already have Chief (SelfA1233).` (Bot first, owner in parentheses, #384), or `Chief (SelfA1233) is back in your list.` when something really came back; never red. Without a home (legacy) the join still throws, with the neutral `You already have this friend's Bot.`. - **#140:** Auto-send defaults off. Accepting a friend request uses that relationship's peer home for Auto-send, not the first roster entry. The owner sees Approve / Reject in the chat, not “Waiting for the owner's approval.” Composer drafts are labelled You. - **#136 (suffix):** Onboard retries truncate the sanitized handle body so `Name2` / `Name3` still change within the 32-character `homeHandleOf` limit. +- **#666:** A friendship can be removed outside this plugin — the agent's `remove_friend` tool, or the Bot Social console — and the cached roster used to keep that Bot for up to `REMOTE_TTL_MS` (30 s). A send in that window was accepted and held, and approving it lost the line: Bot Social answered `send_as_member: no active relationship between these bots`. The send policy now asks Bot Social about the friendship itself before it asks the owner: `friendshipActive` reads `list_friends` for the home and for the sending Bot's own member identity (a per-Bot friendship, #420, is listed there), accepts only an entry whose `status` is `active` (or absent) and whose peer handle belongs to that friend's home, and caches the answer for 2 s so a burst of lines costs one call per friend, not one per line. No answer at all (an older server, a link that is down) keeps the roster's verdict: the check catches a removal, it does not invent one. When the friendship is gone the send is denied with the usual *is not a Bot a friend shared with you* reason, above the hold and Auto-send branches, and `forgetFriendHome` drops that friend from the cached roster and republishes, so the list and the picker stop offering a Bot that can no longer be reached. Room lines (`via: room` / `room-share`) skip the check. The other half is the cache: a `relationship.changed` event in either direction now invalidates the roster (`fetchedAt = 0`) **and** every cached friendship answer (`forgetFriendStatus`), so a send in the same millisecond is judged on a fresh answer rather than the two-second one — the earlier version only dropped the roster, and a send 500 ms after the event still read "still friends". Two more holes close with it (#666 review): an answer without a `friends` array is not evidence of a removal (it keeps the roster's verdict, or one server that changed shape would unfriend everyone), and releasing a held card re-asks `list_friends` with `{ fresh: true }` — bypassing the two-second answer at the moment the owner approves — so a card held before the removal fails with the *is not a Bot a friend shared with you* reason instead of being handed to a server that refuses it with `no active relationship`. The probe also carries its own `FRIEND_CALL_MS = 1.5 s` deadline through `callTool(name, args, { timeoutMs })`, because it runs on the send path and must not add the client's 30 s deadline to every line when Bot Social is down. Tests: `plugins/social/test/social.test.js` (`#666`, including the +0 ms and +500 ms cases). ### The server in Settings (#337) diff --git a/plugins/social/src/mcp-client.js b/plugins/social/src/mcp-client.js index 4ec442358..82bd85fc1 100644 --- a/plugins/social/src/mcp-client.js +++ b/plugins/social/src/mcp-client.js @@ -105,11 +105,13 @@ export function createMcpClient({ server, token, fetch: fetchImpl = socialFetch, if (!token) throw new Error('flybot-social: owner connector token is required') const url = `${server.replace(/\/+$/, '')}/mcp` let seq = 0 - async function callTool(name, args = {}) { + // `timeoutMs` in the third argument narrows the deadline for one call: a probe on the send path + // gets a second or two, so a server that is down cannot add the full deadline to a message (#666). + async function callTool(name, args = {}, { timeoutMs: callTimeoutMs = timeoutMs } = {}) { // Every call has a deadline: a hung server is an error, never a stall. // Stateless tools/call only: Bot Social's owner MCP does not need initialize, and // this client is created once per plugin start (not per Friends' Bots refresh). - const signal = AbortSignal.timeout(timeoutMs) + const signal = AbortSignal.timeout(callTimeoutMs) let response let body try { @@ -127,7 +129,7 @@ export function createMcpClient({ server, token, fetch: fetchImpl = socialFetch, body = parseBody(await response.text(), response.headers?.get?.('content-type') ?? '') } catch (error) { if (error instanceof BotSocialError) throw error - if (signal.aborted || error?.name === 'TimeoutError' || error?.name === 'AbortError') throw new BotSocialError(`Bot Social ${name}: no answer in ${Math.round(timeoutMs / 1000)}s`, 'timeout') + if (signal.aborted || error?.name === 'TimeoutError' || error?.name === 'AbortError') throw new BotSocialError(`Bot Social ${name}: no answer in ${Math.round(callTimeoutMs / 1000)}s`, 'timeout') throw error } if (body.error) throw new BotSocialError(`Bot Social ${name}: ${body.error.message}`, 'rpc') @@ -502,7 +504,7 @@ export function createSocialApi({ server, token, fetch: fetchImpl = socialFetch, } return { - callTool: (name, args) => mcp.callTool(name, args), + callTool: (name, args, options) => mcp.callTool(name, args, options), rest, listRemoteBots: args => mcp.callTool('list_remote_bots', args), listFriends: args => mcp.callTool('list_friends', args ?? {}), diff --git a/plugins/social/src/social.js b/plugins/social/src/social.js index 7af916dda..5d53f34ed 100644 --- a/plugins/social/src/social.js +++ b/plugins/social/src/social.js @@ -1020,13 +1020,74 @@ export function createSocial(service, config = {}, { client: given, api: givenAp peerOwners.set(handle, name) if (peerOwners.size > 200) peerOwners.delete(peerOwners.keys().next().value) } - const sendTarget = async msg => (await remoteBot(msg.target.handle)) ?? replyTarget(msg) const friendEntry = async friendHome => (await remoteBots()).homes.find(entry => entry.home === norm(friendHome)) const homeOfPeer = (peerHandle) => { const key = norm(peerHandle) const parts = key.slice(1).split('.') return parts.length > 2 ? `@${parts.slice(1).join('.')}` : key } + // A friendship can be removed outside this plugin — the agent's `remove_friend` tool, or the Bot + // Social console — and the roster we cached keeps that Bot until the next refresh. A send in that + // window was accepted and held, and approving it lost the line: Bot Social answered `no active + // relationship` (#666). The send policy therefore asks Bot Social about the friendship itself + // before it asks the owner, and two seconds of cache keep a burst of lines at one call per friend. + const FRIEND_CHECK_MS = 2_000 + // A probe is a send-path call, so it gets its own deadline instead of the client's 30 s one: a Bot + // Social that is down must not add half a minute to every line to a friend. + const FRIEND_CALL_MS = 1_500 + const friendStatus = new Map() + // An answer we can act on, or nothing. A removal, or a friendship that changed at all, makes every + // cached answer stale: the roster is dropped for the same reason, one line below it. + function forgetFriendStatus(homeKey) { + if (homeKey === undefined) { friendStatus.clear(); return } + const prefix = `${norm(homeKey)}|` + for (const entry of [...friendStatus.keys()]) if (entry.startsWith(prefix)) friendStatus.delete(entry) + } + function noteFriendStatus(key, active) { + friendStatus.delete(key) + friendStatus.set(key, { at: now(), active }) + if (friendStatus.size > 64) friendStatus.delete(friendStatus.keys().next().value) + } + // The friendship is gone: drop the row now, so the list and the picker stop offering a Bot that can + // no longer be reached instead of waiting out the roster TTL. + function forgetFriendHome(homeKey) { + const key = norm(homeKey) + if (!key) return + forgetFriendStatus(key) + const before = remote.homes.length + serverRoster.length + remote = { homes: remote.homes.filter(entry => entry.home !== key) } + serverRoster = serverRoster.filter(entry => entry.home !== key) + if (remote.homes.length + serverRoster.length !== before) flybot.remote?.publish?.(listedRemoteBots()) + } + // `list_friends` lists the active friendships of one identity: the home, and each member Bot that + // has its own (a per-Bot invite, #420). No answer at all (an older server, a link that is down) + // keeps the roster's verdict: this check catches a removal, it does not invent one. + async function friendshipActive(friendHome, fromBotId, { fresh = false } = {}) { + const homeKey = norm(friendHome) + if (!homeKey) return true + const cacheKey = `${homeKey}|${typeof fromBotId === 'string' ? fromBotId : ''}` + const cached = friendStatus.get(cacheKey) + if (!fresh && cached && now() - cached.at < FRIEND_CHECK_MS) return cached.active + const member = typeof fromBotId === 'string' ? activeMember(fromBotId) : undefined + const ids = [...new Set([home?.id, config.botId, member?.bot_id].filter(Boolean))] + let answered = false + for (const botId of ids) { + let answer + try { answer = await client.callTool('list_friends', { bot_id: botId }, { timeoutMs: FRIEND_CALL_MS }) } + catch (error) { warn('friendship not checked', error); continue } + // A reply without a friends list is not evidence of a removal: an answer we cannot read keeps + // the roster's verdict, or one server that changed shape would unfriend everyone (#666 INC). + if (!Array.isArray(answer?.friends)) continue + answered = true + const listed = answer.friends.some(item => (item?.status === undefined || item.status === 'active') && norm(item.peer?.handle) && homeOfPeer(item.peer.handle) === homeKey) + if (listed) { noteFriendStatus(cacheKey, true); return true } + } + if (!answered) return true + forgetFriendHome(homeKey) + noteFriendStatus(cacheKey, false) + return false + } + const sendTarget = async msg => (await remoteBot(msg.target.handle)) ?? replyTarget(msg) // Hide a friend's Bot: decline the grant on Bot Social when we have a grant_id so the // owner's Manage access drops it; an older server (no decline tool) falls back to a // local hide. It stays hidden across refresh while it remains granted. If the owner @@ -1271,6 +1332,9 @@ export function createSocial(service, config = {}, { client: given, api: givenAp // Adding a friend's Bot to a group (or a line in that group) is the owner's consent // to share that room. The invited owner's Bot replies still go through the hold. if (msg.via === 'room' || msg.via === 'room-share') return undefined + // The friendship may be gone since the roster was cached (#666): deny now, never ask the owner + // to approve a line that can no longer leave, and let the row go with it. + if (!(await friendshipActive(bot.home, msg.from))) return { action: 'deny', reason: `${msg.target.handle} is not a Bot a friend shared with you, so your Bots cannot message it. Its owner can share it with you in that Bot's settings → Sharing.` } // Auto-send on for this pair (#417): it leaves without a card, and the chat marks it Auto-sent. if (approvalFor(msg.from, bot.home) === 'auto') return { action: 'allow', autoSent: true, ...named } return { action: 'hold', reason: `messages to ${bot.home}'s Bots need your approval`, ...named } @@ -1369,6 +1433,14 @@ export function createSocial(service, config = {}, { client: given, api: givenAp const name = flybot.bot.get(msg.from)?.name ?? 'This Bot' throw new Error(`${name} is not shared with that friend, so it cannot message their Bots. To allow it, share ${name} with them in its settings → Sharing.`) } + // The card was held before the friendship changed (#666): an approved line must not be handed + // to a server that will refuse it with `no active relationship`. Room lines are the room's + // consent, not this pair's, so they are left alone. + // Asked fresh, not from the two-second cache: the answer that matters is the one at the + // moment the owner approves, and the card may have waited far longer than that cache. + if (msg.approved === true && msg.via !== 'room' && msg.via !== 'room-share' && !(await friendshipActive(bot.home, msg.from, { fresh: true }))) { + throw new Error(`${bot.handle} is not a Bot a friend shared with you, so your Bots cannot message it. Its owner can share it with you in that Bot's settings → Sharing.`) + } const attachments = await uploadAll(msg.attachments, member.bot_id) const sent = await client.callTool('send_as_member', { from_bot_id: member.bot_id, to: bot.handle, text: msg.text, ...(attachments.length ? { attachments } : {}) }) .catch((error) => { if (isPaused(error)) notePaused(bot.handle, error); throw error }) @@ -1655,10 +1727,22 @@ export function createSocial(service, config = {}, { client: given, api: givenAp // comes before the friend's first message in this Bot's queue: record the grant now, so that message // is judged with it. A failed sync leaves it to the retry below. if ((event.event_type === 'relationship.changed' && event.status === 'active') || (event.event_type === 'grant.changed' && event.status === 'granted')) { + if (event.event_type === 'relationship.changed') forgetFriendStatus() if (!seen.has(event.event_id)) await syncOutgoingGrants({ only: local.id }).catch(error => warn('grants not synced', error)) done() continue } + // A relationship that changed to anything but active is gone here, and the roster we hold may + // still name its Bots: a send would be held and then lost on approve (#666). Drop the cache and + // let the next read ask the server, rather than serve a friend this home no longer has. + if (event.event_type === 'relationship.changed' && event.status !== 'active') { + fetchedAt = 0 + // The roster is one half; the friendship answers are the other. Without this a send in the + // two-second cache window still read "still friends" and was held (#666 INC). + forgetFriendStatus() + done() + continue + } if (event.event_type === 'message.not_delivered') { if (!seen.has(event.event_id)) await noteNotDelivered(local, event).catch(error => warn('not-delivered notice failed', error)) done() diff --git a/plugins/social/test/social.test.js b/plugins/social/test/social.test.js index 8ae3bfcd3..6ea45d7e9 100644 --- a/plugins/social/test/social.test.js +++ b/plugins/social/test/social.test.js @@ -4164,6 +4164,145 @@ test('#90: hide a friend\'s Bot updates the list, leaves rooms, and does not unf assert.equal(again.homes[0].bots.some(b => b.handle === '@coder.bob.home'), false, 'still hidden after refresh') }) +// A friendship can be removed outside this plugin (the agent's `remove_friend` tool, the Bot Social +// console). The cached roster used to keep that Bot for up to 30 s, so a send in that window was +// accepted and held, and approving it lost the line: Bot Social answered `no active relationship` +// (#666). The send policy asks Bot Social about the friendship before it asks the owner, and the +// answer is dropped the moment Bot Social reports the relationship changed (#666 INC). +test('#666: a removal event drops the friendship answer, so a send in the same millisecond is denied', async (t) => { + const { ctx, call, home, stop } = await team() + t.after(stop) + const server = mockServer() + let clock = 1_000_000 + const social = createSocial(ctx.flybot, { autoAccept: false, inboxPollMs: 0, settingsPath: join(home, 'flybot-social-settings.json') }, { + client: createMcpClient({ server: 'https://s', token: 'owner-token', fetch: server.fetch }), + home: { id: 'h1', handle: '@danyang.home' }, + now: () => clock, + }) + t.after(() => social.dispose()) + await grantTeam(ctx.flybot) + await social.registerAll() + const listed = (await call('flybot/social/remote-bots', { fresh: true })).value + assert.equal(listed.homes[0].bots.some(bot => bot.handle === '@coder.bob.home'), true) + // A first line makes the policy ask Bot Social once and cache "still friends" for two seconds. + const primed = await call('flybot/social/send', { from: 'a', to: '@coder.bob.home', text: 'still there?' }) + assert.equal(primed.value.action, 'hold', 'the friendship is real, so this line asks the owner') + // Removed on Bot Social's side, and reported as such. No clock moves: the send below is the same + // millisecond the old cache would still have answered "friends". + server.tools.list_friends = () => ({ friends: [] }) + const memberA = ctx.flybot.scope('social').fields.get('a', 'member').bot_id + server.events.set(memberA, [{ event_id: 'evt-revoked', cursor: 5, event_type: 'relationship.changed', relationship_id: 'r1', status: 'revoked' }]) + assert.equal(await social.pollInbox(), 0) + const sent = await call('flybot/social/send', { from: 'a', to: '@coder.bob.home', text: 'one more' }) + assert.equal(sent.value.action, 'deny', 'a line that cannot leave must not become an approval card') + assert.match(sent.value.reason, /is not a Bot a friend shared with you/) + assert.equal(ctx.flybot.approvals.list().length, 1, 'only the line sent while the friendship stood was held') + assert.equal(server.calls.some(entry => entry.name === 'send_as_member'), false, 'nothing was sent') + // The stale row goes with the deny: the list stops offering a Bot this home no longer has. + const after = (await call('flybot/social/remote-bots')).value + assert.equal((after.homes ?? []).some(entry => entry.bots.some(bot => bot.handle === '@coder.bob.home')), false) +}) + +test('#666: the same removal 500 ms after the event is still denied', async (t) => { + const { ctx, call, home, stop } = await team() + t.after(stop) + const server = mockServer() + let clock = 1_000_000 + const social = createSocial(ctx.flybot, { autoAccept: false, inboxPollMs: 0, settingsPath: join(home, 'flybot-social-settings.json') }, { + client: createMcpClient({ server: 'https://s', token: 'owner-token', fetch: server.fetch }), + home: { id: 'h1', handle: '@danyang.home' }, + now: () => clock, + }) + t.after(() => social.dispose()) + await grantTeam(ctx.flybot) + await social.registerAll() + await call('flybot/social/remote-bots', { fresh: true }) + await call('flybot/social/send', { from: 'a', to: '@coder.bob.home', text: 'still there?' }) + server.tools.list_friends = () => ({ friends: [] }) + const memberA = ctx.flybot.scope('social').fields.get('a', 'member').bot_id + server.events.set(memberA, [{ event_id: 'evt-revoked', cursor: 5, event_type: 'relationship.changed', relationship_id: 'r1', status: 'revoked' }]) + assert.equal(await social.pollInbox(), 0) + clock += 500 + const sent = await call('flybot/social/send', { from: 'a', to: '@coder.bob.home', text: 'half a second later' }) + assert.equal(sent.value.action, 'deny', '500 ms is inside the old two-second cache and must not change the answer') +}) + +test('#666: approving a card held before the removal is refused before Bot Social is asked', async (t) => { + const { ctx, call, home, stop } = await team() + t.after(stop) + const server = mockServer() + let clock = 1_000_000 + const social = createSocial(ctx.flybot, { autoAccept: false, inboxPollMs: 0, settingsPath: join(home, 'flybot-social-settings.json') }, { + client: createMcpClient({ server: 'https://s', token: 'owner-token', fetch: server.fetch }), + home: { id: 'h1', handle: '@danyang.home' }, + now: () => clock, + }) + t.after(() => social.dispose()) + await grantTeam(ctx.flybot) + await social.registerAll() + await call('flybot/social/remote-bots', { fresh: true }) + const held = await call('flybot/social/send', { from: 'a', to: '@coder.bob.home', text: 'held while we were friends' }) + assert.equal(held.value.action, 'hold') + const [pending] = ctx.flybot.approvals.list() + // The friendship is gone by the time the owner answers the card, and no event reached this host: + // the roster still names Bob, so the release path is the only place that can catch it. + server.tools.list_friends = () => ({ friends: [] }) + clock += 30_000 + const failed = await ctx.flybot.approvals.decide(pending.id, 'approve') + assert.equal(failed.status, 'failed', 'the card fails with a reason instead of losing the line') + assert.match(failed.error, /is not a Bot a friend shared with you/) + assert.equal(server.calls.some(entry => entry.name === 'send_as_member'), false, 'Bot Social is not asked for a send it will refuse') +}) + +test('#666: an answer without a friends list is not a removal', async (t) => { + const { ctx, call, home, stop } = await team() + t.after(stop) + const server = mockServer() + const social = createSocial(ctx.flybot, { autoAccept: false, inboxPollMs: 0, settingsPath: join(home, 'flybot-social-settings.json') }, { + client: createMcpClient({ server: 'https://s', token: 'owner-token', fetch: server.fetch }), + home: { id: 'h1', handle: '@danyang.home' }, + }) + t.after(() => social.dispose()) + await grantTeam(ctx.flybot) + await social.registerAll() + await call('flybot/social/remote-bots', { fresh: true }) + // A server that answers something else — an error envelope, an older shape — is not evidence that + // the friendship is gone: it keeps the roster's verdict instead of unfriending a real friend. + server.tools.list_friends = () => ({ ok: true }) + const sent = await call('flybot/social/send', { from: 'a', to: '@coder.bob.home', text: 'hello' }) + assert.equal(sent.value.action, 'hold', 'an unreadable answer is not a deny') + assert.equal((sent.value.reason ?? '').includes('not a Bot a friend shared'), false) + const after = (await call('flybot/social/remote-bots')).value + assert.equal(after.homes[0].bots.some(bot => bot.handle === '@coder.bob.home'), true, 'the friend is still listed') +}) + +// Same defect, other half: once Bot Social reports the removal, the cached roster must not keep +// serving the friend for the rest of its TTL. +test('#666: a relationship removal event drops the cached roster instead of serving the removed friend', async (t) => { + const { ctx, call, home, stop } = await team() + t.after(stop) + const server = mockServer() + let clock = 1_000_000 + const social = createSocial(ctx.flybot, { autoAccept: false, inboxPollMs: 0, settingsPath: join(home, 'flybot-social-settings.json') }, { + client: createMcpClient({ server: 'https://s', token: 'owner-token', fetch: server.fetch }), + home: { id: 'h1', handle: '@danyang.home' }, + now: () => clock, + }) + t.after(() => social.dispose()) + await grantTeam(ctx.flybot) + await social.registerAll() + const listed = (await call('flybot/social/remote-bots', { fresh: true })).value + assert.equal(listed.homes.length, 1) + server.tools.list_remote_bots = () => ({ homes: [] }) + server.tools.list_friends = () => ({ friends: [] }) + const memberA = ctx.flybot.scope('social').fields.get('a', 'member').bot_id + server.events.set(memberA, [{ event_id: 'evt-revoked', cursor: 5, event_type: 'relationship.changed', relationship_id: 'r1', status: 'revoked' }]) + clock += 2_000 + assert.equal(await social.pollInbox(), 0) + const after = (await call('flybot/social/remote-bots')).value + assert.deepEqual(after.homes, [], 'the next read asks the server instead of the roster the event invalidated') +}) + test('#90: hide-remote returns the filtered roster without waiting for a refresh', async (t) => { const { ctx, call, home, stop } = await team() t.after(stop)