[Vansh0Sharma]

What does this PR do?

Fixes #975
Prevents 'parentId' updates from creating cyclic scene hierarchies.

• rejects reparenting a node beneath itself or one of its descendents.
• applies the guard in both 'apply-patch' and core scene mutation paths.
• adds global parent chain cycle detection to 'validate-scene' for imported or legacy scenes.
• adds regression coverage for the two site cycle reproduction.

How to test

1. run 'bun run test'
2. confirm the full suite completes successfully
3. verify the focused regression with :

   bun test \ 
packages/core/src/store/actions/reparent.test.ts \
packages/mcp/src/tools/apply-patch-identity.test.ys \
packages/mcp/src/tools/validate-scene.test.ts

4. confirm that reparenting Site_B under descendant Site_A is refused with invalid_parent an validate_scene reports an existing Site_A->Site_B->Site_B cycle.

## Screenshots / screen recording

not applicable - this is a core/mcp validation change with no visual UI change.

## Checklist

- [x] I've tested this locally with `bun dev`
- [x] My code follows the existing code style (run `bun check` to verify)
- [ ] I've updated relevant documentation (if applicable)
- [x] This PR targets the `main` branch

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Touches all reparent/create paths in the scene store and MCP patch guards; incorrect cycle logic could block valid reparents or miss edge cases, but changes are narrowly scoped to hierarchy checks with test coverage.
> 
> **Overview**
> Adds shared **parent-chain cycle detection** (`wouldCreateHierarchyCycle`, `findHierarchyCycles`) and wires it into scene mutations and MCP validation.
> 
> **Core store:** `parentId` changes on create and update (including batch `applyNodeChanges`) now **throw** before mutating if the new parent is the node itself, a descendant, or an already-cyclic chain—so reparents fail atomically with no partial graph update.
> 
> **MCP:** `apply_patch` dry-run rejects the same reparents as **`invalid_parent`**; `validate_scene` / bridge validation also flags **existing cycles** in imported or legacy scenes (per-node `parentId` errors with a cycle path), beyond per-node Zod checks.
> 
> Regression tests cover site A↔B cycles in the store, `apply_patch`, and `validate_scene`.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit c5b51f5121fa2eae9b84f7ef3cd4ff7948dd9855. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

[pascal]

I hit an error while handling your request (Model unavailable on AI Gateway free tier: Free tier users do not have access to this model. Upgrade to paid credits at https://vercel.com/d?to=%2F%5Bteam%5D%2F%7E%2Fai%3Fmodal%3Dtop-up for unrestricted…).

Please try again, rephrase, or reach out if it keeps failing.

Error id: 235eccc9-a8c2-434e-9271-1e6a05796267

[ovurrsl]

Architectural Review: PR #980 (fix: prevent cyclic scene hierarchies)

Layer & Package Boundary Analysis

• packages/core (lib/hierarchy.ts, store/actions/node-actions.ts): Pure domain logic and graph algorithms with zero dependencies on Three.js, R3F, viewer, or editor packages. Complies fully with Invariant E-001.
• packages/mcp (bridge/scene-bridge.ts): Consumes findHierarchyCycles from @pascal-app/core cleanly to report diagnostics in validateScene().
• Composition & Registry: Respects the registry-driven model; hierarchy constraints remain orthogonal to node kind definitions.

---

Findings

Blockers: 0

None. The implementation cleanly adheres to all package boundaries and store invariants.

Suggestions: 1

• packages/core/src/store/actions/node-actions.ts:
  During multi-node batch operations (applyNodeChangesActionImpl and createNodesActionImpl), nextNodes is updated incrementally as each node is processed. The step-by-step invocation of wouldCreateHierarchyCycle(newNode.id, effectiveParentId, nextNodes) guarantees that both direct self-references and intra-batch multi-node loops are caught before commit.
  Suggestion: Consider adding a unit test specifically asserting that a 2-node circular swap within a single applyNodeChanges batch call is cleanly rejected with the exact error message.

Nits: 0

None. Code is concise, types are strict (NodeMap pick on parentId), and time complexity ($O(D)$ for incremental checks, $O(N)$ for global validation) is optimal.

---

Verification Summary

• Algorithmic Soundness: The dual approach — fast $O(D)$ ancestor walk for live mutations (wouldCreateHierarchyCycle) paired with linear $O(N)$ cycle detection for scene imports (findHierarchyCycles) — provides optimal performance without regression risks to high-frequency transform updates.
• Regression Coverage: reparent.test.ts and apply-patch-identity.test.ts test suites provide clean regression locks.

Verdict: Ready to merge (Fully complies with Pascal architectural guidelines).

[Aymericr]

Thanks @Vansh0Sharma, this looks good. The guard covers every reparent and create path in core and MCP, and validate_scene now catches existing cycles. The regression tests are solid too. The fork's CI is waiting on maintainer approval, and I'll merge once it's green. If it conflicts with main by then, please rebase.

[Aymericr]

Hi @Vansh0Sharma, heads up: #1027 (detached scene runtime) just merged into main and this PR now has merge conflicts. It reworked packages/mcp/src/bridge/scene-bridge.ts, packages/mcp/src/tools/patch-guards.ts and packages/core/src/store/actions/node-actions.ts, which are the files your cycle guard touches. MCP patches are now computed on a detached copy and applied with dependency checks, so the parentId cycle check may need to move to where the new runtime validates patches (see packages/core/src/runtime/ and wiki/architecture/scene-runtime.md).

Could you rebase on the latest main, port the guard, and make sure the validate_scene and reparent regression tests still pass? The fix for #975 is still very much wanted, thanks!

[Aymericr]

Thank you @Vansh0Sharma! Your cycle-guard fix was ported onto current main in #1234 with credit to you, which fixes #975. Closing this one as superseded by that merge. Thanks for the contribution!