fix(cli): keep non-self-contained hook handlers out of body-only lowering (#1876)

.changeset/cli-hook-body-self-containment.md

@@ -0,0 +1,27 @@
+---
+"@objectstack/cli": patch
+---
+
+fix(cli): keep non-self-contained hook/action handlers out of body-only lowering (#1876)
+
+A hook/action handler that references a **module-scope identifier** (a helper,
+an import, a top-level const) was lowered to a metadata-only `body` by
+`objectstack build` — but that body ships without the referenced definition, so
+it throws `ReferenceError` at runtime. Build was green; the app didn't boot —
+exactly the build↔runtime parity gap #1876 describes.
+
+`extractHookBody` now runs a conservative free-identifier analysis (via the
+`ts` AST already available through `ts-morph`): it computes the handler's free
+variables — names referenced but bound neither by the function (params/locals)
+nor by the JS runtime (a generous global allow-list). When any are found,
+extraction is refused, so `lowerCallables` falls back to **bundling** the real
+function (esbuild carries the closure along) — no `ReferenceError`, no build
+break. The analysis is biased to never over-report: a missed case preserves
+today's behavior, and a false positive only causes a self-contained handler to
+be bundled instead of inlined (a size cost, never a correctness or build
+failure).
+
+Note: the other #1876 repro — legacy `object`/`aggregate` dashboard widgets
+passing build but rejected by the runtime — is already closed on `main` by the
+ADR-0021 single-form cutover (`DashboardWidgetSchema` now requires
+`dataset`/`values`, enforced by the same schema build and runtime both use).

packages/cli/src/utils/detect-free-identifiers.test.ts

@@ -0,0 +1,84 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+import { describe, it, expect } from 'vitest';
+import { detectFreeIdentifiers } from './detect-free-identifiers.js';
+
+/** Helper: stringify a real function so we test the exact `.toString()` path. */
+const src = (fn: (...a: any[]) => any) => String(fn);
+
+describe('detectFreeIdentifiers (#1876 — body self-containment)', () => {
+  describe('flags module-scope references (would ReferenceError at runtime)', () => {
+    it('a helper call (arrow)', () => {
+      const r = detectFreeIdentifiers('(ctx) => { ctx.record.slug = slugify(ctx.record.name); }');
+      expect(r.unparsed).toBe(false);
+      expect(r.free).toEqual(['slugify']);
+    });
+
+    it('a top-level const (function expression)', () => {
+      const r = detectFreeIdentifiers('function h(ctx){ return TAX_RATE * ctx.amount; }');
+      expect(r.free).toEqual(['TAX_RATE']);
+    });
+
+    it('object-method shorthand form', () => {
+      const r = detectFreeIdentifiers('handler(ctx){ return fmt(ctx.x); }');
+      expect(r.free).toEqual(['fmt']);
+    });
+
+    it('an implicit-return arrow', () => {
+      const r = detectFreeIdentifiers('(ctx) => compute(ctx.a, ctx.b)');
+      expect(r.free).toEqual(['compute']);
+    });
+
+    it('multiple distinct free names, sorted & de-duped', () => {
+      const r = detectFreeIdentifiers('(ctx) => { a(ctx); b(ctx); a(ctx); return CONST; }');
+      expect(r.free).toEqual(['CONST', 'a', 'b']);
+    });
+  });
+
+  describe('does NOT flag self-contained handlers (false-positive guards)', () => {
+    const selfContained: Array<[string, string]> = [
+      ['member access only', '(ctx) => { ctx.record.slug = ctx.record.name.toLowerCase(); }'],
+      ['local const', '(ctx) => { const s = ctx.record.name; return s.trim(); }'],
+      ['globals (Math/JSON)', '(ctx) => { ctx.record.id = Math.round(JSON.parse(ctx.x).y); }'],
+      ['destructured params', '({ record, api }) => { record.x = record.y; return api; }'],
+      ['locally-declared helper', '(ctx) => { const f = (a) => a * 2; return f(ctx.n); }'],
+      ['object shorthand of a local', '(ctx) => { const a = ctx.a; return { a }; }'],
+      ['object literal keys', '(ctx) => ({ total: ctx.a, count: ctx.b })'],
+      ['for-of loop binding', '(ctx) => { let sum = 0; for (const x of ctx.items) { sum += x; } ctx.sum = sum; }'],
+      ['catch binding', '(ctx) => { try { ctx.run(); } catch (err) { ctx.log = err; } }'],
+      ['param default uses global', '({ x = Math.PI }) => x'],
+      ['returned object method closes over param', '(ctx) => ({ run() { return ctx.x; } })'],
+      ['element access with local key', '(ctx) => { const k = ctx.key; return ctx.data[k]; }'],
+      ['named function expression recursion', 'function fact(n){ return n <= 1 ? 1 : n * fact(n - 1); }'],
+      ['nested destructuring', '({ a: { b } }) => b + 1'],
+      ['rest params', '(...args) => args.length'],
+      ['typeof a local', '(ctx) => { const v = ctx.v; return typeof v; }'],
+    ];
+    for (const [label, source] of selfContained) {
+      it(label, () => {
+        const r = detectFreeIdentifiers(source);
+        expect(r.unparsed).toBe(false);
+        expect(r.free).toEqual([]);
+      });
+    }
+  });
+
+  describe('real compiled `.toString()` shapes', () => {
+    it('does not flag a self-contained closure', () => {
+      const handler = (ctx: any) => {
+        const name = String(ctx.record.name ?? '').trim();
+        ctx.record.slug = name.toLowerCase().replace(/\s+/g, '-');
+      };
+      expect(detectFreeIdentifiers(src(handler)).free).toEqual([]);
+    });
+  });
+
+  it('never invents free vars for non-handler junk (conservative — caller won\'t block)', () => {
+    // Whether or not TS error-recovers a node, the safe outcome is no free vars
+    // so extraction is never blocked on garbage. (peelToBlockBody rejects such
+    // input earlier in the real path anyway.)
+    expect(detectFreeIdentifiers('this is not a function').free).toEqual([]);
+    expect(detectFreeIdentifiers('').free).toEqual([]);
+    expect(detectFreeIdentifiers('{ not: valid').free).toEqual([]);
+  });
+});

packages/cli/src/utils/detect-free-identifiers.ts

@@ -0,0 +1,241 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * Detect FREE identifiers in a hook/action handler — names the body references
+ * but that are bound neither by the function (params, locals) nor by the JS
+ * runtime (globals). The canonical case (#1876, build↔runtime parity) is a
+ * handler that calls a **module-scope helper**:
+ *
+ *   const slugify = (s) => s.toLowerCase();           // module scope
+ *   defineStack({ hooks: [{ ..., handler: (ctx) => { ctx.record.slug = slugify(ctx.record.name); } }] });
+ *
+ * When such a handler is lowered to a metadata-only `body`, the `slugify`
+ * reference ships without its definition and throws `ReferenceError` at runtime
+ * — `objectstack build` is green but the app does not boot. By reporting the
+ * free identifier the caller can keep the handler OUT of the body-only form and
+ * fall back to BUNDLING it (esbuild bundles the real closure, so `slugify` comes
+ * along) — no ReferenceError, no build break.
+ *
+ * Safety bias: this analysis is deliberately **conservative**. `bindings`
+ * over-approximates (every name declared ANYWHERE in the function counts as
+ * bound), and `GLOBALS` is generous. Both bias toward NOT flagging — a missed
+ * case merely preserves today's behavior, whereas a false positive would only
+ * ever cause a self-contained handler to be bundled instead of inlined (a
+ * size/over-caution cost, never a correctness or build failure). We never
+ * trade that bias for completeness.
+ */
+
+// `ts-morph` is already a CLI runtime dependency and re-exports the full
+// TypeScript compiler namespace, so we use its `ts` rather than adding a direct
+// `typescript` dependency.
+import { ts } from 'ts-morph';
+
+/**
+ * Identifiers the JS runtime provides ambiently. Generous on purpose — listing
+ * a name here means "assume the runtime has it" → don't flag → don't over-bundle
+ * the rare false positive. A genuinely-missing global is a different problem
+ * (sandbox capability), not a module-scope-helper leak.
+ */
+const GLOBALS: ReadonlySet<string> = new Set([
+  // Value/namespace globals
+  'Math', 'JSON', 'Date', 'Object', 'Array', 'String', 'Number', 'Boolean',
+  'RegExp', 'Map', 'Set', 'WeakMap', 'WeakSet', 'Promise', 'Symbol', 'BigInt',
+  'Function', 'Reflect', 'Proxy', 'Intl',
+  'ArrayBuffer', 'SharedArrayBuffer', 'DataView',
+  'Int8Array', 'Uint8Array', 'Uint8ClampedArray', 'Int16Array', 'Uint16Array',
+  'Int32Array', 'Uint32Array', 'Float32Array', 'Float64Array', 'BigInt64Array', 'BigUint64Array',
+  // Error constructors
+  'Error', 'TypeError', 'RangeError', 'SyntaxError', 'ReferenceError',
+  'EvalError', 'URIError', 'AggregateError',
+  // Global functions
+  'parseInt', 'parseFloat', 'isNaN', 'isFinite',
+  'encodeURIComponent', 'decodeURIComponent', 'encodeURI', 'decodeURI',
+  'structuredClone', 'queueMicrotask', 'atob', 'btoa',
+  'setTimeout', 'clearTimeout', 'setInterval', 'clearInterval',
+  // Web-ish that the sandbox / Node commonly provide
+  'URL', 'URLSearchParams', 'TextEncoder', 'TextDecoder', 'console',
+  // Literal-ish globals & implicit bindings
+  'undefined', 'NaN', 'Infinity', 'globalThis', 'arguments',
+]);
+
+export interface FreeIdentifierResult {
+  /** Sorted, de-duplicated free identifier names (empty when self-contained). */
+  free: string[];
+  /** True when the source could not be parsed into a single function node. */
+  unparsed: boolean;
+}
+
+/**
+ * Parse `rawFunctionSource` (the result of `String(fn)`) into a single
+ * function-like node. Handlers come in three `.toString()` shapes — arrow,
+ * function expression/declaration, and object-method shorthand — so we try
+ * three wraps and take the first that yields exactly one function-like node.
+ */
+function parseFunction(rawFunctionSource: string): ts.FunctionLikeDeclarationBase | null {
+  const wraps = [
+    rawFunctionSource, // function decl / named function expression statement
+    `(${rawFunctionSource})`, // arrow / anonymous function expression
+    `({${rawFunctionSource}})`, // object-method shorthand `name(ctx){...}`
+  ];
+  for (const code of wraps) {
+    const sf = ts.createSourceFile('__handler__.js', code, ts.ScriptTarget.Latest, true, ts.ScriptKind.JS);
+    let found: ts.FunctionLikeDeclarationBase | null = null;
+    let count = 0;
+    const visit = (node: ts.Node): void => {
+      if (
+        ts.isArrowFunction(node) ||
+        ts.isFunctionExpression(node) ||
+        ts.isFunctionDeclaration(node) ||
+        ts.isMethodDeclaration(node)
+      ) {
+        count += 1;
+        if (!found) found = node;
+        return; // don't descend — nested functions are part of THIS one's body
+      }
+      ts.forEachChild(node, visit);
+    };
+    ts.forEachChild(sf, visit);
+    // Exactly one top-level function-like node means the wrap matched cleanly.
+    if (found && count === 1) return found;
+  }
+  return null;
+}
+
+/** Collect every binding name declared ANYWHERE within `fn` (over-approx). */
+function collectBindings(fn: ts.FunctionLikeDeclarationBase): Set<string> {
+  const bound = new Set<string>();
+
+  const addBindingName = (name: ts.BindingName | ts.PropertyName | undefined): void => {
+    if (!name) return;
+    if (ts.isIdentifier(name)) {
+      bound.add(name.text);
+    } else if (ts.isObjectBindingPattern(name) || ts.isArrayBindingPattern(name)) {
+      for (const el of name.elements) {
+        if (ts.isBindingElement(el)) addBindingName(el.name);
+      }
+    }
+  };
+
+  const walk = (node: ts.Node): void => {
+    if (ts.isParameter(node)) {
+      addBindingName(node.name);
+    } else if (ts.isVariableDeclaration(node)) {
+      addBindingName(node.name);
+    } else if (ts.isFunctionDeclaration(node) && node.name) {
+      bound.add(node.name.text);
+    } else if (ts.isClassDeclaration(node) && node.name) {
+      bound.add(node.name.text);
+    } else if (
+      (ts.isFunctionExpression(node) || ts.isClassExpression(node)) &&
+      node.name
+    ) {
+      // A named function/class expression binds its own name in its body.
+      bound.add(node.name.text);
+    } else if (ts.isCatchClause(node) && node.variableDeclaration) {
+      addBindingName(node.variableDeclaration.name);
+    } else if (ts.isBindingElement(node)) {
+      addBindingName(node.name);
+    }
+    ts.forEachChild(node, walk);
+  };
+
+  // The function's own name (named function decl/expr) is in scope within it.
+  if (
+    (ts.isFunctionDeclaration(fn) || ts.isFunctionExpression(fn)) &&
+    fn.name
+  ) {
+    bound.add(fn.name.text);
+  }
+  for (const p of fn.parameters) addBindingName(p.name);
+  if (fn.body) walk(fn.body);
+  // Parameter default initializers may declare nothing but reference things —
+  // covered by the reference pass. Destructuring defaults are bindings:
+  for (const p of fn.parameters) ts.forEachChild(p, walk);
+
+  return bound;
+}
+
+/**
+ * Collect identifiers used in VALUE position (potential references). Excludes
+ * the false-positive sources: property-access member names, non-shorthand
+ * object/class member keys, and statement labels. Binding names that slip
+ * through are harmless — they are subtracted via `bindings` downstream.
+ */
+function collectReferences(fn: ts.FunctionLikeDeclarationBase): Set<string> {
+  const refs = new Set<string>();
+
+  const walk = (node: ts.Node): void => {
+    // Skip type annotations entirely (compiled JS rarely has them, but be safe).
+    if (ts.isTypeNode(node)) return;
+
+    if (ts.isPropertyAccessExpression(node)) {
+      // `a.b` — visit `a` (could be a ref) but NOT `b` (member name).
+      walk(node.expression);
+      return;
+    }
+    if (ts.isQualifiedName(node)) {
+      walk(node.left);
+      return;
+    }
+    if (ts.isPropertyAssignment(node)) {
+      // `{ key: value }` — `key` is not a ref (unless computed). Visit value;
+      // visit computed key names.
+      if (ts.isComputedPropertyName(node.name)) walk(node.name.expression);
+      walk(node.initializer);
+      return;
+    }
+    if (ts.isMethodDeclaration(node) || ts.isPropertyDeclaration(node) || ts.isGetAccessor(node) || ts.isSetAccessor(node)) {
+      if (node.name && ts.isComputedPropertyName(node.name)) walk(node.name.expression);
+      ts.forEachChild(node, (c) => { if (c !== node.name) walk(c); });
+      return;
+    }
+    if (ts.isLabeledStatement(node)) {
+      // The label identifier is not a reference; visit the statement body.
+      walk(node.statement);
+      return;
+    }
+    if (ts.isBreakOrContinueStatement(node)) {
+      return; // label, if any, is not a value reference
+    }
+    if (ts.isShorthandPropertyAssignment(node)) {
+      // `{ x }` — x IS a value reference.
+      refs.add(node.name.text);
+      return;
+    }
+    if (ts.isIdentifier(node)) {
+      refs.add(node.text);
+      return;
+    }
+    ts.forEachChild(node, walk);
+  };
+
+  if (fn.body) walk(fn.body);
+  // Parameter DEFAULT initializers are evaluated in scope and may reference
+  // free identifiers; include them (their binding names are excluded above).
+  for (const p of fn.parameters) {
+    if (p.initializer) walk(p.initializer);
+  }
+  return refs;
+}
+
+/**
+ * Compute the free identifiers of a handler function source.
+ * Returns `{ free: [], unparsed: true }` when the source can't be parsed — the
+ * caller treats "unparsed" as "don't block extraction" (conservative).
+ */
+export function detectFreeIdentifiers(rawFunctionSource: string): FreeIdentifierResult {
+  const fn = parseFunction(rawFunctionSource);
+  if (!fn) return { free: [], unparsed: true };
+
+  const bound = collectBindings(fn);
+  const refs = collectReferences(fn);
+
+  const free: string[] = [];
+  for (const name of refs) {
+    if (bound.has(name)) continue;
+    if (GLOBALS.has(name)) continue;
+    free.push(name);
+  }
+  free.sort();
+  return { free, unparsed: false };
+}

packages/cli/src/utils/extract-hook-body.ts

@@ -21,8 +21,15 @@
  * `ctx.crypto.*` access patterns and add the matching capability tokens to
  * `body.capabilities` automatically. Authors can still override by setting
  * `// @capabilities api.read api.write` as the first line of the function.
+ *
+ * Self-containment (#1876): a handler that references a module-scope identifier
+ * (helper, import, top-level const) cannot be shipped body-only — the reference
+ * would `ReferenceError` at runtime. {@link detectFreeIdentifiers} finds those;
+ * extraction throws so the caller falls back to bundling the real closure.
  */
 
+import { detectFreeIdentifiers } from './detect-free-identifiers.js';
+
 const FORBIDDEN_PATTERNS: Array<{ rx: RegExp; reason: string }> = [
   { rx: /\bimport\s*[\(\*\{]/, reason: 'dynamic `import()` and ES imports are not allowed in hook/action bodies — declare a Connector recipe instead' },
   { rx: /\brequire\s*\(/, reason: '`require()` is not allowed in hook/action bodies' },
@@ -81,6 +88,23 @@ export function extractHookBody(fn: (...a: unknown[]) => unknown, originLabel: s
     }
   }
 
+  // #1876 — a handler that references a MODULE-SCOPE identifier (a helper, an
+  // imported binding, a top-level const) is NOT self-contained: shipping it as
+  // a metadata-only `body` would throw `ReferenceError` at runtime (the
+  // reference ships without its definition). Detect those free identifiers and
+  // throw — the caller catches this and keeps the handler in the BUNDLED form,
+  // where esbuild carries the real closure along. The whole `fn` source (params
+  // included) is analyzed so parameters are correctly in scope.
+  const { free, unparsed } = detectFreeIdentifiers(raw);
+  if (!unparsed && free.length > 0) {
+    throw new Error(
+      `[hook-body-extract] ${originLabel}: handler references identifier(s) not in scope at runtime: ` +
+        `${free.join(', ')}. Module-scope helpers/imports aren't shipped with a metadata-only body, so ` +
+        `this handler will be BUNDLED instead (no behavior change). To make it body-only, inline the ` +
+        `helper(s) into the handler or move the logic behind \`ctx\` (e.g. \`ctx.api\`).`,
+    );
+  }
+
   // Infer capabilities from API surface usage.
   const inferred = new Set<ExtractedBody['capabilities'][number]>();
   for (const { rx, cap } of CAPABILITY_PATTERNS) {