diff --git a/changelog.d/764-team-name-clash.fixed.md b/changelog.d/764-team-name-clash.fixed.md new file mode 100644 index 000000000..85103ac24 --- /dev/null +++ b/changelog.d/764-team-name-clash.fixed.md @@ -0,0 +1 @@ +`create_team` now refuses a team before it writes anything when the lead's name or a member's name is already taken anywhere on this home: it names every clashing name and the path of the Bot that holds it ([#764](https://github.com/ThinkFlowLab/FlyBot/issues/764)). Previously it created the lead first, so a member whose name was taken was dropped and the team was left half-built with a childless ` Lead`. diff --git a/docs/subsystems/tree.md b/docs/subsystems/tree.md index 88f22b151..f511384c6 100644 --- a/docs/subsystems/tree.md +++ b/docs/subsystems/tree.md @@ -61,7 +61,7 @@ Allocation puts a new Bot in a sensible place once; the self-organization below - **Reuse before create.** `find_bots(query?)` (read-only, every tree Bot) lists the Bots in the caller's branch (for Chief: the whole tree) with `id`, `name`, `role`, `parent`, `depth`, `path`, `children`, `lead` and `via` (the caller's direct child on the path, whom it delegates to). With a query, only matches, best first: same role (`sameRole`), the query in the name or role, shared role words (stems: "test" finds "Tester"), the same project, the same domain. Empty query: an overview (first 100). `create_child_bot` checks the whole branch first: a Bot with the same role at any depth is returned as `{ reused, note }` and nothing is created. A Bot outside the caller's branch is not looked at (the daily tidy merges duplicates across branches). - **Placement-aware creation.** `create_child_bot(name, role_description, parent?)`: `parent` (id or unique name) must be the caller or a descendant, else `parent must be you or one of your descendants`. Limits, depth and the sibling-name check apply at the target, under that parent's lock. Without `parent`, a **marked group lead** in the caller's branch whose group the new Bot belongs to takes it (most specific group first, then the shallower lead, when it has room): "FlyBot Docs" goes under "FlyBot Lead". A Bot that could lead that group itself, or a namesake, never goes under it; with no such lead the caller is the parent. The result names the parent and the tree path. -- **Teams: lead plus children.** `create_team(team, members, parent?)` with `members` as `"Name: role"` strings (2 to `maxChildren`): `" Lead"` (role "Lead of the group: coordinates …", `lead { key, label }` from `roles.js`) under the caller or `parent`, and the members under it: `Chief → FlyBot Lead → {Dev, Test, Maintenance, Repo}`, one slot under Chief. An existing lead of that group in the caller's branch (marked for it, named "" / " Lead", or coordinating it) is reused and marked; a member with the same role already in the branch is reused, and moved under the lead when it sits flat next to it (that frees its slot, so a team fits at a full level when it reuses a flat member). Members that do not fit are listed as not created. A team that moves or marks existing Bots is a journaled run (trigger `team`, one-click undo); a fresh team is a plain create. +- **Teams: lead plus children.** `create_team(team, members, parent?)` with `members` as `"Name: role"` strings (2 to `maxChildren`): `" Lead"` (role "Lead of the group: coordinates …", `lead { key, label }` from `roles.js`) under the caller or `parent`, and the members under it: `Chief → FlyBot Lead → {Dev, Test, Maintenance, Repo}`, one slot under Chief. An existing lead of that group in the caller's branch (marked for it, named "" / " Lead", or coordinating it) is reused and marked; a member with the same role already in the branch is reused, and moved under the lead when it sits flat next to it (that frees its slot, so a team fits at a full level when it reuses a flat member). Members that do not fit are listed as not created. A team is refused **before anything is written** when the lead's name or a member's name is already taken anywhere on the home — the host keeps names unique across the whole roster, not just under one parent — and the error names every clashing name with the path of the Bot that holds it; that is why a clash can no longer leave a childless ` Lead` behind ([#764](https://github.com/ThinkFlowLab/FlyBot/issues/764)). A team that moves or marks existing Bots is a journaled run (trigger `team`, one-click undo); a fresh team is a plain create. - **Plan card**: not built. The codebase has no chat card for a model-proposed plan (approval cards belong to social's message holds), so forward allocation proceeds and names the resulting paths; deferral recorded on #447. Automatic reorganization never asks first. - **Prompts.** Chief (and every coordinator) is told: `find_bots` before creating and reuse what fits; a project with several roles is one lead with children (`create_team`); a new Bot of a group goes under its lead; at `maxChildren - 2` children or more, group related children under a lead instead of adding another flat one; name the tree path of every new Bot; a full level, orphans and duplicates are fixed by the automatic reorganization, without asking the owner first. - **Over capacity and orphans → #446.** A full target refuses with `limit reached` (pass a fitting `parent`, or let the tree regroup the level by role). A Bot made outside `create_child_bot` is placed by role (below). An **orphan** (its `parent` no longer in the tree) is a reparent candidate, never parked outside for good: each repair pass (startup, every `bot.created`, after a deletion) moves it with its subtree to the home its role points to (its group's lead, else the root) where it fits; a journaled, undoable move. One that fits nowhere yet is retried on the next pass. @@ -117,7 +117,7 @@ The plugin's discarded `.catch(() => {})` calls carry a one-line reason comment ## Tests -Covered by the plugin's own tests under `plugins/tree/` / core test suite as wired. The `#447` tests cover reuse of a same-role Bot at depth 3 and `find_bots` (scope, fields, ranking), `create_child_bot(parent)` refusing a non-descendant (sibling, ancestor, outsider, unknown) and checking limits at the target under its lock, `create_team` (Chief → FlyBot Lead → four members, one slot; reuse of the lead and members; a flat member's slot at a full level, journal and undo), later Bots joining their group's lead (only down leads of groups they belong to), and orphan reattachment; `test/host.test.js` runs `find_bots`, `create_team` and `create_child_bot(parent)` on the real host. The `#446` tests in `test/tree.test.js` cover role grouping (Life Lead, FlyBot Lead), existing leads, no unrelated fallback, the cap at every step, the daily tidy (merge, idle, archive), journal, undo and the client notice; `test/host.test.js` runs `create_bot` and the `tree/reorg` / `tree/undo` endpoints on the real host. `#16 Option B` in `test/tree.test.js` (and the matching host test) asserts a non-Main tree Bot can create a child under itself, that `maxChildren`/depth still refuse, and that the child is not auto-granted. Runtime load (#507) is covered by `test/load.test.js` (pure) and a `test/tree.test.js` case: a child's running turns show in the coordinator's stored prompt and `list_children`, and drop to zero at `turn.end`. The age of an open turn (#624) is covered in both: `openFor` reads the oldest start and falls back to the newer turn once one ends, an end that names its turn closes that one and not the oldest (#696, unknown and missing ids included), the line names the age only past the threshold and never calls an idle child stalled, and `list_children` adds `openForMs`/`stalled` for an open turn while an idle child keeps the old shape. +Covered by the plugin's own tests under `plugins/tree/` / core test suite as wired. The `#447` tests cover reuse of a same-role Bot at depth 3 and `find_bots` (scope, fields, ranking), `create_child_bot(parent)` refusing a non-descendant (sibling, ancestor, outsider, unknown) and checking limits at the target under its lock, `create_team` (Chief → FlyBot Lead → four members, one slot; reuse of the lead and members; a flat member's slot at a full level, journal and undo; a name that is already taken anywhere refusing the whole team with no Bot created, no journal entry and the tree byte-identical), later Bots joining their group's lead (only down leads of groups they belong to), and orphan reattachment; `test/host.test.js` runs `find_bots`, `create_team` and `create_child_bot(parent)` on the real host. The `#446` tests in `test/tree.test.js` cover role grouping (Life Lead, FlyBot Lead), existing leads, no unrelated fallback, the cap at every step, the daily tidy (merge, idle, archive), journal, undo and the client notice; `test/host.test.js` runs `create_bot` and the `tree/reorg` / `tree/undo` endpoints on the real host. `#16 Option B` in `test/tree.test.js` (and the matching host test) asserts a non-Main tree Bot can create a child under itself, that `maxChildren`/depth still refuse, and that the child is not auto-granted. Runtime load (#507) is covered by `test/load.test.js` (pure) and a `test/tree.test.js` case: a child's running turns show in the coordinator's stored prompt and `list_children`, and drop to zero at `turn.end`. The age of an open turn (#624) is covered in both: `openFor` reads the oldest start and falls back to the newer turn once one ends, an end that names its turn closes that one and not the oldest (#696, unknown and missing ids included), the line names the age only past the threshold and never calls an idle child stalled, and `list_children` adds `openForMs`/`stalled` for an open turn while an idle child keeps the old shape. ## Common mistakes diff --git a/plugins/tree/src/allocate.js b/plugins/tree/src/allocate.js index d7846a9f6..0a88def85 100644 --- a/plugins/tree/src/allocate.js +++ b/plugins/tree/src/allocate.js @@ -190,7 +190,6 @@ export function createAllocator({ fb, limits, snapshot, childrenOf, depthOf, tra } async function buildTeam(callerId, label, list, { parentId, key, leadId: existingLead }) { let nodes = snapshot() - const run = begin('team') const done = list.map(m => ({ name: m.name, role: m.role })) // Reuse: the same role already in the caller's branch (never the lead itself). const taken = new Set(existingLead ? [existingLead] : []) @@ -198,6 +197,21 @@ export function createAllocator({ fb, limits, snapshot, childrenOf, depthOf, tra const id = sameRoleIn(nodes, callerId, d.name, d.role, taken) if (id) { d.reusedId = id; taken.add(id) } } + // #764: a Bot's name is unique across the whole roster, not only under its parent — the host + // refuses a duplicate wherever the existing Bot sits. Check every name this team would create + // (the lead included) before anything is written: otherwise the lead is created, the member whose + // name is taken is dropped, and the team is left half-built with a childless lead. + const creating = [...(existingLead ? [] : [leadNameOf(label)]), ...done.filter(d => !d.reusedId).map(d => d.name)] + const roster = fb.listBots() + const clashes = creating + .map(name => ({ name, hit: roster.find(b => norm(b.name) === norm(name)) })) + .filter(c => c.hit) + if (clashes.length) { + const where = clashes.map(c => `"${c.name}" is already taken by ${nodes[c.hit.id] ? traceOf(nodes, c.hit.id) : `a Bot outside the tree (${c.hit.name})`}`) + const one = clashes.length === 1 + throw new Error(`nothing was created: ${where.join('; ')}. Rename ${one ? 'that member' : 'those members'}, or give the existing Bot the same role so it is reused`) + } + const run = begin('team') let leadId = existingLead const home = leadId ? nodes[leadId].parent : parentId // A reused member flat next to the lead (or at the parent, for a new lead) joins the lead. diff --git a/plugins/tree/test/tree.test.js b/plugins/tree/test/tree.test.js index a8b9c3249..4f2f66d9e 100644 --- a/plugins/tree/test/tree.test.js +++ b/plugins/tree/test/tree.test.js @@ -1759,3 +1759,33 @@ test('#677: heightOf measures the whole subtree, so move_bot refuses a subtree t const r = await m.call(m.root, 'move_bot', { bot: 'A', parent: 'D' }) assert.match(r.error, /limit reached: A would end at depth 5 \(maxDepth=4\)/) }) + +test('#764: a name taken anywhere refuses the whole team before anything is created', async () => { + const m = await setup() + const review = await make(m, m.root, 'Review') + const existing = (await child(m, review, 'Reviewer', 'checks pull requests')).created.id + const before = m.bots.size + const snap = JSON.stringify([...m.bots.values()].map(b => [b.id, b.name, b.systemPrompt, b.fields])) + // The host refuses a duplicate name wherever the existing Bot sits (roster.js), not only under + // the parent; building the lead first left it childless and the team half-built. + const r = await m.call(m.root, 'create_team', { team: 'FlyBot', members: ['Dev: writes the FlyBot code', 'Reviewer: checks the tree and the CI'] }) + assert.match(r.error, /nothing was created/) + assert.match(r.error, /"Reviewer" is already taken by Chief → Review → Reviewer/) + assert.equal(m.bots.size, before, 'no lead, no partial team') + assert.equal(JSON.stringify([...m.bots.values()].map(b => [b.id, b.name, b.systemPrompt, b.fields])), snap, 'the tree is byte-identical') + assert.deepEqual(m.tree.overview().runs, [], 'no journal entry either') + assert.equal(f(m, existing, 'treeRole'), 'checks pull requests', 'the existing Bot is untouched') + // Every clash is named, not just the first one; a name whose role does not match is not reused. + await child(m, m.root, 'Scout', 'watches the neighbourhood') + const two = await m.call(m.root, 'create_team', { team: 'Ship', members: ['Reviewer: checks releases', 'Scout: ships releases'] }) + assert.match(two.error, /nothing was created/) + assert.match(two.error, /"Reviewer" is already taken by Chief → Review → Reviewer/) + assert.match(two.error, /"Scout" is already taken by Chief → Scout/) + assert.equal(m.bots.size, before + 1, 'still nothing created by the refused team, no Ship Lead either') + // Reuse by role is not a clash: the member with the same role is reused and nothing is created for it. + const reused = await m.call(m.root, 'create_team', { team: 'FlyBot', members: ['Dev: writes the FlyBot code', 'Reviewer: checks pull requests'] }) + assert.equal(reused.error, undefined) + assert.deepEqual(reused.team.members.map(x => [x.name, Boolean(x.reused)]), [['Dev', false], ['Reviewer', true]]) + assert.match(reused.note, /Reused where they are: Chief → Review → Reviewer/) + assert.equal(f(m, existing, 'parent'), review, 'reused where it stands, not moved') +})