From 8580b0114312dd0d45784f1cf410657b50fedb57 Mon Sep 17 00:00:00 2001
From: Danyang Chen <birfychen@gmail.com>
Date: Thu, 22 Oct 2026 13:35:12 +0800
Subject: [PATCH] fix(tree): guard heightOf against a parent cycle and count
 the whole subtree (#677)

`heightOf` passed the child *node* to its own recursion, so `childrenOf` matched nothing and
the height was always 1: `move_bot`'s maxDepth check counted only one level. It recurses on
the id now, and a `seen` set ends the walk at a node already on the path, like `depthOf`, so a
corrupted pair of parents is answered instead of dying on the stack.

Docs-not-needed: the change is internal to the move check; docs/subsystems/tree.md already
documents move_bot's maxDepth refusal, and no user-visible behaviour or command changes.
---
 plugins/tree/src/tree.js       | 11 +++++++---
 plugins/tree/test/tree.test.js | 39 ++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/plugins/tree/src/tree.js b/plugins/tree/src/tree.js
index 46b91a2d9..a3fa8e7e0 100644
--- a/plugins/tree/src/tree.js
+++ b/plugins/tree/src/tree.js
@@ -36,10 +36,15 @@ export function createTree(fb, { limits, rootName = 'Root', rootBotId, adoptExis
   }
   const childrenOf = (nodes, id) => Object.values(nodes).filter(n => n.parent === id)
   const depthOf = (nodes, id) => { let d = 0; for (let x = id, seen = new Set(); x && nodes[x] && !seen.has(x); x = nodes[x].parent) { seen.add(x); d++ } return d }
-  // Height of the subtree rooted at id: 0 for a leaf.
-  const heightOf = (nodes, id) => {
+  // Height of the subtree rooted at id: 0 for a leaf. Two things make this walk safe and true: it recurses
+  // on the child's id (it used to pass the node object, so `childrenOf` matched nothing and the height was
+  // always 1), and a `seen` set ends it at a node already on the path, like `depthOf` — a corrupted
+  // snapshot can make two Bots each other's parent, and without the set the walk dies on the stack (#677).
+  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))) : 0
+    return kids.length ? 1 + Math.max(...kids.map(k => heightOf(nodes, k.id, seen))) : 0
   }
   const traceOf = (nodes, id) => { const p = []; for (let x = id; x && nodes[x] && p.length < 64; x = nodes[x].parent) p.unshift(nodes[x].name); return p.join(' → ') }
 
diff --git a/plugins/tree/test/tree.test.js b/plugins/tree/test/tree.test.js
index 089a5da14..52e22abb4 100644
--- a/plugins/tree/test/tree.test.js
+++ b/plugins/tree/test/tree.test.js
@@ -1521,3 +1521,42 @@ test('#635: move_bot is visible to the root only', async () => {
 })
 
 
+
+// #677: `heightOf` walks children, so it needs the guard `depthOf` has. Two defects lived here: the
+// recursion passed the child *node* instead of its id (so `childrenOf` matched nothing and the height was
+// always 1, which let `move_bot`'s maxDepth check undercount a subtree), and once it recursed properly a
+// corrupted snapshot, where two Bots are each other's parent, died on the stack instead of answering.
+test('#677: a two-node parent cycle ends the height walk instead of overflowing the stack', async () => {
+  const m = await setup({ maxDepth: 3 })
+  const a = await make(m, m.root, 'A')
+  const b = await make(m, a, 'B')
+  // Corrupt the stored pair: A and B become each other's parent, the shape a half-applied move leaves.
+  m.bots.get(a).fields['flybot-tree'].parent = b
+  m.bots.get(b).fields['flybot-tree'].parent = a
+  const nodes = m.tree.snapshot()
+  assert.equal(nodes[a].parent, b)
+  assert.equal(nodes[b].parent, a)
+  assert.equal(m.tree.heightOf(nodes, a), 2, 'the walk counts the pair once, then stops at a node on the path')
+  assert.doesNotThrow(() => m.tree.heightOf(nodes, a), 'the walk is bounded, not recursive to the stack limit')
+
+  // The move path answers with its depth refusal instead of throwing: with the pair counted as 2, A under the
+  // root would end at depth 4. Before the id fix the height was 1 and the move was wrongly allowed; with the
+  // ids right and no `seen` it threw RangeError.
+  const call = m.call(m.root, 'move_bot', { bot: 'A', parent: 'Chief' })
+  await assert.doesNotReject(call, 'the corrupted pair must be answered, not thrown')
+  const r = await call
+  assert.match(r.error, /limit reached: A would end at depth 4 \(maxDepth=3\)/)
+})
+
+test('#677: heightOf measures the whole subtree, so move_bot refuses a subtree that is too deep', async () => {
+  const m = await setup({ maxDepth: 4 })
+  const a = await make(m, m.root, 'A')
+  const b = await make(m, a, 'B')
+  const c = await make(m, b, 'C')
+  const d = await make(m, m.root, 'D')
+  assert.ok(a && b && c && d)
+  assert.equal(m.tree.heightOf(m.tree.snapshot(), a), 2, 'A → B → C is two levels below A')
+  // Moving A under D puts C at depth 5, past maxDepth=4. The old height (1) let this through.
+  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\)/)
+})