diff --git a/craft/src/utils/orphan-repair.test.ts b/craft/src/utils/orphan-repair.test.ts index 5110b8e..edb8630 100644 --- a/craft/src/utils/orphan-repair.test.ts +++ b/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); + }); }); diff --git a/craft/src/utils/orphan-repair.ts b/craft/src/utils/orphan-repair.ts index 3811a06..566be71 100644 --- a/craft/src/utils/orphan-repair.ts +++ b/craft/src/utils/orphan-repair.ts @@ -43,7 +43,11 @@ export function findUnreachableNodeIds(nodes: Record): 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; 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. - const orphanSet = new Set(unreachable); - const 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 = []; - for (const id of tops) { - nodes[id].parent = ROOT_ID; - nodes[ROOT_ID].nodes.push(id); + 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); + + // 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)); + }); + + // 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); + } + + unreachable = findUnreachableNodeIds(nodes); } - return { state: JSON.stringify(nodes), repaired: tops }; + return { state: JSON.stringify(nodes), repaired }; }