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)