Deny a send to a friend removed since the roster was cached (#666)

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.

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)
 

plugins/social/src/mcp-client.js

@@ -41,7 +41,7 @@
   const lib = parsed.protocol === 'http:' ? http : https
   const agent = init.agent ?? agentFor(url)
   const method = String(init.method ?? 'GET').toUpperCase()
   const headers = { ...(init.headers ?? {}) }
   const signal = init.signal
   return new Promise((resolve, reject) => {
     const req = lib.request(parsed, { method, headers, agent }, (res) => {
@@ -76,7 +76,7 @@
   const list = [...items]
   if (list.length === 0) return []
   const cap = Math.max(1, Math.min(limit || GRANT_CONCURRENCY, list.length))
   const out = new Array(list.length)
   let next = 0
   async function worker() {
     for (;;) {
@@ -105,11 +105,13 @@
   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 @@
       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 @@
   }
 
   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 ?? {}),

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()