fix(tools): refuse an unknown bot colour instead of dropping it silently

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 <name>.` while the Bot keeps the colour it had; the refusal names the accepted colours ([#2775](https://github.com/ThinkFlowLab/FlyBot/issues/2775)).

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)

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 <name>." 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')
+})

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 <name>.` 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 <name>.` while `bots.<id>.color` stayed where it was. An unknown colour now throws `Color must be one of <COLORS>, <EXTRA_COLORS> 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.