[Maintainer] Refuse parent changes that create hierarchy cycles

packages/cli/scripts/stage-runtime.ts

@@ -145,6 +145,9 @@ async function bundleMcpServer(
       'node',
       '--format',
       'esm',
+      // Whitespace only: the program stays the same token for token, so function and class
+      // names stay as written for stack traces and for code that reads them.
+      '--minify-whitespace',
       '--define',
       `process.env.PASCAL_MCP_VERSION=${JSON.stringify(version)}`,
     ],

packages/core/src/agent-operations/operations.test.ts

@@ -10,6 +10,7 @@ import {
   BaseNode,
   generateId,
   LevelNode,
+  SiteNode,
   StairNode,
   StairSegmentNode,
 } from '../schema'
@@ -318,3 +319,24 @@ test('scene verification validates registered plugin kinds, rejects malformed no
     restore()
   }
 })
+
+test('scene verification lists each node on a parent cycle by its parentId', () => {
+  const siteA = SiteNode.parse({ id: 'site_a', parentId: 'site_b', children: ['site_b'] })
+  const siteB = SiteNode.parse({ id: 'site_b', parentId: 'site_a', children: ['site_a'] })
+  const { result } = AGENT_OPERATIONS.verify_scene(
+    { [siteA.id]: siteA, [siteB.id]: siteB } as Record<string, AnyNode>,
+    {},
+    { activeLevelId: null },
+  )
+  expect(result.valid).toBe(false)
+  for (const nodeId of [siteA.id, siteB.id]) {
+    expect(result.issues).toContainEqual(
+      expect.objectContaining({
+        type: 'schema_invalid',
+        nodeId,
+        path: 'parentId',
+        message: expect.stringContaining('hierarchy cycle detected: site_a -> site_b -> site_a'),
+      }),
+    )
+  }
+})

packages/core/src/agent-operations/verify-scene.ts

@@ -1,6 +1,7 @@
 import { levelBuildingId } from '../building/level-duplication'
 import { levelGradeStanding } from '../lib/grade-standing'
 import { groundSeat } from '../lib/ground-seat'
+import { hierarchyCycleErrors } from '../lib/hierarchy'
 import { getLevelDisplayName } from '../lib/level-name'
 import { HIDDEN_SITE_NOTE } from '../lib/node-visibility'
 import { wallSupportForNodes } from '../lib/opening-floor-datum'
@@ -294,7 +295,7 @@ function withDefaults(node: AnyNode, parsed: unknown): AnyNode {
  * kind's, else core's. The checks that measure read each node with its defaults filled. A node its schema
  * rejects is listed field by field and left `unread`: they could not read it. A node of a kind no
  * schema here knows (a plugin's, where the plugin is not loaded) is counted by kind and kept as
- * stored: the checks never read it.
+ * stored: the checks never read it. Each node on a parent cycle is listed by its `parentId`.
  */
 function checkSchemas(scene: SceneNodes, hostSchema: AgentContext['nodeSchema']) {
   const errors: { nodeId: string; path: string; message: string }[] = []
@@ -321,6 +322,7 @@ function checkSchemas(scene: SceneNodes, hostSchema: AgentContext['nodeSchema'])
       errors.push({ nodeId: id, path: issue.path.join('.'), message: issue.message })
     unread.add(id)
   }
+  errors.push(...hierarchyCycleErrors(scene))
   return { errors, unknownKinds, unread, measured }
 }
 

packages/core/src/index.ts

@@ -404,6 +404,7 @@ export {
   levelStackGroundBox,
   terrainBetweenEyeAndBox,
 } from './lib/ground-window'
+export { hierarchyCycleErrors, wouldCreateHierarchyCycle } from './lib/hierarchy'
 export {
   type ExposedInterval,
   exposedIntervals,

packages/core/src/lib/hierarchy.test.ts

@@ -0,0 +1,21 @@
+import { describe, expect, test } from 'bun:test'
+import type { AnyNodeId } from '../schema'
+import { findHierarchyCycles, wouldCreateHierarchyCycle } from './hierarchy'
+
+const id = (value: string) => value as AnyNodeId
+
+describe('wouldCreateHierarchyCycle', () => {
+  test('a cycle already on the chain only counts when the node is on it', () => {
+    const nodes = {
+      site_a: { parentId: 'site_b' },
+      site_b: { parentId: 'site_a' },
+      level_x: { parentId: 'site_a' },
+      wall_new: { parentId: null },
+    }
+
+    expect(wouldCreateHierarchyCycle(id('wall_new'), id('level_x'), nodes)).toBe(false)
+    expect(wouldCreateHierarchyCycle(id('site_a'), id('level_x'), nodes)).toBe(true)
+    expect(wouldCreateHierarchyCycle(id('level_x'), id('level_x'), nodes)).toBe(true)
+    expect(findHierarchyCycles(nodes)).toEqual([['site_a', 'site_b']])
+  })
+})

packages/core/src/lib/hierarchy.ts

@@ -0,0 +1,65 @@
+import type { AnyNode, AnyNodeId } from '../schema/types'
+
+type NodeMap = Readonly<Record<string, Pick<AnyNode, 'parentId'>>>
+type NodeLookup = (id: AnyNodeId) => Pick<AnyNode, 'parentId'> | undefined
+
+/**
+ * Whether assigning `newParentId` to `nodeId` would put `nodeId` on a cycle of
+ * parent links. A cycle the chain already holds without `nodeId` is not one it
+ * creates: counting it would refuse every edit beneath an old cyclic scene.
+ */
+export function wouldCreateHierarchyCycle(
+  nodeId: AnyNodeId,
+  newParentId: AnyNodeId | null,
+  nodes: NodeMap | NodeLookup,
+): boolean {
+  if (!newParentId) return false
+  const nodeAt: NodeLookup = typeof nodes === 'function' ? nodes : (id) => nodes[id]
+
+  const seen = new Set<AnyNodeId>()
+  let currentId: AnyNodeId | null = newParentId
+  while (currentId) {
+    if (currentId === nodeId) return true
+    if (seen.has(currentId)) return false
+    seen.add(currentId)
+    currentId = (nodeAt(currentId)?.parentId as AnyNodeId | null | undefined) ?? null
+  }
+  return false
+}
+
+/** Return each distinct cycle encoded by `parentId` links. */
+export function findHierarchyCycles(nodes: NodeMap): AnyNodeId[][] {
+  const visited = new Set<AnyNodeId>()
+  const cycles: AnyNodeId[][] = []
+
+  for (const startId of Object.keys(nodes) as AnyNodeId[]) {
+    if (visited.has(startId)) continue
+
+    const chain: AnyNodeId[] = []
+    const positions = new Map<AnyNodeId, number>()
+    let currentId: AnyNodeId | null = startId
+    while (currentId && nodes[currentId] && !visited.has(currentId)) {
+      const position = positions.get(currentId)
+      if (position !== undefined) {
+        cycles.push(chain.slice(position))
+        break
+      }
+      positions.set(currentId, chain.length)
+      chain.push(currentId)
+      currentId = (nodes[currentId]?.parentId as AnyNodeId | null | undefined) ?? null
+    }
+    for (const id of chain) visited.add(id)
+  }
+
+  return cycles
+}
+
+/** The `parentId` error each node on a cycle raises, as every scene check lists it. */
+export function hierarchyCycleErrors(
+  nodes: NodeMap,
+): { nodeId: AnyNodeId; path: 'parentId'; message: string }[] {
+  return findHierarchyCycles(nodes).flatMap((cycle) => {
+    const message = `hierarchy cycle detected: ${[...cycle, cycle[0]].join(' -> ')}`
+    return cycle.map((nodeId) => ({ nodeId, path: 'parentId' as const, message }))
+  })
+}

packages/core/src/store/actions/node-actions.ts

@@ -1,6 +1,7 @@
 import { planWallDeletion } from '../../commands/structure/plan-wall-deletion'
 import type { StructurePlan } from '../../commands/structure/shared'
 import { withoutFloorStepOverrideKeys } from '../../lib/floor-step-finish'
+import { wouldCreateHierarchyCycle } from '../../lib/hierarchy'
 import { isSpaceDetectionPaused } from '../../lib/space-detection'
 import { deletedSurfaceSupportPatch } from '../../lib/surface-support-references'
 import { nodeRegistry } from '../../registry/registry'
@@ -1212,6 +1213,11 @@ const createNodesActionImpl = (
         parseCreatedNode(node, effectiveParentId),
         collectionIds,
       )
+      if (wouldCreateHierarchyCycle(newNode.id, effectiveParentId, nextNodes)) {
+        throw new Error(
+          `invalid hierarchy: placing "${newNode.id}" under "${effectiveParentId}" would create a cycle`,
+        )
+      }
 
       nextNodes[newNode.id] = newNode
 
@@ -1372,6 +1378,12 @@ const applyNodeChangesActionImpl = (
       addLeanToHostRoofId(updatedNode, nextNodes, roofsToRefresh)
 
       if (data.parentId !== undefined && data.parentId !== currentNode.parentId) {
+        const newParentId = data.parentId as AnyNodeId | null
+        if (wouldCreateHierarchyCycle(id, newParentId, nextNodes)) {
+          throw new Error(
+            `invalid hierarchy: placing "${id}" under "${newParentId}" would create a cycle`,
+          )
+        }
         const oldParentId = currentNode.parentId as AnyNodeId | null
         if (oldParentId && nextNodes[oldParentId]) {
           const oldParent = nextNodes[oldParentId] as AnyContainerNode
@@ -1382,7 +1394,6 @@ const applyNodeChangesActionImpl = (
           parentsToMarkDirty.add(oldParent.id)
         }
 
-        const newParentId = data.parentId as AnyNodeId | null
         if (newParentId && nextNodes[newParentId]) {
           const newParent = nextNodes[newParentId] as AnyContainerNode
           nextNodes[newParent.id] = {
@@ -1413,6 +1424,11 @@ const applyNodeChangesActionImpl = (
         parseCreatedNode(node, effectiveParentId),
         collectionIds,
       )
+      if (wouldCreateHierarchyCycle(newNode.id, effectiveParentId, nextNodes)) {
+        throw new Error(
+          `invalid hierarchy: placing "${newNode.id}" under "${effectiveParentId}" would create a cycle`,
+        )
+      }
 
       nextNodes[newNode.id as AnyNodeId] = newNode
       nodesToMarkDirty.add(newNode.id as AnyNodeId)
@@ -1566,6 +1582,12 @@ const updateNodesActionImpl = (
 
       // Handle Reparenting Logic
       if (data.parentId !== undefined && data.parentId !== currentNode.parentId) {
+        const newParentId = data.parentId as AnyNodeId | null
+        if (wouldCreateHierarchyCycle(id, newParentId, nextNodes)) {
+          throw new Error(
+            `invalid hierarchy: placing "${id}" under "${newParentId}" would create a cycle`,
+          )
+        }
         // 1. Remove from old parent
         const oldParentId = currentNode.parentId as AnyNodeId | null
         if (oldParentId && nextNodes[oldParentId]) {
@@ -1586,7 +1608,6 @@ const updateNodesActionImpl = (
         // and a spread of `undefined` here throws and aborts the entire
         // `set` callback. Initialising to `[]` matches what the schema's
         // default would have produced.
-        const newParentId = data.parentId as AnyNodeId | null
         if (newParentId && nextNodes[newParentId]) {
           const newParent = nextNodes[newParentId] as AnyContainerNode
           const newChildren = Array.isArray((newParent as { children?: unknown }).children)

packages/core/src/store/actions/reparent.test.ts

@@ -1,4 +1,5 @@
 import { beforeEach, describe, expect, test } from 'bun:test'
+import { SiteNode } from '../../schema/nodes/site'
 import type { AnyNode, AnyNodeId } from '../../schema/types'
 import useScene from '../use-scene'
 
@@ -242,3 +243,20 @@ describe('node-actions reparent — repeated segment-hopping', () => {
     expect(totalListings).toBe(1)
   })
 })
+
+describe('node-actions reparent — hierarchy cycles', () => {
+  test('refuses moving a node beneath one of its descendants', () => {
+    const siteA = SiteNode.parse({ id: 'site_a', children: ['site_b'] })
+    const siteB = SiteNode.parse({ id: 'site_b', parentId: siteA.id })
+    useScene.setState({
+      nodes: { [siteA.id]: siteA, [siteB.id]: siteB },
+      rootNodeIds: [siteA.id],
+    } as never)
+
+    expect(() => useScene.getState().updateNode(siteA.id, { parentId: siteB.id })).toThrow(
+      'would create a cycle',
+    )
+    expect(useScene.getState().nodes[siteA.id]?.parentId).toBeNull()
+    expect(useScene.getState().nodes[siteB.id]?.parentId).toBe(siteA.id)
+  })
+})