From 8825c74dbbe066d9279925ce744920895bff4c54 Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:13:48 +0800 Subject: [PATCH] fix(roster): refuse a colour update_bot cannot store (#2775) The colour was the one field updateBot let pass without storing anything, so update_bot answered "Updated ." for a value the Bot never kept and a caller could not tell a stored colour from a discarded one. An unknown colour now throws the accepted set through the tool's existing catch; null and every accepted name, legacy aliases included, are byte-identical to the base. --- changelog.d/2775-bot-color-invalid.fixed.md | 1 + core/src/host/roster.js | 10 +- core/test/update-bot.test.js | 47 ++++++++- docs/subsystems/core-host.md | 1 + patches/ds-bot/0002-flybot-api-host.patch | 100 +++++++++++++++++--- 5 files changed, 144 insertions(+), 15 deletions(-) create mode 100644 changelog.d/2775-bot-color-invalid.fixed.md diff --git a/changelog.d/2775-bot-color-invalid.fixed.md b/changelog.d/2775-bot-color-invalid.fixed.md new file mode 100644 index 000000000..81afa3033 --- /dev/null +++ b/changelog.d/2775-bot-color-invalid.fixed.md @@ -0,0 +1 @@ +`update_bot` now refuses a colour the host does not know, instead of answering `Updated .` while the Bot keeps the colour it had; the refusal names the accepted colours ([#2775](https://github.com/ThinkFlowLab/FlyBot/issues/2775)). diff --git a/core/src/host/roster.js b/core/src/host/roster.js index 7a105ee88..a8b26ab37 100644 --- a/core/src/host/roster.js +++ b/core/src/host/roster.js @@ -2,7 +2,7 @@ // the first Main Bot of a new team. import { mkdir } from 'node:fs/promises' import { join } from 'node:path' -import { MAX_AVATAR_IMAGE, ROOM_MAX_MEMBERS, ROOM_MODES, ROOM_NAME_MAX, ROOM_NOTICE_MAX } from './constants.js' +import { COLORS, EXTRA_COLORS, MAX_AVATAR_IMAGE, ROOM_MAX_MEMBERS, ROOM_MODES, ROOM_NAME_MAX, ROOM_NOTICE_MAX } from './constants.js' import { currentColor, cut, isColor, stripInvisible, unquote } from './text.js' import { nameProblem } from '../shared/mentions.js' import { cleanExternal } from './room-participant-external.js' @@ -210,7 +210,13 @@ export function install(rt) { let modelChanged = false if (patch.color !== undefined) { if (patch.color === null) delete bot.color - else if (isColor(currentColor(patch.color))) bot.color = currentColor(patch.color) + else { + const color = currentColor(patch.color) + // A colour the host does not know is refused like a bad name or model (#2775): dropping + // it in silence let the tool answer "Updated …" for a value that never landed. + if (!isColor(color)) throw new Error(`Color must be one of ${[...COLORS, ...EXTRA_COLORS].join(', ')} or #rrggbb`) + bot.color = color + } } if (patch.avatar !== undefined) { const avatar = cleanAvatar(patch.avatar) diff --git a/core/test/update-bot.test.js b/core/test/update-bot.test.js index 615dd5269..5759c50b9 100644 --- a/core/test/update-bot.test.js +++ b/core/test/update-bot.test.js @@ -46,14 +46,19 @@ function start(seed = TEAM) { sessions: { flush: async () => {} }, workspaceRegistry: { archiveSession: async () => {} }, }) + // The settings page's own endpoint, where a colour is cleared with `color: null`: the + // `update_bot` tool's schema takes a string and refuses null before any host code runs. + const register = ctx.connection.fetch.register + ctx.connection.fetch.register = (route) => { ctx.route = route; return register(route) } apply(ctx, { home }) const tool = (name, args, sessionId) => ctx.registeredTools.get(name).execute(args, { agent: { id: sessionId } }) + const call = async (endpoint, payload) => (await ctx.route.fetch(new Request('http://localhost/api/bot', { method: 'POST', body: JSON.stringify({ endpoint, payload }) }))).json() const state = () => JSON.parse(readFileSync(join(home, 'state.json'), 'utf8')) const stop = async () => { await ctx.dispose() rmSync(home, { recursive: true, force: true, maxRetries: 5 }) } - return { ctx, tool, state, stop } + return { ctx, tool, call, state, stop } } test('a Bot changes itself by leaving out bot, or by naming itself', async (t) => { @@ -164,3 +169,43 @@ test('#1844: with authz off the write surface behaves exactly as before', async assert.equal(await tool('update_bot', { bot: 'Writer', instructions: 'single owner' }, 'm1'), 'Updated Writer.') assert.equal(state().bots.b2.instructions, 'single owner') }) + +// #2775: the colour was the one field `updateBot` let pass without storing anything, and the +// tool still answered "Updated ." A Bot retried the call or reported a colour the user +// never got. Every other field this function owns refuses through the tool's own catch. +test('#2775: a colour the host does not know is refused, not dropped behind "Updated"', async (t) => { + const { tool, state, stop } = start() + t.after(stop) + const before = state().bots.b2.color + const reply = await tool('update_bot', { bot: 'Writer', color: 'purple' }, 'm1') + assert.doesNotMatch(reply, /^Updated/, `answered ${JSON.stringify(reply)} while storing nothing`) + assert.match(reply, /colou?r|one of/i, `the refusal names the field: ${JSON.stringify(reply)}`) + assert.equal(state().bots.b2.color, before, 'the stored colour did not move') +}) + +// The three guards stay byte-identical to the base: a known name, a legacy alias, and null. +test('#2775: a colour the host knows still lands (guard, green on the base)', async (t) => { + const { tool, state, stop } = start() + t.after(stop) + assert.equal(await tool('update_bot', { bot: 'Writer', color: 'violet' }, 'm1'), 'Updated Writer.') + assert.equal(state().bots.b2.color, 'violet') +}) + +test('#2775: a legacy colour name still lands as its current name (guard, green on the base)', async (t) => { + const { tool, state, stop } = start() + t.after(stop) + assert.equal(await tool('update_bot', { bot: 'Writer', color: 'kimi' }, 'm1'), 'Updated Writer.') + assert.equal(state().bots.b2.color, 'charcoal') +}) + +test('#2775: null still clears the colour (guard, green on the base)', async (t) => { + const { tool, call, state, stop } = start() + t.after(stop) + assert.equal(await tool('update_bot', { bot: 'Writer', color: 'violet' }, 'm1'), 'Updated Writer.') + assert.equal(state().bots.b2.color, 'violet') + // `color: null` is the settings page's clear (settings → colour → reset), not `update_bot`: + // the tool's parameter is a string, so null is refused by the schema before the host sees it. + const cleared = await call('update-bot', { id: 'b2', color: null }) + assert.equal(cleared.ok, true, JSON.stringify(cleared)) + assert.equal('color' in state().bots.b2, false, 'the colour is removed, not set to empty') +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 1698b9021..ae67127e1 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -244,6 +244,7 @@ The host's own files carry the user's content — a Bot's memory and the memory - `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`. +- **`updateBot` refuses a field it cannot store (#2775).** The name, the model and the thinking level already threw; the colour was the one field let pass, so `update_bot { color: 'purple' }` answered `Updated .` while `bots..color` stayed where it was. An unknown colour now throws `Color must be one of , or #rrggbb`, which reaches the caller through the tool's existing `Could not update the Bot: …` catch; `null` still clears the field, and an accepted name — a current one or a legacy alias mapped through `currentColor` — is stored exactly as before. The settings page's own palette (`client/inks.js`) is that same set, so its swatches are unaffected. Covered by `core/test/update-bot.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/0002-flybot-api-host.patch b/patches/ds-bot/0002-flybot-api-host.patch index ec6678576..47dbdd7dc 100644 --- a/patches/ds-bot/0002-flybot-api-host.patch +++ b/patches/ds-bot/0002-flybot-api-host.patch @@ -160,11 +160,13 @@ diff --git a/src/host/home-lock.js b/src/host/home-lock.js diff --git a/src/host/roster.js b/src/host/roster.js --- a/src/host/roster.js +++ b/src/host/roster.js -@@ -3,8 +3,11 @@ +@@ -2,9 +2,12 @@ + // the first Main Bot of a new team. import { mkdir } from 'node:fs/promises' import { join } from 'node:path' - import { MAX_AVATAR_IMAGE, ROOM_MAX_MEMBERS, ROOM_MODES, ROOM_NAME_MAX, ROOM_NOTICE_MAX } from './constants.js' +-import { MAX_AVATAR_IMAGE, ROOM_MAX_MEMBERS, ROOM_MODES, ROOM_NAME_MAX, ROOM_NOTICE_MAX } from './constants.js' -import { currentColor, isColor, unquote } from './text.js' ++import { COLORS, EXTRA_COLORS, MAX_AVATAR_IMAGE, ROOM_MAX_MEMBERS, ROOM_MODES, ROOM_NAME_MAX, ROOM_NOTICE_MAX } from './constants.js' +import { currentColor, cut, isColor, stripInvisible, unquote } from './text.js' import { nameProblem } from '../shared/mentions.js' +import { cleanExternal } from './room-participant-external.js' @@ -357,7 +359,7 @@ diff --git a/src/host/roster.js b/src/host/roster.js }) } -@@ -166,18 +166,20 @@ export function install(rt) { +@@ -166,29 +166,39 @@ export function install(rt) { const moving = patch.workDir !== undefined const workDir = moving ? await rt.cleanWorkDir(patch.workDir ?? '') : undefined if (patch.name !== undefined) { @@ -381,7 +383,18 @@ diff --git a/src/host/roster.js b/src/host/roster.js if (patch.instructions !== undefined) bot.instructions = String(patch.instructions).trim() let modelChanged = false if (patch.color !== undefined) { -@@ -189,6 +189,8 @@ export function install(rt) { + if (patch.color === null) delete bot.color +- else if (isColor(currentColor(patch.color))) bot.color = currentColor(patch.color) ++ else { ++ const color = currentColor(patch.color) ++ // A colour the host does not know is refused like a bad name or model (#2775): dropping ++ // it in silence let the tool answer "Updated …" for a value that never landed. ++ if (!isColor(color)) throw new Error(`Color must be one of ${[...COLORS, ...EXTRA_COLORS].join(', ')} or #rrggbb`) ++ bot.color = color ++ } + } + if (patch.avatar !== undefined) { + const avatar = cleanAvatar(patch.avatar) if (avatar) bot.avatar = avatar else delete bot.avatar } @@ -696,7 +709,7 @@ diff --git a/src/host/roster.js b/src/host/roster.js next = others.with(at, id) } else if (primary) { next = [id, ...others] -@@ -417,12 +417,43 @@ export function install(rt) { +@@ -417,14 +417,45 @@ export function install(rt) { } // The sidebar shows every Main Bot, so a hidden Bot would vanish again once demoted. if (main) delete bot.hidden @@ -706,9 +719,9 @@ diff --git a/src/host/roster.js b/src/host/roster.js - } state.mainBotIds = next + rt.syncDesignatedChief() -+ await rt.save() -+ } -+ + await rt.save() + } + + // #638: retire a Bot instead of deleting it. The tree already reads fields['flybot-tree'].retired: + // adoption skips the record and it no longer counts against maxChildren, while its chat, its parts + // and its memory stay where they are. `hidden` keeps it out of the default Bot list the way the @@ -739,11 +752,13 @@ diff --git a/src/host/roster.js b/src/host/roster.js + delete state.grants[id] + await rt.reachChanged?.(id) + } - await rt.save() ++ await rt.save() + return { bot, freed } - } - ++ } ++ async function deleteBot(id) { + const bot = rt.botOf(id) + if (bot === undefined) throw new Error('Unknown Bot') @@ -434,22 +434,54 @@ export function install(rt) { const current = rt.chatOf(id) // Questions and exchange turns are kept per Session, so every part of the chat is cleared. @@ -2800,7 +2815,28 @@ diff --git a/test/update-bot.test.js b/test/update-bot.test.js 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) => +@@ -46,14 +46,19 @@ function start() { + sessions: { flush: async () => {} }, + workspaceRegistry: { archiveSession: async () => {} }, + }) ++ // The settings page's own endpoint, where a colour is cleared with `color: null`: the ++ // `update_bot` tool's schema takes a string and refuses null before any host code runs. ++ const register = ctx.connection.fetch.register ++ ctx.connection.fetch.register = (route) => { ctx.route = route; return register(route) } + apply(ctx, { home }) + const tool = (name, args, sessionId) => ctx.registeredTools.get(name).execute(args, { agent: { id: sessionId } }) ++ const call = async (endpoint, payload) => (await ctx.route.fetch(new Request('http://localhost/api/bot', { method: 'POST', body: JSON.stringify({ endpoint, payload }) }))).json() + const state = () => JSON.parse(readFileSync(join(home, 'state.json'), 'utf8')) + const stop = async () => { + await ctx.dispose() + rmSync(home, { recursive: true, force: true, maxRetries: 5 }) + } +- return { ctx, tool, state, stop } ++ return { ctx, tool, call, state, stop } + } + + test('a Bot changes itself by leaving out bot, or by naming itself', async (t) => { +@@ -95,3 +95,112 @@ 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/) }) @@ -2873,3 +2909,43 @@ diff --git a/test/update-bot.test.js b/test/update-bot.test.js + assert.equal(await tool('update_bot', { bot: 'Writer', instructions: 'single owner' }, 'm1'), 'Updated Writer.') + assert.equal(state().bots.b2.instructions, 'single owner') +}) ++ ++// #2775: the colour was the one field `updateBot` let pass without storing anything, and the ++// tool still answered "Updated ." A Bot retried the call or reported a colour the user ++// never got. Every other field this function owns refuses through the tool's own catch. ++test('#2775: a colour the host does not know is refused, not dropped behind "Updated"', async (t) => { ++ const { tool, state, stop } = start() ++ t.after(stop) ++ const before = state().bots.b2.color ++ const reply = await tool('update_bot', { bot: 'Writer', color: 'purple' }, 'm1') ++ assert.doesNotMatch(reply, /^Updated/, `answered ${JSON.stringify(reply)} while storing nothing`) ++ assert.match(reply, /colou?r|one of/i, `the refusal names the field: ${JSON.stringify(reply)}`) ++ assert.equal(state().bots.b2.color, before, 'the stored colour did not move') ++}) ++ ++// The three guards stay byte-identical to the base: a known name, a legacy alias, and null. ++test('#2775: a colour the host knows still lands (guard, green on the base)', async (t) => { ++ const { tool, state, stop } = start() ++ t.after(stop) ++ assert.equal(await tool('update_bot', { bot: 'Writer', color: 'violet' }, 'm1'), 'Updated Writer.') ++ assert.equal(state().bots.b2.color, 'violet') ++}) ++ ++test('#2775: a legacy colour name still lands as its current name (guard, green on the base)', async (t) => { ++ const { tool, state, stop } = start() ++ t.after(stop) ++ assert.equal(await tool('update_bot', { bot: 'Writer', color: 'kimi' }, 'm1'), 'Updated Writer.') ++ assert.equal(state().bots.b2.color, 'charcoal') ++}) ++ ++test('#2775: null still clears the colour (guard, green on the base)', async (t) => { ++ const { tool, call, state, stop } = start() ++ t.after(stop) ++ assert.equal(await tool('update_bot', { bot: 'Writer', color: 'violet' }, 'm1'), 'Updated Writer.') ++ assert.equal(state().bots.b2.color, 'violet') ++ // `color: null` is the settings page's clear (settings → colour → reset), not `update_bot`: ++ // the tool's parameter is a string, so null is refused by the schema before the host sees it. ++ const cleared = await call('update-bot', { id: 'b2', color: null }) ++ assert.equal(cleared.ok, true, JSON.stringify(cleared)) ++ assert.equal('color' in state().bots.b2, false, 'the colour is removed, not set to empty') ++})