From f8d63cce661c98bcc3a152aeb54f730e48860832 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Mon, 15 Jun 2026 17:42:15 +0800 Subject: [PATCH] fix(cli): keep non-self-contained hook handlers out of body-only lowering (#1876) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A hook/action handler referencing a module-scope identifier (helper, import, top-level const) was lowered to a metadata-only `body`; that body ships without the definition and throws `ReferenceError` at runtime — build green, app won't boot (a build↔runtime parity gap, #1876). `extractHookBody` now runs a conservative free-identifier analysis (TS AST via the already-present `ts-morph`): free vars = names referenced but bound neither by the function (params/locals) nor by the runtime (generous global allow-list). Any hit refuses extraction, so `lowerCallables` falls back to BUNDLING the real closure (esbuild carries it along) — no ReferenceError, no build break. Biased to never over-report: a miss preserves today's behavior; a false positive only bundles a self-contained handler (size cost, never a build/correctness failure). Tests: 23-case analyzer battery (flagging + false-positive guards), extractor integration (throws on free var, extracts self-contained), and an end-to-end lowerCallables fallback assertion. All example app builds stay green. Note: the other #1876 repro (legacy object/aggregate widgets) is already closed on main by the ADR-0021 single-form cutover — verified. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/cli-hook-body-self-containment.md | 27 ++ .../src/utils/detect-free-identifiers.test.ts | 84 ++++++ .../cli/src/utils/detect-free-identifiers.ts | 241 ++++++++++++++++++ packages/cli/src/utils/extract-hook-body.ts | 24 ++ packages/cli/test/extract-hook-body.test.ts | 24 ++ packages/cli/test/lower-callables.test.ts | 38 +++ 6 files changed, 438 insertions(+) create mode 100644 .changeset/cli-hook-body-self-containment.md create mode 100644 packages/cli/src/utils/detect-free-identifiers.test.ts create mode 100644 packages/cli/src/utils/detect-free-identifiers.ts diff --git a/.changeset/cli-hook-body-self-containment.md b/.changeset/cli-hook-body-self-containment.md new file mode 100644 index 0000000000..bf753f5e3f --- /dev/null +++ b/.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). diff --git a/packages/cli/src/utils/detect-free-identifiers.test.ts b/packages/cli/src/utils/detect-free-identifiers.test.ts new file mode 100644 index 0000000000..29d4cbbc77 --- /dev/null +++ b/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([]); + }); +}); diff --git a/packages/cli/src/utils/detect-free-identifiers.ts b/packages/cli/src/utils/detect-free-identifiers.ts new file mode 100644 index 0000000000..068637867f --- /dev/null +++ b/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 = 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 { + const bound = new Set(); + + 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 { + const refs = new Set(); + + 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 }; +} diff --git a/packages/cli/src/utils/extract-hook-body.ts b/packages/cli/src/utils/extract-hook-body.ts index 06f914c196..7f6e4d7392 100644 --- a/packages/cli/src/utils/extract-hook-body.ts +++ b/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(); for (const { rx, cap } of CAPABILITY_PATTERNS) { diff --git a/packages/cli/test/extract-hook-body.test.ts b/packages/cli/test/extract-hook-body.test.ts index 2647b77360..0d0586fbdf 100644 --- a/packages/cli/test/extract-hook-body.test.ts +++ b/packages/cli/test/extract-hook-body.test.ts @@ -93,4 +93,28 @@ describe('extractHookBody', () => { const ext = extractHookBody(fn, 'hook e'); expect(ext.capabilities.sort()).toEqual(['api.read', 'api.write', 'log']); }); + + // #1876 — a handler that references a module-scope helper is not self- + // contained; extraction must throw so lowerCallables falls back to bundling + // (which carries the closure) instead of shipping a body that ReferenceErrors. + it('rejects a handler that references a module-scope helper (#1876)', () => { + const fn = (ctx: any) => { + ctx.record.slug = moduleScopeHelper(ctx.record.name); + }; + expect(() => extractHookBody(fn, 'hook free')).toThrow(/not in scope at runtime|moduleScopeHelper/); + }); + + it('extracts a self-contained handler that only uses params + globals (#1876)', () => { + const fn = (ctx: any) => { + ctx.record.id = Math.round(Number(ctx.record.raw)); + ctx.record.tags = JSON.stringify(ctx.record.list ?? []); + }; + const ext = extractHookBody(fn, 'hook contained'); + expect(ext.source).toContain('Math.round'); + }); }); + +/** Module-scope helper used by the #1876 free-identifier test above. */ +function moduleScopeHelper(s: string): string { + return String(s).toLowerCase(); +} diff --git a/packages/cli/test/lower-callables.test.ts b/packages/cli/test/lower-callables.test.ts index b1374b0ea4..c694ec9da2 100644 --- a/packages/cli/test/lower-callables.test.ts +++ b/packages/cli/test/lower-callables.test.ts @@ -70,6 +70,39 @@ describe('lowerCallables', () => { expect(out.functions.dup__2).toBe(f2); }); + // #1876 — a handler referencing a module-scope helper must NOT be lowered to a + // metadata body (it would ReferenceError at runtime). Instead it stays + // registered for BUNDLING (closure preserved) and a body-extraction warning is + // recorded naming the offending identifier. + it('does not body-lower a handler that references a module-scope helper; keeps it for bundling (#1876)', () => { + const handler = (ctx: any) => { + ctx.record.slug = moduleHelper(ctx.record.name); + }; + const out = lowerCallables({ + hooks: [{ name: 'slugify_hook', handler, events: ['beforeInsert'], object: 'doc' }], + }); + // Still registered (so the bundle carries the real closure)… + expect(out.functions.slugify_hook).toBe(handler); + expect((out.lowered.hooks as any[])[0].handler).toBe('slugify_hook'); + // …but NOT shipped as a metadata-only body… + expect((out.lowered.hooks as any[])[0].body).toBeUndefined(); + expect(out.bodyExtracted).toBe(0); + // …and the reason is recorded, naming the free identifier. + expect(out.bodyExtractionWarnings).toHaveLength(1); + expect(out.bodyExtractionWarnings[0].reason).toMatch(/moduleHelper|not in scope at runtime/); + }); + + it('still body-lowers a self-contained handler (params + globals only) (#1876)', () => { + const handler = (ctx: any) => { + ctx.record.id = Math.round(Number(ctx.record.raw)); + }; + const out = lowerCallables({ + hooks: [{ name: 'contained_hook', handler, events: ['beforeInsert'], object: 'doc' }], + }); + expect(out.bodyExtracted).toBe(1); + expect((out.lowered.hooks as any[])[0].body).toBeDefined(); + }); + it('produces a JSON-serializable lowered shape', () => { const out = lowerCallables({ hooks: [ @@ -81,3 +114,8 @@ describe('lowerCallables', () => { expect(JSON.parse(json).hooks[0].handler).toBe('h'); }); }); + +/** Module-scope helper referenced by the #1876 bundle-fallback test. */ +function moduleHelper(s: string): string { + return String(s).toLowerCase().replace(/\s+/g, '-'); +}