Guard heightOf against a parent cycle and count the whole subtree (#677)

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(' → ') }
 

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\)/)
+})