fix(extensions): one AST depth bound, so the two walks over author source cannot diverge

[BIMvoice]

Two copies of MAX_AST_DEPTH bound walks over extension-author source, and moving one alone opens a band where one accepts what the other refuses.

packages/extensions/src/host/source-wrap.ts:156 held a private 1000 (from #3025); packages/extensions/src/ast/bounded-walk.ts:55 held another (from #3027, merged the same day). Both refuse a script past the bound.

The divergence band, reproduced directly

A throwaway probe exercising wrapEntrySource and validateCode on the same 300-level nested script:

shared constant	source-wrap.ts	wrapEntrySource	validateCode
1000	imports it (fixed)	accepts	accepts
100	imports it (fixed)	refuses (depth)	refuses (depth)
100	private 1000 (pre-fix)	accepts	refuses (depth)

The third row is the defect: before this change, moving the bound makes wrapEntrySource accept exactly what validateCode refuses. After it, that row is unreachable.

The value is unchanged, so no script changes verdict. source-wrap.test.ts:163 still passes asserting the literal 'Entry script is nested more than 1000 AST levels deep.'

A doc that overclaimed

bounded-walk.ts described itself as "the single traversal used by every AST consumer here", with callers who "do not re-implement the traversal". checkBannedConstructs always did.

The corrected doc deliberately does not claim the two cut the same script at the same node — they count a level differently, and the mutation table above is what that distinction looks like. They share the budget, nothing more.

One claim in it is established by reading rather than running: that checkBannedConstructs's generic property enumeration visits a superset of the positions acorn-walk's base descends. It is sound from the code — generic enumeration reaches computed member properties, object keys and labels that base skips — but "superset" is a stronger word than the evidence, so read it as "a different, broader set".

Checks

packages/extensions 60 files / 810 tests passed, 0 failed at upstream/main, and 60 / 810 passed, 0 failed with the patch — identical, which is the expected shape of a no-behaviour-change consolidation.

No cycle: ast/bounded-walk.ts imports only acorn-walk, and the new edge is host/ → ast/, one-way. No host-only code entered ast/; that file's change is doc-only.

A third copy? Not in production — MAX_AST_DEPTH now has exactly one definition, consumed by validate/code.ts, inference/capability.ts and host/source-wrap.ts, and a repo-wide grep for AST_DEPTH / nested more than / AST levels deep outside packages/extensions returns nothing.

The literal 1000 does survive in source-wrap.test.ts:163, which asserts the full message string. Left deliberately: pinning the value in a test is a guard rather than a divergence hole, since changing the constant now fails loudly there instead of passing silently. Worth knowing that a future change to the bound touches that assertion — validate/code.test.ts uses /nested more than \d+/ and would not notice.

tsc --noEmit exit 0; typecheck-tests OK across 60 files; oxlint clean on both changed files; check-changesets exit 0.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@BIMvoice, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 8 minutes

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4f54fcb0-b6ce-4166-bf41-e89f7cbeec7a

📥 Commits

Reviewing files that changed from the base of the PR and between 6095fe0 and 9cd4748.

📒 Files selected for processing (3)

• .changeset/one-ast-depth-bound-for-author-source.md
• packages/extensions/src/ast/bounded-walk.ts
• packages/extensions/src/host/source-wrap.ts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Viewer benchmark

✅ No threshold regressions detected.

01_Snowdon_Towers_Sample_Structural(1).ifc

Baseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric	Current	Baseline	Delta	Threshold	Status
firstBatchWaitMs	1352ms	2905ms	-53.5%	+50%	✅
firstVisibleGeometryMs	2120ms	3652ms	-41.9%	+50%	✅
streamCompleteMs	2364ms	3598ms	-34.3%	+50%	✅
spatialReadyMs	1025ms	1032ms	-0.7%	+50%	✅
metadataCompleteMs	1381ms	3063ms	-54.9%	+50%	✅
totalWallClockMs	2900ms	3700ms	-21.6%	+50%	✅

AC20-FZK-Haus.ifc

Baseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric	Current	Baseline	Delta	Threshold	Status
firstBatchWaitMs	255ms	1075ms	-76.3%	+50%	✅
firstVisibleGeometryMs	788ms	1572ms	-49.9%	+50%	✅
streamCompleteMs	919ms	1980ms	-53.6%	+50%	✅
spatialReadyMs	962ms	915ms	+5.1%	+50%	✅
metadataCompleteMs	1126ms	1392ms	-19.1%	+50%	✅
totalWallClockMs	1100ms	3300ms	-66.7%	+50%	✅

Refresh the baseline from a CI run: dispatch the Benchmark workflow with record_baseline, download the benchmark-baseline artifact, and commit baseline.json (see tests/benchmark/README.md).

[louistrue]

Closing as superseded by #3070, which is now merged. Proved the subsumption rather than inferring it from the titles, because both PRs are about "one AST depth bound" and it would be easy to assume they are the same change.

They are not. #3070 removes the duplicate traversal; this one keeps it and shares only the constant.

Measured against both branch heads, in packages/extensions/src/host/source-wrap.ts:

                        own depth-tracking walk    uses walkBounded
#3070                   0                          yes (4 sites)
#3082 (this)            1                          no

This PR does:

import { MAX_AST_DEPTH as SHARED_MAX_AST_DEPTH } from '../ast/bounded-walk.js';
const MAX_AST_DEPTH = SHARED_MAX_AST_DEPTH;
...
if (depth > MAX_AST_DEPTH) {      // still its own traversal

#3070 does:

import { MAX_AST_DEPTH, walkBounded } from '../ast/bounded-walk.js';

and deletes the second walk entirely. Confirmed on main after the merge: walkBounded appears 4 times in source-wrap.ts and the private depth loop is gone.

So the divergence band this PR documents is closed more thoroughly by #3070: sharing a constant between two hand-written walks leaves two walks that can still drift in what they visit, which is the wider version of the same fault. The table in this PR's body — showing wrapEntrySource accepting at 300 levels while validateCode refused, under a private 1000 against a shared 100 — is a good reproduction and it argued for the deeper fix.

Nothing lost. If anything here is not covered by #3070, reopen and say which and I will look again.

Credit where it is due: the depth-parity fixture on #3070 is better than what I suggested when I raised the vacuous-straddle problem. I said the arrow range never crossed the bound. The commit found the sharper thing — that reusing IF_DEPTHS made the arrow half assert a straddle it reached only by acorn running out of stack at 450+, not by the bound at all. Same verdict for an unrelated reason, and flaky with it, since the crossover moves with the host's remaining stack. Splitting the lists and asserting the honest weaker claim for arrows is the right resolution.