Repository navigation
[Maintainer] Refuse parent changes that create hierarchy cycles - #1234
Conversation
(cherry picked from commit c5b51f5)
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.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c8d993c7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const newParentId = typeof data.parentId === 'string' ? (data.parentId as AnyNodeId) : null | ||
| if (wouldCreateHierarchyCycle(patch.id, newParentId, scene().nodes)) { |
There was a problem hiding this comment.
When a create embeds parentId in its node—such as a site whose parentId equals its own ID, or two creates whose embedded parents reference each other—the create branch never calls wouldCreateHierarchyCycle, so this new check is reached only later when the store throws. registerApplyPatch then converts the ordinary error into an MCP InvalidParams protocol error rather than the documented structured invalid_parent refusal. Apply the same progressive-scene cycle check to creates before calling put, so every hierarchy-cycle refusal has the promised { code, patchIndex, id, message } envelope.
Useful? React with 👍 / 👎.
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.
…acked 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.
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.
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.
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.
Aymericr
changed the title
What does this PR do?
Fixes #975. A parent change can no longer make a node its own ancestor:
createNodes,updateNodesandapplyNodeChangesthrowinvalid hierarchy: placing "<id>" under "<parent>" would create a cycleinstead of writing the cycle.apply_patchrefuses an update or a create that would close a cycle asinvalid_parentand leaves the scene unchanged. That includes a cycle closed by a later op in the same patch.SceneBridge.validateSceneandverify_scenereport an existing parent cycle, oneparentIderror per node in it (schema_invalidinverify_scene). As a result,save_scenerefuses to save the current scene while it holds a cycle (scene_invalid). Such scenes still load. Edits beneath an existing cycle still apply, and moving one of its nodes to a valid parent repairs it.A placement adds one parent link, so the only cycle it can close runs through the placed node. The guard refuses exactly that case. A cycle the scene already holds elsewhere on the chain does not block the edit; the scene checks report it instead.
This ports #980 by @Vansh0Sharma onto current
main; the first commit keeps his authorship. It differs from #980 in two ways:validate_scenewas retired in One scene check: verify_scene lists every schema error and the rooms the walls leave open, validate_scene retires #1081, so fix: prevent cyclic scene hierarchies #980's change and test for that tool are dropped. ASceneBridge.validateScenetest takes the test's place.at(), so a create-only patch still never copies the scene.Later commits:
apply_patchrow inpackages/mcp/README.md.parentIdcloses a cycle is refused asinvalid_parentinstead of surfacing the store's error as a protocol error.stage-runtime.tsbuilds the bundled MCP server and geometry worker with--minify-whitespace, so the packed CLI fits its existing budget again (see CI notes).verify_scenelists parent cycles asvalidateScenedoes, from one sharedhierarchyCycleErrors.stage-runtime.tscomment now gives the actual reason for whitespace-only minification.Not covered:
save_scenewith an explicitgraphargument still checks node schemas only, as before.resolveLevelId(packages/core/src/lib/node-ancestry.ts), whose parent walk has no guard. That predates this PR, which only stops new cycles from being written, and is left for a follow-up.How to test
apply_patchcontaining{ op: 'update', id: 'site_a', data: { parentId: 'site_b' } }and{ op: 'update', id: 'site_b', data: { parentId: 'site_a' } }. Expected: a refusal{ code: 'invalid_parent', patchIndex: 1, id: 'site_b' }, and both sites keepparentId: null.apply_patchthat moves the site under its own level. Expected:invalid_parent, and the hierarchy is unchanged.apply_patchcreatingsite_awithparentId: 'site_b'andsite_bwithparentId: 'site_a'. Expected:invalid_parentat patch index 1, and neither site exists.validateScenereportshierarchy cycle detected: site_a -> site_b -> site_afor both nodes,verify_scenereturnsvalid: falsewith aschema_invalidissue on each one'sparentId, andsave_sceneof the current scene returnsscene_invalid.apply_patchan update that moves a free wall across the room. Expected: the whole patch applies, including the zone it derives for the split room, andvalidateScenestill reports the cycle.Run locally on this branch, which is based on
mainat 78aaf93, at the head commit:bun run ciwithTURBO_FORCE=1(nothing cached):biome check,i18n:check,skills:validate,check-types(14 of 14 tasks),turbo run test(15 of 15 tasks, among them core 7065 pass and 3 skip, mcp 1523, nodes 5230 and 1 skip, editor 2856, viewer 1350, cli 139) plus the root script tests,turbo run build(9 of 9), and the production editor server smokecli-smokeruns: a portable editor build, thenbuild,stage-runtimeandsmoke-runtimeinpackages/cliScreenshots / screen recording
N/A — non-visual change.
Checklist
bun devbun checkto verify)mainbranchCI notes
qualityis green at the head. Two runs at earlier heads failed on timing assertions that also fail onmain: ifc-converter's "grows linearly with the number of rooms" and viewer's "a big floor is planned in one frame's budget". A local A/B of the viewer test showed no difference betweenmainand this branch.cli-smokeis green at the head: "Packed CLI smoke passed (1.9 MB compressed, 7.9 MB unpacked, 72 files)." The budget inpackages/cli/scripts/smoke-packed-runtime.tsis unchanged.mainsat within a few KB of its 10 MiB unpacked limit, and this PR's cycle checks pushed the packed CLI 984 bytes over (10,486,744 B).stage-runtime.tsnow builds both bundles with--minify-whitespace.npm packmeasures 1,978,102 B compressed, 8,295,851 B unpacked and 72 files, against limits of 3 MiB, 10 MiB and 250, which leaves 2,189,909 B of unpacked headroom. The bundled MCP server shrinks from 6.3 MB to 5.0 MB and the geometry worker from 3.9 MB to 3.1 MB.Note
Medium Risk
Changes core scene mutation and MCP patch validation paths; incorrect cycle logic could block valid reparents or allow bad graphs, but behavior is heavily tested and scoped to parentId assignments.
Reviewed by Cursor Bugbot for commit 498419a. Bugbot is set up for automated code reviews on this repo. Configure here.