[Birfy]

Written-by: Dev2

Fixes #732

Tip SHA: df5d6f7

INC answered

The INC (6338326988) was right: the fix stopped below onBotRemoved's if (parent === undefined) return, so a Bot the tree never held kept its records — and index.js feeds the tracker for every Bot (fb.onFeed('turn.start', …)), with no tree-membership test. The leak this issue is about had a second half, and the doc sentence I added ("so every record belongs to a live Bot") was false as written.

• load.clear(botId) now runs before the guard: the tracker holds turn records only, never the tree shape, so clearing on a non-tree removal is safe.
• The existing case is extended rather than duplicated — bot.removed for a non-tree bot is ignored, and still drops its load records: start a turn for the outsider, assert loadOf(o) === 1, remove it, assert loadOf(o) === 0 and openForOf(o) === 0, and the A child is still re-attached to the root.
• Rebuilt on current main d5fefe01 (was a718f345). Nothing on main touched plugins/tree/ or docs/subsystems/tree.md in between (a718f345..d5fefe01 changes only plugins/timeline/, client, docs and labels), so the six files keep main's content and this change sits on top of it.

Reproduce

plugins/tree/src/tree.js onBotRemoved() re-attaches the deleted Bot's children, repairs the organizer, refreshes prompts and runs adoption — it never touched the load tracker, and after the first revision it touched it only for a Bot the tree held. createLoadTracker keeps one record list per Bot id in a process-local Map, so a deleted id stays forever: snapshot() still counts a Bot that no longer exists and openFor() still answers the age of a turn nobody can end.

Two cases fail on main — they are the reproduction: a tree Bot with two open turns (expected 2 to equal 0), and an outsider, created through core create_bot, that the tracker records and then never forgets.

What changed

• plugins/tree/src/load.js: createLoadTracker gains clear(id) — removes that Bot's records, an unknown or empty id is a no-op, pure in the style of start/end/of.
• plugins/tree/src/tree.js: onBotRemoved() calls load.clear(botId) first, before the "not a tree bot" guard, so both halves of the leak are closed.
• plugins/tree/test/load.test.js: clear drops one Bot's records, leaves another Bot's alone, is a no-op for an unknown id and a second clear, and of/openFor read 0 for the cleared id.
• plugins/tree/test/tree.test.js: two tree-level cases — two open turns for Work and one for Dev, removeBot(Work) leaves loadOf(Work) === 0/openForOf(Work) === 0 while Dev still counts; and the outsider case above.
• docs/subsystems/tree.md says the removal drops the Bot's turn records; changelog.d/732-load-tracker-clear.fixed.md added.

What I ran

At this tip df5d6f7e9c3854a17d02bf4767819e735e53f74e (base d5fefe01, 6 files, +105/−4):

• cd plugins/tree && TZ=UTC DSH_HARNESS=/opt/flybot FLYBOT_CORE=<repo>/core node --import ./test/resolve.mjs --test --test-concurrency=2 "test/*.test.js" → 105 tests, 105 pass, 0 fail. This is the INC's command: the resolve hook maps ds-bot to the core checkout, which is what made the two host.test.js/Cordis failures environmental in my earlier runs, not real.
• node --test test/load.test.js → 6/6.
• node scripts/lint-plugins.mjs (harness set) → 0 errors, 1 pre-existing warning (for…of over Object.entries in classify, untouched here), packed @flybot/tree imported.
• arch-docs: OK (6 changed file(s)); changelog.mjs check → 19 fragments well formed; patches/ds-bot/ untouched by this diff (plugins only), so ds-bot-patches check is unaffected.

Red without the source change

• Move load.clear(botId) back below the guard (the shape the INC caught) → 104/105, and the single red is exactly bot.removed for a non-tree bot is ignored, and still drops its load records. The tree-Bot case stays green, which is why the first revision looked right.
• Delete the load.clear(botId) call entirely → 103/105: both tree-level cases fail.
• Delete the clear() method from createLoadTracker → test/load.test.js 5/6 on the missing function.
• All restored → 105/105 again.

Not run

• npm run verify as a whole (core lint + core suite + the other two plugin suites) and the package/typecheck jobs — this diff touches plugins/tree, its tests, one doc and one changelog fragment.
• No smoke test and no real DSH session: the change is pure bookkeeping in an in-memory Map, exercised through the plugin's own harness.

Risk

T2 (plugins/tree/src/** is plugin code in .github/agent/risk-rules.json). The blast radius is one Map delete on a path that already disposes of the Bot, and it now also runs for a Bot the tree did not hold — which is the fix, not a widening: the tracker is keyed by Bot id and holds nothing the tree reads. A live Bot's records are asserted untouched in both tree-level cases, clear of an unknown or empty id is a no-op, and nothing persists, so a restart behaves as before.

[Birfy]

Verdict: INC at e74b999
Written-by: Reviewer

One item. Everything else in this PR is right: clear(id) is pure and in the style of start/end, the issue's acceptance cases are all pinned, docs and changelog are in place, and the suite is green at this tip.

The fix stops below the "not a tree bot" guard, so the invariant it claims is still false. onBotRemoved returns early on if (parent === undefined) return, and load.clear(botId) sits under that return. But the tracker is fed from the core activity feed for every Bot — index.js does fb.onFeed('turn.start', e => { tree.noteTurnStart(e?.botId, e?.turnId); … }) with no tree-membership test. A Bot outside the tree still runs turns and still gets records: one the organizer could not place because a level was full (the pending adoption in onBotRemoved, #635), a host started with adoptExisting: false or autoRoot: false, or a Bot created straight through core create_bot. Delete such a Bot and nothing clears its records, so snapshot() and loadOf() keep counting a Bot that no longer exists — the same leak the issue is about, for the other half of the Bots, and the sentence this PR adds to docs/subsystems/tree.md ("so every record belongs to a live Bot", and the same claim in the PR body) is not true.

Proof at this tip, run as a case added to test/tree.test.js (restored afterwards):

test('PROBE #732: a removed non-tree Bot keeps its load records', async () => {
  const m = await setup()
  const o = await outsider(m)
  m.feed('turn.start', { botId: o, turnId: 'o-1' })
  assert.equal(m.tree.loadOf(o), 1, 'the outsider is tracked while it runs')
  m.removeBot(o)
  assert.equal(m.tree.loadOf(o), 0, 'PROBE: a removed non-tree Bot is still counted')
})

→ not ok 89 … PROBE: a removed non-tree Bot is still counted, # pass 88 # fail 1. The rest of tree.test.js is unaffected, so this is the guard's placement alone.

Fix: move load.clear(botId) above the guard (function onBotRemoved({ botId, parent }) { load.clear(botId); if (parent === undefined) return … }) — the tracker holds turn records only, so it is safe for a non-tree removal too — and pin it in the test that already has the shape, bot.removed for a non-tree bot is ignored (test/tree.test.js, it already builds outsider(m)): start a turn for that Bot, remove it, assert loadOf(o) === 0 and openForOf(o) === 0. Then the doc sentence is true as written instead of needing a caveat.

Ran:

• cd plugins/tree && TZ=UTC DSH_HARNESS=/opt/flybot node --import ./test/resolve.mjs --test --test-concurrency=2 "test/*.test.js" → 105/105 pass at this tip
• red, as the PR body claims: load.clear(botId) commented out in onBotRemoved → test/tree.test.js 87 pass / 1 fail, exactly #732: a removed Bot leaves no open-turn record behind, and a live one keeps its own
• red, as the PR body claims: clear(id) renamed in createLoadTracker → test/load.test.js 5 pass / 1 fail
• the probe above against the merged-order fix: non-tree Bot still counted → the gap in this review

Not run: root npm run verify, the core suite, the package/typecheck jobs (this PR touches plugins/tree, docs and one changelog fragment only); no browser check (no client file changed).

[Birfy]

Verdict: PASS at df5d6f7
Written-by: Reviewer

Both halves of #732 are closed and each one is held by a test. clear(id) is pure bookkeeping in the tracker's own map, onBotRemoved calls it before the "not a tree bot" guard, and the outsider case from the INC now fails exactly when the call is moved back below that guard.

Checks: mergeable: clean, base d5fefe01, 9/9 check runs green at this tip (including tests-ok, merge-gate, package); one INC comment on this PR names an older tip (e74b9993) and is superseded by this one.

Red, three cuts on plugins/tree, each restored:

• delete load.clear(botId) in onBotRemoved → 103/105 (both new tree cases)
• move it back below if (parent === undefined) return → 104/105, only bot.removed for a non-tree bot is ignored, and still drops its load records
• (body's clear() removal → load.test.js 5/6, not re-run here)

Ran:

• cd plugins/tree && TZ=UTC DSH_HARNESS=/opt/flybot node --import ./test/resolve.mjs --test --test-concurrency=2 "test/*.test.js" → 105 tests / 105 pass / 0 fail
• the two cuts above, each restored to 105/105

Not run: npm run verify as a whole, the package/typecheck jobs locally (green in CI), smoke.

[Birfy]

Re-enqueueing after the queue ejected this PR at 10:54:11Z. The Test job failed on core/test/bot-messages.test.js:463 ("a request the model keeps turning down moves the Bot on…"), which asserts the DS handoff copy — nothing this diff touches (plugins/tree/src/load.js, plugins/tree/src/tree.js, two test files, one doc, one changelog).

It is a flake, not this change: the same test passed on this PR's own Test run at 10:52 and on main d5fefe01 at 10:22, and #750's merge-queue Test run one entry later (10:56) passed the same suite. Verdict stays PASS at df5d6f7e (see above); nothing about the tip changed, so I am not writing a second verdict.

Worth a separate issue if it recurs: that case renders a handoff message built from the installed DSH package, so a copy change in the harness can turn it red for a PR that never touched it.