fix(lint): guard collectBare's recursion so a self-referential page terminates instead of killing the stack

.changeset/collectbare-cycle-guard.md

@@ -0,0 +1,34 @@
+---
+'@objectstack/lint': patch
+---
+
+fix(lint): guard `collectBare`'s recursion so a self-referential page terminates instead of killing the stack (#13235)
+
+`page-envelope-audit`'s `collectBare` is a lockstep raw/parsed value walker,
+separate from the shared `walkPageComponents` traversal, and it carried no cycle
+guard. A page whose component tree contains itself (`A -> B -> A` through
+`properties.children`) is input the schema **admits** — `properties` is
+`z.record(z.unknown())` and `properties.children` is `z.array(z.unknown())`, so
+`PageSchema.safeParse` succeeds — and door 1 then recursed until
+`RangeError: Maximum call stack size exceeded`. Door 1 runs over the whole page
+before any other door, so this is the first thing that died on such a page.
+
+The guard is an **ancestor set on the authored side**: objects are added on
+entry to the descent and removed on exit, so a node is skipped only when it is
+its own ancestor. Two consequences are pinned by tests:
+
+- **Report-neutral on acyclic input, by construction.** No node is ever its own
+ ancestor on an acyclic page, so the guard never fires and findings are
+ unchanged. A visited-set would instead have skipped merely *shared* subtrees —
+ the same component literal referenced from two slots is legal authoring — and
+ silently dropped their findings.
+- **Nothing distinct is lost on a cyclic page.** Every node of a finite graph is
+ reachable by a simple path, so each authored position is still visited; only
+ the infinite tail of re-reports at ever-longer paths is dropped.
+
+Cycles through arrays are covered as well as cycles through records.
+
+Scope: this is door 1 only. `walkPageComponents` carries its own separate
+unguarded recursion, so `auditPageExpressionEnvelopes` end-to-end still dies at
+the walk on the same input until that lands (#13217). No published type changes
+and no accept/reject behaviour changes on any page that parses today.

packages/lint/src/page-envelope-audit.test.ts

@@ -175,3 +175,120 @@ describe('negative control — the detector fires, and every door earns its plac
  expect(renderBareExpressionFindings(audit.findings)).toContain('deprecated key');
  });
 });
+
+/**
+ * Cycle guard on `collectBare` — termination, and the report-neutrality that
+ * makes the guard's SHAPE (ancestor set, not visited set) the load-bearing bit.
+ *
+ * `properties` is `z.record(z.unknown())` and `properties.children` is
+ * `z.array(z.unknown())`, so a self-referential component tree is input the
+ * schema ADMITS — `PageSchema.safeParse` succeeds on it, which is what makes
+ * this reachable rather than malformed. The first assertion below is the
+ * precondition: if the fixture ever stops parsing, the termination tests would
+ * pass for the wrong reason (door 1 never runs on an unparsed page).
+ *
+ * ⚠️ Scope: these pin DOOR 1 (`collectBare`) in isolation, deliberately, and
+ * not `auditPageExpressionEnvelopes` end-to-end. The shared `walkPageComponents`
+ * carries its own separate unguarded recursion, so the union entry point still
+ * stack-dies at the walk on the same input — a different function, a different
+ * card. Asserting end-to-end termination here would be asserting someone else's
+ * fix. `collectBare` is exported (module-level, not from the barrel) exactly so
+ * a single door can be driven like this.
+ */
+describe('cycle guard — a self-referential page terminates instead of killing the stack', () => {
+ /** `A -> B -> A` through the generic `properties.children` nesting. */
+ function cyclicChildrenPage(): AnyRec {
+ const a: AnyRec = { type: 'page:card', visibleWhen: BARE, properties: { children: [] as unknown[] } };
+ const b: AnyRec = { type: 'page:card', properties: { children: [a] } };
+ a.properties = { children: [b] };
+ return {
+ name: 'cyc_children',
+ label: 'Cyclic',
+ type: 'record',
+ object: 'sys_user',
+ template: 'default',
+ kind: 'slotted',
+ regions: [],
+ slots: { alerts: [a] },
+ };
+ }
+
+ const parsePage = (page: AnyRec) =>
+ (PageSchema as unknown as Parseable).safeParse(page);
+
+ it('PRECONDITION — the cyclic page is input `PageSchema` ACCEPTS, so door 1 really runs', () => {
+ const parsed = parsePage(cyclicChildrenPage());
+ expect(parsed.success).toBe(true);
+ });
+
+ it('terminates on a cycle through `properties.children`, still reporting the bare predicate above it', () => {
+ const page = cyclicChildrenPage();
+ const parsed = parsePage(page);
+ const out: BareExpressionFinding[] = [];
+
+ collectBare(page, parsed.data, '', 'cyc (cyc_children)', 'PageSchema', out);
+
+ // Terminating is the fix; reporting is the proof it terminated by GUARDING
+ // rather than by refusing to descend at all.
+ expect(out.map(f => f.path)).toEqual(['slots.alerts[0].visibleWhen']);
+ expect(out[0]!.authored).toBe(BARE);
+ });
+
+ it('terminates on a cycle that goes through an ARRAY rather than a record', () => {
+ // A ring with NO record on it: the array contains ITSELF. This is the
+ // shape that separates this walk from the shared component walk —
+ // `walkPageComponents` only ever recurses on records, so an ancestor set
+ // of records is enough there, while `collectBare` descends into arrays as
+ // values, which makes an array a cycle carrier in its own right. A
+ // record-only ancestor set still dies here, which is why the guard's set
+ // holds any object identity rather than just records.
+ const ring: unknown[] = [];
+ ring.push(ring);
+ const node: AnyRec = { type: 'page:card', visibleWhen: BARE, properties: { children: ring } };
+ const page: AnyRec = {
+ name: 'cyc_array',
+ label: 'Cyclic',
+ type: 'record',
+ object: 'sys_user',
+ template: 'default',
+ kind: 'slotted',
+ regions: [],
+ slots: { alerts: [node] },
+ };
+ const parsed = parsePage(page);
+ expect(parsed.success).toBe(true);
+
+ const out: BareExpressionFinding[] = [];
+ collectBare(page, parsed.data, '', 'cyc (cyc_array)', 'PageSchema', out);
+ expect(out.map(f => f.path)).toEqual(['slots.alerts[0].visibleWhen']);
+ });
+
+ it('REPORT-NEUTRALITY — a SHARED (acyclic) subtree is reported at BOTH positions, not deduped away', () => {
+ // This is the control that pins the guard's shape. The same component
+ // object is referenced from two slots — legal, acyclic authoring. An
+ // ancestor set never fires here (no node is its own ancestor) and both
+ // positions report. A visited-set guard would report only the first and
+ // silently drop the second, which is a real regression on real pages, not
+ // a cyclic-input edge case.
+ const shared: AnyRec = { type: 'record:alert', visibleWhen: BARE, properties: { severity: 'warning' } };
+ const page: AnyRec = {
+ name: 'shared_subtree',
+ label: 'Shared',
+ type: 'record',
+ object: 'sys_user',
+ template: 'default',
+ kind: 'slotted',
+ regions: [],
+ slots: { alerts: [shared, shared] },
+ };
+ const parsed = parsePage(page);
+ expect(parsed.success).toBe(true);
+
+ const out: BareExpressionFinding[] = [];
+ collectBare(page, parsed.data, '', 'shared (shared_subtree)', 'PageSchema', out);
+ expect(out.map(f => f.path)).toEqual([
+ 'slots.alerts[0].visibleWhen',
+ 'slots.alerts[1].visibleWhen',
+ ]);
+ });
+});

packages/lint/src/page-envelope-audit.ts

@@ -160,6 +160,40 @@ function issueText(error: { issues: ReadonlyArray<{ path: PropertyKey[]; message
  * materialized (defaults) have no authored counterpart and must not be read
  * as findings.
  *
+ * ## The cycle guard — ancestor-scoped, on the authored side
+ *
+ * `properties` is `z.record(z.unknown())` and `properties.children` is
+ * `z.array(z.unknown())`, so a page whose component tree contains itself
+ * (`A -> B -> A`) is input the schema ADMITS: `PageSchema.safeParse` succeeds
+ * and this walk then recursed until the stack died. Door 1 runs it over the
+ * WHOLE page before anything else, so the audit died HERE first — the shared
+ * walk's own guard is a different function and never got the chance to help.
+ *
+ * Two properties of the guard are load-bearing, and each is pinned by a test:
+ *
+ * - **It tracks the current descent path, not every object ever visited.** A
+ * visited-set would also skip a subtree that is merely SHARED — the same
+ * component literal referenced from two slots is legal, acyclic authoring —
+ * and would silently drop its findings. An ancestor set skips a node only
+ * when it is its own ancestor, which on acyclic input never happens, so the
+ * guard is report-neutral there by construction rather than by luck.
+ * - **It tracks `raw`, not `parsed`.** Iteration is driven by the authored
+ * side (above), so every recursive call descends one level in `raw`;
+ * bounding `raw`'s simple-path depth bounds the recursion whatever shape
+ * `parsed` has. Tracking `parsed` too would add nothing and would risk
+ * suppressing findings wherever a parse legitimately shares one object
+ * across positions.
+ *
+ * Cycles through arrays are covered as well as cycles through records: the set
+ * holds any object identity, so an array that contains itself terminates too.
+ *
+ * On a cyclic page the descent stops at the repeat and no DISTINCT position is
+ * lost — every node of a finite graph is reachable by a simple path, so each
+ * authored position is still visited; what is dropped is the infinite tail of
+ * re-reports at ever-longer paths. The truncation is not surfaced on any
+ * precondition channel: unlike a door that could not open, this one read
+ * everything there was to read.
+ *
  * @internal Package-internal (not re-exported from the barrel): a consumer
  * always wants the three-door union {@link auditPageExpressionEnvelopes};
  * this single-door primitive exists so `page-envelope-audit.test.ts` can
@@ -173,41 +207,54 @@ export function collectBare(
  page: string,
  door: EnvelopeAuditDoor,
  out: BareExpressionFinding[],
+ ancestors: Set<object> = new Set(),
 ): void {
  if (typeof raw === 'string') {
  if (isEnvelope(parsed) && parsed.source === raw) {
  out.push({ page, path, authored: raw, door });
  }
  return;
  }
- if (Array.isArray(raw)) {
- if (!Array.isArray(parsed)) return;
- for (let i = 0; i < raw.length; i++) {
- collectBare(raw[i], parsed[i], `${path}[${i}]`, page, door, out);
+ // Primitives carry no descent and no cycle; below here `raw` is an object.
+ if (raw === null || typeof raw !== 'object') return;
+
+ // ── Cycle guard ────────────────────────────────────────────────────────────
+ // `ancestors` holds the objects on the CURRENT descent path — added on entry,
+ // removed on exit. Both halves of that are load-bearing; see the doc above.
+ if (ancestors.has(raw)) return;
+ ancestors.add(raw);
+ try {
+ if (Array.isArray(raw)) {
+ if (!Array.isArray(parsed)) return;
+ for (let i = 0; i < raw.length; i++) {
+ collectBare(raw[i], parsed[i], `${path}[${i}]`, page, door, out, ancestors);
+ }
+ return;
  }
- return;
- }
- if (!isRec(raw) || !isRec(parsed)) return;
+ if (!isRec(raw) || !isRec(parsed)) return;
 
- for (const [key, value] of Object.entries(raw)) {
- const childPath = path ? `${path}.${key}` : key;
- const counterpart = parsed[key];
+for (const [key, value] of Object.entries(raw)) {
+const childPath = path ? `${path}.${key}` : key;
+const counterpart = parsed[key];
 
- // Deprecated-alias case: the parse consumed this key and re-homed its
- // value under the canonical name (`visibility` -> `visibleWhen`). A
- // key-parallel walk alone would see `undefined` on the parsed side and
- // move on, so the value is looked up by its own source text instead.
- if (counterpart === undefined && typeof value === 'string' && value.length > 0) {
- const renamed = Object.entries(parsed).find(
- ([, pv]) => isEnvelope(pv) && pv.source === value,
- );
- if (renamed) {
- out.push({ page, path: childPath, authored: value, normalizedTo: renamed[0], door });
- continue;
+ // Deprecated-alias case: the parse consumed this key and re-homed its
+ // value under the canonical name (`visibility` -> `visibleWhen`). A
+ // key-parallel walk alone would see `undefined` on the parsed side and
+ // move on, so the value is looked up by its own source text instead.
+ if (counterpart === undefined && typeof value === 'string' && value.length > 0) {
+ const renamed = Object.entries(parsed).find(
+ ([, pv]) => isEnvelope(pv) && pv.source === value,
+ );
+ if (renamed) {
+ out.push({ page, path: childPath, authored: value, normalizedTo: renamed[0], door });
+ continue;
+ }
  }
- }
 
- collectBare(value, counterpart, childPath, page, door, out);
+ collectBare(value, counterpart, childPath, page, door, out, ancestors);
+ }
+ } finally {
+ ancestors.delete(raw);
  }
 }