fix(lint): guard walkPageComponents against component cycles

.changeset/page-walk-cycle-guard.md

@@ -0,0 +1,48 @@
+---
+"@objectstack/lint": patch
+---
+
+fix(lint): guard `walkPageComponents` against component cycles (#13217)
+
+`walkPageComponents` — the one shared page-component traversal under every
+page-shaped lint rule and the CLI's i18n object-sections pass — descended the
+untyped composition slots inside `properties` with no cycle guard. Every one of
+those slots is `z.array(z.unknown())` authored data, so a component whose
+`properties.children` contains itself is **legal input**, and feeding one in
+recursed until the stack died with `RangeError: Maximum call stack size
+exceeded`. Because the walk is shared rather than copied, that crash was not
+scoped to one rule: it took every rule standing on the walk down in the same
+process.
+
+The descent now carries an **ancestor set** — the node is added before
+descending and removed on the way out — so a node that is its own ancestor
+stops the descent. Measured on the shapes that matter: a direct self-reference,
+an indirect cycle (`A -> B -> A`) and a longer chain (`A -> B -> C -> A`) all
+terminate, through every descended slot (`properties.children`,
+`properties.items[].children`, `properties.body`, `properties.footer`).
+
+Two deliberate non-changes, both pinned:
+
+- **An ancestor set, not a visited set.** A component object placed twice as a
+ *sibling*, or reached down two different branches, is legitimate re-use at two
+ distinct config paths, and every rule built on this walk must see both
+ placements. A visited set would yield the first and silently drop the rest —
+ trading a loud crash for missing lint coverage. This matches the predicate the
+ sibling resolver `translatePage` already settled on.
+- **No depth cap.** A cap and a cycle guard are different instruments. On a
+ resolver a cap leaves copy untranslated; on a lint walk it would drop real
+ components from the walk output and every rule would go quiet about them — a
+ silent truncation that reads exactly like a clean page. With the cycle guard
+ the descent is bounded by the document's own finite nesting, so a cap could
+ only ever fire on acyclic input, which is the input it must not truncate.
+
+The guard is silent: a cycle stops the descent and yields nothing extra, and no
+finding or warning is produced. Deciding that a self-referential page is itself
+an authoring error would be new reject behaviour on authored input, which is a
+contract call and not this walk's to make. Measured on a cyclic-but-otherwise
+valid page, all six rules that route through the walk report exactly what they
+report for the equivalent acyclic document (zero findings either way).
+
+No authored page in this repo carries such a cycle — swept across 57
+page-shaped objects with a positive control, zero hits — so this fixes a
+reachable crash, not an active incident.

packages/lint/src/page-walk.test.ts

@@ -140,6 +140,106 @@ describe('walkPageComponents — object binding precedence', () => {
  });
 });
 
+describe('walkPageComponents — cycle guard', () => {
+ // `properties.children` is `z.array(z.unknown())`, so a component that
+ // contains itself is LEGAL input. Before the guard each of these died with
+ // `RangeError: Maximum call stack size exceeded`, taking down every rule
+ // built on this walk in the same process.
+
+ it('terminates on an INDIRECT cycle (A -> B -> A), yielding each node once', () => {
+ // The load-bearing case: a guard that only compares a node against its
+ // immediate parent still recurses forever here, so this is the fixture a
+ // direct-only half-fix fails.
+ const a: Record<string, unknown> = { type: 'a', properties: {} };
+ const b: Record<string, unknown> = { type: 'b', properties: {} };
+ (a.properties as Record<string, unknown>).children = [b];
+ (b.properties as Record<string, unknown>).children = [a];
+
+ expect(paths({ regions: [{ name: 'main', components: [a] }] })).toEqual([
+ 'pages[0].regions[0].components[0]',
+ 'pages[0].regions[0].components[0].properties.children[0]',
+ ]);
+ });
+
+ it('terminates on a DIRECT self-reference', () => {
+ const self: Record<string, unknown> = { type: 'a', properties: {} };
+ (self.properties as Record<string, unknown>).children = [self];
+
+ expect(paths({ regions: [{ name: 'main', components: [self] }] })).toEqual([
+ 'pages[0].regions[0].components[0]',
+ ]);
+ });
+
+ it('terminates on a cycle through a longer chain (A -> B -> C -> A)', () => {
+ const a: Record<string, unknown> = { type: 'a', properties: {} };
+ const b: Record<string, unknown> = { type: 'b', properties: {} };
+ const c: Record<string, unknown> = { type: 'c', properties: {} };
+ (a.properties as Record<string, unknown>).children = [b];
+ (b.properties as Record<string, unknown>).children = [c];
+ (c.properties as Record<string, unknown>).children = [a];
+
+ expect(paths({ regions: [{ name: 'main', components: [a] }] })).toHaveLength(3);
+ });
+
+ it('guards every descended slot, not just `properties.children`', () => {
+ // items[].children (`page:tabs`), body and footer (`page:card`) all recurse
+ // through the same `visit`, so each needs the guard to hold.
+ const viaItems: Record<string, unknown> = { type: 'tabs', properties: {} };
+ (viaItems.properties as Record<string, unknown>).items = [{ children: [viaItems] }];
+
+ const viaBody: Record<string, unknown> = { type: 'card', properties: {} };
+ (viaBody.properties as Record<string, unknown>).body = [viaBody];
+
+ const viaFooter: Record<string, unknown> = { type: 'card', properties: {} };
+ (viaFooter.properties as Record<string, unknown>).footer = [viaFooter];
+
+ for (const node of [viaItems, viaBody, viaFooter]) {
+ expect(paths({ regions: [{ name: 'main', components: [node] }] })).toHaveLength(1);
+ }
+ });
+
+ it('is an ANCESTOR guard, not a visited set — a re-used sibling is yielded twice', () => {
+ // The same component object placed twice under one parent is two
+ // legitimate placements at two distinct config paths, and every rule built
+ // on this walk must see both. A visited-set guard would yield the first
+ // and silently drop the second, trading the crash for missing coverage.
+ const leaf: Record<string, unknown> = { type: 'leaf' };
+ const parent = { type: 'flex', properties: { children: [leaf, leaf] } };
+
+ expect(paths({ regions: [{ name: 'main', components: [parent] }] })).toEqual([
+ 'pages[0].regions[0].components[0]',
+ 'pages[0].regions[0].components[0].properties.children[0]',
+ 'pages[0].regions[0].components[0].properties.children[1]',
+ ]);
+ });
+
+ it('re-uses the same node on a SEPARATE branch — it is not an ancestor there', () => {
+ // A shared sub-tree reached down two different branches is legal re-use.
+ // The ancestor set must be popped on the way out, or the second branch
+ // would be silently truncated.
+ const shared: Record<string, unknown> = { type: 'shared' };
+ const left = { type: 'flex', properties: { children: [shared] } };
+ const right = { type: 'flex', properties: { children: [shared] } };
+
+ expect(paths({ regions: [{ name: 'main', components: [left, right] }] })).toEqual([
+ 'pages[0].regions[0].components[0]',
+ 'pages[0].regions[0].components[0].properties.children[0]',
+ 'pages[0].regions[0].components[1]',
+ 'pages[0].regions[0].components[1].properties.children[0]',
+ ]);
+ });
+
+ it('does NOT truncate a legal deep tree — this is a cycle guard, not a depth cap', () => {
+ // A cap would bound a legal-but-deep document by dropping components from
+ // the walk, and every rule would go quiet about them. Nesting well past
+ // the sibling resolver's cap of 32 stays fully walked.
+ let node: Record<string, unknown> = { type: 'leaf' };
+ for (let i = 0; i < 64; i++) node = { type: 'flex', properties: { children: [node] } };
+
+ expect(paths({ regions: [{ name: 'main', components: [node] }] })).toHaveLength(65);
+ });
+});
+
 describe('isSourceAuthoredPage', () => {
  it('treats html/react/jsx as source-authored and skips their regions', () => {
  for (const kind of ['html', 'react', 'jsx']) {

packages/lint/src/page-walk.ts

@@ -63,15 +63,50 @@ export function isSourceAuthoredPage(page: AnyRec): boolean {
  * path and resolved object binding. Source-authored pages yield nothing.
  *
  * `pagePath` is the caller's path prefix for the page (e.g. `pages[3]`).
+ *
+ * The descent is cycle-safe. Every composition slot below is `z.array(z.unknown())`
+ * authored data, so a component that contains itself — directly, or through a
+ * chain of containers — is LEGAL input, and an unguarded walk recurses until the
+ * stack dies. That crash is not scoped to one rule: this is the one shared
+ * traversal under every page-shaped lint rule and the CLI's i18n object-sections
+ * pass, so it takes all of them down in the same process.
+ *
+ * The guard is an ANCESTOR set, not a visited set, and the difference is
+ * load-bearing rather than stylistic — it is the same predicate
+ * `translatePage` (`packages/spec/src/system/i18n-resolver.ts`) settled on. A
+ * component object reused twice as a SIBLING is two legitimate placements at two
+ * distinct config paths, and every rule built on this walk must see both; a
+ * visited set would yield the first and silently drop the second, converting a
+ * crash into missing lint coverage. Only a node that is its own ancestor is a
+ * cycle.
+ *
+ * A cycle stops the descent SILENTLY — the repeated node is not yielded a second
+ * time and no finding is produced. The guard is a safety property of the walk,
+ * not a verdict about the document: deciding that a self-referential page is
+ * itself an authoring error would be new reject behaviour on authored input, and
+ * that is a contract call, not this walk's to make.
+ *
+ * Deliberately NO depth cap, which is a different instrument (`translatePage`
+ * carries both). A cap bounds a legal-but-absurd document; on a resolver it
+ * leaves copy untranslated, but on a LINT walk it would drop real components
+ * from the walk output and every rule would go quiet about them — a silent loss
+ * of coverage that looks exactly like a clean page. With the cycle guard the
+ * descent is bounded by the document's own finite nesting, so the cap would only
+ * ever fire on acyclic input, which is precisely the input it must not truncate.
  */
 export function walkPageComponents(page: AnyRec, pagePath: string): WalkedComponent[] {
  const out: WalkedComponent[] = [];
  if (!isRec(page) || isSourceAuthoredPage(page)) return out;
 
  const pageObject = strName(page.object);
 
+ // The current descent path — ancestors only, removed again on the way out.
+ const ancestors = new Set<AnyRec>();
+
  const visit = (node: unknown, path: string, inheritedObject?: string) => {
  if (!isRec(node)) return;
+ // Cycle guard: this node is already an ancestor of itself.
+ if (ancestors.has(node)) return;
 
  // Per-element `dataSource` overrides the page object so one page can bind
  // several objects; an inline `properties.object` does the same for the
@@ -85,32 +120,37 @@ export function walkPageComponents(page: AnyRec, pagePath: string): WalkedCompon
 
  if (!props) return;
 
- // `page:tabs` / `page:accordion` — items[].children[]
- if (Array.isArray(props.items)) {
- for (let i = 0; i < props.items.length; i++) {
- const item = props.items[i];
- if (!isRec(item) || !Array.isArray(item.children)) continue;
- for (let c = 0; c < item.children.length; c++) {
- visit(item.children[c], `${path}.properties.items[${i}].children[${c}]`, objectName);
+ ancestors.add(node);
+ try {
+ // `page:tabs` / `page:accordion` — items[].children[]
+ if (Array.isArray(props.items)) {
+ for (let i = 0; i < props.items.length; i++) {
+ const item = props.items[i];
+ if (!isRec(item) || !Array.isArray(item.children)) continue;
+ for (let c = 0; c < item.children.length; c++) {
+ visit(item.children[c], `${path}.properties.items[${i}].children[${c}]`, objectName);
+ }
  }
  }
-}
- // Generic layout nesting — `properties.children[]`. Not in any props
- // schema, but it is how real pages compose layout containers (`type:
- // 'flex'` grids in the showcase command-center wrap every chart this way).
-// Omitting it hides whole sub-trees from every rule built on this walk.
-if (Array.isArray(props.children)) {
-for (let i = 0; i < props.children.length; i++) {
-visit(props.children[i], `${path}.properties.children[${i}]`, objectName);
+ // Generic layout nesting — `properties.children[]`. Not in any props
+// schema, but it is how real pages compose layout containers (`type:
+// 'flex'` grids in the showcase command-center wrap every chart this way).
+// Omitting it hides whole sub-trees from every rule built on this walk.
+ if (Array.isArray(props.children)) {
+ for (let i = 0; i < props.children.length; i++) {
+ visit(props.children[i], `${path}.properties.children[${i}]`, objectName);
+}
  }
-}
-// `page:card` — body[] / footer[]
-for (const key of ['body', 'footer'] as const) {
-const slotList = props[key];
-if (!Array.isArray(slotList)) continue;
-for (let i = 0; i < slotList.length; i++) {
-visit(slotList[i], `${path}.properties.${key}[${i}]`, objectName);
+ // `page:card` — body[] / footer[]
+ for (const key of ['body', 'footer'] as const) {
+ const slotList = props[key];
+ if (!Array.isArray(slotList)) continue;
+ for (let i = 0; i < slotList.length; i++) {
+ visit(slotList[i], `${path}.properties.${key}[${i}]`, objectName);
+}
  }
+ } finally {
+ ancestors.delete(node);
  }
  };