fix(site-builder): repair cyclic orphan clusters in repairOrphanNodes

Review found that a cluster of orphan nodes referencing only each other
(no member's parent points outside the orphan set) made the reattach
loop find zero tops and silently no-op, leaving the cluster unreachable
while reporting repaired: []. Replaced the single-pass reattach with a
loop that re-derives the unreachable set each round and force-reattaches
one representative when no ordinary top exists, guaranteeing
findUnreachableNodeIds is empty after repair. Adds 2-node/3-node cycle
and mixed ordinary-subtree-plus-cycle tests; the original 11 tests are
unchanged and still pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

craft/src/utils/orphan-repair.test.ts

@@ -103,4 +103,53 @@ describe('repairOrphanNodes', () => {
     expect(out.state).toBe(orphanOnly);
     expect(out.repaired).toEqual([]);
   });
+
+  // A cluster of orphans whose `parent` fields (and child lists) reference
+  // only each other has no member pointing "outside" the cluster, so the
+  // simple "reattach the top" rule finds no top at all. Regression coverage
+  // for the invariant: after repair, nothing in the returned state may still
+  // be unreachable -- see the loop comment in the implementation.
+
+  test('a 2-node orphan cycle is fully reachable after repair', () => {
+    const nodes = JSON.parse(healthy);
+    nodes.x = node({ parent: 'y', nodes: ['y'] });
+    nodes.y = node({ parent: 'x', nodes: ['x'] });
+    const out = repairOrphanNodes(JSON.stringify(nodes));
+
+    expect(out.repaired.length).toBeGreaterThan(0);
+    const parsed = JSON.parse(out.state);
+    expect(findUnreachableNodeIds(parsed)).toEqual([]);
+  });
+
+  test('a 3-node orphan cycle is fully reachable after repair', () => {
+    const nodes = JSON.parse(healthy);
+    nodes.p = node({ parent: 'r', nodes: ['q'] });
+    nodes.q = node({ parent: 'p', nodes: ['r'] });
+    nodes.r = node({ parent: 'q', nodes: ['p'] });
+    const out = repairOrphanNodes(JSON.stringify(nodes));
+
+    expect(out.repaired.length).toBeGreaterThan(0);
+    const parsed = JSON.parse(out.state);
+    expect(findUnreachableNodeIds(parsed)).toEqual([]);
+  });
+
+  test('an ordinary orphan subtree and a separate orphan cycle in the same document are both repaired', () => {
+    const nodes = JSON.parse(healthy);
+    nodes.stray = node({ parent: 'ghost', nodes: ['strayChild'] });
+    nodes.strayChild = node({ parent: 'stray' });
+    nodes.x = node({ parent: 'y', nodes: ['y'] });
+    nodes.y = node({ parent: 'x', nodes: ['x'] });
+    const out = repairOrphanNodes(JSON.stringify(nodes));
+
+    const parsed = JSON.parse(out.state);
+    expect(findUnreachableNodeIds(parsed)).toEqual([]);
+    // The ordinary subtree keeps its established "only the top is
+    // reattached" behaviour: stray is reparented, strayChild rides along
+    // untouched.
+    expect(out.repaired).toContain('stray');
+    expect(parsed.strayChild.parent).toBe('stray');
+    // Exactly one representative per component is force-reattached: stray
+    // (found via the normal rule) plus one of x/y (via the cycle fallback).
+    expect(out.repaired.length).toBe(2);
+  });
 });

craft/src/utils/orphan-repair.ts

@@ -43,7 +43,11 @@ export function findUnreachableNodeIds(nodes: Record<string, any>): string[] {
 
 /** Reattach every unreachable node to the end of ROOT.nodes. Returns the
  *  possibly-rewritten serialized state and the ids that were moved. Never
- *  throws: malformed input comes back unchanged with an empty `repaired`. */
+ *  throws: malformed input comes back unchanged with an empty `repaired`.
+ *
+ *  Invariant: for any input that parses and has a ROOT, calling
+ *  `findUnreachableNodeIds` on the returned `state` (parsed) always yields
+ *  `[]` -- there is no orphan configuration this leaves half-repaired. */
 export function repairOrphanNodes(serialized: string): { state: string; repaired: string[] } {
   let nodes: Record<string, any>;
   try {
@@ -58,22 +62,51 @@ export function repairOrphanNodes(serialized: string): { state: string; repaired
     return { state: serialized, repaired: [] };
   }
 
-  const unreachable = findUnreachableNodeIds(nodes);
+  let unreachable = findUnreachableNodeIds(nodes);
   if (unreachable.length === 0) return { state: serialized, repaired: [] };
 
-  // Reattach only the TOP of each orphan subtree. An orphan whose parent is
-  // itself an orphan keeps its existing parent and rides along.
+  if (!Array.isArray(nodes[ROOT_ID].nodes)) nodes[ROOT_ID].nodes = [];
+  const repaired: string[] = [];
+
+  // Loop because a single pass can leave orphan CYCLES untouched: if every
+  // member of a cluster points only at other members of that same cluster,
+  // none of them has a parent pointing "out", so nothing qualifies as a top
+  // and a one-shot pass would report `repaired: []` while the cluster is
+  // still unreachable. Each iteration re-derives `unreachable` from the
+  // current (partially repaired) state and terminates once it's empty --
+  // this is what makes the invariant hold rather than just being hoped for.
+  while (unreachable.length > 0) {
     const orphanSet = new Set(unreachable);
-  const tops = unreachable.filter((id) => {
+
+    // Reattach the TOP of each orphan subtree: a node whose `parent` points
+    // outside the current orphan set (to something real, to nothing, or is
+    // null). Its existing child list carries the rest of its subtree along
+    // for free once BFS can reach it again.
+    let tops = unreachable.filter((id) => {
       const parent = nodes[id]?.parent;
       return !(typeof parent === 'string' && orphanSet.has(parent));
     });
 
-  if (!Array.isArray(nodes[ROOT_ID].nodes)) nodes[ROOT_ID].nodes = [];
+    // No such node exists only when every remaining orphan's `parent`
+    // points at another orphan -- i.e. a closed cycle (2+ nodes referencing
+    // only each other). There is no legitimate "outside" anchor to prefer,
+    // so break the cycle by force-reattaching one representative member
+    // (first in iteration order, for determinism). Because a cycle is
+    // strongly connected via the actual nodes/linkedNodes edges, reattaching
+    // any single member pulls the rest of that cycle in on the next
+    // `findUnreachableNodeIds` pass without touching their `parent` fields.
+    if (tops.length === 0) {
+      tops = [unreachable[0]];
+    }
+
     for (const id of tops) {
       nodes[id].parent = ROOT_ID;
       nodes[ROOT_ID].nodes.push(id);
+      repaired.push(id);
     }
 
-  return { state: JSON.stringify(nodes), repaired: tops };
+    unreachable = findUnreachableNodeIds(nodes);
+  }
+
+  return { state: JSON.stringify(nodes), repaired };
 }