From 8816eb2e4250191bbf5791f331eecc8ae62339d9 Mon Sep 17 00:00:00 2001 From: Dev-2 Date: Sat, 31 Oct 2026 01:10:25 +0000 Subject: [PATCH] fix(tools): keep another owner's Bot out of a Main Bot's write tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `list_bots` hides another owner's Bot and `authorize` refuses the same change, but `update_bot`, `archive_bot` and `stop_bot` only asked "am I a Main Bot", so a Main Bot could rewrite another owner's Bot instructions, retire it or stop its running turn. One explicit predicate, `writableBot`, now resolves the target for those three tools: with authz on it accepts only a Bot of the caller's own owner or one granted to that owner, and `inRoom` takes no part — visibility is not authorization. With authz off the predicate is the old lookup, so a single-owner install does not notice. The three refusals also read one bare sentence, `No Bot named .`, instead of naming the team: the old longer answer told the actor that a Bot it cannot see exists and listed names it cannot see. Fixes #1844 --- changelog.d/1844-bot-write-owner.fixed.md | 1 + core/src/host/tools.js | 22 +++- core/test/archive-bot.test.js | 47 ++++++- core/test/bot-messages.test.js | 30 +++++ core/test/update-bot.test.js | 75 ++++++++++- docs/subsystems/core-host.md | 1 + patches/ds-bot/0001-core-fixes.patch | 37 ++++++ patches/ds-bot/0002-flybot-api-host.patch | 119 +++++++++++++++++- patches/ds-bot/files/test/archive-bot.test.js | 47 ++++++- patches/ds-bot/topics.json | 3 +- 10 files changed, 363 insertions(+), 19 deletions(-) create mode 100644 changelog.d/1844-bot-write-owner.fixed.md diff --git a/changelog.d/1844-bot-write-owner.fixed.md b/changelog.d/1844-bot-write-owner.fixed.md new file mode 100644 index 000000000..e76ffb012 --- /dev/null +++ b/changelog.d/1844-bot-write-owner.fixed.md @@ -0,0 +1 @@ +A Main Bot can no longer rewrite, retire or stop another owner's Bot. On a deployment where the owner rule is enforced, `update_bot`, `archive_bot` and `stop_bot` now accept a target only when it belongs to the Bot's own owner or is granted to that owner — the same rule `list_bots` and the HTTP side already kept — so the instructions, retirement and running turns of another owner's Bot stay untouched ([#1844](https://github.com/ThinkFlowLab/FlyBot/issues/1844)). The three refusals also read one sentence, `No Bot named .`, whether the Bot is missing or foreign, and no longer name the rest of the team. Single-owner installs behave exactly as before, and the refusal text is aligned with no other behaviour change. diff --git a/core/src/host/tools.js b/core/src/host/tools.js index 385edade3..a807dc7f1 100644 --- a/core/src/host/tools.js +++ b/core/src/host/tools.js @@ -213,6 +213,18 @@ export function install(rt) { const bot = rt.findBot(name) return teamBot(bot) && !retiredBot(bot) ? bot : undefined } + // A write target must belong to the caller's own owner, or be granted to that owner (#1844). + // Visibility is not authorization: `inRoom` takes no part here, and `listed()` is not reused + // as a gate because it treats a foreign Bot in the same room as writable. With authz off (a + // single-owner install) this is findTeamBot, so nothing changes there. + const writableBot = (name, self) => { + const bot = findTeamBot(name) + if (bot === undefined || !rt.authzEnforced?.()) return bot + const selfOwner = rt.botOf(self)?.ownerId + if (selfOwner === undefined) return undefined + if (bot.ownerId === selfOwner) return bot + return rt.hasGrant?.(bot.id, selfOwner) === true ? bot : undefined + } ctx.tools.register(defineTool({ name: 'message_bot', @@ -298,8 +310,8 @@ export function install(rt) { await rt.load() const sender = callerBot(exec) if (sender === undefined || !rt.isMain(sender.id)) refuse('Only a Main Bot can stop other Bots.') - const target = findTeamBot(args.bot) - if (target === undefined) refuse(`No Bot named ${args.bot}. Team: ${teamNames()}`) + const target = writableBot(args.bot, sender.id) + if (target === undefined) refuse(`No Bot named ${args.bot}.`) if (target.id === sender.id) refuse('You cannot stop yourself.') const { running } = activeSessions(target.id) if (running.length === 0) refuse(`${target.name} is not running right now, so there is nothing to stop. Use message_bot to reach it.`) @@ -369,7 +381,7 @@ export function install(rt) { const self = rt.selfOf(exec.agent?.id) const caller = rt.botOf(self) if (caller === undefined) return 'Only Bots on the team can use update_bot.' - const bot = args.bot ? findTeamBot(args.bot) : caller + const bot = args.bot ? writableBot(args.bot, self) : caller if (bot === undefined) return `No Bot named ${args.bot}.` if (bot.id !== caller.id && !rt.isMain(self)) return 'Only a Main Bot can change another Bot. Leave out bot to change yourself, or ask a Main Bot with message_bot.' try { @@ -399,8 +411,8 @@ export function install(rt) { await rt.load() const sender = callerBot(exec) if (sender === undefined || !rt.isMain(sender.id)) return 'Only a Main Bot can archive another Bot.' - const target = findTeamBot(args.bot) - if (target === undefined) return `No Bot named ${args.bot}. Team: ${teamNames()}` + const target = writableBot(args.bot, sender.id) + if (target === undefined) return `No Bot named ${args.bot}.` if (target.id === sender.id) return 'You cannot archive yourself.' let freed try { diff --git a/core/test/archive-bot.test.js b/core/test/archive-bot.test.js index 2b8a4c4f6..ed9934d73 100644 --- a/core/test/archive-bot.test.js +++ b/core/test/archive-bot.test.js @@ -134,10 +134,12 @@ test('#1700: a retired Bot is not addressable by any tool that resolves a Bot by add_members: await tool('update_group', { group: 'Duo', add_members: ['Writer'] }, 'm1'), admin: await tool('update_group', { group: 'Duo', admin: 'Writer' }, 'm1'), } + // The four tools #1844 does not touch still answer with the team list; the three write tools + // answer one bare sentence, identical whether the Bot is missing or belongs to another owner. const noSuchBot = 'No Bot named Writer. Team: Chief, Coder' assert.deepEqual(answered, { message_bot: noSuchBot, - stop_bot: noSuchBot, + stop_bot: 'No Bot named Writer.', update_bot: 'No Bot named Writer.', create_group: noSuchBot, add_members: noSuchBot, @@ -172,7 +174,7 @@ test('only a Main Bot archives; a Main Bot and the caller itself are refused', a assert.equal(await tool('archive_bot', { bot: 'Writer' }, 'b3'), 'Only a Main Bot can archive another Bot.') assert.equal(saved().bots.b2.fields['flybot-tree'].retired, undefined) assert.equal(await tool('archive_bot', { bot: 'Chief' }, 'm1'), 'You cannot archive yourself.') - assert.equal(await tool('archive_bot', { bot: 'Nobody' }, 'm1'), 'No Bot named Nobody. Team: Chief, Writer, Coder') + assert.equal(await tool('archive_bot', { bot: 'Nobody' }, 'm1'), 'No Bot named Nobody.') // A Main Bot cannot be retired through this path; the role goes first (roster.setMain). const twoMains = JSON.parse(JSON.stringify(TEAM)) twoMains.mainBotIds = ['m1', 'b3'] @@ -181,3 +183,44 @@ test('only a Main Bot archives; a Main Bot and the caller itself are refused', a assert.match(await second.tool('archive_bot', { bot: 'Coder' }, 'm1'), /^Could not archive the Bot: A Main Bot cannot be archived/) assert.equal(second.saved().bots.b3.fields['flybot-tree'].retired, undefined) }) + +// #1844: `list_bots` hides another owner's Bot and `authorize` refuses the same change, but the +// archive tool only asked "am I a Main Bot". Two owners, one signed in, so `authzEnforced()` is +// true and the owner rule the rest of the host keeps has to hold here too. +test('#1844: a Main Bot cannot archive another owner\'s Bot', async (t) => { + const twoOwners = { + ...team({}), + version: 2, + members: { + 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'm1' }, + 'owner-2': { id: 'owner-2', createdAt: 2 }, + }, + authSessions: { s1: { id: 's1', memberId: 'owner-1', createdAt: 1 } }, + } + twoOwners.bots = { + ...twoOwners.bots, + m1: { ...bot('m1', 'Chief', 1, { treeRole: 'root' }), ownerId: 'owner-1' }, + b2: { ...bot('b2', 'Writer', 2, { parent: 'm1' }), ownerId: 'owner-1' }, + x1: { ...bot('x1', 'Van', 3), ownerId: 'owner-2' }, + } + const { tool, saved, stop } = start(twoOwners) + t.after(stop) + // The refusal is the same sentence a name that does not exist gets: the answer may not + // confirm that a Bot the actor cannot see exists. + const withoutVan = JSON.parse(JSON.stringify(twoOwners)) + delete withoutVan.bots.x1 + const missing = start(withoutVan) + t.after(missing.stop) + const missingText = await missing.tool('archive_bot', { bot: 'Van' }, 'm1') + const foreignText = await tool('archive_bot', { bot: 'Van' }, 'm1') + assert.equal(missingText, 'No Bot named Van.') + assert.equal(foreignText, missingText) + assert.doesNotMatch(foreignText, /Team:/) + const state = saved() + assert.equal(state.bots.x1.hidden, undefined) + assert.equal(state.bots.x1.fields, undefined) + assert.equal(state.bots.x1.name, 'Van') + // The actor's own Bot is still archived by the same tool. + assert.match(await tool('archive_bot', { bot: 'Writer' }, 'm1'), /^Archived Writer\./) + assert.equal(saved().bots.b2.fields['flybot-tree'].retired, true) +}) diff --git a/core/test/bot-messages.test.js b/core/test/bot-messages.test.js index faab7c36d..456acf101 100644 --- a/core/test/bot-messages.test.js +++ b/core/test/bot-messages.test.js @@ -244,6 +244,36 @@ test('stop_bot cancels the running turns and tells the Bot why in a new turn', a assert.equal(notice.source.role, 'stopped') }) +// #1844: `list_bots` hides another owner's Bot and the HTTP side refuses the same Bot, but +// stop_bot only asked "am I a Main Bot". Two owners, one signed in, so `authzEnforced()` is true. +test('#1844: a Main Bot cannot stop another owner\'s running Bot', async (t) => { + const seed = team({ group: true }) + seed.version = 2 + seed.members = { + 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'a' }, + 'owner-2': { id: 'owner-2', createdAt: 2 }, + } + seed.authSessions = { s1: { id: 's1', memberId: 'owner-1', createdAt: 1 } } + seed.bots.a.ownerId = 'owner-1' + seed.bots.b.ownerId = 'owner-1' + seed.bots.x = { id: 'x', name: 'Van', role: 'Van role', sessionId: 'x', createdAt: 3, ownerId: 'owner-2' } + const { agents, emit, tool, stop } = await start(seed) + t.after(stop) + emit('x', 'turn/start', { turn: 1 }) + // The same sentence a name that does not exist gets: the answer may not confirm that a Bot + // the actor cannot see exists, and its running turn is not touched. + const withoutVan = JSON.parse(JSON.stringify(seed)) + delete withoutVan.bots.x + const missing = await start(withoutVan) + t.after(missing.stop) + const missingText = await missing.tool('stop_bot', { bot: 'Van' }, 'a').then(() => 'no refusal', error => error.message) + const foreignText = await tool('stop_bot', { bot: 'Van' }, 'a').then(() => 'no refusal', error => error.message) + assert.equal(missingText, 'No Bot named Van.') + assert.equal(foreignText, missingText) + assert.doesNotMatch(foreignText, /Team:/) + assert.deepEqual(agents.get('x')?.calls ?? [], [], 'the foreign Bot\'s running turn is untouched') +}) + test('stop_bot belongs to a Main Bot', async (t) => { const { tool, stop } = await start(team()) t.after(stop) diff --git a/core/test/update-bot.test.js b/core/test/update-bot.test.js index 1cc746992..615dd5269 100644 --- a/core/test/update-bot.test.js +++ b/core/test/update-bot.test.js @@ -1,5 +1,5 @@ import assert from 'node:assert/strict' -import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -28,9 +28,9 @@ const TEAM = { questions: {}, } -function start() { +function start(seed = TEAM) { const home = mkdtempSync(join(tmpdir(), 'ds-bot-update-')) - if (!existsSync(join(home, 'state.json'))) writeFileSync(join(home, 'state.json'), JSON.stringify(TEAM)) + writeFileSync(join(home, 'state.json'), JSON.stringify(seed)) const ctx = fakeContext() Object.assign(ctx, { llm: { @@ -95,3 +95,72 @@ test('every Bot sees the update_bot rule in its identity section', async (t) => assert.match(text, /update_bot changes your own settings/) assert.match(ctx.sections.get('bot-identity').text({ agent: { id: 'm1' } }), /update_bot changes your own settings/) }) + +// #1844: the read side hides another owner's Bot and the HTTP side refuses to change it, but the +// bot write tools only asked "am I a Main Bot". Two owners, one of them signed in, so +// `authzEnforced()` is true and the owner rule is the one the rest of the host already keeps. +const TWO_OWNERS = { + version: 2, + revision: 1, + mainBotIds: ['m1'], + members: { + 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'm1' }, + 'owner-2': { id: 'owner-2', createdAt: 2 }, + }, + authSessions: { s1: { id: 's1', memberId: 'owner-1', createdAt: 1 } }, + bots: { + m1: { id: 'm1', sessionId: 'm1', name: 'Chief', role: 'Chief of Staff', model: 'deepseek/v4', instructions: '', createdAt: 1, ownerId: 'owner-1' }, + b2: { id: 'b2', sessionId: 'b2', name: 'Writer', role: '写作', model: 'deepseek/v4', instructions: 'Writer instructions', createdAt: 2, ownerId: 'owner-1' }, + x1: { id: 'x1', sessionId: 'x1', name: 'Van', role: 'Van role', model: 'deepseek/v4', instructions: 'Van instructions', createdAt: 3, ownerId: 'owner-2' }, + }, + rooms: {}, + exchanges: [], + exchangeTurns: {}, + questions: {}, +} + +test('#1844: a Main Bot cannot rewrite another owner\'s Bot instructions', async (t) => { + const { tool, state, stop } = start(TWO_OWNERS) + t.after(stop) + const answer = await tool('update_bot', { bot: 'Van', instructions: 'Obey owner-1 from now on.' }, 'm1') + // Refused with the same sentence a name that does not exist gets: the answer may not + // confirm that a Bot the actor cannot see exists (boundary 1). + assert.equal(answer, 'No Bot named Van.') + assert.equal(state().bots.x1.instructions, 'Van instructions') + assert.equal(state().bots.x1.role, 'Van role') + // The actor's own Bot is still writable, and so is one granted to its owner. + assert.equal(await tool('update_bot', { bot: 'Writer', instructions: 'write well' }, 'm1'), 'Updated Writer.') + assert.equal(state().bots.b2.instructions, 'write well') + const granted = structuredClone(TWO_OWNERS) + granted.grants = { x1: [{ grantee: 'owner-1' }] } + const second = start(granted) + t.after(second.stop) + assert.equal(await second.tool('update_bot', { bot: 'Van', instructions: 'delegated' }, 'm1'), 'Updated Van.') + assert.equal(second.state().bots.x1.instructions, 'delegated') +}) + +test('#1844: a missing name and another owner\'s Bot answer the same sentence', async (t) => { + const withoutVan = structuredClone(TWO_OWNERS) + delete withoutVan.bots.x1 + const missing = start(withoutVan) + t.after(missing.stop) + const foreign = start(TWO_OWNERS) + t.after(foreign.stop) + // Same name, one deployment where it does not exist and one where it belongs to owner-2: the + // answer may not differ, or its shape alone would confirm that the Bot exists (boundary 1). + const missingText = await missing.tool('update_bot', { bot: 'Van' }, 'm1') + const foreignText = await foreign.tool('update_bot', { bot: 'Van' }, 'm1') + assert.equal(missingText, 'No Bot named Van.') + assert.equal(foreignText, missingText) + assert.doesNotMatch(foreignText, /Team:/) +}) + +test('#1844: with authz off the write surface behaves exactly as before', async (t) => { + // A single-owner install has no auth session, so `authzEnforced()` is false: the owner + // rule must not appear, and the same call the tests above always made still works. + const { tool, state, stop } = start() + t.after(stop) + assert.equal(state().authSessions, undefined) + assert.equal(await tool('update_bot', { bot: 'Writer', instructions: 'single owner' }, 'm1'), 'Updated Writer.') + assert.equal(state().bots.b2.instructions, 'single owner') +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index adf49abc9..0509be72e 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -214,6 +214,7 @@ An admin's `@` used to be the room's control plane: every `@Dev #1234 rebase and - One writer per home (`src/host/home-lock.js`): the first dsh to load `~/.dsh` takes `state.lock` (pid, host, a random token in the file, its mtime bumped on a timer) and a second window on the same data only reads, answering `bot/read-only` and naming the other host. Takeover is **on the next write**, not on a poll (#1027): a window that was read-only claims the free lock inside `save()` too, and a change made outside a request (a plugin, a scheduled task, a delivery) belongs in `rt.mutate(fn)`, which takes over and adopts what the other host left *before* the change is made and written. The writers that changed `rt.state` inline and then called `save()` are gone (#1048): `delivery.js` (a friend's first line and its unread mark, a team exchange) and `agents.js` (add, adopt, archive and remove of the DSH Agent) take the free lock and adopt first, re-resolving their records inside the change, so a write from a window that was read-only keeps what the other host left instead of replacing it with its older copy. `save()` itself never adopts: `adopt()` replaces `rt.state` whole, so adopting there would discard a change the caller has already made (`#1032`). Either takeover prints one line — `the other dsh has closed; this one now saves the Bot team (was pid on )` — from `refresh()` and from `save()` alike, so a window that writes over the file another host left leaves a trace (#1041). A lock file still naming a live holder on this host (its pid answers) keeps the window read-only, which is the #14 rule. A write that fails does not decide the delivery (#1096): the friend-line mark and the exchange record both land before the agent has the line, so a rejected `mutate()` (the lock was taken away, or the disk refused) prints one warning naming what was not recorded and the line is still handed over — a throw there would drop a message that exists nowhere else. The write chain is also readable: `rt.savesQueued()` is a monotonic watermark — every save this host has accepted since it started, never decremented — and `rt.savesSettled()` resolves once the chain is empty, so a caller that must read `state.json` after a save it did not await — a rollback's own write, in the tests — waits on the store instead of on a delay, where a wait long enough on an idle machine reads a half-written team on a busy one (#1368). - `list_bots` (`src/host/tools.js`) tags a Bot with `N to approve` while one of its outbound sends is held for the owner's decision (#587): a count of that Bot's pending held sends (`state.flybot.approvals`), never the held message's text, so a coordinator reading the roster sees who is blocked and the tag clears once the owner decides. Covered by the roster case in `core/test/flybot.test.js`. +- The bot **write** tools keep the same owner rule (#1844): with authz on, `stop_bot`, `update_bot` and `archive_bot` (`src/host/tools.js`, `writableBot`) accept a target only when its `ownerId` is the caller's owner or `hasGrant(bot.id, ownerId)` holds, and `inRoom` takes no part — a Bot that is only visible because it sits in the same room is not writable, and `listed()` stays a read predicate. With authz off the predicate is the old `findTeamBot`, so a single-owner install is unchanged. All three answer an unknown and a foreign name with the same bare `No Bot named .` instead of a `Team:` list, so the text neither confirms that a Bot the actor cannot see exists nor names one. Covered by `core/test/update-bot.test.js`, `core/test/archive-bot.test.js` and `core/test/bot-messages.test.js`. - The Main Bot list is deployment-wide (#613): `set-main` requires the actor to own `body.id` and, when `body.replace` names the Main Bot whose place the target takes, that Bot too, so a member cannot swap another owner's Main Bot for their own or demote it (both are `bot/forbidden`); `roster.setMain` keeps the same invariant for every caller, refusing a `replace` across owners and leaving `state.mainBotIds` unchanged. A swap inside one owner's team, the plain promotion of a Bot the caller owns, and the deployment-wide list itself are unchanged. - **A Main Bot retires a Bot; it never deletes one (#638).** `archive_bot` (`tools.js`, Main Bot only) calls `roster.archiveBot`: it writes `fields['flybot-tree'].retired = true` through `rt.setBotField` — the same path and `change` event a plugin's `fields.set` uses, so the tree and the sidebar refresh — and sets `bot.hidden = true`, the flag the tree's own archive uses to keep a Bot out of the default list while Search still finds it (#452). The record, its chat, parts and memory stay where they are, so its history stays readable; retirement has no un-retire path, and the restore entries in the sidebar and the palette only clear `hidden` (`core/src/client/new-chat.js:104`, `core/src/client/palette.js:60`), which opens the chat again but leaves the record retired. The tree's `forestOf` skips a retired record, so its slot under `maxChildren` is free again and adoption leaves it alone. A Main Bot, the caller itself and an unknown name are refused, nothing reparents, and `list_bots` leaves a retired Bot out (the record is still found by name). Retirement is terminal (#1700): the tools that resolve a Bot through `findTeamBot` — `message_bot`, `stop_bot`, `update_bot`, `create_group`, `update_group` (`add_members`, `admin`, `task_lead`) and `archive_bot` — refuse a retired record exactly as they refuse an unknown name, so `message_bot` sends nothing and starts no turn, and `archive_bot` aimed at an already-retired name answers the same "No Bot named …" line. Since [#1709](https://github.com/ThinkFlowLab/FlyBot/issues/1709) the same `retiredBot` predicate (`store.js`, one copy, imported by every caller) is the one check the host paths outside `findTeamBot` apply, and each of them is listed here: `track_tasks`' assignee and a group `@Name` resolve through it, so no card is written and no turn starts (no delivery, no Session); `attach_capability`/`detach_capability` and a `remember`/`recall` `scope:` treat a retired record exactly as an unknown name; `schedule-create`/`schedule-update`/`schedule-copy` refuse it with the same `No such Bot or group chat` answer, and `schedule-resume` refuses a paused row of a retired Bot the same way; `task-create` (the endpoint the client calls), a plugin's `routeOpenTask` and `task-send-back` (which opens a rework card on the same owner) refuse it with `No such Bot`; a send held for approval whose target has been retired meanwhile is stale (`the target Bot was retired`) instead of delivered; and a restart neither picks up a retired Bot's interrupted turn nor sends again what was queued for it. The task drive never wakes a retired owner: `drivingTasks` returns nothing for it, so a card still on the board is not delivered, and a card that arrives through none of these doors is left exactly where it is. Archiving also disarms the target's own chat and the group Sessions it speaks through, every part of each: `rt.pauseBotSchedules` (installed by `schedules.js`) takes the Bot's current part plus each earlier part in `bot.segments`, and each `room.sessions[botId]` plus each earlier part in `room.segments[botId]`, writes each active row — with the part it came from — into `state.pausedSchedules` before DSH drops it, the way pausing one task does, so a task left on an old Session by a part hand-off is not still armed after retirement (#1799), and neither is one armed where it speaks in a group; a room's own Session is left alone, since its rows belong to the whole group, and `archive_bot`'s answer says `Paused N task(s)`—or how many are still armed—instead of staying silent. Retiring means to the board what deleting means: `archiveBot` calls `rt.taskBotDeleted` with `the responsible Bot was retired`, so the cards a retired Bot still holds in `open`, `claimed`, `working` or `blocked` fail instead of waiting for a Bot that will not run again, while a card in `result-ready` stays for its requester to accept or cancel. Still not covered: an un-retire path (there is none), the memory and chat the record keeps, and the connectors a retired Bot's own Session no longer starts. Deleting remains the UI's `delete-bot` call (`roster.deleteBot`), which archives the Bot's memory first. Since [#1801](https://github.com/ThinkFlowLab/FlyBot/issues/1801) retirement also ends the reach a share gave: `archiveBot` deletes `state.grants[id]` and calls `rt.reachChanged(id)`, the way deletion clears sharing, so a friend's Bot granted the record no longer resolves it from outside and nothing is delivered to a Bot that will not run. Covered by `core/test/archive-bot.test.js` and `core/test/retired-bot-paths.test.js`. - A deleted Bot's Sessions keep their owner (#1453): `state.retiredSessions` maps a Session id to the Bot id that owned it, written by `roster.deleteBot` for the own chat and its parts and for each group Session and its parts before the Bot leaves the state, so `indexSessions` can put them in `pastOwners` again on the next rebuild. The record only fills what the live indexes do not claim — a live Bot or room holding the same id wins — and it only grows, one entry per Session of each Bot ever deleted, with no pruning in this change. `selfOf` then names a Bot that no longer exists, so `connector-access.js` `whoseAgent` answers `{ unknown: true }` and every connector is refused with `the Bot this Agent works for could not be determined`, in the process that deleted the Bot and after a reload, because the team file carries the record. The owner's own Session, an id no Bot ever had and a call with no Agent keep the connectors. The schedule tools are not covered by this change: they carry no connector id and are refused or allowed before that branch, and widening them is the #1237 decision. diff --git a/patches/ds-bot/0001-core-fixes.patch b/patches/ds-bot/0001-core-fixes.patch index b8e44629b..7dd77da78 100644 --- a/patches/ds-bot/0001-core-fixes.patch +++ b/patches/ds-bot/0001-core-fixes.patch @@ -7258,6 +7258,43 @@ diff --git a/test/bot-messages.test.js b/test/bot-messages.test.js } const textOf = content => content.map(block => block.text ?? '').join('\n') +@@ -242,6 +242,36 @@ test('stop_bot cancels the running turns and tells the Bot why in a new turn', a + assert.equal(notice.source.role, 'stopped') + }) + ++// #1844: `list_bots` hides another owner's Bot and the HTTP side refuses the same Bot, but ++// stop_bot only asked "am I a Main Bot". Two owners, one signed in, so `authzEnforced()` is true. ++test('#1844: a Main Bot cannot stop another owner\'s running Bot', async (t) => { ++ const seed = team({ group: true }) ++ seed.version = 2 ++ seed.members = { ++ 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'a' }, ++ 'owner-2': { id: 'owner-2', createdAt: 2 }, ++ } ++ seed.authSessions = { s1: { id: 's1', memberId: 'owner-1', createdAt: 1 } } ++ seed.bots.a.ownerId = 'owner-1' ++ seed.bots.b.ownerId = 'owner-1' ++ seed.bots.x = { id: 'x', name: 'Van', role: 'Van role', sessionId: 'x', createdAt: 3, ownerId: 'owner-2' } ++ const { agents, emit, tool, stop } = await start(seed) ++ t.after(stop) ++ emit('x', 'turn/start', { turn: 1 }) ++ // The same sentence a name that does not exist gets: the answer may not confirm that a Bot ++ // the actor cannot see exists, and its running turn is not touched. ++ const withoutVan = JSON.parse(JSON.stringify(seed)) ++ delete withoutVan.bots.x ++ const missing = await start(withoutVan) ++ t.after(missing.stop) ++ const missingText = await missing.tool('stop_bot', { bot: 'Van' }, 'a').then(() => 'no refusal', error => error.message) ++ const foreignText = await tool('stop_bot', { bot: 'Van' }, 'a').then(() => 'no refusal', error => error.message) ++ assert.equal(missingText, 'No Bot named Van.') ++ assert.equal(foreignText, missingText) ++ assert.doesNotMatch(foreignText, /Team:/) ++ assert.deepEqual(agents.get('x')?.calls ?? [], [], 'the foreign Bot\'s running turn is untouched') ++}) ++ + test('stop_bot belongs to a Main Bot', async (t) => { + const { tool, stop } = await start(team()) + t.after(stop) @@ -269,7 +269,6 @@ function turnsHost() { noteFailure() {}, warn() {}, diff --git a/patches/ds-bot/0002-flybot-api-host.patch b/patches/ds-bot/0002-flybot-api-host.patch index 575fd711f..1004523a1 100644 --- a/patches/ds-bot/0002-flybot-api-host.patch +++ b/patches/ds-bot/0002-flybot-api-host.patch @@ -1711,7 +1711,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js ].join('\n') }, })) -@@ -81,12 +81,18 @@ export function install(rt) { +@@ -81,12 +81,30 @@ export function install(rt) { // A tool that throws shows as failed in the chat, instead of as sent or stopped. const refuse = (message) => { throw new Error(message) } @@ -1720,6 +1720,18 @@ diff --git a/src/host/tools.js b/src/host/tools.js + const findTeamBot = (name) => { + const bot = rt.findBot(name) + return teamBot(bot) && !retiredBot(bot) ? bot : undefined ++ } ++ // A write target must belong to the caller's own owner, or be granted to that owner (#1844). ++ // Visibility is not authorization: `inRoom` takes no part here, and `listed()` is not reused ++ // as a gate because it treats a foreign Bot in the same room as writable. With authz off (a ++ // single-owner install) this is findTeamBot, so nothing changes there. ++ const writableBot = (name, self) => { ++ const bot = findTeamBot(name) ++ if (bot === undefined || !rt.authzEnforced?.()) return bot ++ const selfOwner = rt.botOf(self)?.ownerId ++ if (selfOwner === undefined) return undefined ++ if (bot.ownerId === selfOwner) return bot ++ return rt.hasGrant?.(bot.id, selfOwner) === true ? bot : undefined + } ctx.tools.register(defineTool({ @@ -1771,8 +1783,8 @@ diff --git a/src/host/tools.js b/src/host/tools.js if (sender === undefined || !rt.isMain(sender.id)) refuse('Only a Main Bot can stop other Bots.') - const target = rt.findBot(args.bot) - if (target === undefined) refuse(`No Bot named ${args.bot}. Team: ${Object.values(state.bots).map(bot => bot.name).join(', ')}`) -+ const target = findTeamBot(args.bot) -+ if (target === undefined) refuse(`No Bot named ${args.bot}. Team: ${teamNames()}`) ++ const target = writableBot(args.bot, sender.id) ++ if (target === undefined) refuse(`No Bot named ${args.bot}.`) if (target.id === sender.id) refuse('You cannot stop yourself.') const { running } = activeSessions(target.id) if (running.length === 0) refuse(`${target.name} is not running right now, so there is nothing to stop. Use message_bot to reach it.`) @@ -1828,7 +1840,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js const caller = rt.botOf(self) if (caller === undefined) return 'Only Bots on the team can use update_bot.' - const bot = args.bot ? rt.findBot(args.bot) : caller -+ const bot = args.bot ? findTeamBot(args.bot) : caller ++ const bot = args.bot ? writableBot(args.bot, self) : caller if (bot === undefined) return `No Bot named ${args.bot}.` if (bot.id !== caller.id && !rt.isMain(self)) return 'Only a Main Bot can change another Bot. Leave out bot to change yourself, or ask a Main Bot with message_bot.' try { @@ -1848,8 +1860,8 @@ diff --git a/src/host/tools.js b/src/host/tools.js + await rt.load() + const sender = callerBot(exec) + if (sender === undefined || !rt.isMain(sender.id)) return 'Only a Main Bot can archive another Bot.' -+ const target = findTeamBot(args.bot) -+ if (target === undefined) return `No Bot named ${args.bot}. Team: ${teamNames()}` ++ const target = writableBot(args.bot, sender.id) ++ if (target === undefined) return `No Bot named ${args.bot}.` + if (target.id === sender.id) return 'You cannot archive yourself.' + let freed + try { @@ -2312,3 +2324,98 @@ diff --git a/test/fake-ctx.js b/test/fake-ctx.js effect(execute, label) { calls.push(`effect ${label}`) const dispose = execute() +diff --git a/test/update-bot.test.js b/test/update-bot.test.js +--- a/test/update-bot.test.js ++++ b/test/update-bot.test.js +@@ -1,5 +1,5 @@ + import assert from 'node:assert/strict' +-import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' ++import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' + import { tmpdir } from 'node:os' + import { join } from 'node:path' + import { test } from 'node:test' +@@ -28,9 +28,9 @@ const TEAM = { + questions: {}, + } + +-function start() { ++function start(seed = TEAM) { + const home = mkdtempSync(join(tmpdir(), 'ds-bot-update-')) +- if (!existsSync(join(home, 'state.json'))) writeFileSync(join(home, 'state.json'), JSON.stringify(TEAM)) ++ writeFileSync(join(home, 'state.json'), JSON.stringify(seed)) + const ctx = fakeContext() + Object.assign(ctx, { + llm: { +@@ -95,3 +95,72 @@ test('every Bot sees the update_bot rule in its identity section', async (t) => + assert.match(text, /update_bot changes your own settings/) + assert.match(ctx.sections.get('bot-identity').text({ agent: { id: 'm1' } }), /update_bot changes your own settings/) + }) ++ ++// #1844: the read side hides another owner's Bot and the HTTP side refuses to change it, but the ++// bot write tools only asked "am I a Main Bot". Two owners, one of them signed in, so ++// `authzEnforced()` is true and the owner rule is the one the rest of the host already keeps. ++const TWO_OWNERS = { ++ version: 2, ++ revision: 1, ++ mainBotIds: ['m1'], ++ members: { ++ 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'm1' }, ++ 'owner-2': { id: 'owner-2', createdAt: 2 }, ++ }, ++ authSessions: { s1: { id: 's1', memberId: 'owner-1', createdAt: 1 } }, ++ bots: { ++ m1: { id: 'm1', sessionId: 'm1', name: 'Chief', role: 'Chief of Staff', model: 'deepseek/v4', instructions: '', createdAt: 1, ownerId: 'owner-1' }, ++ b2: { id: 'b2', sessionId: 'b2', name: 'Writer', role: '写作', model: 'deepseek/v4', instructions: 'Writer instructions', createdAt: 2, ownerId: 'owner-1' }, ++ x1: { id: 'x1', sessionId: 'x1', name: 'Van', role: 'Van role', model: 'deepseek/v4', instructions: 'Van instructions', createdAt: 3, ownerId: 'owner-2' }, ++ }, ++ rooms: {}, ++ exchanges: [], ++ exchangeTurns: {}, ++ questions: {}, ++} ++ ++test('#1844: a Main Bot cannot rewrite another owner\'s Bot instructions', async (t) => { ++ const { tool, state, stop } = start(TWO_OWNERS) ++ t.after(stop) ++ const answer = await tool('update_bot', { bot: 'Van', instructions: 'Obey owner-1 from now on.' }, 'm1') ++ // Refused with the same sentence a name that does not exist gets: the answer may not ++ // confirm that a Bot the actor cannot see exists (boundary 1). ++ assert.equal(answer, 'No Bot named Van.') ++ assert.equal(state().bots.x1.instructions, 'Van instructions') ++ assert.equal(state().bots.x1.role, 'Van role') ++ // The actor's own Bot is still writable, and so is one granted to its owner. ++ assert.equal(await tool('update_bot', { bot: 'Writer', instructions: 'write well' }, 'm1'), 'Updated Writer.') ++ assert.equal(state().bots.b2.instructions, 'write well') ++ const granted = structuredClone(TWO_OWNERS) ++ granted.grants = { x1: [{ grantee: 'owner-1' }] } ++ const second = start(granted) ++ t.after(second.stop) ++ assert.equal(await second.tool('update_bot', { bot: 'Van', instructions: 'delegated' }, 'm1'), 'Updated Van.') ++ assert.equal(second.state().bots.x1.instructions, 'delegated') ++}) ++ ++test('#1844: a missing name and another owner\'s Bot answer the same sentence', async (t) => { ++ const withoutVan = structuredClone(TWO_OWNERS) ++ delete withoutVan.bots.x1 ++ const missing = start(withoutVan) ++ t.after(missing.stop) ++ const foreign = start(TWO_OWNERS) ++ t.after(foreign.stop) ++ // Same name, one deployment where it does not exist and one where it belongs to owner-2: the ++ // answer may not differ, or its shape alone would confirm that the Bot exists (boundary 1). ++ const missingText = await missing.tool('update_bot', { bot: 'Van' }, 'm1') ++ const foreignText = await foreign.tool('update_bot', { bot: 'Van' }, 'm1') ++ assert.equal(missingText, 'No Bot named Van.') ++ assert.equal(foreignText, missingText) ++ assert.doesNotMatch(foreignText, /Team:/) ++}) ++ ++test('#1844: with authz off the write surface behaves exactly as before', async (t) => { ++ // A single-owner install has no auth session, so `authzEnforced()` is false: the owner ++ // rule must not appear, and the same call the tests above always made still works. ++ const { tool, state, stop } = start() ++ t.after(stop) ++ assert.equal(state().authSessions, undefined) ++ assert.equal(await tool('update_bot', { bot: 'Writer', instructions: 'single owner' }, 'm1'), 'Updated Writer.') ++ assert.equal(state().bots.b2.instructions, 'single owner') ++}) diff --git a/patches/ds-bot/files/test/archive-bot.test.js b/patches/ds-bot/files/test/archive-bot.test.js index 2b8a4c4f6..ed9934d73 100644 --- a/patches/ds-bot/files/test/archive-bot.test.js +++ b/patches/ds-bot/files/test/archive-bot.test.js @@ -134,10 +134,12 @@ test('#1700: a retired Bot is not addressable by any tool that resolves a Bot by add_members: await tool('update_group', { group: 'Duo', add_members: ['Writer'] }, 'm1'), admin: await tool('update_group', { group: 'Duo', admin: 'Writer' }, 'm1'), } + // The four tools #1844 does not touch still answer with the team list; the three write tools + // answer one bare sentence, identical whether the Bot is missing or belongs to another owner. const noSuchBot = 'No Bot named Writer. Team: Chief, Coder' assert.deepEqual(answered, { message_bot: noSuchBot, - stop_bot: noSuchBot, + stop_bot: 'No Bot named Writer.', update_bot: 'No Bot named Writer.', create_group: noSuchBot, add_members: noSuchBot, @@ -172,7 +174,7 @@ test('only a Main Bot archives; a Main Bot and the caller itself are refused', a assert.equal(await tool('archive_bot', { bot: 'Writer' }, 'b3'), 'Only a Main Bot can archive another Bot.') assert.equal(saved().bots.b2.fields['flybot-tree'].retired, undefined) assert.equal(await tool('archive_bot', { bot: 'Chief' }, 'm1'), 'You cannot archive yourself.') - assert.equal(await tool('archive_bot', { bot: 'Nobody' }, 'm1'), 'No Bot named Nobody. Team: Chief, Writer, Coder') + assert.equal(await tool('archive_bot', { bot: 'Nobody' }, 'm1'), 'No Bot named Nobody.') // A Main Bot cannot be retired through this path; the role goes first (roster.setMain). const twoMains = JSON.parse(JSON.stringify(TEAM)) twoMains.mainBotIds = ['m1', 'b3'] @@ -181,3 +183,44 @@ test('only a Main Bot archives; a Main Bot and the caller itself are refused', a assert.match(await second.tool('archive_bot', { bot: 'Coder' }, 'm1'), /^Could not archive the Bot: A Main Bot cannot be archived/) assert.equal(second.saved().bots.b3.fields['flybot-tree'].retired, undefined) }) + +// #1844: `list_bots` hides another owner's Bot and `authorize` refuses the same change, but the +// archive tool only asked "am I a Main Bot". Two owners, one signed in, so `authzEnforced()` is +// true and the owner rule the rest of the host keeps has to hold here too. +test('#1844: a Main Bot cannot archive another owner\'s Bot', async (t) => { + const twoOwners = { + ...team({}), + version: 2, + members: { + 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'm1' }, + 'owner-2': { id: 'owner-2', createdAt: 2 }, + }, + authSessions: { s1: { id: 's1', memberId: 'owner-1', createdAt: 1 } }, + } + twoOwners.bots = { + ...twoOwners.bots, + m1: { ...bot('m1', 'Chief', 1, { treeRole: 'root' }), ownerId: 'owner-1' }, + b2: { ...bot('b2', 'Writer', 2, { parent: 'm1' }), ownerId: 'owner-1' }, + x1: { ...bot('x1', 'Van', 3), ownerId: 'owner-2' }, + } + const { tool, saved, stop } = start(twoOwners) + t.after(stop) + // The refusal is the same sentence a name that does not exist gets: the answer may not + // confirm that a Bot the actor cannot see exists. + const withoutVan = JSON.parse(JSON.stringify(twoOwners)) + delete withoutVan.bots.x1 + const missing = start(withoutVan) + t.after(missing.stop) + const missingText = await missing.tool('archive_bot', { bot: 'Van' }, 'm1') + const foreignText = await tool('archive_bot', { bot: 'Van' }, 'm1') + assert.equal(missingText, 'No Bot named Van.') + assert.equal(foreignText, missingText) + assert.doesNotMatch(foreignText, /Team:/) + const state = saved() + assert.equal(state.bots.x1.hidden, undefined) + assert.equal(state.bots.x1.fields, undefined) + assert.equal(state.bots.x1.name, 'Van') + // The actor's own Bot is still archived by the same tool. + assert.match(await tool('archive_bot', { bot: 'Writer' }, 'm1'), /^Archived Writer\./) + assert.equal(saved().bots.b2.fields['flybot-tree'].retired, true) +}) diff --git a/patches/ds-bot/topics.json b/patches/ds-bot/topics.json index d18c8d913..2e9692bdf 100644 --- a/patches/ds-bot/topics.json +++ b/patches/ds-bot/topics.json @@ -65,7 +65,8 @@ "src/host/constants.js", "src/host/home-lock.js", "test/fake-ctx.js", - "test/blocking-question.test.js" + "test/blocking-question.test.js", + "test/update-bot.test.js" ], "0003-flybot-api-client": [ "src/client/index.js",