diff --git a/.changeset/collectbare-cycle-guard.md b/.changeset/collectbare-cycle-guard.md new file mode 100644 index 0000000000..01ab4d5f9f --- /dev/null +++ b/.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. diff --git a/packages/lint/src/page-envelope-audit.test.ts b/packages/lint/src/page-envelope-audit.test.ts index 1bced8dc28..bf46d41cb1 100644 --- a/packages/lint/src/page-envelope-audit.test.ts +++ b/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', + ]); + }); +}); diff --git a/packages/lint/src/page-envelope-audit.ts b/packages/lint/src/page-envelope-audit.ts index 1bf3b77910..2815833e1f 100644 --- a/packages/lint/src/page-envelope-audit.ts +++ b/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,6 +207,7 @@ export function collectBare( page: string, door: EnvelopeAuditDoor, out: BareExpressionFinding[], + ancestors: Set = new Set(), ): void { if (typeof raw === 'string') { if (isEnvelope(parsed) && parsed.source === raw) { @@ -180,34 +215,46 @@ export function collectBare( } 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); } }