Skip to content

Commit f8b30ad

Browse files
committed
test(social): pin that a local reject is a decision, not a failed send
Core's decide('reject') answers normally with the approval it saved as rejected. The dock keeps such an answer out of the notSent() path, so the card leaves and nothing is receipted or offered for retry; nothing pinned that, so a failure condition that also treated 'rejected' as a failed send stayed green. The new case fails under that mutant and passes on the real code. Fixes #3093
1 parent 3e901fa commit f8b30ad

4 files changed

Lines changed: 251 additions & 12 deletions

File tree

‎changelog.d/3093.fixed.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
**Approve all** no longer reports **Sent to …** for a local hold whose send failed: the host answers such a send normally, with the approval it saved as failed, so the batch counted it as reached, dropped its card and named people who never got anything. A hold whose decision was refused now also keeps its reason on its own card — local holds included — so the reason survives the next click and a chat switch instead of living only in the dock's one line (#3093).

‎docs/subsystems/social.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ Bot Social plugin for chatting with friends' Bots (send middleware `social` with
1414

1515
A saved server choice that exists but cannot be read or parsed refuses instead of reading as "nothing saved yet" (#2717), and that refusal is answered on the plugin's own endpoints — the three `social/server*` endpoints and the UI endpoint list — and written once to the diagnostics log, so a damaged `flybot-social-server.json` shows its path and reason on the settings page and in Settings → Feedback instead of making Bot Social look uninstalled (#2945).
1616

17-
The chat dock's own memory lives outside the component: `conversation.input.dock` is session-scoped, so opening another chat unmounts the dock and coming back mounts a new one. What a failed send says, and which Retry already went out, are kept in a module-level record keyed `<approval id>:<session id>` — the Map + listener shape `core/src/client/approval-claims.js` uses — and the dock only subscribes; otherwise the way back drew a failed card as a fresh wait and offered a Retry that had already gone out. A failure record goes when its id leaves the two lists the dock reads; a spent Retry is kept instead — bounded to the 50 most recent — because the send did go out and the poll that still writes the hold back as failed would otherwise offer it again (#3082).
17+
The chat dock's own memory lives outside the component: `conversation.input.dock` is session-scoped, so opening another chat unmounts the dock and coming back mounts a new one. What a failed send says, and which Retry already went out, are kept in a module-level record keyed `<approval id>:<session id>` — the Map + listener shape `core/src/client/approval-claims.js` uses — and the dock only subscribes; otherwise the way back drew a failed card as a fresh wait and offered a Retry that had already gone out. Every card kind draws its reason from that record, a local hold included: the host describes a send that failed, but not a decision it refused, and the batch's one line is cleared by the next click and lost with the chat (#3093). A failure record goes when its id leaves the two lists the dock reads; a spent Retry is kept instead — at most 50 spent records, earliest-inserted dropped first — because the send did go out and the poll that still writes the hold back as failed would otherwise offer it again (#3082).
1818

1919
A second, read-only window starts the social layer from the credentials file the writable window owns, so friends and messages stay visible, but it never onboards a home and never writes that file: the plugin start is a lifecycle path (`apply` → `ready` → `launch`), not an API endpoint, so the core's read-only guard cannot stop it and `startSocial` reads `config.readOnly` itself (#2675).
2020

‎plugins/social/client.js‎

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2466,8 +2466,8 @@ window.__ModuleLoader__.load({
24662466
dockRecordChanged()
24672467
}
24682468
// A spent Retry is never dropped because its card left the lists: the send went out, and the poll that
2469-
// still writes the hold back as failed would otherwise offer that send again (#3082). The set is kept
2470-
// bounded instead — the oldest spent record goes once there are more than this many.
2469+
// still writes the hold back as failed would otherwise offer that send again (#3082). The records are
2470+
// kept bounded instead: once more than this many are spent, the one entered longest ago goes first.
24712471
const DOCK_RETRIED_MAX = 50
24722472
function trimDockRetried() {
24732473
let spent = 0
@@ -2648,6 +2648,18 @@ window.__ModuleLoader__.load({
26482648
// The line is cleared where the person acted, not here (#3071): a batch calls this once per
26492649
// hold, so clearing on entry let a send that worked erase the reason the one before it had
26502650
// just written.
2651+
// #3075: the line alone dies with the next click; the card keeps the reason as its own state,
2652+
// so a poll that still sees the hold pending cannot draw it back as if nothing happened.
2653+
// (#3093) that record is the card's, whatever kind it is — a local hold the host marked failed
2654+
// draws its own status, and a decision the host refused never reached a send to be described.
2655+
const notSent = (failure) => {
2656+
const reason = String(failure?.message ?? failure).slice(0, 200)
2657+
setError(reason)
2658+
// A retry the host refused may be tried again; the id was never marked spent.
2659+
if (retry) unmarkDockRetried(sessionId, item.id)
2660+
rememberDockFailure(sessionId, item.id, reason)
2661+
return { error: reason }
2662+
}
26512663
try {
26522664
const always = decision === 'always'
26532665
// #3075: Bot Social's endpoint takes approve or reject only, and a hold that failed is still
@@ -2656,7 +2668,14 @@ window.__ModuleLoader__.load({
26562668
const mapped = always || wanted === 'approve' || wanted === 'approved' ? 'approve' : wanted
26572669
let autoSend
26582670
if (item.source === 'local') {
2659-
await call('decide', { id: item.id, decision: mapped })
2671+
const answer = await call('decide', { id: item.id, decision: mapped })
2672+
// (#3093) the local host answers a send that failed with the approval its own `release`
2673+
// saved as failed — status plus error, no throw — so a normal answer is not a send. Counting
2674+
// it as one dropped the card and let a batch's receipt name people it never reached. A
2675+
// rejection is the one non-approved answer that is not a failure.
2676+
if (answer && typeof answer === 'object' && (answer.error || answer.status === 'failed')) {
2677+
return notSent(String(answer.error ?? 'the send failed'))
2678+
}
26602679
if (always) {
26612680
// A face-room line turns on Auto-send for the Bot that wrote it only (#457).
26622681
if (item.kind === 'face_room_message' || room?.origin === 'face-to-face') {
@@ -2685,14 +2704,7 @@ window.__ModuleLoader__.load({
26852704
// A stale or failed hold must say why, never swallow the click. The reason also goes back to
26862705
// the caller: a batch has to report it after its last send, not lose it to that send's own
26872706
// clear (#3071).
2688-
const reason = String(failure?.message ?? failure).slice(0, 200)
2689-
setError(reason)
2690-
// A retry the host refused may be tried again; the id was never marked spent.
2691-
if (retry) unmarkDockRetried(sessionId, item.id)
2692-
// #3075: the line alone dies with the next click; the card keeps the reason as its own state,
2693-
// so a poll that still sees the hold pending cannot draw it back as if nothing happened.
2694-
if (item.source !== 'local') rememberDockFailure(sessionId, item.id, reason)
2695-
return { error: reason }
2707+
return notSent(failure)
26962708
} finally { setBusy('') }
26972709
}
26982710
// #3011/#3029: one entry per person, in draw order; a hold whose person never resolved keeps the
Lines changed: 226 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,226 @@
1+
// #3093: a batch's failures were reported only in the dock's one error line. For a Bot Social hold
2+
// the card keeps the reason itself (#3075), but a local hold was left out of that record on the
3+
// ground that the host describes its own sends — and the host does not describe a decision it
4+
// refused. So a batch in a Bot's chat said "Eve: the hold is stale Bob: the hold is stale" in a line
5+
// the next click clears and a chat switch drops, while both cards still read "Waiting for approval".
6+
// A local hold whose dispatch failed is the same shape from the other side: the host answers
7+
// normally with the approval it saved as failed, so the batch counted it as a send and told the
8+
// person "Sent to Eve, Bob" for messages that never left.
9+
// This file drives the dock the way the app does — the real effects, the real module-level record —
10+
// because both halves are only visible after the click, in what the cards say next.
11+
import assert from 'node:assert/strict'
12+
import { readFileSync } from 'node:fs'
13+
import { test } from 'node:test'
14+
15+
const live = { inst: null, idx: 0, rIdx: 0, eIdx: 0 }
16+
const reactStub = {
17+
createElement: (type, props, ...children) => ({ type, props: props ?? {}, children: children.flat(Infinity) }),
18+
Fragment: 'F',
19+
Component: class {},
20+
useState(init) {
21+
const { inst } = live
22+
const i = live.idx++
23+
if (!(i in inst.hooks)) inst.hooks[i] = { value: typeof init === 'function' ? init() : init }
24+
const slot = inst.hooks[i]
25+
return [slot.value, (next) => { slot.value = typeof next === 'function' ? next(slot.value) : next; inst.dirty = true }]
26+
},
27+
useRef(init) {
28+
const { inst } = live
29+
const i = live.rIdx++
30+
if (!(i in inst.refs)) inst.refs[i] = { current: init }
31+
return inst.refs[i]
32+
},
33+
useEffect(fn, deps) {
34+
const { inst } = live
35+
const i = live.eIdx++
36+
inst.effects.push([i, fn, deps])
37+
},
38+
}
39+
for (const name of ['useMemo', 'useCallback', 'useLayoutEffect', 'useContext', 'useReducer', 'useId', 'useSyncExternalStore', 'useTransition', 'useDeferredValue', 'memo', 'forwardRef', 'createContext', 'createPortal', 'cloneElement', 'isValidElement', 'startTransition']) reactStub[name] = (value) => value
40+
41+
globalThis.document ??= { createElement: () => ({ setAttribute() {} }), head: { appendChild() {} }, getElementById: () => null, addEventListener() {}, removeEventListener() {} }
42+
43+
// The browser bundle registers its factory once; one load is one module, so two mounts of it share
44+
// the dock's module-level record — the way a chat switch keeps it in the app.
45+
function loadDock() {
46+
const modules = new Map()
47+
const window = { __ModuleLoader__: { load: ({ id, factory }) => modules.set(id, factory(name => (name === 'react' ? reactStub : {}))) } }
48+
new Function('window', readFileSync(new URL('../client.js', import.meta.url), 'utf8'))(window)
49+
return modules.get('@flybot/social')
50+
}
51+
52+
const fresh = () => ({ hooks: {}, refs: {}, effects: [], dirty: false })
53+
function render(Component, inst, props = inst.props) {
54+
live.inst = inst
55+
live.idx = 0
56+
live.rIdx = 0
57+
live.eIdx = 0
58+
inst.effects = []
59+
inst.props = props
60+
inst.dirty = false
61+
const out = Component(props)
62+
inst.rendered = true
63+
return out
64+
}
65+
// A child component element is a function in `type`: it is called to see the card it draws, with its
66+
// own hook slots, the way React gives it its own.
67+
function mount(element) {
68+
const outer = { inst: live.inst, idx: live.idx, rIdx: live.rIdx, eIdx: live.eIdx }
69+
live.inst = fresh()
70+
live.idx = 0
71+
live.rIdx = 0
72+
live.eIdx = 0
73+
const out = element.type(element.props ?? {})
74+
live.inst = outer.inst
75+
live.idx = outer.idx
76+
live.rIdx = outer.rIdx
77+
live.eIdx = outer.eIdx
78+
return out
79+
}
80+
function drawn(node) {
81+
if (node == null || node === false || node === true) return ''
82+
if (typeof node === 'string' || typeof node === 'number') return String(node)
83+
if (Array.isArray(node)) return node.map(drawn).join(' ')
84+
if (typeof node.type === 'function') return drawn(mount(node))
85+
return (node.children ?? []).map(drawn).join(' ')
86+
}
87+
const tick = () => new Promise(resolve => setTimeout(resolve, 0))
88+
// A frame is drawn and then every effect runs, as React does on mount.
89+
async function frame(app, inst, props) {
90+
render(app.ChatApprovalDock, inst, props)
91+
for (const [, fn] of inst.effects) fn()
92+
await tick()
93+
return render(app.ChatApprovalDock, inst, props)
94+
}
95+
function find(node, predicate) {
96+
if (Array.isArray(node)) {
97+
for (const child of node) {
98+
const hit = find(child, predicate)
99+
if (hit) return hit
100+
}
101+
return undefined
102+
}
103+
if (!node || typeof node !== 'object') return undefined
104+
if (predicate(node)) return node
105+
if (typeof node.type === 'function') return find(mount(node), predicate)
106+
return find(node.children, predicate)
107+
}
108+
function findAll(node, predicate, out = []) {
109+
if (Array.isArray(node)) { for (const child of node) findAll(child, predicate, out); return out }
110+
if (!node || typeof node !== 'object') return out
111+
if (predicate(node)) out.push(node)
112+
if (typeof node.type === 'function') { findAll(mount(node), predicate, out); return out }
113+
findAll(node.children, predicate, out)
114+
return out
115+
}
116+
const byFlybot = id => node => node.props?.['data-flybot'] === id
117+
118+
// A local hold as the host's own `approvals` list returns it (core's held send): the person it is
119+
// for, the Bot that wrote it, and the chat it belongs to. Two of them are the batch.
120+
const localHold = (id, handle, name) => ({
121+
id,
122+
status: 'pending',
123+
senderName: 'Chief',
124+
targetName: name,
125+
request: { from: 'mine', to: { handle }, text: `${name}'s draft`, meta: { sessionId: 's1' } },
126+
})
127+
const holds = () => [localHold('l1', '@eve.home', 'Eve'), localHold('l2', '@bob.home', 'Bob')]
128+
129+
function harness({ decide }) {
130+
const state = { calls: [], notes: [], decide }
131+
globalThis.__flybot__ = {
132+
call: (endpoint, payload) => {
133+
state.calls.push([endpoint, payload])
134+
if (endpoint === 'social/status') return Promise.resolve({ approvals: [] })
135+
if (endpoint === 'approvals') return Promise.resolve(holds())
136+
if (endpoint === 'decide') {
137+
const answer = state.decide(payload)
138+
return answer instanceof Error ? Promise.reject(answer) : Promise.resolve(answer)
139+
}
140+
return Promise.resolve({})
141+
},
142+
}
143+
state.props = { sessionId: 's1', roster: { bots: [] }, bot: { id: 'mine', name: 'Chief' }, claim: () => {}, actions: { notify: text => state.notes.push(text) } }
144+
return state
145+
}
146+
async function openChat(app, state) {
147+
const inst = { ...fresh(), props: state.props }
148+
const tree = await frame(app, inst, state.props)
149+
return { inst, tree }
150+
}
151+
152+
test('#3093: a batch whose sends fail keeps each reason on its own card', async () => {
153+
const app = loadDock()
154+
const state = harness({ decide: () => new Error('the hold is stale') })
155+
try {
156+
const first = await openChat(app, state)
157+
const all = find(first.tree, byFlybot('approve-all'))
158+
assert.ok(all, 'two waiting holds offer Approve all')
159+
assert.deepEqual(all.children, ['Approve all (2)'], `the label carries the count, saw ${JSON.stringify(all.children)}`)
160+
await all.props.onClick()
161+
162+
const after = render(app.ChatApprovalDock, first.inst, state.props)
163+
const cards = findAll(after, byFlybot('chat-approval'))
164+
assert.equal(cards.length, 2, 'both holds are still there to read')
165+
for (const card of cards) {
166+
assert.equal(card.props['data-state'], 'failed', `a refused send is drawn as failed, saw ${JSON.stringify(drawn(card))}`)
167+
assert.match(drawn(card), /Not sent/, 'the card says the send did not go')
168+
assert.match(drawn(card), /the hold is stale/, 'and carries the reason the line carried')
169+
}
170+
assert.ok(find(after, byFlybot('hold-retry')), 'a refused send offers Retry')
171+
172+
// The line is cleared by the next click and dies with the chat; the reason has to live on the
173+
// card, which is what a chat switch reads.
174+
const second = await openChat(app, state)
175+
const again = findAll(second.tree, byFlybot('chat-approval'))
176+
assert.equal(again.length, 2, 'the way back draws both holds')
177+
for (const card of again) {
178+
assert.equal(card.props['data-state'], 'failed', `the way back still draws the refused card as failed, saw ${JSON.stringify(drawn(card))}`)
179+
assert.match(drawn(card), /the hold is stale/, 'the reason survives the chat switch')
180+
}
181+
assert.doesNotMatch(drawn(second.tree), /Waiting for approval/, 'a card whose send was refused is never a fresh wait again')
182+
} finally { delete globalThis.__flybot__ }
183+
})
184+
185+
test('#3093: a batch never reports a local hold whose send failed as sent', async () => {
186+
const app = loadDock()
187+
// Core's `decide` answers normally with the approval its own `release` saved as failed: the send
188+
// did not go, and the reason is on the answer, not in a thrown error.
189+
const state = harness({ decide: payload => ({ id: payload.id, status: 'failed', error: 'Bot Social is unreachable' }) })
190+
try {
191+
const first = await openChat(app, state)
192+
await find(first.tree, byFlybot('approve-all')).props.onClick()
193+
assert.deepEqual(state.notes, [], 'no receipt claims a send that never went out')
194+
195+
const after = render(app.ChatApprovalDock, first.inst, state.props)
196+
const cards = findAll(after, byFlybot('chat-approval'))
197+
assert.equal(cards.length, 2, 'both holds stay drawn: neither went out')
198+
for (const card of cards) {
199+
assert.equal(card.props['data-state'], 'failed', `the unsent card is drawn as failed, saw ${JSON.stringify(drawn(card))}`)
200+
assert.match(drawn(card), /Bot Social is unreachable/, 'and says why it did not go')
201+
}
202+
} finally { delete globalThis.__flybot__ }
203+
})
204+
205+
test('#3093: a local reject is a decision, not a failed send', async () => {
206+
const app = loadDock()
207+
// Core's `decide('reject')` also answers normally, with the approval it saved as rejected; that is
208+
// the person's decision landing, not a send that did not go, so the card leaves and nothing is
209+
// receipted or offered for retry.
210+
const state = harness({ decide: payload => ({ id: payload.id, status: 'rejected' }) })
211+
try {
212+
const first = await openChat(app, state)
213+
const card = findAll(first.tree, byFlybot('chat-approval'))[0]
214+
const reject = findAll(card, node => node.type === 'button' && drawn(node) === 'Reject')[0]
215+
assert.ok(reject, 'a local pending hold offers Reject')
216+
await reject.props.onClick()
217+
const after = render(app.ChatApprovalDock, first.inst, state.props)
218+
const cards = findAll(after, byFlybot('chat-approval'))
219+
assert.equal(cards.length, 1, 'the rejected card leaves; the other hold stays')
220+
assert.doesNotMatch(drawn(after), /Eve's draft/, 'the rejected card is the one that went')
221+
assert.match(drawn(after), /Bob's draft/, 'the other hold is untouched')
222+
assert.doesNotMatch(drawn(after), /Not sent/, 'a decision that landed is not a failed send')
223+
assert.equal(find(after, byFlybot('hold-retry')), undefined, 'and it offers no Retry')
224+
assert.deepEqual(state.notes, [], 'a rejected card is not receipted as sent')
225+
} finally { delete globalThis.__flybot__ }
226+
})

0 commit comments

Comments
 (0)