Skip to content

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

Draft
os-elon wants to merge 2 commits into
mainfrom
claude/issue-13235-collectbare-cycle-guard
Draft

fix(lint): guard collectBare's recursion so a self-referential page terminates instead of killing the stack#13289
os-elon wants to merge 2 commits into
mainfrom
claude/issue-13235-collectbare-cycle-guard

Conversation

@os-elon

Copy link
Copy Markdown
Collaborator

Fixes#13235

What was wrong

collectBare in packages/lint/src/page-envelope-audit.ts 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 is input the schema admits: properties is
z.record(z.unknown()) and properties.children is z.array(z.unknown()), so
PageSchema.safeParse succeeds and the walk then recursed until the stack died. Measured at
the base commit 74049254d4, all four layers on one A -> B -> A page:

safeParseSuccess: true <- legal authored input, not a malformed document
collectBare: RangeError: Maximum call stack size exceeded
walkPageComponents: RangeError: Maximum call stack size exceeded
audit end-to-end: RangeError: Maximum call stack size exceeded

Door 1 runs collectBare over the whole page (line 264) before the shared walk (line 276),
so this is the first thing that dies.

The fix

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. Three properties are
load-bearing, and each is pinned by a test whose necessity was measured by ablation rather
than argued:

propertywhy it is that wayablation: what breaks without it
ancestor set, not a visited seta visited set also skips a merely SHARED subtree — the same component literal referenced from two slots is legal, acyclic authoring — and silently drops its findingsnever removing on exit turns the set into a visited set: the report-neutrality control fails, and only that one
tracks raw, notparsediteration is driven by the authored side, so every recursive call descends one level in raw; bounding raw's simple-path depth bounds the recursion whatever shape parsed hasn/a — this is why one set suffices
holds any object identity, not just recordscollectBare descends into arrays as values, so an array that contains itself is a cycle carrier with no record on the ringnarrowing the set to records only (an AnyRec set) still dies with RangeError on the array-only ring

That third row is the reason this is not a reuse of the shared walk's guard. The guard
armed on the walkPageComponents card (PR #13236, not merged at this branch point) reaches
the same ancestor-set conclusion independently, but its set holds records only (AnyRec) — correct there,
because that walk only ever recurses on records. Here a record-only set is measurably
insufficient. So this carries a local guard by necessity, not by duplication, and there is
no dependency on unmerged code.

Verification

Everything below was run at the final commit 040433a848.

  • pnpm --filter @objectstack/lint test82 files, 2331 passed.
  • The three real consuming gates, over the actual authored corpus:
    platform-objects 20, mcp 16, cloud-connection 11 — 47 passed. All three alias
    @objectstack/lint to source, so they ran against the patched walker. This is the
    report-neutrality measurement on real pages, not on fixtures.
  • pnpm lint (eslint . --no-inline-config, whole repo) — clean, 49s. No narrowing claimed.
  • pnpm --filter @objectstack/lint typecheck — exit 0. ⚠️ That program excludes
    **/*.test.ts, so it says nothing about the test edits; re-run with tests included, the
    count is 19, exactly the figure ledgered for this package in
    scripts/check-type-check-coverage.mjs, and 0 of the 19 are in either edited file.
  • Derived gate family (node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack,
    28 matched + convention-triggered): all green. check:nul-bytes,
    check:cross-package-test-inputs, check:test-source-alias, check:where-matcher,
    check:engine-double-contract, check:query-options-erasure, check:type-check-coverage,
    check:published-files, check:page-declaration-shape, check:slot-lookup,
    check:type-source-resolution, and the changeset family.
  • pnpm --filter @objectstack/lint build — dts emitted 2/2, built CJS loads.

Three gates are NOT MEASURED locally, by their own verdict lines, and are left to CI:
check:test-completeness and check:dual-build-cjs-loads (both exit 3, PREREQUISITE NOT
MET — they need a saved turbo log and a full workspace build respectively) and
check:pm-half-states (exit 3, no usable GitHub credential in this container). None of the
three is a red.

Ablations restored by byte proof each time: restored blob hash compared against the HEAD
blob and git diff HEAD empty, under an EXIT/INT/TERM trap with absolute paths.

Scope, and what this does not do

The card's fence is collectBare, and that is fully discharged: door 1 terminates and still
reports every finding on a simple path.

⚠️auditPageExpressionEnvelopes end-to-end still stack-dies on the same input at this
branch point, because walkPageComponents carries its own separate unguarded recursion —
a different function and a different card (#13217, whose PR #13236 was still unmerged when
this branched). The regression tests here therefore drive door 1 in isolation, which is what
collectBare is exported for; asserting end-to-end termination here would be asserting
someone else's change, and would go red or green for reasons unrelated to this diff.
⛔ Nothing here reopens or supersedes that card, and this one is not covered by it.

Declarations

  • Contract accept/reject behaviour: UNCHANGED. No page that parses today reports
    anything different. The guard cannot fire on acyclic input, because no node is ever its
    own ancestor there — that is by construction, and it is the property the report-neutrality
    test pins. On a cyclic page nothing distinct is lost either: every node of a finite graph
    is reachable by a simple path, so each authored position is still visited, and what is
    dropped is only the infinite tail of re-reports at ever-longer paths.
  • Published surface: UNCHANGED.collectBare is package-internal and not re-exported
    from the barrel; confirmed mechanically against the built artifact — loading
    packages/lint/dist/index.js gives 'collectBare' in exports === false. The added
    parameter is optional and trailing, so every existing call site is untouched.
  • Severity is unchanged from triage's reading. A sweep of the authored corpus found zero
    cyclic pages, so this stays a reachable crash on legal input rather than a live incident.

Open question for review, deliberately not decided here

The truncation is silent — a cyclic page reports its findings with no signal that a
descent stopped. That is defensible (unlike a door that could not open, this one read
everything there was to read, and every distinct position is still visited), and it is why
this ships as report-neutral. But this module's own doctrine is that preconditions are
reported and never assumed, and surfacing truncation would mean adding a channel to
PageEnvelopeAudit, which consumers are required to assert. That is a reporting-shape
choice on a published interface, so it is left to a contract call rather than taken in this
PR.

Generated by Claude Code


Generated by Claude Code

collectBare is a lockstep raw/parsed walker separate from walkPageComponents
and carried no cycle guard, so a page whose component tree contains itself --
input PageSchema accepts, since properties is z.record(z.unknown()) -- killed
the stack at door 1 before any other door ran.
The guard is an ancestor set on the authored side: added on entry, removed on
exit, so a node is skipped only when it is its own ancestor. That makes it
report-neutral on acyclic input by construction, where a visited-set would
have silently dropped findings from merely shared subtrees.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPrUz21stTFhJRUirdc4yw
The previous fixture put a record back on the ring, so a record-only ancestor
set would have caught it too. An array that contains ITSELF has no record on
the cycle, which is the shape that actually discriminates: walkPageComponents
only recurses on records, while collectBare descends into arrays as values.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPrUz21stTFhJRUirdc4yw
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

1 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 74049254d47bd0edd2a2fcd732dcc01c91504f10packageMentionDocs.

Which tree this was computed on

This run read content/docs from 09804ee2ba1c5472d4419e4da90afbc6f47eb2e9 — the merge of head 040433a84819340c6a3e59ac6a2812b8c547d72a into base 74049254d47bd0edd2a2fcd732dcc01c91504f10, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 09804ee2ba1c5472d4419e4da90afbc6f47eb2e9 && git checkout 09804ee2ba1c5472d4419e4da90afbc6f47eb2e9
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 74049254d47bd0edd2a2fcd732dcc01c91504f10 040433a84819340c6a3e59ac6a2812b8c547d72a && git checkout -B drift-repro 74049254d47bd0edd2a2fcd732dcc01c91504f10 && git merge --no-ff 040433a84819340c6a3e59ac6a2812b8c547d72a
node scripts/docs-audit/affected-docs.mjs --json 74049254d47bd0edd2a2fcd732dcc01c91504f10

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@github-actionsgithub-actionsBot added size/m documentation Improvements or additions to documentation tests tooling labels Aug 29, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-elon@claude