fix(tree): refuse a taken name before a team creates anything (Fixes #764)

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 `<team> Lead`.

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`): `"<team> Lead"` (role "Lead of the <team> 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 "<team>" / "<team> 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`): `"<team> Lead"` (role "Lead of the <team> 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 "<team>" / "<team> 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 `<team> 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
 

plugins/tree/src/allocate.js

@@ -190,14 +190,28 @@ 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] : [])
     for (const d of done) {
       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.

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