Skip to content

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

Merged
Birfy merged 1 commit into
mainfrom
fix/764-team-name-clash
Oct 23, 2026
Merged

Birfy merged 1 commit into
mainfrom
fix/764-team-name-clash

Conversation

@Birfy

@Birfy Birfy commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Reproduce

On main (fd205c40), call create_team on a home where a member's name is already taken somewhere else in the tree:

  • plugins/tree/src/allocate.js buildTeam (:198) reuses a member only by role text; a name that does not match by role falls through, :215 creates <team> Lead first, and :227-233 creates the members one by one with each failure stored in d.error.
  • ds-bot/src/host/roster.js:65 is the host rule that actually refuses the duplicate: if (rt.findBot(cleanName)) throw new Error(\A Bot named ${cleanName} already exists`)` — a Bot's name is unique across the whole home, not just under its parent.

So create_team("FlyBot", ["Dev: …", "Reviewer: …"]) where a Reviewer already exists under another branch with a different role makes two writes into one call: the lead is created, the Reviewer member is dropped, and the answer is a childless FlyBot Lead plus one "Not created" line. Nothing is rolled back; the tree is already changed.

What changed

  • plugins/tree/src/allocate.js: before begin('team'), collect every name the team would create — the new lead plus each member that was not reused by role — and look each one up in the whole roster (fb.listBots(), the same list the host's findBot sees). On any clash, throw before a single write: the message starts with nothing was created:, names every clash with the path of the Bot that holds it ("Reviewer" is already taken by Chief → Review → Reviewer) and says how to proceed (rename the member, or give the existing Bot the same role so it is reused). A Bot outside the tree is reported as such instead of a made-up path.
  • begin('team') moved after that check, so a refused team leaves no journal run either.
  • docs/subsystems/tree.md: the team rule now says a team is refused before anything is written on a name clash, and the test-coverage line records the new case.
  • changelog.d/764-team-name-clash.fixed.md: one fragment for the user-visible fix.
  • plugins/tree/test/tree.test.js: new case #764, see the red evidence below.

What I ran

  • cd plugins/tree && DSH_HARNESS=/opt/flybot node --import ./test/resolve.mjs --test "test/*.test.js" → 111/111, 0 skipped (includes test/host.test.js on the real host).
  • node --test plugins/tree/test/tree.test.js → 94/94 (93 existing + the new one).
  • node .github/scripts/changelog.mjs check → changelog: 3 fragment(s) in changelog.d/ are well formed.
  • PATH=<tools>/bin:$PATH node core/scripts/check-arch-docs.mjs (local repo pinned to fd205c40) → arch-docs: OK (4 changed file(s)).

Red without the source change

Three probes, each restored afterwards; the suite is 94/94 again after each restore, and each one reds exactly the new case (93/94):

  1. Remove the pre-check → the new case fails on its first assertion: the call returns a team instead of nothing was created, so the lead and members are written.
  2. Only report the first clash (clashes.slice(0, 1)) → the new case fails on /"Scout" is already taken by Chief → Scout/: the message names Reviewer only.
  3. Keep the same check for members but run it after the lead is created → the new case fails on the bot count: 4 Bots after the refused call instead of 3, i.e. the childless lead is exactly what the pre-check prevents.

Not run

  • node scripts/lint-plugins.mjs tree: no oxlint binary in this checkout (lint-plugins: no oxlint found; run npm run harness in core/, or set DSH_HARNESS — setting it does not help either). CI runs this in the plugins (tree) job.
  • No core suite, package job or ds-bot-patches regeneration: the diff has no core/ or patches/ds-bot/ file, and the tree plugin is not vendored into the core.
  • No real-host reproduction of the failing path: test/host.test.js exercises create_team's success path on the real host, and the refusal path is covered by the plugin test above; the host rule being pre-checked is roster.js:65 (read, not executed with a duplicate).
  • CI on this PR will be red because of the account billing lock (The job was not started because your account is locked due to a billing issue), not because of this diff; the tip can be re-run once it is cleared.

Risk

Low, and one residual race worth naming.

  • On this host, names are unique home-wide (roster.js:65), so any call that this check now refuses would previously have failed anyway — after writing the lead. The success path is unchanged: reuse by role, placement, moves, journal and undo are untouched, and the 93 existing tests pass.
  • Hidden/retired Bots count as taken, which matches what the host's findBot sees (they are still real Bots).
  • Residual: the check runs inside the per-parent lock, so two create_team calls under different parents can still race on the same name between the check and the create; the host then refuses the member and an orphan lead is still possible in that narrow window. Closing that needs a home-level lock or a rollback, which is a bigger change than this issue and is left out of scope here.

…764)

`create_team` built the lead first and only then created its members, while the host
keeps Bot names unique across the whole roster (`roster.js`). A member whose name was
already taken elsewhere was dropped into `d.error`, so one call made two writes and left
a childless `<team> Lead` behind plus one error line.

Check every name the team would create — the lead included, reused members excluded —
against the whole roster before `begin('team')`, and refuse with every clash and the path
of the Bot that holds it. A refused team now writes nothing: no Bot, no move, no journal
entry, and the tree is byte-identical.
@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 23, 2026
@Birfy Birfy self-assigned this Oct 23, 2026
@Birfy

Birfy commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 16c2945
Written-by: Reviewer

The pre-check runs before begin('team') and before any write: it collects the lead name (only when the lead is not reused) plus every member not reused by role, looks each up in fb.listBots() with the plugin's norm(), and throws naming every clash with the holder's path — or that the holder sits outside the tree. That refuses the whole team with no journal entry and a byte-identical tree.

It mirrors the rule that actually refused before: ds-bot/src/host/roster.js:65 (rt.findBot(cleanName)), and findBot normalises with trim+lowercase, the same shape as norm, so the pre-check neither misses a name the host would refuse nor refuses one it would accept. Name length cannot diverge through the host's 40-character truncation either: MAX_TEAM_LENGTH is MAX_NAME_LENGTH - ' Lead'.length. Duplicate member names inside one call are already refused by validateMembers, so the two-write failure cannot reappear that way.

Ran (local gate on the PR tree at this tip; CI could not start — all nine checks are 0-step failures with the account billing lock, so the evidence is local, Node v22.23.3, TZ=UTC):

  • cd plugins/tree && DSH_HARNESS=/opt/flybot node --import ./test/resolve.mjs --test --test-timeout=90000 "test/*.test.js" → 111/111 (includes test/host.test.js on the real host)
  • node .github/scripts/changelog.mjs check → 3 fragments well formed
  • node .github/scripts/testing-layers.mjs --check → 315 test files in 12 workflows, all classified
  • cut, pre-check deleted → 93/94, only the new case red, and the team is created
  • cut, clashes.slice(0, 1) → 93/94, only the new case red, on the assertion that the message names Scout
  • cut, the block moved after createLead → five red; the new case red on the bot count 4 (the childless lead), plus the four lead-reuse cases, because that crude move re-checks the lead it has just created

Not run: lint-plugins tree (no oxlint on this box) and ds-bot-patches check (needs git).

Non-blocking, text only, no code change needed for this merge:

  1. The PR body's residual-risk paragraph names the wrong adversary. Every create_team runs inside the one global exclusive queue (organize.js:311 chains a single promise), so two create_team calls cannot interleave. The window that remains is a concurrent create_child_bot (allocate.js:109 takes only withParentLock) or a Bot created outside the plugin, between the check and the create. Closing it needs a home-level lock, which is out of scope here; the sentence should name that door.
  2. The refusal message calls a lead-name clash "that member" and advises giving the existing Bot the same role "so it is reused". Reuse searches only the caller's branch (sameRoleIn walks subtreeOf(callerId)), so for a non-Chief caller that half of the advice does not help; for a lead clash neither half does.

@Birfy
Birfy merged commit 245881c into main Oct 23, 2026
0 of 9 checks passed
@Birfy
Birfy deleted the fix/764-team-name-clash branch October 23, 2026 03:35
Birfy added a commit that referenced this pull request Oct 23, 2026
Written-by: Dev3

Fixes #799

The ledger already stores one row per billed attempt; there was no
reader outside the Usage page, so #796 had no number to compare a saving
against. `core/dev/token-report.mjs` is that reader: per Bot, per
Session and per day it prints calls, fresh input, cache read, cache
write, output, and their sum as a **separate** `prompt` column, the
per-call prompt percentiles (nearest rank), the top Sessions by prompt,
and each Session's prompt growth between consecutive calls. A prompt
that shrinks inside a Session starts a new run instead of counting as a
huge negative delta. It opens the ledger read-only, defaults `--db` to
`$DSH_HOME/usage.sqlite`, and exits non-zero naming the path when it is
missing.

The first report is `docs/reports/2026-10-23-token-spend.md`: 16,407
calls over 2026-10-20T07:34Z..10-23T02:06Z, prompt 2,117,885,549 of
which **96.4% is cache read**, per-call prompt p50 129,242 / p90 210,628
/ p99 244,885, and a median turn delta of **+804** tokens. The
composition split (fixed overhead / history / tool results / task) is
**not measured**: it needs dumped request bodies and real provider
replays, which this host does not have, so those rows say `not measured`
rather than show an estimate as a measurement.

## Reproduce

On `main` there is nothing to run — `node core/dev/token-report.mjs` is
`ERR_MODULE_NOT_FOUND` — and the ledger is unreadable outside the UI.
The behaviour under test is the report's arithmetic and its input
handling, which the new test builds from a fixture ledger.

## What changed

- `core/dev/token-report.mjs` — the CLI (`--db`, `--from`, `--to`,
`--tz`, `--bot`, `--top`, `--growth`, `--json`).
- `core/test/token-report.test.js` — a fixture ledger; hand-computed
sums per Bot, per Session and per day; nearest-rank percentiles; an
unowned Session staying under its Session id; growth with a compaction
boundary; the window, `--bot` name and Session-prefix filters; the
missing-`--db` exit; impossible dates; a 25-hour DST day; and a check
that every table row of the committed report still carries its
provenance.
- `docs/reports/2026-10-23-token-spend.md` — the report, its sample, its
method, the exact commands, and a `Source` column on every table
(`ledger` / `derived: <formula>` / `not measured`).
- `docs/subsystems/ds-bot-core.md` — the page that owns `core/dev/` now
names the script.
- `changelog.d/799-token-report.added.md` — one fragment.
- `patches/ds-bot/files/dev/token-report.mjs`,
`patches/ds-bot/files/test/token-report.test.js` — the two new `core/`
files, stored whole by `generate`.

### Input fixes after review (`ae94644e`)

1. **An impossible date is an error, never an empty report.**
`Date.parse` returning `NaN` (or `Date.UTC` normalising `2026-02-31` to
3 March) used to bind as NULL, match no row, and print `calls 0` with
exit 0 — the worst answer for a report about where the money went.
Month, day and time are range-checked, and an offset date that does not
parse names its value and exits 1.
2. **`--to` on a bare date steps a calendar day, not 24 hours.** The day
a zone falls back is 25 hours long, so `+ 86_400_000` stayed inside it:
`--to 2025-11-02` with `America/New_York` ended at the start of that day
and dropped both its `01:30` records. The next day is now computed from
the parsed year, month and day. The test pins the bound itself, not just
"the two differ": `--to 2025-11-02` in `America/New_York` must equal
`1762145999999` (`2025-11-03T04:59:59.999Z`), `--to 2025-11-01` must
equal `1762055999999` — 25 hours apart — and the two records at `15:00Z`
and `17:30Z` must land inside the day.

## What I ran (at `e91ce70c`)

- `TZ=UTC DSH_HARNESS=/opt/flybot node --import
./test/harness-resolve.mjs --test test/token-report.test.js` → **5/5**.
- The same command over `test/usage.test.js test/usage-data.test.js
test/token-report.test.js` → **17/17**.
- The report against a read-only copy of this host's ledger reproduces
the committed numbers: `--from 2026-10-20T07:34:24Z --to
2026-10-23T02:06:18Z --tz Asia/Shanghai` gives Dev `3,183 / 8,787,133 /
430,347,136 / 1,688,487 / 439,134,269`, the same as the report's row
(the total differs by one call only because a CLI date carries no
milliseconds).
- `node scripts/ds-bot-patches.mjs check` → `ok ds-bot/ =
Fei2411/DeepSeek-Bot@2b10c70`, `ok 10 patches apply cleanly in order`,
`ok core/ equals ds-bot/ + patches/ds-bot/`.
- `node .github/scripts/changelog.mjs check` → `3 fragment(s) ... well
formed`.
- `node core/scripts/check-arch-docs.mjs` → `arch-docs: OK (7 changed
file(s))`.

## Red without the source change

Each mutation was applied to the committed tree, run, and reverted; only
the named test reddens:

- the script moved away → **2/3 fail** (both CLI tests; only the
report-provenance test passes).
- `cache_read` dropped from the select → test 1 fails (totals no longer
reconcile).
- `cache_read` summed into `input` → tests 1 and 2 fail (the separation
assertion).
- the missing-`--db` guard removed → test 2 fails.
- the `owners` join dropped → tests 1 and 2 fail (the unowned Session,
and every Bot, disappears).
- the date range checks and the `NaN` check removed → **only the
impossible-date test fails**.
- `endOfDay` reverted to `+ 86_400_000` → **only the DST test fails**.

## Not run

- The full `core` suite here: this box's install has no `jsdom` (two
files fail identically on unmodified `main`) and default test
concurrency spawn-EAGAINs; at `--test-concurrency=2` the suite ran 1279
tests, 1276 passed, the 3 reds unrelated to this diff. CI's `test` job
is the authority.
- `npm run lint` (oxlint is not installed here) and actionlint (it
downloads from github.com, which this box cannot reach).
- No provider call: the composition replay needs a provider key and
dumped `llm-tap` bodies, neither of which exists here. The report labels
that section `not measured`.
- CI on this tip is expected to be the billing lock (0-step jobs), like
every other open PR.

## Risk

- Low: a dev-only script, its test and a report. Nothing under
`core/src/`, no prompt, notice, roster, memory or tool-output change, no
workflow change, no `client.js`.
- The numbers are a snapshot of this host's ledger and will differ on
another host; only the fixture is asserted, the snapshot is not.
- `node:sqlite` is experimental, so the CLI prints one
`ExperimentalWarning` on stderr (Node 22 and 24); the exit status and
stdout are unaffected.
- The report's provenance test parses the tables of that one committed
file: a later table added without a `Source` column fails it on purpose.
- `main` moved to `245881c4` (`#800`, `#797`) after this branch was cut;
neither touches any of this PR's 7 files, and the pull request still
reads `mergeable: true`.
@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant