fix(extensions): one AST walker, and fail closed when a node type cannot be walked

.changeset/bounded-ast-walk-for-author-source.md

@@ -16,8 +16,10 @@ stack the caller happened to have left.
 
 Both now traverse through a new internal `walkBounded`
 (`src/ast/bounded-walk.ts`), which keeps its own stack on the heap and stops at
-`MAX_AST_DEPTH = 1000` (~500 source levels of `if (1) { … }`, below acorn's own
-parse floor of roughly 1200). It descends using `acorn-walk`'s `base` visitor
+`MAX_AST_DEPTH = 1000` (~500 source levels of `if (1) { … }`; acorn's own parser
+gives up somewhere above that, but where depends on the host's remaining stack —
+measured on Node 22 between 1100 and 4000 source levels, so it is not a fixed
+floor to sit under). It descends using `acorn-walk`'s `base` visitor
 and reports nodes in `walk.simple`'s post-order, so which child positions count
 as nodes — non-computed member properties and object keys stay unvisited — and
 the order they arrive in are unchanged. Behaviour below the bound is identical.

.changeset/one-ast-walker-that-fails-closed.md

@@ -0,0 +1,62 @@
+---
+'@ifc-lite/extensions': patch
+---
+
+Put the entry-script scan on the package's one AST walker, and fail closed on a
+subtree the walker cannot descend.
+
+Three follow-ups to the bounded-walk work, all latent rather than live — no
+input reaching this package today takes any of the paths below.
+
+**One walker, one bound.** `src/ast/bounded-walk.ts` opened with "this module is
+the single traversal used by every AST consumer here… Callers vary the visitor;
+they do not re-implement the traversal", while `host/source-wrap.ts` ran its own
+hand-written traversal with its own private `MAX_AST_DEPTH = 1000` and its own
+generic child enumeration. Two walkers and two constants with a comment telling
+the next reader the second one did not exist. `checkBannedConstructs` now calls
+`walkBounded`; the duplicate constant and the generic `childNodes` helper are
+gone.
+
+The migration narrows which child positions get *reported* — `acorn-walk`'s
+`base` skips non-computed member properties, plain object keys, labels,
+`ExportSpecifier`s and pattern `Property` wrappers, which the generic
+property-crawl reported as nodes. It does not narrow what the scan *catches*: a
+differential run over 59 sources placing each banned construct in an exotic
+position found no banned node reached by the generic crawl and missed by
+`base`, including the pattern-default case where the `Property` wrapper is
+skipped but the `ImportExpression` under it is still visited via `ObjectPattern`.
+The accept/reject depths are unchanged for both shapes measured (`if`-nesting
+and arrow chains), and a test now pins `wrapEntrySource` and `validateCode`
+against each other across the boundary so a future divergence fails.
+
+**A missing `base` is now a failure, not a silent stop.** `walkBounded` reported
+a node it had no `base` for and skipped its entire subtree. Every caller is a
+scanner looking for things it must not find, so a skipped subtree was a scan
+that failed open: `validateCode` returned `ok`, `inferCapabilities` published an
+under-counted capability set, and `wrapEntrySource` wrapped the script — none of
+them could tell "found nothing" from "never looked". `acorn-walk` throws on a
+missing `base` for exactly this reason; we report instead of throwing because
+these callers are declared to return a result. The result now carries
+`unwalkableTypes`, and all three callers treat a non-empty list the way they
+already treat `depthExceeded`. This becomes reachable the first time acorn is
+upgraded ahead of `acorn-walk` — the skew that landed class static blocks,
+import attributes and `await using`. Verified against acorn 8.18.0 /
+acorn-walk 8.3.5: no node type the walk actually reaches is missing a base.
+(`ExportSpecifier` has no `base` entry, but `base.ExportNamedDeclaration` never
+descends into `specifiers`, so the walk never dispatches on it — it is unreached,
+not unwalkable.) The tests reproduce the skew by removing one `base` entry rather
+than waiting for an upgrade.
+
+**Two comments that named a number acorn does not have.** The walker's docstring
+claimed acorn "gives up at roughly 1200 source levels" and `source-wrap.ts`
+claimed "roughly twice this depth". Both understate — so they erred safe — but
+as written they were the numbers a future reader would cite to justify raising
+the bound. Measured on Node 22, the same script parses at 1100 source levels and
+aborts the process at 1200 in a default-stack run (a fatal V8 abort, exit 134,
+not a catchable error), is rejected at 1200 under this repo's vitest workers,
+and parses at 4000 under `node --stack-size=4000`. The parser's give-up point is
+a property of the host's remaining stack, not of acorn, and the docstring now
+says so — which is the argument for a fixed heap-based bound, not against it.
+
+`MAX_AST_DEPTH` is unchanged at 1000. No public API change; `walkBounded` is
+still not exported from the package entry point.

packages/extensions/src/ast/bounded-walk.test.ts

@@ -105,4 +105,71 @@ describe('walkBounded', () => {
     expect(res.depthExceeded).toBe(false);
     expect(seen).toBe(MAX_AST_DEPTH * 3);
   });
+
+  it('leaves unwalkableTypes empty for ordinary source', () => {
+    const res = walkBounded(
+      parse('async function activate(ctx) { const w = await ctx.bim.query.byType("IfcWall"); return w.length; }'),
+      () => {},
+    );
+    expect(res.unwalkableTypes).toEqual([]);
+  });
+});
+
+/**
+ * The node types acorn can emit are not the node types `acorn-walk`
+ * knows how to descend — the two packages version independently, and
+ * every new syntax (class static blocks, import attributes, `await
+ * using`) lands in the parser first. `withoutBase` reproduces that
+ * skew deliberately instead of waiting for an upgrade to produce it:
+ * it removes one `base` entry for the duration of the callback, so a
+ * node type acorn still emits becomes one the walker cannot descend.
+ */
+function withoutBase<T>(type: string, run: () => T): T {
+  const base = walk.base as unknown as Record<string, unknown>;
+  const saved = base[type];
+  expect(saved).toBeTypeOf('function');
+  delete base[type];
+  try {
+    return run();
+  } finally {
+    base[type] = saved;
+  }
+}
+
+describe('walkBounded — a subtree it cannot descend', () => {
+  it('reports the type instead of silently skipping the subtree', () => {
+    const ast = parse('try { eval("payload"); } catch (e) {}');
+    const seen: string[] = [];
+    const res = withoutBase('TryStatement', () =>
+      walkBounded(ast, (_n, type) => {
+        seen.push(type);
+      }),
+    );
+
+    // The subtree really was skipped — this is the fail-open shape.
+    expect(seen).not.toContain('CallExpression');
+    // …and the result says so, so the caller cannot read the silence
+    // as "nothing found".
+    expect(res.unwalkableTypes).toEqual(['TryStatement']);
+    // Not a depth problem: the two causes stay distinguishable.
+    expect(res.depthExceeded).toBe(false);
+  });
+
+  it('keeps walking the siblings of the subtree it could not descend', () => {
+    const ast = parse('try {} catch (e) {} eval("after");');
+    const seen: string[] = [];
+    const res = withoutBase('TryStatement', () =>
+      walkBounded(ast, (_n, type) => {
+        seen.push(type);
+      }),
+    );
+    expect(seen).toContain('CallExpression');
+    expect(res.unwalkableTypes).toEqual(['TryStatement']);
+  });
+
+  it('deduplicates repeated unwalkable types', () => {
+    const ast = parse('try {} catch (e) {} try {} catch (e) {}');
+    const res = withoutBase('TryStatement', () => walkBounded(ast, () => {}));
+    expect(res.unwalkableTypes).toEqual(['TryStatement']);
+  });
 });

packages/extensions/src/ast/bounded-walk.ts

@@ -12,10 +12,13 @@
  * `RangeError: Maximum call stack size exceeded` out of the middle of
  * whatever function invoked the walk.
  *
- * This module is the single traversal used by every AST consumer here.
- * It keeps its own stack on the heap and stops at {@link MAX_AST_DEPTH},
- * *reporting* that it stopped rather than throwing. Callers vary the
- * visitor; they do not re-implement the traversal.
+ * This module is the single traversal used by every AST consumer here
+ * — `validateCode`, `inferCapabilities` and the entry-script
+ * banned-construct scan in `host/source-wrap.ts`. It keeps its own
+ * stack on the heap and stops at {@link MAX_AST_DEPTH}, *reporting*
+ * that it stopped rather than throwing. Callers vary the visitor; they
+ * do not re-implement the traversal, and there is exactly one depth
+ * bound for them to disagree about.
  *
  * It descends using `acorn-walk`'s own `base` visitor rather than
  * enumerating object properties generically, so which child positions
@@ -40,11 +43,29 @@ import * as walk from 'acorn-walk';
  * caller reports a validation failure instead of continuing.
  *
  * One `if (1) { … }` source level costs two levels here
- * (`IfStatement` -> `BlockStatement`), and acorn's own parser gives up
- * at roughly 1200 *source* levels ("Not enough stack space to parse
- * input"). That parser limit moves with however much stack the host
- * happens to have left; this one does not, which is the entire point —
- * the accept/reject boundary must not depend on the caller's remaining
+ * (`IfStatement` -> `BlockStatement`), so the bound bites at 500 such
+ * source levels.
+ *
+ * The bound is in AST levels, so its effective *source*-level threshold
+ * varies by construct, and for cheap constructs it is unreachable. An
+ * arrow link (`() => () => …`) costs one level, not two, so this bound
+ * would need ~1000 of them — and acorn runs out of stack parsing that
+ * shape at a few hundred links, well before the walk is ever asked. The
+ * asymmetry is intended: the bound guards the walk's own stack, and a
+ * construct that the parser rejects first never reaches the walk.
+ * `host/source-wrap.test.ts` pins both the 1:2 cost ratio and the fact
+ * that every parseable arrow depth is accepted.
+ *
+ * Do NOT think of this as "well under acorn's own parser limit": acorn
+ * has no fixed limit to be under. The same script, on Node 22, parses
+ * at 1100 source levels and aborts the process at 1200 in a
+ * default-stack run, is rejected at 1200 under this repo's vitest
+ * workers ("Not enough stack space to parse input"), and parses at
+ * 4000 under `node --stack-size=4000`. The parser's give-up point is a
+ * property of the host's remaining stack, not of acorn — and one of
+ * those three failures is a fatal V8 abort (exit 134), not a catchable
+ * error. This bound does not move, which is the entire point: the
+ * accept/reject boundary must not depend on the caller's remaining
  * stack.
  *
  * Catching the `RangeError` instead would reintroduce exactly that
@@ -68,6 +89,21 @@ export interface BoundedWalkResult {
    * never as "nothing found".
    */
   depthExceeded: boolean;
+  /**
+   * Node types `acorn-walk` had no `base` entry for, deduplicated. The
+   * subtree under such a node was NOT descended, so — exactly like
+   * {@link depthExceeded} — the visit is incomplete and the caller MUST
+   * treat a non-empty list as a failure. Every call site here is a
+   * scanner looking for things it must not find, so an unwalkable
+   * subtree is a scan that found nothing because it never looked.
+   *
+   * This is how a walk sees an acorn upgrade that lands a new node type
+   * ahead of `acorn-walk` (class static blocks, import attributes and
+   * `await using` all arrived that way). `acorn-walk` throws on a
+   * missing base for the same reason; we report instead of throwing
+   * because the callers are declared to return a result, not to throw.
+   */
+  unwalkableTypes: readonly string[];
 }
 
 function isAstNode(value: unknown): value is AstNode {
@@ -100,21 +136,25 @@ interface Frame {
  * key. One traversal is therefore shared across sites that care about
  * entirely different node types.
  *
- * Returns `{ depthExceeded: true }` if the bound stopped the walk. The
- * traversal never throws for depth reasons.
+ * Returns `depthExceeded: true` if the bound stopped the walk, and
+ * lists in `unwalkableTypes` any node type it could not descend. The
+ * traversal never throws for either reason — but a caller that ignores
+ * either field is reporting "clean" on a tree it did not finish
+ * reading.
  */
 export function walkBounded(
   root: unknown,
   visit: (node: AstNode, type: string) => void,
 ): BoundedWalkResult {
-  if (!isAstNode(root)) return { depthExceeded: false };
+  if (!isAstNode(root)) return { depthExceeded: false, unwalkableTypes: [] };
 
   const baseVisitor = walk.base as unknown as Record<
     string,
     ((node: unknown, state: unknown, c: (child: unknown, state: unknown, override?: string) => void) => void) | undefined
   >;
 
   const stack: Frame[] = [{ node: root, depth: 0, expanded: false }];
+  const unwalkable = new Set<string>();
 
   while (stack.length > 0) {
     const frame = stack.pop()!;
@@ -125,12 +165,18 @@ export function walkBounded(
       continue;
     }
 
-    if (frame.depth > MAX_AST_DEPTH) return { depthExceeded: true };
+    if (frame.depth > MAX_AST_DEPTH) {
+      return { depthExceeded: true, unwalkableTypes: [...unwalkable] };
+    }
 
     const baseFn = baseVisitor[type];
     if (!baseFn) {
-      // Unknown node type: report it and stop descending, matching
-      // acorn-walk's own behaviour of throwing only on a missing base.
+      // No base for this type: we cannot enumerate its children, so its
+      // whole subtree goes uninspected. Report the node itself and keep
+      // walking its siblings — the rest of the tree is still worth
+      // scanning — but record the type so the caller fails instead of
+      // reading the visitor's silence as "nothing found".
+      unwalkable.add(type);
       visit(frame.node, type);
       continue;
     }
@@ -158,5 +204,5 @@ export function walkBounded(
     }
   }
 
-  return { depthExceeded: false };
+  return { depthExceeded: false, unwalkableTypes: [...unwalkable] };
 }