fix(approvals): roll a standing grant back when its save is refused

changelog.d/2764-refused-save-grant.fixed.md

@@ -0,0 +1 @@
+A refused state save no longer leaves an unaccepted **Always allow** live in memory: the grant is rolled back and the refusal is reported, so the next matching card still asks instead of being answered on the owner's behalf; a refused **Remove** likewise keeps the grant it could not delete ([#2764](https://github.com/ThinkFlowLab/FlyBot/issues/2764)).

core/src/host/approvals.js

@@ -139,8 +139,19 @@ export function install(rt) {
     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]
-      await rt.save()
+      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()) {
@@ -154,12 +165,21 @@ 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 after = before.filter(grant => grant.kind !== String(kind ?? '') || (scope === 'room' && ownerId !== undefined && grant.ownerId !== ownerId))
-    if (after.length === before.length) return false
+    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
-    await rt.save()
+    // #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
   }
 

core/test/group-status.test.js

@@ -1,5 +1,5 @@
 import assert from 'node:assert/strict'
-import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
+import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
 import { tmpdir } from 'node:os'
 import { join } from 'node:path'
 import { test } from 'node:test'
@@ -508,6 +508,95 @@ test('Always allow for this group is refused for an ask that did not come from a
   assert.equal((await host.call('state', {})).value.bots.find(entry => entry.id === 'a').standingApprovals, undefined)
 })
 
+// #2764: "Always allow" was written into memory first and only then saved, and the catch did
+// nothing but refuse. A save the store refuses (a link or a directory at the temp name its bytes
+// land through) left the grant live in memory while nothing was on disk: the card stayed pending,
+// but the next matching ask was answered `allowed-once` by a grant the owner never gave. The same
+// shape hid a failed removal: the settings list said "gone" while the grant still answered.
+const tempName = home => `${join(home, 'state.json')}.${process.pid}.tmp`
+// Breaking the temp name refuses every write that starts afterwards, so the store has to be
+// idle first: a card or a turn still saving would fail on the same name and answer as an
+// unhandled rejection, which is not the write under test.
+const quietSaves = async (host, home) => {
+  let stable = 0
+  let last = -1
+  for (let i = 0; i < 400 && stable < 3; i += 1) {
+    const memory = (await host.call('state', {})).value.revision
+    const disk = JSON.parse(readFileSync(join(home, 'state.json'), 'utf8')).revision
+    if (memory === disk && disk === last) stable += 1
+    else stable = 0
+    last = disk
+    await new Promise(resolve => setImmediate(resolve))
+  }
+  assert.ok(stable >= 3, 'the store did not settle')
+}
+const refuseSave = async (host) => { await quietSaves(host, host.home); mkdirSync(tempName(host.home)) }
+const allowSave = home => rmSync(tempName(home), { recursive: true, force: true })
+
+test('#2764: a refused Bot-scope save leaves no standing grant in memory', async (t) => {
+  const host = await start()
+  t.after(host.stop)
+  await openTurn(t, host)
+  const first = host.askApproval('gb', { toolName: 'bash', reason: ESCALATE })
+  const [card] = (await host.call('state', {})).value.work.approvals
+  await refuseSave(host)
+  const refused = await host.call('approval-answer', { id: card.id, choice: 'always-bot' })
+  assert.equal(refused.ok, false)
+  assert.match(refused.error.message, /Not saved: .*is not a plain file of its own/)
+  // The card still waits, and the grant the save could not write is not in memory either.
+  assert.deepEqual((await host.call('state', {})).value.work.approvals.map(entry => entry.id), [card.id])
+  assert.equal((await host.call('state', {})).value.bots.find(entry => entry.id === 'b').standingApprovals, undefined, 'the refused grant stayed in the Bot’s record')
+  allowSave(host.home)
+  // The next matching ask is a card again: nothing answers it on the owner's behalf.
+  const second = host.askApproval('gb', { toolName: 'bash', reason: ESCALATE })
+  const waiting = (await host.call('state', {})).value.work.approvals
+  assert.deepEqual(waiting.map(entry => entry.answerable), [true, true])
+  for (const entry of waiting) assert.equal((await host.call('approval-answer', { id: entry.id, choice: 'reject' })).ok, true)
+  assert.equal(await first, 'rejected')
+  assert.equal(await second, 'rejected')
+})
+
+test('#2764: a refused room-scope save leaves no grant on the group', async (t) => {
+  const host = await start()
+  t.after(host.stop)
+  await openTurn(t, host)
+  const first = host.askApproval('gb', { toolName: 'bash', reason: ESCALATE })
+  const [card] = (await host.call('state', {})).value.work.approvals
+  await refuseSave(host)
+  const refused = await host.call('approval-answer', { id: card.id, choice: 'always-room' })
+  assert.equal(refused.ok, false)
+  assert.match(refused.error.message, /Not saved: /)
+  assert.deepEqual((await host.call('state', {})).value.work.approvals.map(entry => entry.id), [card.id])
+  assert.equal((await host.call('state', {})).value.rooms.find(entry => entry.id === 'r1').standingApprovals, undefined, 'the refused grant stayed on the group')
+  allowSave(host.home)
+  const second = host.askApproval('gb', { toolName: 'bash', reason: ESCALATE })
+  const waiting = (await host.call('state', {})).value.work.approvals
+  assert.deepEqual(waiting.map(entry => entry.answerable), [true, true])
+  for (const entry of waiting) assert.equal((await host.call('approval-answer', { id: entry.id, choice: 'reject' })).ok, true)
+  assert.equal(await first, 'rejected')
+  assert.equal(await second, 'rejected')
+})
+
+test('#2764: a refused removal keeps the grant it could not delete', async (t) => {
+  const host = await start()
+  t.after(host.stop)
+  await openTurn(t, host)
+  await grantFromGroup(host, 'always-bot')
+  const grants = async () => (await host.call('state', {})).value.bots.find(entry => entry.id === 'b').standingApprovals
+  assert.deepEqual((await grants()).map(entry => entry.kind), [KIND])
+  await refuseSave(host)
+  const refused = await host.call('standing-approval-remove', { scope: 'bot', id: 'b', kind: KIND })
+  assert.equal(refused.ok, false)
+  assert.match(refused.error.message, /Not saved: /)
+  // Still on disk, so still in memory — and still answering the asks it covers.
+  assert.deepEqual(((await grants()) ?? []).map(entry => entry.kind), [KIND], 'the grant the refused removal could not delete is gone')
+  chatTurn(host, 'b')
+  assert.equal(await host.askApproval('b', { toolName: 'bash', reason: ESCALATE }, () => 'from-dsh'), 'allowed-once')
+  allowSave(host.home)
+  assert.equal((await host.call('standing-approval-remove', { scope: 'bot', id: 'b', kind: KIND })).value, true)
+  assert.equal(await grants(), undefined)
+})
+
 test('a running Bot\'s work names its current step, in one short line, until the turn ends', async (t) => {
   const host = await start()
   t.after(host.stop)

docs/subsystems/core-host.md

@@ -614,7 +614,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. 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. 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)