From 02964141327f003fc32b57f3b5db189e83f1281b Mon Sep 17 00:00:00 2001 From: FlyBot Dev-3 Date: Tue, 10 Nov 2026 01:43:29 +0000 Subject: [PATCH] fix(approvals): roll a standing grant back when its save is refused "Always allow" wrote the grant into memory and saved afterwards, and the catch only refused. A save the store refuses left the grant live with nothing on disk: the card stayed pending, but the next matching ask was answered `allowed-once` by a grant the owner never gave. A refused removal had the mirror shape: the settings list said "gone" while the grant still answered. Both now snapshot the list, keep it only once the save lands, put the old reference back on failure and rethrow; the answers a new grant releases stay on the success path, and `revision` is untouched. Fixes #2764 --- changelog.d/2764-refused-save-grant.fixed.md | 1 + core/src/host/approvals.js | 30 ++++- core/test/group-status.test.js | 91 +++++++++++++- docs/subsystems/core-host.md | 2 +- patches/ds-bot/0001-core-fixes.patch | 125 +++++++++++++++++-- 5 files changed, 234 insertions(+), 15 deletions(-) create mode 100644 changelog.d/2764-refused-save-grant.fixed.md diff --git a/changelog.d/2764-refused-save-grant.fixed.md b/changelog.d/2764-refused-save-grant.fixed.md new file mode 100644 index 000000000..ce5f89d16 --- /dev/null +++ b/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)). diff --git a/core/src/host/approvals.js b/core/src/host/approvals.js index 2e1f944ad..b9586b4a7 100644 --- a/core/src/host/approvals.js +++ b/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 } diff --git a/core/test/group-status.test.js b/core/test/group-status.test.js index aca85285e..1092f5b09 100644 --- a/core/test/group-status.test.js +++ b/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) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 655bb1dde..656e99450 100644 --- a/docs/subsystems/core-host.md +++ b/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 :`, 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 was not used: the turn is ""`. `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 :`, 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 was not used: the turn is ""`. `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) diff --git a/patches/ds-bot/0001-core-fixes.patch b/patches/ds-bot/0001-core-fixes.patch index 1c5907846..c28843efd 100644 --- a/patches/ds-bot/0001-core-fixes.patch +++ b/patches/ds-bot/0001-core-fixes.patch @@ -3632,7 +3632,7 @@ diff --git a/src/host/approvals.js b/src/host/approvals.js detail: rt.callDetail?.(sessionId, request.callId) ?? {}, at: Date.now(), } -@@ -47,14 +47,82 @@ export function install(rt) { +@@ -47,14 +47,102 @@ export function install(rt) { }) }, { prepend: true }) @@ -3679,8 +3679,19 @@ diff --git a/src/host/approvals.js b/src/host/approvals.js + 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()) { @@ -3694,12 +3705,21 @@ diff --git a/src/host/approvals.js b/src/host/approvals.js + 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 + } + @@ -11437,7 +11457,7 @@ diff --git a/test/group-status.test.js b/test/group-status.test.js @@ -1,5 +1,5 @@ import assert from 'node:assert/strict' -import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' -+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' @@ -11569,7 +11589,7 @@ diff --git a/test/group-status.test.js b/test/group-status.test.js assert.deepEqual(round.failed, ['Writer']) assert.equal(host.agentOf('gb').calls.length, 1) }) -@@ -294,3 +294,204 @@ test('an approval asked inside a PTC program shows what that call does', async ( +@@ -294,3 +294,293 @@ test('an approval asked inside a PTC program shows what that call does', async ( assert.deepEqual(approval.detail, { text: 'Copy the doc', code: 'Copy-Item a.md E:/docs' }) assert.equal(value.work.bots.b.tool, 'pwsh') }) @@ -11709,6 +11729,95 @@ diff --git a/test/group-status.test.js b/test/group-status.test.js + 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)