fix(host): serialize two answers on one approval record (#2782)

changelog.d/2782-approvals-serialization.fixed.md

@@ -0,0 +1 @@
+Two "Always allow" answers on the same Bot could lose one of the grants. A refused save put the whole list back the way it was found, and the second answer had already added its own grant to that list in memory, so the rollback dropped it — and a save queued behind the refused one could write a grant whose own save had been refused. Answers on one record now run one after another, so each rollback touches only its own grant.

core/src/host/approvals.js

@@ -129,34 +129,56 @@ export function install(rt) {
     return standingGrant(botId, roomId, kind)
   }
 
+  // Security F1: two writers on the same record must not overlap their read-modify-save-rollback.
+  // The store snapshots the whole state when `save()` is called and queues the write, so a second
+  // writer can slip in between a first writer's mutation and its refused write: rolling the
+  // "before" snapshot back then discards the second writer's grant (and a queued save carrying the
+  // first writer's grant writes a refused "Always allow" to disk). One chain per record closes that
+  // window — the next writer starts only after the previous save and its rollback settled, so its
+  // snapshot already reflects the rolled-back list and memory stays equal to the file.
+  const chains = new Map()
+  const inTurn = async (target, work) => {
+    const previous = chains.get(target) ?? Promise.resolve()
+    const run = previous.then(work, work)
+    const tail = run.then(() => {}, () => {})
+    chains.set(target, tail)
+    try {
+      return await run
+    } finally {
+      if (chains.get(target) === tail) chains.delete(target)
+    }
+  }
+
   async function grantStanding(entry, scope) {
     await rt.load()
     const bot = rt.botOf(entry.botId)
     const target = scope === 'room' ? rt.roomOf(entry.roomId) : bot
     if (bot === undefined || bot.remote !== undefined) throw Object.assign(new Error('Only your own Bot can be given a standing approval'), { code: 'bot/forbidden' })
     if (target === undefined) throw Object.assign(new Error('This approval did not come from a group'), { code: 'bot/invalid' })
     const kind = approvalKind(entry.tool, entry.reason)
-    const grants = target.standingApprovals ?? []
-    if (!grants.some(grant => grant.kind === kind && grant.ownerId === bot.ownerId)) {
-      const grant = { kind, label: kindLabel(entry.tool, entry.reason), at: Date.now(), ...(bot.ownerId === undefined ? {} : { ownerId: bot.ownerId }) }
-      // #2764: the grant exists only once the store holds it. A refused save changed nothing on
-      // disk, so a grant left in memory would answer the next matching card without ever being
-      // written — an "Always allow" the owner never actually gave. The list is replaced, never
-      // edited in place, so the old reference is the snapshot to put back.
-      const before = target.standingApprovals
-      target.standingApprovals = [...grants, grant]
-      try {
-        await rt.save()
-      } catch (error) {
-        if (before === undefined) delete target.standingApprovals
-        else target.standingApprovals = before
-        throw error
+    await inTurn(target, async () => {
+      const grants = target.standingApprovals ?? []
+      if (!grants.some(grant => grant.kind === kind && grant.ownerId === bot.ownerId)) {
+        const grant = { kind, label: kindLabel(entry.tool, entry.reason), at: Date.now(), ...(bot.ownerId === undefined ? {} : { ownerId: bot.ownerId }) }
+        // #2764: the grant exists only once the store holds it. A refused save changed nothing on
+        // disk, so a grant left in memory would answer the next matching card without ever being
+        // written — an "Always allow" the owner never actually gave. The list is replaced, never
+        // edited in place, so the old reference is the snapshot to put back.
+        const before = target.standingApprovals
+        target.standingApprovals = [...grants, grant]
+        try {
+          await rt.save()
+        } catch (error) {
+          if (before === undefined) delete target.standingApprovals
+          else target.standingApprovals = before
+          throw error
+        }
       }
-    }
-    // Other cards the new grant already covers stop waiting too.
-    for (const other of pending.values()) {
-      if (other.id !== entry.id && other.answer !== undefined && standingCovers(other.sessionId, other.botId, other.roomId, approvalKindOf(other))) other.answer('allowed-once')
-    }
+      // Other cards the new grant already covers stop waiting too.
+      for (const other of pending.values()) {
+        if (other.id !== entry.id && other.answer !== undefined && standingCovers(other.sessionId, other.botId, other.roomId, approvalKindOf(other))) other.answer('allowed-once')
+      }
+    })
   }
   const approvalKindOf = entry => approvalKind(entry.tool, entry.reason)
 
@@ -165,22 +187,24 @@ export function install(rt) {
     await rt.load()
     const target = scope === 'room' ? rt.roomOf(String(id ?? '')) : rt.botOf(String(id ?? ''))
     if (target === undefined) throw Object.assign(new Error('Not found'), { code: 'bot/not-found' })
-    const before = target.standingApprovals
-    const grants = before ?? []
-    const after = grants.filter(grant => grant.kind !== String(kind ?? '') || (scope === 'room' && ownerId !== undefined && grant.ownerId !== ownerId))
-    if (after.length === grants.length) return false
-    if (after.length === 0) delete target.standingApprovals
-    else target.standingApprovals = after
-    // #2764: a removal the store refused is not a removal — the grant is still on disk, so it
-    // must still be in memory, or the settings card says "gone" while every new ask is refused.
-    try {
-      await rt.save()
-    } catch (error) {
-      if (before === undefined) delete target.standingApprovals
-      else target.standingApprovals = before
-      throw error
-    }
-    return true
+    return inTurn(target, async () => {
+      const before = target.standingApprovals
+      const grants = before ?? []
+      const after = grants.filter(grant => grant.kind !== String(kind ?? '') || (scope === 'room' && ownerId !== undefined && grant.ownerId !== ownerId))
+      if (after.length === grants.length) return false
+      if (after.length === 0) delete target.standingApprovals
+      else target.standingApprovals = after
+      // #2764: a removal the store refused is not a removal — the grant is still on disk, so it
+      // must still be in memory, or the settings card says "gone" while every new ask is refused.
+      try {
+        await rt.save()
+      } catch (error) {
+        if (before === undefined) delete target.standingApprovals
+        else target.standingApprovals = before
+        throw error
+      }
+      return true
+    })
   }
 
   const approvalsView = () => [...pending.values()].map(({ answer, ...entry }) => ({ ...entry, answerable: answer !== undefined }))

core/test/approvals-serialization.test.js

@@ -0,0 +1,149 @@
+// #2782: a refused save puts the whole standing-approval list back the way it was found. The
+// snapshot is taken before the write and the writes are only queued (store.js:1014-1018), so two
+// writers on the same Bot can overlap: the one whose save is refused restores a list that already
+// lacks — or already has — the other writer's grant. The refusal here is the store's own (the
+// temp name is a directory, store.js:1022), so the two saves stay refused however the calls land.
+import assert from 'node:assert/strict'
+import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import { test } from 'node:test'
+import { createRuntime } from '../index.js'
+import { fakeContext } from './fake-ctx.js'
+
+const KIND_1 = 'bash:escalate:danger-full-access'
+const KIND_2 = 'pwsh'
+const seed = (grants) => {
+  const state = {
+    version: 1,
+    mainBotIds: ['a'],
+    bots: {
+      a: { id: 'a', name: 'Chief', role: 'Boss', sessionId: 'a', createdAt: 1 },
+      b: { id: 'b', name: 'Writer', role: 'Write', sessionId: 'b', createdAt: 2 },
+    },
+    rooms: { r1: { id: 'r1', name: 'Ops', members: ['a', 'b'], mode: 'admin', admin: 'b', named: true, sessions: { b: 'gb' }, log: [], seq: 0, seen: {} } },
+    agents: {}, exchanges: [], exchangeTurns: {}, questions: {}, prefs: {}, revision: 0,
+  }
+  if (grants !== undefined) state.bots.b.standingApprovals = grants
+  return state
+}
+const kindsOf = grants => (grants ?? []).map(grant => grant.kind)
+const tick = () => new Promise(resolve => setImmediate(resolve))
+const until = async (check, what) => {
+  const deadline = Date.now() + 3000
+  while (!check() && Date.now() < deadline) await tick()
+  assert.ok(check(), `never reached: ${what}`)
+}
+const onDisk = home => {
+  const state = JSON.parse(readFileSync(join(home, 'state.json'), 'utf8'))
+  return kindsOf(state.bots.b?.standingApprovals)
+}
+// The store refuses every write whose temp name is not a plain file of its own (store.js:1022).
+const refuseSaves = home => mkdirSync(`${join(home, 'state.json')}.${process.pid}.tmp`)
+const allowSaves = home => rmSync(`${join(home, 'state.json')}.${process.pid}.tmp`, { recursive: true, force: true })
+
+async function start(t, grants, { gated = false } = {}) {
+  const home = mkdtempSync(join(tmpdir(), 'approvals-serialization-'))
+  writeFileSync(join(home, 'state.json'), JSON.stringify(seed(grants)))
+  const ctx = fakeContext()
+  Object.assign(ctx, {
+    llm: { listProviders: () => [], listModels: async () => [], resolveModelInfo: async () => ({}) },
+    workspaceRegistry: { get: () => undefined, resolveByPath: async () => undefined, create: async path => ({ id: 'w1', path }), archiveSession: async () => {} },
+    sessionController: {
+      create: async () => ({ sessionId: 's1' }),
+      resolveAgent: async () => ({ agent: { session: { id: 'gb' }, calls: [], cancelled: [], inbox: { nextStep: [], nextTurn: [], remove: () => false }, followup() {}, steer() {}, cancel() {} } }),
+      selectModel: async () => {},
+    },
+    sessionTitle: { rename: async () => {} },
+    sessions: { flush: async () => {} },
+  })
+  const rt = createRuntime(ctx, { home })
+  await rt.load()
+  t.after(() => rmSync(home, { recursive: true, force: true, maxRetries: 5 }))
+  const ask = (toolName, reason) => ctx.listeners.get('approval/request')[0]({ agent: { session: { id: 'gb' } }, toolName, reason }, () => 'from-dsh')
+  // Held writes: the store still takes its snapshot when the test releases the call, so a test
+  // can put one write in front of the other on purpose instead of racing the clock.
+  const gates = []
+  if (gated) {
+    const realSave = rt.save
+    rt.save = () => new Promise((resolve, reject) => { gates.push(() => realSave().then(resolve, reject)) })
+  }
+  return { rt, home, ask, gates }
+}
+const refusedSave = (outcome) => {
+  assert.equal(outcome.status, 'rejected', 'the store must refuse this save')
+  assert.match(String(outcome.reason.message), /Not saved: .*is not a plain file of its own/)
+}
+const sameAfterRefusal = (t, rt, home) => {
+  const memory = kindsOf(rt.botOf('b').standingApprovals)
+  assert.deepEqual(memory, onDisk(home), 'memory and the file disagree after a refused save')
+  return memory
+}
+
+// Reading 1: two "Always allow" answers on the same Bot overlap and both saves are refused.
+// Neither grant ever reached the store, so neither may be live in memory after the rollback.
+test('overlapping grants: a refused save leaves no grant live that the file does not hold', async (t) => {
+  const { rt, home, ask } = await start(t)
+  refuseSaves(home)
+  void ask('bash', 'escalate sandbox to danger-full-access: read AGENTS.md')
+  void ask('pwsh', 'run the migration script')
+  const cards = rt.approvalsView()
+  assert.equal(cards.length, 2, 'two cards must be waiting')
+
+  const outcomes = await Promise.allSettled([
+    rt.answerApproval(cards[0].id, 'always-bot'),
+    rt.answerApproval(cards[1].id, 'always-bot'),
+  ])
+  for (const outcome of outcomes) refusedSave(outcome)
+
+  const memory = sameAfterRefusal(t, rt, home)
+  assert.deepEqual(memory, [], `a refused grant came back as another writer's old value: ${JSON.stringify(memory)}`)
+})
+
+// Reading 2a: the same two answers, started in the other order.
+test('overlapping grants in the other order: memory still equals the file', async (t) => {
+  const { rt, home, ask } = await start(t)
+  refuseSaves(home)
+  void ask('bash', 'escalate sandbox to danger-full-access: read AGENTS.md')
+  void ask('pwsh', 'run the migration script')
+  const cards = rt.approvalsView()
+  assert.equal(cards.length, 2, 'two cards must be waiting')
+
+  const outcomes = await Promise.allSettled([
+    rt.answerApproval(cards[1].id, 'always-bot'),
+    rt.answerApproval(cards[0].id, 'always-bot'),
+  ])
+  for (const outcome of outcomes) refusedSave(outcome)
+
+  const memory = sameAfterRefusal(t, rt, home)
+  assert.deepEqual(memory, [], `a refused grant came back as another writer's old value: ${JSON.stringify(memory)}`)
+})
+
+// Reading 2b: a removal of one kind overlaps a grant of another kind. The removal is refused and
+// the grant lands, so the Bot must end up holding both grants, in memory and in state.json.
+test('a refused removal: a grant landed in the same window is not dropped', async (t) => {
+  const { rt, home, ask, gates } = await start(t, [{ kind: KIND_1, label: 'bash · danger-full-access', at: 1 }], { gated: true })
+
+  const removed = rt.removeStandingApproval('bot', 'b', KIND_1).then(() => 'saved', error => error)
+  await until(() => kindsOf(rt.botOf('b').standingApprovals).length === 0 && gates.length === 1, 'the removal mutates and its save is held')
+
+  refuseSaves(home)
+  void ask('pwsh', 'run the migration script')
+  const cards = rt.approvalsView()
+  assert.equal(cards.length, 1, 'one card must be waiting')
+  const granted = rt.answerApproval(cards[0].id, 'always-bot').then(() => 'saved', error => error)
+
+  // The removal is refused: it never happened, and it must not take the grant with it.
+  gates.shift()()
+  const refused = await removed
+  assert.notEqual(refused, 'saved', "the removal's save must be refused")
+  assert.match(String(refused.message), /Not saved: .*is not a plain file of its own/)
+
+  await until(() => gates.length === 1, 'the grant\'s save is held after the removal settled')
+  allowSaves(home)
+  gates.shift()()
+  assert.equal(await granted, 'saved', 'the grant\'s save must land')
+
+  const memory = sameAfterRefusal(t, rt, home)
+  assert.deepEqual(memory, [KIND_1, KIND_2], `a refused removal dropped a grant landed in the same window: ${JSON.stringify(memory)}`)
+})

docs/subsystems/core-host.md

@@ -621,7 +621,7 @@ A local, replayable record of every change to the saved team, for multi-Bot bugs
 
 ### Standing tool approvals (#540)
 
-DSH's approval outcomes are one-shot (`ApprovalOutcome`: `allowed-once` / `rejected` / `cancelled` / `unavailable`; `ApprovalPolicy` is `ask` / `never`), so a permanent approval lives in FlyBot, not in DSH (no DSH patch). `src/host/approvals.js` keys a request by its kind (`approvalKind`: the tool, plus the sandbox mode when DSH's reason reads `escalate sandbox to <mode>:`, e.g. `bash:escalate:danger-full-access`). `approval-answer` takes `always-bot` (kept on the Bot as `standingApprovals: [{ kind, label, at, ownerId? }]`) and `always-room` (kept on the room the ask came from; refused for an own-chat ask); both save, answer this ask `allowed-once` and release other waiting asks the grant covers. A save the store refuses changes nothing: the grant is taken back out of memory and the error is rethrown, so an **Always allow** that was never written — the temp name its bytes land through is a link or a directory — cannot answer a later card (#2764), and `standing-approval-remove` keeps a grant whose removal its own refused save could not delete. The `approval/request` listener answers `allowed-once` itself, before any card, only for a turn the owner started (`settled(...) === 'owner'` from `turn-origin.js`; a friend's message, another member's line or a room with other people in it still asks), or for a turn that carries **no verdict at all in the Bot's own chat** — DSH starts such a turn for its own retry after the owner answered, and nobody's request is in it (#608) — and only when the Bot (a local one, never `bot.remote`) has the kind, or the ask comes from a group Session of a room holding the kind for the Bot's owner (`ownerId` matches while authz is enforced; before anyone signs in every local Bot is the one person's); a group grant does not cover the Bot's own chat. DSH's audit pair (`approval/asked` / `approval/decided`) still logs each auto-answered ask. A grant that a turn cannot use is not skipped silently: the first such ask per Session and kind logs `the standing approval for <kind> was not used: the turn is "<verdict>"`. `standing-approval-remove { scope: 'bot' | 'room', id, kind }` revokes (a signed-in remover takes only their own group grant). Authz: an `always-*` answer needs the Bot's owner (owning the room is enough only for Allow once); removing a Bot's grant needs the Bot's owner, a group's the member who gave it. Tests: `core/test/group-status.test.js`, `core/test/authz.test.js`.
+DSH's approval outcomes are one-shot (`ApprovalOutcome`: `allowed-once` / `rejected` / `cancelled` / `unavailable`; `ApprovalPolicy` is `ask` / `never`), so a permanent approval lives in FlyBot, not in DSH (no DSH patch). `src/host/approvals.js` keys a request by its kind (`approvalKind`: the tool, plus the sandbox mode when DSH's reason reads `escalate sandbox to <mode>:`, e.g. `bash:escalate:danger-full-access`). `approval-answer` takes `always-bot` (kept on the Bot as `standingApprovals: [{ kind, label, at, ownerId? }]`) and `always-room` (kept on the room the ask came from; refused for an own-chat ask); both save, answer this ask `allowed-once` and release other waiting asks the grant covers. A save the store refuses changes nothing: the grant is taken back out of memory and the error is rethrown, so an **Always allow** that was never written — the temp name its bytes land through is a link or a directory — cannot answer a later card (#2764), and `standing-approval-remove` keeps a grant whose removal its own refused save could not delete. Two answers that touch the same record are applied one after the other (`chains` / `inTurn` in `approvals.js`): the next reads the rolled-back list, so a refused save takes back only its own grant and can never write one that its own save refused (#2782). The `approval/request` listener answers `allowed-once` itself, before any card, only for a turn the owner started (`settled(...) === 'owner'` from `turn-origin.js`; a friend's message, another member's line or a room with other people in it still asks), or for a turn that carries **no verdict at all in the Bot's own chat** — DSH starts such a turn for its own retry after the owner answered, and nobody's request is in it (#608) — and only when the Bot (a local one, never `bot.remote`) has the kind, or the ask comes from a group Session of a room holding the kind for the Bot's owner (`ownerId` matches while authz is enforced; before anyone signs in every local Bot is the one person's); a group grant does not cover the Bot's own chat. DSH's audit pair (`approval/asked` / `approval/decided`) still logs each auto-answered ask. A grant that a turn cannot use is not skipped silently: the first such ask per Session and kind logs `the standing approval for <kind> was not used: the turn is "<verdict>"`. `standing-approval-remove { scope: 'bot' | 'room', id, kind }` revokes (a signed-in remover takes only their own group grant). Authz: an `always-*` answer needs the Bot's owner (owning the room is enough only for Allow once); removing a Bot's grant needs the Bot's owner, a group's the member who gave it. Tests: `core/test/group-status.test.js`, `core/test/authz.test.js`.
 
 ### Discarded promise `.catch` (#213)