fix: honour a standing approval on a turn with no verdict (#608)

changelog.d/608-always-allow-own-chat-turn.fixed.md

@@ -0,0 +1 @@
+"Always allow" now keeps working when DSH opens its own retry turn after the owner answered: a turn with no message verdict in a Bot's own chat uses the Bot's standing approval instead of showing the same card again, and a grant that a turn cannot use says why once in the log ([#608](https://github.com/ThinkFlowLab/FlyBot/issues/608)).

core/src/host/approvals.js

@@ -73,9 +73,26 @@ export function install(rt) {
     return (room?.standingApprovals ?? []).some(grant => grant.kind === kind && sameOwner(grant))
   }
 
-  // The grant is the owner's: it answers only a turn the owner started (turn-origin.js). A
-  // friend's message, a room with other people in it, or a turn with no verdict still asks.
-  const standingCovers = (sessionId, botId, roomId, kind) => settled(rt, sessionId) === OWNER && standingGrant(botId, roomId, kind)
+  // The grant is the owner's: it answers a turn the owner started (turn-origin.js), and one that
+  // carries no verdict at all in the Bot's own chat (#608) — DSH opens such a turn for its own
+  // retry after the owner's answer, and nobody's request is in it, so the owner's grant stands. A
+  // friend's message, another member's line or a room with other people in it still asks.
+  const ownChat = (sessionId, botId) => rt.chatOwners?.get(sessionId) === botId
+  const skipped = new Set()
+  const standingCovers = (sessionId, botId, roomId, kind) => {
+    const verdict = settled(rt, sessionId)
+    if (verdict !== OWNER && (verdict !== undefined || !ownChat(sessionId, botId))) {
+      // A deliberate skip is said once, never silent: the owner sees the card, the log says why.
+      const key = `${sessionId}:${kind}`
+      if (standingGrant(botId, roomId, kind) && !skipped.has(key)) {
+        if (skipped.size > 200) skipped.clear()
+        skipped.add(key)
+        rt.warn?.('ds-bot: the standing approval for %s was not used: the turn is %s', kind, verdict === undefined ? 'not the owner\'s' : `"${verdict}"`)
+      }
+      return false
+    }
+    return standingGrant(botId, roomId, kind)
+  }
 
   async function grantStanding(entry, scope) {
     await rt.load()

core/test/group-status.test.js

@@ -1,5 +1,5 @@
 import assert from 'node:assert/strict'
-import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
+import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
 import { tmpdir } from 'node:os'
 import { join } from 'node:path'
 import { test } from 'node:test'
@@ -24,9 +24,8 @@ const seed = () => ({
   revision: 0,
 })
 
-async function start() {
-  const home = mkdtempSync(join(tmpdir(), 'ds-bot-status-'))
-  writeFileSync(join(home, 'state.json'), JSON.stringify(seed()))
+async function start(home = mkdtempSync(join(tmpdir(), 'ds-bot-status-'))) {
+  if (!existsSync(join(home, 'state.json'))) writeFileSync(join(home, 'state.json'), JSON.stringify(seed()))
   const ctx = fakeContext()
   const agents = new Map()
   const agentOf = (sessionId) => {
@@ -71,7 +70,8 @@ async function start() {
   const tool = (name, args, sessionId, exec = {}) => ctx.registeredTools.get(name).execute(args, { agent: { id: sessionId }, ...exec })
   const call = async (endpoint, payload) => (await route.fetch(new Request('http://localhost/api/bot', { method: 'POST', body: JSON.stringify({ endpoint, payload }) }))).json()
   const askApproval = (sessionId, extra = {}, next = () => 'from-dsh') => ctx.listeners.get('approval/request')[0]({ agent: { session: { id: sessionId } }, toolName: 'pwsh', reason: 'escalate sandbox', ...extra }, next)
-  const stop = async () => { await ctx.dispose(); rmSync(home, { recursive: true, force: true, maxRetries: 5 }) }
+  // A restart test reopens the same home, so removal is the caller's choice.
+  const stop = async ({ keepHome = false } = {}) => { await ctx.dispose(); if (!keepHome) rmSync(home, { recursive: true, force: true, maxRetries: 5 }) }
   await tool('list_bots', {}, 'a')
   return { agentOf, emit, tool, call, askApproval, stop, archived, home }
 }
@@ -350,6 +350,42 @@ test('a standing grant never answers a turn someone other than the owner started
   assert.equal(await host2.askApproval('a', { toolName: 'bash', reason: ESCALATE }, () => 'from-dsh'), 'from-dsh')
 })
 
+// #608: the owner answered "Always allow for this Bot" and the same card came back. DSH opens a
+// retry turn of its own after an answer; that turn carries no message verdict, so the grant was
+// skipped. Nobody's request is in such a turn in the Bot's own chat, so the owner's grant stands.
+test('#608: an own-chat turn that carries no verdict still honours the Bot grant', async (t) => {
+  const host = await start()
+  t.after(host.stop)
+  await openTurn(t, host)
+  await grantFromGroup(host, 'always-bot')
+  // A turn DSH starts for itself: no message, or only a host notice, so turn-origin has no verdict.
+  host.emit('b', 'turn/start', { turn: 2 })
+  assert.equal(await host.askApproval('b', { toolName: 'bash', reason: ESCALATE }, () => 'from-dsh'), 'allowed-once')
+  host.emit('b', 'turn/end', { reason: { kind: 'completed' } })
+  host.emit('b', 'turn/start', { turn: 3 })
+  host.emit('b', 'user/message', { content: [{ type: 'text', text: 'note' }], source: { kind: 'ds-bot', role: 'mention' } })
+  assert.equal(await host.askApproval('b', { toolName: 'bash', reason: ESCALATE }, () => 'from-dsh'), 'allowed-once')
+  // Someone else's turn in that chat still asks, and says once why the grant was not used.
+  host.emit('b', 'turn/end', { reason: { kind: 'completed' } })
+  host.emit('b', 'turn/start', { turn: 4 })
+  host.emit('b', 'user/message', { content: [{ type: 'text', text: 'hi' }], source: { kind: 'bot', remote: { handle: '@friend' }, form: 'remote-chat' } })
+  assert.equal(await host.askApproval('b', { toolName: 'bash', reason: ESCALATE }, () => 'from-dsh'), 'from-dsh')
+})
+
+test('#608: a grant survives a restart and answers the same ask again', async (t) => {
+  const first = await start()
+  t.after(() => rmSync(first.home, { recursive: true, force: true, maxRetries: 5 }))
+  await openTurn(t, first)
+  await grantFromGroup(first, 'always-bot')
+  await first.stop({ keepHome: true })
+  const second = await start(first.home)
+  t.after(second.stop)
+  // Same store, a session the runtime has to pick up again: the grant is still there.
+  second.emit('b', 'turn/start', { turn: 1 })
+  second.emit('b', 'user/message', { content: [{ type: 'text', text: 'go' }], source: { kind: 'user' } })
+  assert.equal(await second.askApproval('b', { toolName: 'bash', reason: ESCALATE }, () => 'from-dsh'), 'allowed-once')
+})
+
 test('Always allow for this group covers that group only', async (t) => {
   const host = await start()
   t.after(host.stop)

docs/subsystems/core-host.md

@@ -272,7 +272,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, a room with other people in it, or a turn with no verdict still asks), 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. `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. 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)
 

patches/ds-bot/0001-core-fixes.patch

@@ -1939,7 +1939,7 @@ diff --git a/src/host/approvals.js b/src/host/approvals.js
      const id = randomUUID()
      const entry = {
        id,
-@@ -47,14 +47,65 @@ export function install(rt) {
+@@ -47,14 +47,82 @@ export function install(rt) {
      })
    }, { prepend: true })
 
@@ -1955,9 +1955,26 @@ diff --git a/src/host/approvals.js b/src/host/approvals.js
 +    return (room?.standingApprovals ?? []).some(grant => grant.kind === kind && sameOwner(grant))
 +  }
 +
-+  // The grant is the owner's: it answers only a turn the owner started (turn-origin.js). A
-+  // friend's message, a room with other people in it, or a turn with no verdict still asks.
-+  const standingCovers = (sessionId, botId, roomId, kind) => settled(rt, sessionId) === OWNER && standingGrant(botId, roomId, kind)
++  // The grant is the owner's: it answers a turn the owner started (turn-origin.js), and one that
++  // carries no verdict at all in the Bot's own chat (#608) — DSH opens such a turn for its own
++  // retry after the owner's answer, and nobody's request is in it, so the owner's grant stands. A
++  // friend's message, another member's line or a room with other people in it still asks.
++  const ownChat = (sessionId, botId) => rt.chatOwners?.get(sessionId) === botId
++  const skipped = new Set()
++  const standingCovers = (sessionId, botId, roomId, kind) => {
++    const verdict = settled(rt, sessionId)
++    if (verdict !== OWNER && (verdict !== undefined || !ownChat(sessionId, botId))) {
++      // A deliberate skip is said once, never silent: the owner sees the card, the log says why.
++      const key = `${sessionId}:${kind}`
++      if (standingGrant(botId, roomId, kind) && !skipped.has(key)) {
++        if (skipped.size > 200) skipped.clear()
++        skipped.add(key)
++        rt.warn?.('ds-bot: the standing approval for %s was not used: the turn is %s', kind, verdict === undefined ? 'not the owner\'s' : `"${verdict}"`)
++      }
++      return false
++    }
++    return standingGrant(botId, roomId, kind)
++  }
 +
 +  async function grantStanding(entry, scope) {
 +    await rt.load()
@@ -5013,20 +5030,36 @@ 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 { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
++import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
  import { tmpdir } from 'node:os'
  import { join } from 'node:path'
  import { test } from 'node:test'
-@@ -73,7 +73,7 @@ async function start() {
+@@ -24,9 +24,8 @@ const seed = () => ({
+   revision: 0,
+ })
+
+-async function start() {
+-  const home = mkdtempSync(join(tmpdir(), 'ds-bot-status-'))
+-  writeFileSync(join(home, 'state.json'), JSON.stringify(seed()))
++async function start(home = mkdtempSync(join(tmpdir(), 'ds-bot-status-'))) {
++  if (!existsSync(join(home, 'state.json'))) writeFileSync(join(home, 'state.json'), JSON.stringify(seed()))
+   const ctx = fakeContext()
+   const agents = new Map()
+   const agentOf = (sessionId) => {
+@@ -71,9 +71,10 @@ async function start() {
+   const tool = (name, args, sessionId, exec = {}) => ctx.registeredTools.get(name).execute(args, { agent: { id: sessionId }, ...exec })
+   const call = async (endpoint, payload) => (await route.fetch(new Request('http://localhost/api/bot', { method: 'POST', body: JSON.stringify({ endpoint, payload }) }))).json()
    const askApproval = (sessionId, extra = {}, next = () => 'from-dsh') => ctx.listeners.get('approval/request')[0]({ agent: { session: { id: sessionId } }, toolName: 'pwsh', reason: 'escalate sandbox', ...extra }, next)
-   const stop = async () => { await ctx.dispose(); rmSync(home, { recursive: true, force: true, maxRetries: 5 }) }
+-  const stop = async () => { await ctx.dispose(); rmSync(home, { recursive: true, force: true, maxRetries: 5 }) }
++  // A restart test reopens the same home, so removal is the caller's choice.
++  const stop = async ({ keepHome = false } = {}) => { await ctx.dispose(); if (!keepHome) rmSync(home, { recursive: true, force: true, maxRetries: 5 }) }
    await tool('list_bots', {}, 'a')
 -  return { agentOf, emit, tool, call, askApproval, stop, archived }
 +  return { agentOf, emit, tool, call, askApproval, stop, archived, home }
  }
 
  // Waits on real I/O (state writes) without timers, which some tests mock.
-@@ -294,3 +294,107 @@ test('an approval asked inside a PTC program shows what that call does', async (
+@@ -294,3 +294,143 @@ 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')
  })
@@ -5086,6 +5119,42 @@ diff --git a/test/group-status.test.js b/test/group-status.test.js
 +  assert.equal(await host2.askApproval('a', { toolName: 'bash', reason: ESCALATE }, () => 'from-dsh'), 'from-dsh')
 +})
 +
++// #608: the owner answered "Always allow for this Bot" and the same card came back. DSH opens a
++// retry turn of its own after an answer; that turn carries no message verdict, so the grant was
++// skipped. Nobody's request is in such a turn in the Bot's own chat, so the owner's grant stands.
++test('#608: an own-chat turn that carries no verdict still honours the Bot grant', async (t) => {
++  const host = await start()
++  t.after(host.stop)
++  await openTurn(t, host)
++  await grantFromGroup(host, 'always-bot')
++  // A turn DSH starts for itself: no message, or only a host notice, so turn-origin has no verdict.
++  host.emit('b', 'turn/start', { turn: 2 })
++  assert.equal(await host.askApproval('b', { toolName: 'bash', reason: ESCALATE }, () => 'from-dsh'), 'allowed-once')
++  host.emit('b', 'turn/end', { reason: { kind: 'completed' } })
++  host.emit('b', 'turn/start', { turn: 3 })
++  host.emit('b', 'user/message', { content: [{ type: 'text', text: 'note' }], source: { kind: 'ds-bot', role: 'mention' } })
++  assert.equal(await host.askApproval('b', { toolName: 'bash', reason: ESCALATE }, () => 'from-dsh'), 'allowed-once')
++  // Someone else's turn in that chat still asks, and says once why the grant was not used.
++  host.emit('b', 'turn/end', { reason: { kind: 'completed' } })
++  host.emit('b', 'turn/start', { turn: 4 })
++  host.emit('b', 'user/message', { content: [{ type: 'text', text: 'hi' }], source: { kind: 'bot', remote: { handle: '@friend' }, form: 'remote-chat' } })
++  assert.equal(await host.askApproval('b', { toolName: 'bash', reason: ESCALATE }, () => 'from-dsh'), 'from-dsh')
++})
++
++test('#608: a grant survives a restart and answers the same ask again', async (t) => {
++  const first = await start()
++  t.after(() => rmSync(first.home, { recursive: true, force: true, maxRetries: 5 }))
++  await openTurn(t, first)
++  await grantFromGroup(first, 'always-bot')
++  await first.stop({ keepHome: true })
++  const second = await start(first.home)
++  t.after(second.stop)
++  // Same store, a session the runtime has to pick up again: the grant is still there.
++  second.emit('b', 'turn/start', { turn: 1 })
++  second.emit('b', 'user/message', { content: [{ type: 'text', text: 'go' }], source: { kind: 'user' } })
++  assert.equal(await second.askApproval('b', { toolName: 'bash', reason: ESCALATE }, () => 'from-dsh'), 'allowed-once')
++})
++
 +test('Always allow for this group covers that group only', async (t) => {
 +  const host = await start()
 +  t.after(host.stop)