Skip to content

fix(lint): guard walkPageComponents against component cycles - #13236

Open
os-elon wants to merge 1 commit into
mainfrom
claude/issue-13217-page-walk-cycle-guard
Open

fix(lint): guard walkPageComponents against component cycles#13236
os-elon wants to merge 1 commit into
mainfrom
claude/issue-13217-page-walk-cycle-guard

Conversation

@os-elon

Copy link
Copy Markdown
Collaborator

Fixes#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. Feeding one in recursed until the stack died, and because the walk is shared rather than copied, that crash was not scoped to one rule.

The card's landing path was wrong, and so was triage's correction

The card body names packages/lint/src/page-walk.ts. The triage grading comment corrected that to packages/lint/src/page-envelope-audit.ts. Re-anchored from the tree: the card body was right and the correction is not.

$ git grep -n "function walkPageComponents" -- '*.ts'
packages/lint/src/page-walk.ts:67:export function walkPageComponents(...)

page-envelope-audit.ts:102 only imports it (from './page-walk.js'), and index.ts:718 re-exports it from the same module. There is exactly one definition site. The card's own stack trace (visit … page-walk.ts:104) also lands on the properties.children recursion at that line, in this tree, today.

Premise, measured before any edit

inputbeforeafter
direct self-referenceRangeError: Maximum call stack size exceeded at visit1 component
indirect cycle A -> B -> ARangeError: Maximum call stack size exceeded at visit2 components
sibling re-use of one object (legal)3 components3 components (unchanged)
acyclic nesting, depth 4041 components41 components (unchanged)

What shipped: a cycle guard, NOT a depth cap

These are different instruments and the card asked for the choice to be stated. This ships the cycle guard only.

A cap bounds a legal-but-absurd document. On a resolver that 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 already bounded by the document's own finite nesting, so a cap could only ever fire on acyclic input, which is precisely the input it must not truncate. The depth 40 row above is pinned as a test, and the suite pins depth 64 — past the sibling resolver's cap of 32.

An ancestor set, not a visited set

The guard adds the node before descending and removes it on the way out, so only a node that is its own ancestor stops the descent. This is the predicate the sibling translatePage (packages/spec/src/system/i18n-resolver.ts:1048) already settled on, and its reason applies here with more force: 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. Both shapes are pinned as tests.

Reverse verification

Both ablations were run from the committed fix, each proved on disk before the run (anchor grep counts plus git hash-object against the HEAD blob), each restored by an EXIT INT TERM trap that re-verified the restored blob equals the HEAD blob. The test imports ./page-walk.js — a relative, same-package specifier that resolves to src/, not dist/ — so no rebuild is interposed, which the ablations demonstrate by picking up each mutation with no build step.

ablationresult
A — guard removed entirely4 failed / 12 passed, 3 RangeError
B — the half-fix: direct self-reference only3 failed / 13 passed, 3 RangeError

The A-to-B delta is the point: under the direct-only half-fix, terminates on a DIRECT self-referencepasses while terminates on an INDIRECT cycle (A -> B -> A) and the longer chain still die. The pin is on the indirect case, which is the one a half-fix survives.

No new reject behaviour

Measured, because this was the one conditional the card reserved: 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.

validate-component-props cyclic=0 acyclic=0
validate-chart-bindings cyclic=0 acyclic=0
validate-action-name-refs cyclic=0 acyclic=0
validate-visibility-predicates cyclic=0 acyclic=0
validate-translatable-sections cyclic=0 acyclic=0
validate-component-types cyclic=0 acyclic=0

The guard is silent by construction: a cycle stops the descent and yields nothing extra. 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.

Blast radius, measured from the tree

The card named six rules plus the CLI pass. The real consumer set is larger: ten lint modules call the walk, not six — the card did not name validate-component-types, validate-page-field-bindings or validate-translation-references, and all three route through it. So the p1 argument holds and then some.

One member of the named six behaves differently, and it is not fixed by this PR. page-envelope-audit runs collectBare at Door 1 (line 264) before it reaches walkPageComponents (line 276), and collectBare is a separate lockstep raw/parsed value walker with its own unguarded recursion. On a cyclic page it dies at line 264 and never reaches the walk at all. Its membership in the blast-radius list is real; the cause is not the missing walk guard. Filed separately as #13235 with the measurement — a third walk, out of scope both here and for #13218, and its guard shape is a genuine design call rather than a mechanical copy. #13235 is not addressed here.

No authored page in this repo carries such a cycle: swept 57 page-shaped objects across 34 modules with a live positive control, zero hits. This fixes a reachable crash, not an active incident.

Gates

All run at the final commit c79006ebbe, exit codes captured before any pipe.

Derived by node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (27 matched families + 6 convention-triggered by the added test file). All green, including check:cross-package-test-inputs, check:test-source-alias, check:page-declaration-shape, check:engine-double-contract, check:where-matcher, check:query-options-erasure, check:type-check-coverage, check:changeset-gate-self-tests.

Two needed the built closure, so the closure was built (turbo run build, 70/70 successful) and both then measured green:

  • check:type-check-coverage --re-measure: OK — 30 ledger entr(ies) re-measured in 242.9s, 1560 raw tsc error(s) total, none above its recorded number.
  • check:dual-build-cjs-loads — 102 published require entry point(s) across 66 package(s) load; 610 emitted CommonJS file(s) parse.

Tests:

  • pnpm --filter @objectstack/lint test82 files, 2334 tests passed
  • pnpm --filter @objectstack/cli exec vitest run test/i18n-extract.test.ts test/platform-page-i18n-parity.test.ts test/i18n-extract-emitted-files.test.ts3 files, 22 tests passed (the named cross-package consumer)
  • pnpm --filter @objectstack/lint typecheck — exit 0

Two readings recorded as NOT MEASURED rather than green:

ESLint was narrowed rather than swept, and the narrowing is measured: 2 files linted per --format json, 0 errors, 0 warnings, with the population taken from eslint's own config. The repo runs one eslint.config.mjs that never enables type-aware linting for any file (documented at eslint.config.mjs:327-329), so this diff cannot move the verdict on any file it does not touch. The repo-wide pnpm lint is CI's run.

packages/lint/src/validate-jsx-pages.ts is untouched (held by #13154). ⛔ The two walks are not unified — that is #13218's scope and needs a prior decision about which walk gets exported.

Generated by Claude Code


Generated by Claude Code

@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 5a9b7a0b104b0434ec04a763aab739b717add76cpackageMentionDocs.

Which tree this was computed on

This run read content/docs from ab5c66105a36c7834d2ae57d54c58362d2eea8e2 — the merge of head c79006ebbe38c10a7047c642ca83e558f1b95e29 into base 5a9b7a0b104b0434ec04a763aab739b717add76c, 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 ab5c66105a36c7834d2ae57d54c58362d2eea8e2 && git checkout ab5c66105a36c7834d2ae57d54c58362d2eea8e2
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 5a9b7a0b104b0434ec04a763aab739b717add76c c79006ebbe38c10a7047c642ca83e558f1b95e29 && git checkout -B drift-repro 5a9b7a0b104b0434ec04a763aab739b717add76c && git merge --no-ff c79006ebbe38c10a7047c642ca83e558f1b95e29
node scripts/docs-audit/affected-docs.mjs --json 5a9b7a0b104b0434ec04a763aab739b717add76c

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

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 29, 2026
@os-elon
os-elon marked this pull request as ready for review August 29, 2026 14:53
@os-elon
os-elon enabled auto-merge August 29, 2026 14:53
@os-elon
os-elon added this pull request to the merge queueAug 29, 2026
@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to no response for status checks Aug 29, 2026
@os-elonClaude

Copy link
Copy Markdown
CollaboratorAuthor

⚠️One explicit check requested before this merges — not a bug report.

PR #13289 (#13235, the collectBare guard on Door 1) independently reached the same ancestor-set-not-visited-set conclusion as this PR. Independent convergence is a good sign and I am recording it as such.

It also ran an ablation this PR's owner may want to see. Ablation C narrowed its ancestor set to records only — the shape this PR uses — and the array-only ring still died with RangeError (1 failed / 8 passed).

That is not a defect here: walkPageComponents's visit() only ever recurses on records, so a record-typed set is correct for this walker. The request is only that it be checked once rather than inherited — if visit() can reach an array-shaped ring by any path, a record-only ancestor set is measurably insufficient, and #13289's ablation is the ready-made witness.

⚠️Second thing, and it matters for how a green run here is read:#13289 measured that on a cyclic page Door 1 (collectBare) dies FIRST, at page-envelope-audit.ts:264, before walkPageComponents at :276. So this PR landing alone does not make auditPageExpressionEnvelopes terminate — the crash moves rather than clears. #13217's card body attributes the audit rule's death to the missing walk guard; the measurement says both halves are required. ⛔ Do not read a merged #13236 as closing the audit path.

No action needed on the sequencing: the two PRs have no code dependency in either direction and should land independently.


Generated by Claude Code

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