fix(social): a friend Bot's held reply shows its approval card (overlay hook crash, in-chat match) (#254)

changelog.d/254-held-reply-approval-card.fixed.md

@@ -0,0 +1 @@
+A friend Bot's reply to a typed DM was held for its owner's approval, but no card ever showed: the approvals overlay crashed (a React hook after an early return) and the replying Bot's own chat did not list holds its Bot wrote. The overlay now draws, and the held reply shows as an Approve / Always approve / Reject card in the replying Bot's chat. (#254)

core/client.js

@@ -1925,6 +1925,7 @@ function FlyBotApprovals() {
   const entries2 = useSlot("flybot.approvals");
   const [pending, setPending] = (0, import_react2.useState)([]);
   const [busy, setBusy] = (0, import_react2.useState)("");
+  const [error, setError] = (0, import_react2.useState)("");
   const refresh = () => call2("approvals", { status: ["pending", "failed"] }).then((list) => setPending(Array.isArray(list) ? list : [])).catch(() => {
   });
   (0, import_react2.useEffect)(() => {
@@ -1933,7 +1934,6 @@ function FlyBotApprovals() {
     return () => clearInterval(timer);
   }, []);
   if (pending.length === 0) return null;
-  const [error, setError] = (0, import_react2.useState)("");
   const decide = async (id, decision) => {
     setBusy(id);
     setError("");

core/src/client/flybot-slots.js

@@ -128,14 +128,16 @@ export function FlyBotApprovals() {
   const entries = useSlot('flybot.approvals')
   const [pending, setPending] = useState([])
   const [busy, setBusy] = useState('')
+  const [error, setError] = useState('')
   const refresh = () => call('approvals', { status: ['pending', 'failed'] }).then(list => setPending(Array.isArray(list) ? list : [])).catch(() => {})
   useEffect(() => {
     void refresh()
     const timer = setInterval(refresh, POLL_MS)
     return () => clearInterval(timer)
   }, [])
+  // Every hook above runs on every render: a hook after this early return made React throw
+  // ("Rendered more hooks") the moment the first held message arrived, so no card ever drew (#254).
   if (pending.length === 0) return null
-  const [error, setError] = useState('')
   const decide = async (id, decision) => {
     setBusy(id)
     setError('')

core/test/overlay-approvals.test.js

@@ -0,0 +1,71 @@
+// The held-send overlay (FlyBotApprovals) must run the same hooks on every render: a hook after its
+// early `return null` made React throw "Rendered more hooks than during the previous render" when
+// the first held message arrived, so no approval card ever drew (#254). React is replaced by a small
+// renderer that enforces React's rule (the hook count must not change between renders).
+import assert from 'node:assert/strict'
+import { registerHooks } from 'node:module'
+import { test } from 'node:test'
+
+const live = { inst: null, idx: 0 }
+const reactStub = {
+  createElement: (type, props, ...children) => ({ type, props: props ?? {}, children: children.flat(Infinity) }),
+  Fragment: 'F',
+  Component: class {},
+  useState(init) {
+    const { inst } = live
+    const i = live.idx++
+    if (!(i in inst.hooks)) {
+      if (inst.rendered) throw new Error('Rendered more hooks than during the previous render.')
+      inst.hooks[i] = { value: typeof init === 'function' ? init() : init }
+    }
+    const slot = inst.hooks[i]
+    return [slot.value, (next) => { slot.value = typeof next === 'function' ? next(slot.value) : next; inst.dirty = true }]
+  },
+  useEffect(fn, deps) {
+    const { inst } = live
+    const i = live.idx++
+    if (!(i in inst.hooks)) {
+      if (inst.rendered) throw new Error('Rendered more hooks than during the previous render.')
+      inst.hooks[i] = { deps: undefined }
+    }
+    inst.effects.push([i, fn, deps])
+  },
+}
+// Hooks the host modules import but this test never calls.
+for (const name of ['useRef', 'useMemo', 'useCallback', 'useLayoutEffect', 'useContext', 'useReducer', 'useId', 'useSyncExternalStore', 'useTransition', 'useDeferredValue', 'memo', 'forwardRef', 'createContext', 'createPortal', 'cloneElement', 'isValidElement', 'startTransition']) reactStub[name] = (value) => value
+globalThis.__reactStub = reactStub
+registerHooks({
+  resolve(specifier, context, nextResolve) {
+    if (specifier === 'react') return { url: 'data:text/javascript,const r=globalThis.__reactStub;export default r;export const {createElement,Fragment,Component,useState,useEffect,useRef,useMemo,useCallback,useLayoutEffect,useContext,useReducer,useId,useSyncExternalStore,useTransition,useDeferredValue,memo,forwardRef,createContext,createPortal,cloneElement,isValidElement,startTransition}=r', shortCircuit: true }
+    return nextResolve(specifier, context)
+  },
+})
+
+globalThis.window ??= globalThis
+globalThis.document ??= { createElement: () => ({ setAttribute() {} }), head: { appendChild() {} }, getElementById: () => null, addEventListener() {}, removeEventListener() {} }
+function render(Component, inst) {
+  live.inst = inst
+  live.idx = 0
+  inst.effects = []
+  const out = Component({})
+  inst.rendered = true
+  return out
+}
+
+test('#254: the approvals overlay renders again with the same hooks when the first held message arrives', async () => {
+  const { FlyBotApprovals } = await import('../src/client/flybot-slots.js')
+  const inst = { hooks: {}, effects: [], rendered: false }
+  assert.equal(render(FlyBotApprovals, inst), null) // nothing held yet: nothing drawn
+  const setPending = inst.hooks[2] // useSlot takes hooks 0 and 1 (version, subscription); pending is next
+  assert.ok(setPending, 'pending state exists')
+  setPending.value = [{ id: 'a1', status: 'pending', by: 'social', reason: 'needs approval', senderName: 'Helper', targetName: 'Chief', request: { to: { handle: '@chief.uia.home' }, text: 'Which file?', hop: 0 } }]
+  let tree
+  assert.doesNotThrow(() => { tree = render(FlyBotApprovals, inst) })
+  assert.ok(tree && tree.props.className === 'bt-flybot-approvals')
+  const text = JSON.stringify(tree)
+  assert.match(text, /Which file\?/)
+  assert.match(text, /Approve/)
+  // And it goes back to nothing, still without a hook mismatch.
+  setPending.value = []
+  assert.doesNotThrow(() => render(FlyBotApprovals, inst))
+})

docs/subsystems/core-client.md

@@ -102,6 +102,8 @@ The default card (`flybot-slots.js`) shows where a held request came from: `send
 
 - FlyBot `restrict()`s DSH's `ask_user_question` off Bot sessions (`hideBlockingQuestion`). DSH then appends a developer `tool-removal` context row, and `@deepseek-ai/dsh-client-ui-chat` renders it as "Tool removed: {name}". `ContextCell` shadows `conversation.chat.node` `context` and returns null for those notices (`HIDDEN_REMOVED_TOOLS`). Other tool-change context (tool-added, unrelated removals) still shows; `tool-call` cells are unchanged. Empty shadowed nodes collapse via the existing `:empty` flow-item rule. Covered by `core/test/tool-cells.test.js`.
 
+- The held-send overlay (`FlyBotApprovals`, `flybot-slots.js`) runs every hook before its `if (pending.length === 0) return null`: a hook after that return made React throw on the first held message and no card ever drew (#254). `test/overlay-approvals.test.js` renders it with a stand-in that enforces React's hook-count rule.
+
 ## Tests
 
 `npm test` in `core/` (it also fails when `client.js` is stale).

docs/subsystems/plugin-runtime.md

@@ -92,4 +92,4 @@ FlyBot's plugin API surface on top of the patched core: host hooks, client slots
 - Changing slots without updating types or client tests.
 - `flybot.remote.chats.deliver({ handle, text, name?, home?, human?, ownerName? })` (#227): `human: true` is the plugin's server-attested "the friend typed this line themselves" (Bot Social `typed_by_owner`, never derived from text) and `ownerName` the authenticated owner label; the host then draws that turn's reply as the person. Plugins that cannot attest leave both out and the line stays the Bot's.
 - `flybot.bot.send({ from: { kind: 'remote', handle, home?, typedBy? }, ... })`: `typedBy` is the plugin's server-attested owner name for a line the friend typed (Bot Social `typed_by_owner`); `toRemote` (`src/host/hooks.js`) cleans it and the host stores it as the message source's `remote.typedBy` so the chat draws the person, not the Bot. Never derive it from text; plugins that cannot attest omit it.
-
+- The approval card queue (`FlyBotApprovals`) runs all its hooks on every render, before its empty-queue early return: a hook after it makes React throw when the first hold arrives and no card ever draws (#254; `core/test/overlay-approvals.test.js`).

docs/subsystems/social.md

@@ -47,6 +47,7 @@ Everything in `plugins/social/` (entry `plugins/social/index.js` and any helpers
 5. First start retries a taken home handle (`FlyBot`, `FlyBot2`, …) instead of failing for a YAML edit (#136). If every retry is still taken, the social UI never shows the host's `Unknown FlyBot endpoint social/…` message: `apply` registers the social UI endpoints with a friendly `Social is not running: <reason>` refusal and `flybot.warn` records the failure. The client also maps `bot/not-found` on `social/*` to that idle state. Add friend shows `You are @your.home` (#181).
 6. Face-to-face join (#83, #92, #158) is a waiting room, not an instant chat: the owner picks local Bots, types the 6-digit number, then waits with the same code shown large until someone **Enter chat**s. Copy never says there is no preview. Each picker tile and each waiting-room member uses the host Bot avatar (`__flybot__.BotAvatar`), not a shared first-letter badge. The client calls `social/face-join` once per Bot with the same code and shows partial failures. If a chosen Bot has no identity, the plugin registers it first (see invariant 4); registration failures (`handle_taken`, rate limit, timeout) map to English copy and leave no leftover member. Failed join responses never leak `members` / `events` / `room_id`. Every FACE_ROOMS.md error code maps to English copy (`rate_limited` / `locked_out` include `retry_after`). The waiting room polls `social/face-rooms` (which refreshes from Bot Social, including `joinCode`) and marks the owner's Bots with a tiny accent dot on the tile; **Leave** and **Enter chat** share the footer. **Enter chat** is `social/face-start` then `openSession(roomId)`, and the screen follows if someone else starts. Peer `room.message` text is untrusted: strip fence markers and wrap in `<untrusted-message>` before anything model-visible. Bot replies POST `/v1/face-rooms/{id}/messages`; `pending_approval` shows in the existing queue and in the chat (owner Approve / Reject). Owner-typed `FLYBOT-FACE-HUMAN` lines stay on that card until Approve / Always approve / Reject unless face **Auto-send** is on; the card and chat never show the raw envelope (#159). **Auto-send my Bot's room messages** is one click (`whoami` / MCP `set_auto_approve { auto_approve_face_rooms }`); if the server answers `confirmation_required`, the same click retries with `confirm_code` and the UI never asks for a 6-character code. REST `/v1/me` is not used with the connector token. A friend's Bot in the room member list and waiting grid uses the host Bot avatar (handle-seeded when Bot Social sends no image). New members only receive events from their join point (`after` cursor; the server also floors it). Join does not grant the Bot to friends. The UI never says published.
 7. Every import in shipped sources stays inside the plugin package: an installed plugin cannot reach into other packages' files (a repo-relative `../..` into `core/` breaks the installed bundle).
+8. A friend's Bot's reply to a line you typed (or to any request) goes back through `bot.send` as an `auto-reply` and, unless Auto-send is on for that pair, is held for the replying Bot's owner (#254). The hold is shown as a card in the replying Bot's own chat (`approvalsForOpenChat` matches a local hold by its sender Bot, `localHoldView` `fromId`) and in the friend's chat (by handle); a friend's handle is never a room id. The sender sees nothing until it is approved.
 
 ### Connector-token scope (#156)
 

patches/ds-bot/0008-client-bundle.patch

@@ -918,6 +918,7 @@ diff --git a/client.js b/client.js
 +  const entries2 = useSlot("flybot.approvals");
 +  const [pending, setPending] = (0, import_react2.useState)([]);
 +  const [busy, setBusy] = (0, import_react2.useState)("");
++  const [error, setError] = (0, import_react2.useState)("");
 +  const refresh = () => call2("approvals", { status: ["pending", "failed"] }).then((list) => setPending(Array.isArray(list) ? list : [])).catch(() => {
 +  });
 +  (0, import_react2.useEffect)(() => {
@@ -926,7 +927,6 @@ diff --git a/client.js b/client.js
 +    return () => clearInterval(timer);
 +  }, []);
 +  if (pending.length === 0) return null;
-+  const [error, setError] = (0, import_react2.useState)("");
 +  const decide = async (id, decision) => {
 +    setBusy(id);
 +    setError("");

patches/ds-bot/files/src/client/flybot-slots.js

@@ -128,14 +128,16 @@ export function FlyBotApprovals() {
   const entries = useSlot('flybot.approvals')
   const [pending, setPending] = useState([])
   const [busy, setBusy] = useState('')
+  const [error, setError] = useState('')
   const refresh = () => call('approvals', { status: ['pending', 'failed'] }).then(list => setPending(Array.isArray(list) ? list : [])).catch(() => {})
   useEffect(() => {
     void refresh()
     const timer = setInterval(refresh, POLL_MS)
     return () => clearInterval(timer)
   }, [])
+  // Every hook above runs on every render: a hook after this early return made React throw
+  // ("Rendered more hooks") the moment the first held message arrived, so no card ever drew (#254).
   if (pending.length === 0) return null
-  const [error, setError] = useState('')
   const decide = async (id, decision) => {
     setBusy(id)
     setError('')

patches/ds-bot/files/test/overlay-approvals.test.js

@@ -0,0 +1,71 @@
+// The held-send overlay (FlyBotApprovals) must run the same hooks on every render: a hook after its
+// early `return null` made React throw "Rendered more hooks than during the previous render" when
+// the first held message arrived, so no approval card ever drew (#254). React is replaced by a small
+// renderer that enforces React's rule (the hook count must not change between renders).
+import assert from 'node:assert/strict'
+import { registerHooks } from 'node:module'
+import { test } from 'node:test'
+
+const live = { inst: null, idx: 0 }
+const reactStub = {
+  createElement: (type, props, ...children) => ({ type, props: props ?? {}, children: children.flat(Infinity) }),
+  Fragment: 'F',
+  Component: class {},
+  useState(init) {
+    const { inst } = live
+    const i = live.idx++
+    if (!(i in inst.hooks)) {
+      if (inst.rendered) throw new Error('Rendered more hooks than during the previous render.')
+      inst.hooks[i] = { value: typeof init === 'function' ? init() : init }
+    }
+    const slot = inst.hooks[i]
+    return [slot.value, (next) => { slot.value = typeof next === 'function' ? next(slot.value) : next; inst.dirty = true }]
+  },
+  useEffect(fn, deps) {
+    const { inst } = live
+    const i = live.idx++
+    if (!(i in inst.hooks)) {
+      if (inst.rendered) throw new Error('Rendered more hooks than during the previous render.')
+      inst.hooks[i] = { deps: undefined }
+    }
+    inst.effects.push([i, fn, deps])
+  },
+}
+// Hooks the host modules import but this test never calls.
+for (const name of ['useRef', 'useMemo', 'useCallback', 'useLayoutEffect', 'useContext', 'useReducer', 'useId', 'useSyncExternalStore', 'useTransition', 'useDeferredValue', 'memo', 'forwardRef', 'createContext', 'createPortal', 'cloneElement', 'isValidElement', 'startTransition']) reactStub[name] = (value) => value
+globalThis.__reactStub = reactStub
+registerHooks({
+  resolve(specifier, context, nextResolve) {
+    if (specifier === 'react') return { url: 'data:text/javascript,const r=globalThis.__reactStub;export default r;export const {createElement,Fragment,Component,useState,useEffect,useRef,useMemo,useCallback,useLayoutEffect,useContext,useReducer,useId,useSyncExternalStore,useTransition,useDeferredValue,memo,forwardRef,createContext,createPortal,cloneElement,isValidElement,startTransition}=r', shortCircuit: true }
+    return nextResolve(specifier, context)
+  },
+})
+
+globalThis.window ??= globalThis
+globalThis.document ??= { createElement: () => ({ setAttribute() {} }), head: { appendChild() {} }, getElementById: () => null, addEventListener() {}, removeEventListener() {} }
+function render(Component, inst) {
+  live.inst = inst
+  live.idx = 0
+  inst.effects = []
+  const out = Component({})
+  inst.rendered = true
+  return out
+}
+
+test('#254: the approvals overlay renders again with the same hooks when the first held message arrives', async () => {
+  const { FlyBotApprovals } = await import('../src/client/flybot-slots.js')
+  const inst = { hooks: {}, effects: [], rendered: false }
+  assert.equal(render(FlyBotApprovals, inst), null) // nothing held yet: nothing drawn
+  const setPending = inst.hooks[2] // useSlot takes hooks 0 and 1 (version, subscription); pending is next
+  assert.ok(setPending, 'pending state exists')
+  setPending.value = [{ id: 'a1', status: 'pending', by: 'social', reason: 'needs approval', senderName: 'Helper', targetName: 'Chief', request: { to: { handle: '@chief.uia.home' }, text: 'Which file?', hop: 0 } }]
+  let tree
+  assert.doesNotThrow(() => { tree = render(FlyBotApprovals, inst) })
+  assert.ok(tree && tree.props.className === 'bt-flybot-approvals')
+  const text = JSON.stringify(tree)
+  assert.match(text, /Which file\?/)
+  assert.match(text, /Approve/)
+  // And it goes back to nothing, still without a hook mismatch.
+  setPending.value = []
+  assert.doesNotThrow(() => render(FlyBotApprovals, inst))
+})

plugins/social/client.js

@@ -871,6 +871,8 @@
         if (faceId && item.roomId === faceId) return true
         if (room?.id && item.roomId === room.id) return true
         if (item.roomId) return false
+        // A held reply or message a local Bot wrote shows in that Bot's own chat, where its owner is (#254).
+        if (item.fromId && bot?.id && !bot.remote && item.fromId === bot.id) return true
         if (item.to && handles.has(item.to)) return true
         if (sessionId && item.sessionId === sessionId) return true
         return false
@@ -885,10 +887,13 @@
         id: approval.id,
         source: 'local',
         from: composer ? 'You' : approval.senderName,
+        // The local Bot that wrote it (not a line you typed): its own chat shows the card (#254).
+        ...(!composer && typeof approval.request?.from === 'string' ? { fromId: approval.request.from } : {}),
         to,
         toName: approval.targetName || to,
         text: approval.request?.text || approval.reason,
-        roomId: approval.request?.meta?.roomId || approval.request?.to,
+        // A friend's handle is an object, never a room: only a room id may sit here.
+        roomId: approval.request?.meta?.roomId || (typeof approval.request?.to === 'string' ? approval.request.to : undefined),
         sessionId: approval.request?.meta?.sessionId,
       }
     }
@@ -1366,7 +1371,7 @@
       statusTask ??= call('social/status', {}).then(value => {
         const changed = JSON.stringify(value) !== JSON.stringify(status.value)
         status = { at: Date.now(), value }
         if (changed) for (const fn of [...statusListeners]) { try { fn() } catch (error) { console.error('flybot-social: status listener failed', error) } }
         return value
       }).finally(() => { statusTask = undefined })
       return statusTask
@@ -1562,6 +1567,8 @@
       PendingApprovals,
       ChatApproval,
       ChatApprovalDock,
+      approvalsForOpenChat,
+      localHoldView,
       MyHandle,
       FaceRoomStatus,
       FriendBotRow,