[Birfy]

Written-by: Dev

heightOf gets the guard depthOf has, and the recursion now walks child ids — which is the half that actually made the bug visible.

Fixes #677

Tip SHA: 8580b01

Reproduce

On main at 8e80237b, the recursion passed the child node instead of its id:

const heightOf = (nodes, id) => {
  const kids = childrenOf(nodes, id)
  return kids.length ? 1 + Math.max(...kids.map(k => heightOf(nodes, k))) : 0
}

childrenOf(nodes, <node object>) compares n.parent === <object>, which is never true, so the walk ends after one level and the height is always 1 for any node with children. Measured on a real Chief → A → B → C: heightOf(nodes, A) returned 1 where the subtree is 2 levels. moveBot builds its depth check from it — bottom = depthOf(parent) + 1 + heightOf(bot) — so a move that should hit maxDepth was allowed:

maxDepth=4, tree Chief → A → B → C, move A under D  →  old code: moved (exit 0), should be: "A would end at depth 5 (maxDepth=4)"

A note on the issue's stated symptom, because it changes what the red test can be: with that same object-passing bug the recursive call cannot re-enter anything, so a two-node parent cycle does not overflow the stack on main — it returns 1 instead. The stack overflow appears the moment the ids are threaded correctly, which is exactly what the guard has to survive. Both defects are fixed here, because fixing only the ids would turn a wrong answer into a crash on a corrupted snapshot.

What changed

plugins/tree/src/tree.js, heightOf only:

const heightOf = (nodes, id, seen = new Set()) => {
  if (seen.has(id)) return 0
  seen.add(id)
  const kids = childrenOf(nodes, id)
  return kids.length ? 1 + Math.max(...kids.map(k => heightOf(nodes, k.id, seen))) : 0
}

• k.id instead of k: the subtree height is measured, not assumed 1.
• seen threaded like depthOf: a node already on the current path ends the walk, so a snapshot where A.parent = B and B.parent = A is answered instead of recursing until the stack dies.
• Nothing else in the file changes: depthOf, traceOf and every refusal (cycle, maxChildren, duplicate name, own parent) keep their current messages and order of checks.

plugins/tree/test/tree.test.js gains two cases: the corrupted pair through the move_bot path (assert.doesNotReject plus the depth refusal, and heightOf bounded at 2), and a valid A → B → C subtree whose move must be refused by maxDepth — the undercount itself.

No docs change: the invariant is internal, and docs/subsystems/tree.md already documents move_bot's maxDepth refusal. The commit carries Docs-not-needed: for the arch-docs gate. No core/ change, so nothing to regenerate.

What I ran

At this tip (8580b011), from a tarball of it:

$ node --test plugins/tree/test/tree.test.js
# tests 83
# pass 83
# fail 0

node --test "plugins/tree/test/*.test.js" is 90/92: host.test.js cannot run here (ERR_MODULE_NOT_FOUND: ds-bot, the package is not installed in this environment), and classify-model, load, web-entry pass. CI runs the plugin suite in full.

Red without the source change

Two reverts, both at this tip, measured on the same two test files:

• heightOf back to main's version (object recursion, no seen): 83 tests, 81 pass, 2 fail — exactly the two new cases (#677: a two-node parent cycle … fails on heightOf returning 1 and on the move being allowed; #677: heightOf measures the whole subtree … fails because the too-deep move is allowed).
• Only the seen guard removed, ids kept: 83 tests, 82 pass, 1 fail — the cycle case fails with RangeError, which is the crash the guard exists for.

Not run

• plugins/tree/test/host.test.js: needs the ds-bot package, absent on this machine. CI's plugins (tree) job runs it.
• npm run verify as one command, and bench smoke: no dependencies installed here (and no model routing for bench).
• No live two-instance run: the change is a pure tree-plugin computation and the tests drive the same move_bot path the product uses.

Risk

T2 by the repository's risk-gate (path-only rules: plugins/tree/src/tree.js is not a T3 path; the diff is ~20 lines across two files), and the one behaviour change is deliberate: a move that puts a subtree deeper than maxDepth is now refused where it used to be allowed. That refusal is the documented contract, and #678 is the follow-up that adds the missing maxDepth case in the create-path tests. A corrupted pair no longer crashes the move; it is refused by the depth check, or moved when the depth allows it.

[Birfy]

Verdict: PASS at 8580b01
Written-by: Reviewer

Ran:

• node --import ./test/resolve.mjs --test "test/*.test.js" (plugins/tree) → 96/96 at this tip
• the same suite with plugins/tree/src/tree.js reverted to 6b9820b → 94/96, the two red are the new tree: heightOf walks children with no cycle guard, so a corrupted parent pair overflows the stack instead of refusing the move #677 cases

Not run:

• a UI run of move_bot — the tool-level tests cover it

Reviewed at this tip only. All 9 checks are completed/success, mergeable: clean. plugins/tree is 96/96 here (including host.test.js, which needs the ds-bot package).

What I checked

• The two defects are real and the PR body describes them correctly. kids.map(k => heightOf(nodes, k)) compared a node object against n.parent === id (plugins/tree/src/tree.js:36), so childrenOf matched nothing and every parent measured 1; move_bot's bottom = depthOf(parent) + 1 + heightOf(bot) therefore undercounted and let a too-deep subtree through. Recursing on k.id alone would then have walked a corrupted parent cycle to a RangeError, which is why the seen set is not optional.
• The guard is the same shape as depthOf, which is the house pattern here, and it is unwound per call (a fresh Set per heightOf entry), so an ordinary tree is measured exactly: A → B → C gives 2.
• The shared seen across siblings is safe because a node has exactly one parent field, so sibling subtrees are disjoint; under corruption it can only shorten the walk, which is the safe direction for a limit check.
• Red without the source change: reverting tree.js to 6b9820bb turns exactly the two new cases red (94/96). Removing only the seen guard leaves the cycle case red on a RangeError, which is the half the PR body claims.

Non-blocking

• This makes move_bot refuse moves it used to accept (a subtree whose real height is deeper than the limit). That is the fix, but it is a visible behaviour change, so it is worth one line in the reply the tool returns or in docs/subsystems/tree.md — the maxDepth contract itself does not change, so Docs-not-needed: is defensible; the note is only about the tool's answer.
• A very deep tree still recurses once per level. seen bounds it by the node count, not by a small constant; at Bot counts this is irrelevant, at a corrupted 10k-node chain it would still overflow. Iterative would be unconditional.

Risk: T2 (plugins/tree/src/tree.js). Added awaiting-approval; needs a maintainer APPROVED review at this tip. Not enqueued.