From 4f7e42ea5b776c9a2928193794574e7dfb52f75f Mon Sep 17 00:00:00 2001 From: VANSH SHARMA Date: Wed, 30 Sep 2026 09:21:28 +0530 Subject: [PATCH 1/9] fix: prevent cyclic scene hierarchies (cherry picked from commit c5b51f5121fa2eae9b84f7ef3cd4ff7948dd9855) --- packages/core/src/index.ts | 1 + packages/core/src/lib/hierarchy.ts | 51 +++++++++++++++++++ .../core/src/store/actions/node-actions.ts | 25 ++++++++- .../core/src/store/actions/reparent.test.ts | 18 +++++++ packages/mcp/src/bridge/scene-bridge.ts | 10 ++-- .../src/tools/apply-patch-identity.test.ts | 21 ++++++++ packages/mcp/src/tools/apply-patch.ts | 2 +- packages/mcp/src/tools/patch-guards.ts | 10 ++++ 8 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 packages/core/src/lib/hierarchy.ts diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 0d9ef5efa7..7cf25b089c 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -404,6 +404,7 @@ export { levelStackGroundBox, terrainBetweenEyeAndBox, } from './lib/ground-window' +export { findHierarchyCycles, wouldCreateHierarchyCycle } from './lib/hierarchy' export { type ExposedInterval, exposedIntervals, diff --git a/packages/core/src/lib/hierarchy.ts b/packages/core/src/lib/hierarchy.ts new file mode 100644 index 0000000000..de162c0560 --- /dev/null +++ b/packages/core/src/lib/hierarchy.ts @@ -0,0 +1,51 @@ +import type { AnyNode, AnyNodeId } from '../schema/types' + +type NodeMap = Readonly>> + +/** + * Whether assigning `newParentId` to `nodeId` would make its parent chain + * cyclic. A pre-existing cycle on that chain is unsafe to extend as well. + */ +export function wouldCreateHierarchyCycle( + nodeId: AnyNodeId, + newParentId: AnyNodeId | null, + nodes: NodeMap, +): boolean { + if (!newParentId) return false + + const seen = new Set() + let currentId: AnyNodeId | null = newParentId + while (currentId) { + if (currentId === nodeId || seen.has(currentId)) return true + seen.add(currentId) + currentId = (nodes[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() + const cycles: AnyNodeId[][] = [] + + for (const startId of Object.keys(nodes) as AnyNodeId[]) { + if (visited.has(startId)) continue + + const chain: AnyNodeId[] = [] + const positions = new Map() + 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 +} diff --git a/packages/core/src/store/actions/node-actions.ts b/packages/core/src/store/actions/node-actions.ts index 5968e3fe1c..988d43d85d 100644 --- a/packages/core/src/store/actions/node-actions.ts +++ b/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) diff --git a/packages/core/src/store/actions/reparent.test.ts b/packages/core/src/store/actions/reparent.test.ts index 7ee09547c2..7f3d0df700 100644 --- a/packages/core/src/store/actions/reparent.test.ts +++ b/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) + }) +}) diff --git a/packages/mcp/src/bridge/scene-bridge.ts b/packages/mcp/src/bridge/scene-bridge.ts index c134e31c28..6da6b624ea 100644 --- a/packages/mcp/src/bridge/scene-bridge.ts +++ b/packages/mcp/src/bridge/scene-bridge.ts @@ -3,6 +3,7 @@ import './node-shims' import { applyStructureReconciliation, assertDerivedNodeWrites, + findHierarchyCycles, HIDDEN_SITE_NOTE, type NodeDeletionPlan, type NodeDeletionScene, @@ -681,10 +682,7 @@ export class SceneBridge { }) } - /** - * Zod-validate every node in the scene. Reports one error per failed node, - * concatenating Zod issue paths. - */ + /** Validate node schemas and global parent-chain acyclicity. */ validateScene(): ValidationResult { const errors: ValidationError[] = [] const warnings: ValidationError[] = [] @@ -703,6 +701,10 @@ export class SceneBridge { }) } } + for (const cycle of findHierarchyCycles(nodes)) { + const message = `hierarchy cycle detected: ${[...cycle, cycle[0]!].join(' -> ')}` + for (const nodeId of cycle) errors.push({ nodeId, path: 'parentId', message }) + } return { valid: errors.length === 0, errors, warnings } } diff --git a/packages/mcp/src/tools/apply-patch-identity.test.ts b/packages/mcp/src/tools/apply-patch-identity.test.ts index ed22087cb1..9ecb69c492 100644 --- a/packages/mcp/src/tools/apply-patch-identity.test.ts +++ b/packages/mcp/src/tools/apply-patch-identity.test.ts @@ -8,6 +8,7 @@ import { DoorNode, RoofNode, RoofSegmentNode, + SiteNode, WallNode, WindowNode, ZoneNode, @@ -388,6 +389,26 @@ describe('apply_patch identity and validation guards', () => { expect(host?.type === 'wall' && host.children).toEqual([window.id]) }) + test('refuses a reparent beneath a descendant without changing the scene', async () => { + const siteA = SiteNode.parse({ id: 'site_a' }) + const siteB = SiteNode.parse({ id: 'site_b' }) + bridge.setScene({ [siteA.id]: siteA }, [siteA.id]) + + expect(await apply([{ op: 'create', node: siteB }])).toMatchObject({ isError: false }) + expect( + await apply([{ op: 'update', id: siteA.id, data: { parentId: siteB.id } }]), + ).toMatchObject({ isError: false }) + + const cycle = await refusal([{ op: 'update', id: siteB.id, data: { parentId: siteA.id } }]) + expect(cycle).toMatchObject({ code: 'invalid_parent', patchIndex: 0, id: siteB.id }) + expect(cycle.message).toContain('hierarchy cycle') + + expect(bridge.getNode(siteA.id as AnyNodeId)?.parentId).toBe(siteB.id) + const keptParent = bridge.getNode(siteB.id as AnyNodeId) + expect(keptParent?.type === 'site' && keptParent.children).toEqual([siteA.id]) + expect(bridge.getNode(siteB.id as AnyNodeId)?.parentId).toBeNull() + }) + test('default gutters a delete regenerates cannot be addressed later in the same patch', async () => { const roof = RoofNode.parse({}) const segment = (x: number) => diff --git a/packages/mcp/src/tools/apply-patch.ts b/packages/mcp/src/tools/apply-patch.ts index c621bdba03..15a7cece23 100644 --- a/packages/mcp/src/tools/apply-patch.ts +++ b/packages/mcp/src/tools/apply-patch.ts @@ -32,7 +32,7 @@ export function registerApplyPatch(server: McpServer, bridge: SceneOperations): { title: 'Apply patch', description: - "Apply a batch of create/update/delete operations atomically. All patches are validated before any are applied; the entire batch forms a single undo step. Batch-first is the default: prefer one apply_patch call containing all create/update/delete ops for a build step, in stable order so later ops can reference ids created by earlier ops. A single call is atomic (all or nothing) and pays the snapshot and save cost once; do not loop one-op calls. A create whose id already exists fails the whole patch with node_exists; to replace a node, delete it earlier in the same patch. An update cannot change id or type (identity_change) or object or children (immutable_field); restating the current value is fine. A parentId change must name an existing node that holds children (invalid_parent). An update cannot add schema issues to a node (invalid_update). After a delete or a roof update that regenerates a roof's default gutters, those gutters and their downspouts are addressable only in a later call (regenerated_default). A roof created or moved onto the storey whose walls carry it goes to the level above them (the roof level over a top storey), said in notes; storey content (walls, zones, slabs, ceilings, items, stairs) on a roof level (roof_level), a walled level declared the roof level (roof_level_holds_storey) and floor indexes putting a roof level under a storey (roof_level_on_top) are refused. Refusals come back as a tool error whose text is JSON {code, patchIndex, id, message}.", + "Apply a batch of create/update/delete operations atomically. All patches are validated before any are applied; the entire batch forms a single undo step. Batch-first is the default: prefer one apply_patch call containing all create/update/delete ops for a build step, in stable order so later ops can reference ids created by earlier ops. A single call is atomic (all or nothing) and pays the snapshot and save cost once; do not loop one-op calls. A create whose id already exists fails the whole patch with node_exists; to replace a node, delete it earlier in the same patch. An update cannot change id or type (identity_change) or object or children (immutable_field); restating the current value is fine. A parentId change must name an existing node that holds children and cannot point to the node or one of its descendants (invalid_parent). An update cannot add schema issues to a node (invalid_update). After a delete or a roof update that regenerates a roof's default gutters, those gutters and their downspouts are addressable only in a later call (regenerated_default). A roof created or moved onto the storey whose walls carry it goes to the level above them (the roof level over a top storey), said in notes; storey content (walls, zones, slabs, ceilings, items, stairs) on a roof level (roof_level), a walled level declared the roof level (roof_level_holds_storey) and floor indexes putting a roof level under a storey (roof_level_on_top) are refused. Refusals come back as a tool error whose text is JSON {code, patchIndex, id, message}.", inputSchema: z.object(applyPatchInput), outputSchema: z.object(applyPatchOutput), annotations: DESTRUCTIVE_TOOL_ANNOTATIONS, diff --git a/packages/mcp/src/tools/patch-guards.ts b/packages/mcp/src/tools/patch-guards.ts index 79b66e7432..0fb239a9b6 100644 --- a/packages/mcp/src/tools/patch-guards.ts +++ b/packages/mcp/src/tools/patch-guards.ts @@ -5,6 +5,7 @@ import { planNodeDeletion, previewDefaultGutterRefresh, validateNodeRelations, + wouldCreateHierarchyCycle, } from '@pascal-app/core' import { scriptedFieldRefusal, @@ -301,6 +302,15 @@ export function assertPatchKeepsIdentity( } } if ('parentId' in data && data.parentId !== current.parentId) { + const newParentId = typeof data.parentId === 'string' ? (data.parentId as AnyNodeId) : null + if (wouldCreateHierarchyCycle(patch.id, newParentId, scene().nodes)) { + throw new PatchRefusedError( + 'invalid_parent', + index, + patch.id, + `update cannot move "${patch.id}" under ${JSON.stringify(data.parentId)}: it would create a hierarchy cycle.`, + ) + } const newParent = typeof data.parentId === 'string' ? at(data.parentId) : undefined if (!(newParent && Array.isArray((newParent as { children?: unknown }).children))) { throw new PatchRefusedError( From b423e75535140994791a222fb48b3ee063786b70 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 8 Nov 2026 23:06:50 +0000 Subject: [PATCH 2/9] test: cover the hierarchy cycles reported in #975 Add apply_patch regressions for the two reproductions in #975: a cycle closed by a later update in the same patch, and a site moved under its own level. Both are refused as invalid_parent and leave the scene unchanged. Cover SceneBridge.validateScene reporting a parent cycle, which replaces the validate_scene test that no longer applies on main. --- packages/mcp/src/bridge/scene-bridge.test.ts | 16 ++++++++++++ .../src/tools/apply-patch-identity.test.ts | 26 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/packages/mcp/src/bridge/scene-bridge.test.ts b/packages/mcp/src/bridge/scene-bridge.test.ts index 141f9e61f2..c8a1ba30fa 100644 --- a/packages/mcp/src/bridge/scene-bridge.test.ts +++ b/packages/mcp/src/bridge/scene-bridge.test.ts @@ -321,6 +321,22 @@ describe('SceneBridge', () => { expect(res.valid).toBe(false) expect(res.errors.some((e) => e.nodeId === 'wall_bogus')).toBe(true) }) + + test('reports a parent cycle that each node schema accepts on its own', () => { + 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'] }) + bridge.setScene({ [siteA.id]: siteA, [siteB.id]: siteB }, []) + + const res = bridge.validateScene() + expect(res.valid).toBe(false) + const message = 'hierarchy cycle detected: site_a -> site_b -> site_a' + expect(res.errors).toEqual( + expect.arrayContaining([ + { nodeId: siteA.id, path: 'parentId', message }, + { nodeId: siteB.id, path: 'parentId', message }, + ]), + ) + }) }) describe('traversal: site quirk & generic helpers', () => { diff --git a/packages/mcp/src/tools/apply-patch-identity.test.ts b/packages/mcp/src/tools/apply-patch-identity.test.ts index 9ecb69c492..98ddcd5930 100644 --- a/packages/mcp/src/tools/apply-patch-identity.test.ts +++ b/packages/mcp/src/tools/apply-patch-identity.test.ts @@ -409,6 +409,32 @@ describe('apply_patch identity and validation guards', () => { expect(bridge.getNode(siteB.id as AnyNodeId)?.parentId).toBeNull() }) + test('refuses a cycle closed by a later update in the same patch', async () => { + const siteA = SiteNode.parse({ id: 'site_a' }) + const siteB = SiteNode.parse({ id: 'site_b' }) + bridge.setScene({ [siteA.id]: siteA, [siteB.id]: siteB }, [siteA.id, siteB.id]) + + const cycle = await refusal([ + { op: 'update', id: siteA.id, data: { parentId: siteB.id } }, + { op: 'update', id: siteB.id, data: { parentId: siteA.id } }, + ]) + expect(cycle).toMatchObject({ code: 'invalid_parent', patchIndex: 1, id: siteB.id }) + + expect(bridge.getNode(siteA.id as AnyNodeId)?.parentId).toBeNull() + expect(bridge.getNode(siteB.id as AnyNodeId)?.parentId).toBeNull() + }) + + test('refuses moving the site under its own level', async () => { + const site = Object.values(bridge.getNodes()).find((n) => n.type === 'site')! + + const cycle = await refusal([{ op: 'update', id: site.id, data: { parentId: level.id } }]) + expect(cycle).toMatchObject({ code: 'invalid_parent', patchIndex: 0, id: site.id }) + + expect(bridge.getNode(site.id as AnyNodeId)?.parentId).toBe(site.parentId) + const keptLevel = bridge.getNode(level.id as AnyNodeId) + expect(keptLevel?.type === 'level' && keptLevel.children).not.toContain(site.id) + }) + test('default gutters a delete regenerates cannot be addressed later in the same patch', async () => { const roof = RoofNode.parse({}) const segment = (x: number) => From 1c8d993c7ad593ae58429f5d130167694c392463 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 8 Nov 2026 23:10:05 +0000 Subject: [PATCH 3/9] docs(mcp): apply_patch refuses a reparent under the node's own subtree --- packages/mcp/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mcp/README.md b/packages/mcp/README.md index 48ec91926b..f16e7cd267 100644 --- a/packages/mcp/README.md +++ b/packages/mcp/README.md @@ -431,7 +431,7 @@ is a "pitch" fraction like `"1/4 pitch"`, which is rise over span. | `furnish_room` | Furnish a room for its type from the host's item catalog: the main piece against the wall facing the door, the rest along a side wall. Nothing lands in a door's clear zone or on another item: a piece that does not fit is nudged, else skipped, and `skipped` says why. | `{ zoneId?, polygon?, levelId?, roomType, doorWallIndex? }` | `{ placed, itemIds, skipped, doorWallIndex, doorsDetected }`; refusals `room_required`, `roof_level`, `level_not_found`, `no_catalog` | | `add_corner_window` | A window wrapping the corner where two walls meet: one window per wall, both running to the corner and joined, the glass fused (a fixed window) or at a thin post (the other window types, whose frames stay), at any angle from 10° to 170° (a square corner, a bay's 135°, an acute one). `corner` is the point the walls of `levelId` end at (default the floor in view: stacked storeys share their corners' points), `[x, z]` in the level's plan as `get_walls` gives their ends; `width` is how far the glass runs along each wall from the corner (default 1.2 m) and `widths` overrides it per wall id; `post` is `'none'` (the glass fused, a fixed window's default) or `'post'` (the other types' default and only choice); `height` defaults to 1.5 m and the window is centred on the wall unless `sillHeight` (its bottom edge above the floor) is given; `wallIds` (two) is needed only when more than two walls end at the corner; `windowType` and `style` are `add_window`'s (a style needs a Fixed window). By hand: select a window that ends at a corner and use the window panel's Corner section (Wrap the corner, This side and Other side, Fused glass or Corner post for a fixed window, Unwrap). | `{ corner: [x, z], levelId? (or level), wallIds?, width?, widths?, height?, sillHeight?, post?, windowType?, style? }` | `{ windowIds, wallIds, angle, post, achieved }`; refusals `no_corner` (no two walls end there), `corner_ambiguous` (more than two do: pass `wallIds`), `corner_angle` (under 10° or over 170°: use `add_window`), `corner_not_fused` (`post: 'none'` on a type other than fixed), `width_exceeds_wall`, `style_needs_fixed_window` | | `merge_windows` | Make two windows that exist one window, as the editor's Merge windows button does; nothing is recreated. On two walls that meet at a corner, each window within 0.30 m of it, they become one corner window (the `add_corner_window` result): each is slid to the corner keeping its width, the glass fused (a fixed window's; other types meet at a post), and both take the first window's height, sill and type, so name first the window whose look you want; the corner is refused over 170° or under 10°. On one wall, two windows with at most 0.30 m between them and nothing between become one window spanning both (overlapping or one inside the other, their union), from the lower sill to the higher head, with the first window's type and style. Use it for two windows already built that open a corner or one wide opening (`verify_scene`'s `corner_unjoined` names a pair for the first); `add_corner_window` builds a new pair. By hand: select two windows and use Merge windows in the multi-selection panel, or place or drag a window near a corner where one waits and click or release (Alt places it plain). | `{ windowIds: [a, b] }` | `{ kind: 'corner' \| 'same_wall', windowIds, removedIds, achieved }`; refusals `window_not_found`, `not_a_window`, `same_window`, `window_not_on_wall`, `window_already_joined` (unwrap it first), `windows_already_joined`, `windows_not_adjacent`, `opening_between`, `walls_not_meeting`, `corner_angle`, `window_not_at_corner`, `height_exceeds_wall`, `opening_overlap` | -| `apply_patch` | Batched create/update/delete/move, validated and dry-run before commit. Batch-first is the default: send all create/update/delete ops for a build step in one atomic call (stable order, later ops may reference earlier created ids); do not loop one-op calls. A create with an id already in the scene is refused (`node_exists`); delete it earlier in the same patch to replace it. Updates cannot change `id` or `type` (`identity_change`), `object` or `children` (`immutable_field`), move a node under a missing or childless parent (`invalid_parent`), or add schema issues (`invalid_update`); default gutters and downspouts a delete regenerates are addressable only in a later call (`regenerated_default`). Refusals are tool errors with JSON text `{ code, patchIndex, id, message }`. A level created without a `height`, or an update that clears it, gets the height the editor's load derives from what the level holds (an empty level: 2.5 m), one line per level in `notes`. A roof the patch makes or moves onto the storey whose walls carry it is seated on the level above those walls, one line per roof in `notes`; a new storey at or above the roof level moves it up. Refused: storey content on a roof level (`roof_level`), declaring a level that holds storey content the roof level (`roof_level_holds_storey`), floor indexes that put the roof level under a storey (`roof_level_on_top`). | `{ patches: Patch[] }` | `{ applied: number, notes?: string[] }` | +| `apply_patch` | Batched create/update/delete/move, validated and dry-run before commit. Batch-first is the default: send all create/update/delete ops for a build step in one atomic call (stable order, later ops may reference earlier created ids); do not loop one-op calls. A create with an id already in the scene is refused (`node_exists`); delete it earlier in the same patch to replace it. Updates cannot change `id` or `type` (`identity_change`), `object` or `children` (`immutable_field`), move a node under a missing or childless parent, itself or one of its descendants (`invalid_parent`), or add schema issues (`invalid_update`); default gutters and downspouts a delete regenerates are addressable only in a later call (`regenerated_default`). Refusals are tool errors with JSON text `{ code, patchIndex, id, message }`. A level created without a `height`, or an update that clears it, gets the height the editor's load derives from what the level holds (an empty level: 2.5 m), one line per level in `notes`. A roof the patch makes or moves onto the storey whose walls carry it is seated on the level above those walls, one line per roof in `notes`; a new storey at or above the roof level moves it up. Refused: storey content on a roof level (`roof_level`), declaring a level that holds storey content the roof level (`roof_level_holds_storey`), floor indexes that put the roof level under a storey (`roof_level_on_top`). | `{ patches: Patch[] }` | `{ applied: number, notes?: string[] }` | | `add_level` | Add an empty level to a building, as the editor does: on top of its storeys, under its roof level (which moves up one), or below the lowest for a basement (every existing floor, the ground floor included, keeps its place). Omit `buildingId` for the scene's only building. | `{ buildingId?, position?, name?, height? }` | `{ levelId, buildingId, floorIndex, height, shiftedLevelIds? }`; refusals `building_required`, `building_not_found`, `no_building` | | `set_ground_level` | Make a level the building's ground floor, as the level panel's "Set as ground floor": the building names it, is seated so that floor stands on grade (height only, the plan stays), and the floor indexes are renumbered so it is 0 and the levels under it are the basements. | `{ levelId }` | `{ levelId, buildingId, floorElevation, gradeElevation, movedBuildingBy, renumbered, unchanged? }`; refusals `level_not_found`, `not_a_level`, `no_building`, `not_a_storey` | | `add_wall` | Add a wall to a level, straight or, with `curveOffset`, an arc. Omit `levelId` for the lowest storey. wallType railing builds a guard (default 0.9144 m high); railing sets style, posts, infill and cap; postSize sets thickness. | `{ start, end, levelId?, thickness?, height?, curveOffset?, type?, wallType?, railing? }` | `{ wallId, levelId, length }`; refusals `roof_level`, `wall_too_short`, `level_not_found`, `unknown_wall_type` | From e9f2970b020f0f1c6caa952a4d8ce5e82623fe98 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 8 Nov 2026 23:18:29 +0000 Subject: [PATCH 4/9] ci: rerun after the ifc-converter scaling timing flake From 4c52210d985a5d0af46a34fa6068a3eb5df12b5e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 8 Nov 2026 23:31:04 +0000 Subject: [PATCH 5/9] mcp: refuse a create that closes a hierarchy cycle as invalid_parent A create whose own parentId closes a cycle (itself, or a node created earlier in the same patch that names it) reached the store's guard and came back as a protocol error. Check creates in the patch guard too, so every cycle refusal carries { code, patchIndex, id, message }. Both checks walk the progressive view through at(), so a create-only patch still never copies the scene. --- packages/core/src/lib/hierarchy.ts | 6 ++++-- .../src/tools/apply-patch-identity.test.ts | 21 +++++++++++++++++++ packages/mcp/src/tools/patch-guards.ts | 10 ++++++++- 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/packages/core/src/lib/hierarchy.ts b/packages/core/src/lib/hierarchy.ts index de162c0560..a5a09b1b9d 100644 --- a/packages/core/src/lib/hierarchy.ts +++ b/packages/core/src/lib/hierarchy.ts @@ -1,6 +1,7 @@ import type { AnyNode, AnyNodeId } from '../schema/types' type NodeMap = Readonly>> +type NodeLookup = (id: AnyNodeId) => Pick | undefined /** * Whether assigning `newParentId` to `nodeId` would make its parent chain @@ -9,16 +10,17 @@ type NodeMap = Readonly>> export function wouldCreateHierarchyCycle( nodeId: AnyNodeId, newParentId: AnyNodeId | null, - nodes: NodeMap, + nodes: NodeMap | NodeLookup, ): boolean { if (!newParentId) return false + const nodeAt: NodeLookup = typeof nodes === 'function' ? nodes : (id) => nodes[id] const seen = new Set() let currentId: AnyNodeId | null = newParentId while (currentId) { if (currentId === nodeId || seen.has(currentId)) return true seen.add(currentId) - currentId = (nodes[currentId]?.parentId as AnyNodeId | null | undefined) ?? null + currentId = (nodeAt(currentId)?.parentId as AnyNodeId | null | undefined) ?? null } return false } diff --git a/packages/mcp/src/tools/apply-patch-identity.test.ts b/packages/mcp/src/tools/apply-patch-identity.test.ts index 98ddcd5930..ea704bfd67 100644 --- a/packages/mcp/src/tools/apply-patch-identity.test.ts +++ b/packages/mcp/src/tools/apply-patch-identity.test.ts @@ -435,6 +435,27 @@ describe('apply_patch identity and validation guards', () => { expect(keptLevel?.type === 'level' && keptLevel.children).not.toContain(site.id) }) + test('refuses creates whose own parentId closes a cycle', async () => { + const self = SiteNode.parse({ id: 'site_self', parentId: 'site_self' }) + expect(await refusal([{ op: 'create', node: self }])).toMatchObject({ + code: 'invalid_parent', + patchIndex: 0, + id: self.id, + }) + + const siteA = SiteNode.parse({ id: 'site_a', parentId: 'site_b' }) + const siteB = SiteNode.parse({ id: 'site_b', parentId: 'site_a' }) + const cycle = await refusal([ + { op: 'create', node: siteA }, + { op: 'create', node: siteB }, + ]) + expect(cycle).toMatchObject({ code: 'invalid_parent', patchIndex: 1, id: siteB.id }) + + for (const id of [self.id, siteA.id, siteB.id]) { + expect(bridge.getNode(id as AnyNodeId)).toBeNull() + } + }) + test('default gutters a delete regenerates cannot be addressed later in the same patch', async () => { const roof = RoofNode.parse({}) const segment = (x: number) => diff --git a/packages/mcp/src/tools/patch-guards.ts b/packages/mcp/src/tools/patch-guards.ts index 0fb239a9b6..71da5d5955 100644 --- a/packages/mcp/src/tools/patch-guards.ts +++ b/packages/mcp/src/tools/patch-guards.ts @@ -249,6 +249,14 @@ export function assertPatchKeepsIdentity( `invalid patch: patches[${index}] create parentId "${patch.parentId}" not found`, ) } + if (wouldCreateHierarchyCycle(node.id as AnyNodeId, parentId as AnyNodeId | null, at)) { + throw new PatchRefusedError( + 'invalid_parent', + index, + node.id, + `create cannot place "${node.id}" under ${JSON.stringify(parentId)}: it would create a hierarchy cycle.`, + ) + } put({ ...node, parentId } as AnyNode) const parent = parentId ? at(parentId) : undefined if (parent) put(withChild(parent, node.id)) @@ -303,7 +311,7 @@ export function assertPatchKeepsIdentity( } if ('parentId' in data && data.parentId !== current.parentId) { const newParentId = typeof data.parentId === 'string' ? (data.parentId as AnyNodeId) : null - if (wouldCreateHierarchyCycle(patch.id, newParentId, scene().nodes)) { + if (wouldCreateHierarchyCycle(patch.id, newParentId, at)) { throw new PatchRefusedError( 'invalid_parent', index, From cfb8dea75c5499995beb02de95a8dbcfd87f8dad Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 8 Nov 2026 23:54:24 +0000 Subject: [PATCH 6/9] cli: minify the bundled MCP services' whitespace to stay within the packed budget The hierarchy cycle guard adds 4.1 KB to both bundles and takes the packed CLI 984 bytes over its 10 MiB unpacked budget. Whitespace-only minification leaves identifiers and syntax as written, so zod's and three's runtime constructor.name reads are unaffected, and both bundles parse to the same syntax tree as before. Packed size drops from 10,486,744 to 8,295,698 bytes unpacked and from 2,182,122 to 1,978,032 bytes compressed. --- packages/cli/scripts/stage-runtime.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/cli/scripts/stage-runtime.ts b/packages/cli/scripts/stage-runtime.ts index 525e914512..baf4617d5c 100644 --- a/packages/cli/scripts/stage-runtime.ts +++ b/packages/cli/scripts/stage-runtime.ts @@ -145,6 +145,9 @@ async function bundleMcpServer( 'node', '--format', 'esm', + // Whitespace only: zod and three read `constructor.name` at runtime, which identifier + // minification would rename. + '--minify-whitespace', '--define', `process.env.PASCAL_MCP_VERSION=${JSON.stringify(version)}`, ], From 608f9a21aed889f146e21d07cc6b8be93e921cbf Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 9 Nov 2026 00:42:01 +0000 Subject: [PATCH 7/9] core: only a cycle through the moved node counts as one it creates wouldCreateHierarchyCycle refused any placement whose parent chain revisits a node, even when the walk never reaches the node being placed. In a scene that already holds a cycle, every create beneath it was refused, including the zones the structure commit derives after an agent's update has landed, so apply_patch moved the wall and then reported failure. A placement adds one edge, so the only cycle it can close runs through the placed node; a revisit elsewhere ends the walk with no new cycle, and the old one stays reported by validateScene. --- packages/core/src/lib/hierarchy.test.ts | 21 +++++++ packages/core/src/lib/hierarchy.ts | 8 ++- .../src/tools/apply-patch-identity.test.ts | 58 +++++++++++++++++++ 3 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 packages/core/src/lib/hierarchy.test.ts diff --git a/packages/core/src/lib/hierarchy.test.ts b/packages/core/src/lib/hierarchy.test.ts new file mode 100644 index 0000000000..39821a49d2 --- /dev/null +++ b/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']]) + }) +}) diff --git a/packages/core/src/lib/hierarchy.ts b/packages/core/src/lib/hierarchy.ts index a5a09b1b9d..b9790b18f2 100644 --- a/packages/core/src/lib/hierarchy.ts +++ b/packages/core/src/lib/hierarchy.ts @@ -4,8 +4,9 @@ type NodeMap = Readonly>> type NodeLookup = (id: AnyNodeId) => Pick | undefined /** - * Whether assigning `newParentId` to `nodeId` would make its parent chain - * cyclic. A pre-existing cycle on that chain is unsafe to extend as well. + * 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, @@ -18,7 +19,8 @@ export function wouldCreateHierarchyCycle( const seen = new Set() let currentId: AnyNodeId | null = newParentId while (currentId) { - if (currentId === nodeId || seen.has(currentId)) return true + if (currentId === nodeId) return true + if (seen.has(currentId)) return false seen.add(currentId) currentId = (nodeAt(currentId)?.parentId as AnyNodeId | null | undefined) ?? null } diff --git a/packages/mcp/src/tools/apply-patch-identity.test.ts b/packages/mcp/src/tools/apply-patch-identity.test.ts index ea704bfd67..ebf451fa43 100644 --- a/packages/mcp/src/tools/apply-patch-identity.test.ts +++ b/packages/mcp/src/tools/apply-patch-identity.test.ts @@ -456,6 +456,64 @@ describe('apply_patch identity and validation guards', () => { } }) + test('edits beneath a cycle an old scene already holds apply whole', async () => { + const scene = bridge.exportJSON() + const site = Object.values(scene.nodes).find((n) => n.type === 'site')! + const corners = [ + [0, 0], + [8, 0], + [8, 4], + [0, 4], + ] + const room = corners.map((start, i) => + WallNode.parse({ id: `wall_room${i}`, parentId: level.id, start, end: corners[(i + 1) % 4] }), + ) + const divider = WallNode.parse({ + id: 'wall_divider', + parentId: level.id, + start: [20, 20], + end: [20, 24], + }) + const loop = SiteNode.parse({ id: 'site_loop', parentId: site.id, children: [site.id] }) + bridge.loadJSON({ + ...scene, + nodes: { + ...scene.nodes, + ...Object.fromEntries([...room, divider, loop].map((node) => [node.id, node])), + [site.id]: { + ...site, + parentId: loop.id, + children: [...(site as { children: string[] }).children, loop.id], + } as AnyNode, + [level.id]: { + ...level, + children: [ + ...(level as { children: string[] }).children, + ...room.map((w) => w.id), + divider.id, + ], + } as AnyNode, + }, + }) + const zones = () => Object.values(bridge.getNodes()).filter((n) => n.type === 'zone').length + const before = zones() + + const split = await apply([ + { op: 'update', id: divider.id, data: { start: [4, 0], end: [4, 4] } }, + ]) + expect(split).toMatchObject({ isError: false }) + expect(zones()).toBe(before + 1) + const create = await apply([ + { + op: 'create', + node: WallNode.parse({ id: 'wall_new', start: [0, 9], end: [3, 9] }), + parentId: level.id, + }, + ]) + expect(create).toMatchObject({ isError: false }) + expect(bridge.validateScene().valid).toBe(false) + }) + test('default gutters a delete regenerates cannot be addressed later in the same patch', async () => { const roof = RoofNode.parse({}) const segment = (x: number) => From 74f819bb0e3e796325c931a56b116d547effaeee Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 9 Nov 2026 00:42:07 +0000 Subject: [PATCH 8/9] core: verify_scene lists parent cycles as validate_scene does save_scene refused a cyclic scene through the bridge's validateScene while verify_scene, which every surface shares, still called it valid. Both checks now take each cycle's parentId errors from hierarchyCycleErrors, so verify_scene's valid is validate_scene's valid and each cycle node is a schema_invalid issue on its parentId. --- .../src/agent-operations/operations.test.ts | 22 +++++++++++++++++++ .../core/src/agent-operations/verify-scene.ts | 4 +++- packages/core/src/index.ts | 2 +- packages/core/src/lib/hierarchy.ts | 10 +++++++++ packages/mcp/src/bridge/scene-bridge.test.ts | 13 ++++++++++- packages/mcp/src/bridge/scene-bridge.ts | 7 ++---- 6 files changed, 50 insertions(+), 8 deletions(-) diff --git a/packages/core/src/agent-operations/operations.test.ts b/packages/core/src/agent-operations/operations.test.ts index 519268ae12..ffa3987729 100644 --- a/packages/core/src/agent-operations/operations.test.ts +++ b/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, + {}, + { 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'), + }), + ) + } +}) diff --git a/packages/core/src/agent-operations/verify-scene.ts b/packages/core/src/agent-operations/verify-scene.ts index ccc2787a4a..bff82a4416 100644 --- a/packages/core/src/agent-operations/verify-scene.ts +++ b/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 } } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 7cf25b089c..6c83638939 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -404,7 +404,7 @@ export { levelStackGroundBox, terrainBetweenEyeAndBox, } from './lib/ground-window' -export { findHierarchyCycles, wouldCreateHierarchyCycle } from './lib/hierarchy' +export { hierarchyCycleErrors, wouldCreateHierarchyCycle } from './lib/hierarchy' export { type ExposedInterval, exposedIntervals, diff --git a/packages/core/src/lib/hierarchy.ts b/packages/core/src/lib/hierarchy.ts index b9790b18f2..047e106c67 100644 --- a/packages/core/src/lib/hierarchy.ts +++ b/packages/core/src/lib/hierarchy.ts @@ -53,3 +53,13 @@ export function findHierarchyCycles(nodes: NodeMap): AnyNodeId[][] { 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 })) + }) +} diff --git a/packages/mcp/src/bridge/scene-bridge.test.ts b/packages/mcp/src/bridge/scene-bridge.test.ts index c8a1ba30fa..b975f2af8a 100644 --- a/packages/mcp/src/bridge/scene-bridge.test.ts +++ b/packages/mcp/src/bridge/scene-bridge.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, test } from 'bun:test' +import { AGENT_OPERATIONS } from '@pascal-app/core/agent-operations' import { BuildingNode, DoorNode, @@ -322,7 +323,7 @@ describe('SceneBridge', () => { expect(res.errors.some((e) => e.nodeId === 'wall_bogus')).toBe(true) }) - test('reports a parent cycle that each node schema accepts on its own', () => { + test('reports a parent cycle that each node schema accepts on its own, as verify_scene does', () => { 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'] }) bridge.setScene({ [siteA.id]: siteA, [siteB.id]: siteB }, []) @@ -336,6 +337,16 @@ describe('SceneBridge', () => { { nodeId: siteB.id, path: 'parentId', message }, ]), ) + + const verified = AGENT_OPERATIONS.verify_scene(bridge.getNodes(), {}, { activeLevelId: null }) + expect(verified.result.valid).toBe(false) + expect(verified.result.issues).toEqual( + expect.arrayContaining( + res.errors.map(({ nodeId, path }) => + expect.objectContaining({ type: 'schema_invalid', nodeId, path }), + ), + ), + ) }) }) diff --git a/packages/mcp/src/bridge/scene-bridge.ts b/packages/mcp/src/bridge/scene-bridge.ts index 6da6b624ea..c11db84842 100644 --- a/packages/mcp/src/bridge/scene-bridge.ts +++ b/packages/mcp/src/bridge/scene-bridge.ts @@ -3,8 +3,8 @@ import './node-shims' import { applyStructureReconciliation, assertDerivedNodeWrites, - findHierarchyCycles, HIDDEN_SITE_NOTE, + hierarchyCycleErrors, type NodeDeletionPlan, type NodeDeletionScene, pauseSceneHistory, @@ -701,10 +701,7 @@ export class SceneBridge { }) } } - for (const cycle of findHierarchyCycles(nodes)) { - const message = `hierarchy cycle detected: ${[...cycle, cycle[0]!].join(' -> ')}` - for (const nodeId of cycle) errors.push({ nodeId, path: 'parentId', message }) - } + errors.push(...hierarchyCycleErrors(nodes)) return { valid: errors.length === 0, errors, warnings } } From 498419aa3762ba532aa9ecf639ba4f748b9e1c9f Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 9 Nov 2026 00:42:12 +0000 Subject: [PATCH 9/9] cli: say why the bundled services minify whitespace only zod pins its class names and three reads constructor.name on typed arrays, so neither is what identifier renaming would break; the reason is that whitespace-only output is the same program token for token, keeping the names stack traces and name-reading code see. --- packages/cli/scripts/stage-runtime.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/cli/scripts/stage-runtime.ts b/packages/cli/scripts/stage-runtime.ts index baf4617d5c..84634421c2 100644 --- a/packages/cli/scripts/stage-runtime.ts +++ b/packages/cli/scripts/stage-runtime.ts @@ -145,8 +145,8 @@ async function bundleMcpServer( 'node', '--format', 'esm', - // Whitespace only: zod and three read `constructor.name` at runtime, which identifier - // minification would rename. + // 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)}`,