fix(cli): a renamed destructuring key off ctx is a key, not a free identifier

.changeset/22459-hook-lowering-destructuring-alias.md

@@ -0,0 +1,11 @@
+---
+'@objectstack/cli': patch
+---
+
+fix(cli): a hook that destructures a renamed key off `ctx` is no longer refused as not-lowerable (#22459)
+
+Clause-②: no
+
+- **What changed.** The free-identifier scan behind `objectstack lint`'s `hook-body/not-lowerable` rule and `objectstack build`'s hook lowering counted a renamed destructuring key as a free identifier. For `const { event, input, previous: prev } = ctx`, it reported `previous`, a property read off `ctx`, although the only name the pattern binds is `prev`. So `objectstack lint` failed the hook with the advice "reach them through `ctx`", which the hook already did, and `objectstack build` shipped it as a bundled closure instead of a metadata-only body. A non-computed destructuring key is now a key, as the key of `{ key: value }` already was. Nested patterns (`{ a: { b: c } } = ctx`), patterns inside array patterns, the parameters of a callback inside the handler, and `catch` clauses are covered by the same rule.
+- **What is still refused.** A destructuring default or a computed key that names something out of scope is still free: `const { key: alias = FALLBACK } = ctx` and `const { [KEY]: v } = ctx` report `FALLBACK` and `KEY`, as before. A genuinely free `previous` that is not read off `ctx` is still refused.
+- **What to do.** Nothing. A hook written this way now lowers to a body on the next `objectstack build`, and `objectstack lint` stops reporting it. The un-renamed spelling (`const prev = ctx.previous`) keeps working as before.

packages/cli/src/lint/hook-body-lowering.test.ts

@@ -22,6 +22,8 @@ import { lowerCallables } from '../utils/lower-callables.js';
 
 // Module scope — exactly what a lowered body cannot reach.
 const SLA_MATRIX: Record<string, number> = { high: 4, low: 48 };
+// Module scope, spelled like a `ctx` key — a reference to THIS is free.
+const previous = { rank: 1 };
 
 const freeIdentifierHook = {
   name: 'case_sla',
@@ -139,6 +141,56 @@ describe('checkHookBodyLowering', () => {
     });
   });
 
+  describe('a hook that destructures a renamed key off `ctx` reaches its value through `ctx`', () => {
+    // `previous` is a property READ OFF ctx — exactly what this rule's own
+    // remedy prescribes — and `prev` is the only name the pattern binds. The
+    // scan used to count the key as a free identifier, so lint refused the
+    // hook (advising what it already did) and the build bundled it.
+    const renamedKeyHook = {
+      name: 'carry_rank',
+      object: 'task',
+      events: ['beforeInsert', 'beforeUpdate'],
+      handler: (ctx: any) => {
+        const { event, input, previous: prev } = ctx;
+        if (event === 'beforeUpdate' && prev) input.rank = prev.rank;
+      },
+    };
+    // The control: the same spelling, but read from module scope — not `ctx`.
+    const freePreviousHook = {
+      name: 'carry_rank_free',
+      object: 'task',
+      events: ['beforeUpdate'],
+      handler: (ctx: any) => {
+        const prev = previous;
+        ctx.input.rank = prev.rank;
+      },
+    };
+
+    it('lint reports nothing, and the build lowers it to a body', () => {
+      expect(checkHookBodyLowering({ hooks: [{ ...renamedKeyHook }] })).toEqual([]);
+
+      const lowering = lowerCallables({ hooks: [{ ...renamedKeyHook }] });
+      expect(lowering.bodyExtractionWarnings).toEqual([]);
+      expect(lowering.bodyExtracted).toBe(1);
+      const [hook] = lowering.lowered.hooks as Array<{ body?: { source: string } }>;
+      expect(hook.body?.source).toContain('previous: prev');
+    });
+
+    it('a genuinely free `previous` is still refused, by both', () => {
+      const issues = checkHookBodyLowering({ hooks: [{ ...freePreviousHook }] });
+      expect(issues).toHaveLength(1);
+      expect(issues[0].severity).toBe('error');
+      expect(issues[0].rule).toBe(NOT_LOWERABLE_RULE);
+      expect(issues[0].path).toBe('hooks[0].handler');
+
+      const lowering = lowerCallables({ hooks: [{ ...freePreviousHook }] });
+      expect(lowering.bodyExtracted).toBe(0);
+      expect(lowering.bodyExtractionWarnings.map((w) => [w.kind, w.freeIdentifiers])).toEqual([
+        ['free-identifiers', ['previous']],
+      ]);
+    });
+  });
+
   it('keeps the STRUCTURAL refusal a warning — the bundle is its designed answer', () => {
     const issues = checkHookBodyLowering({ hooks: [forbiddenTokenHook] });
 

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

@@ -75,6 +75,65 @@ describe('detectFreeIdentifiers (#1876 — body self-containment)', () => {
     }
   });
 
+  // A renamed destructuring key — `const { previous: prev } = ctx` — reads
+  // `previous` OFF `ctx`; the only name it introduces is `prev`. The key is a
+  // property name exactly like the `key` of `{ key: value }`, so counting it as
+  // a free identifier refused a self-contained handler (and bundled it), with
+  // advice — "reach them through `ctx`" — describing what it already did.
+  describe('a destructuring KEY is not a reference; its alias is the binding', () => {
+    const lowerable: Array<[string, string]> = [
+      [
+        'a renamed key off ctx, beside shorthand keys',
+        "(ctx) => { const { event, input, previous: prev } = ctx; if (event === 'beforeUpdate' && prev) input.n = prev.n; }",
+      ],
+      ['a nested renamed pattern', '(ctx) => { const { a: { b: c } } = ctx; return c; }'],
+      [
+        'a defaulted alias whose default is in scope',
+        '(ctx) => { const d = ctx.fallback; const { key: alias = d } = ctx; return alias; }',
+      ],
+      [
+        'a computed key and a default, both in scope',
+        '(ctx) => { const k = ctx.k; const d = 0; const { [k]: v = d } = ctx; return v; }',
+      ],
+      // The same element shape reached two other ways.
+      ['an object pattern inside an array pattern', '(ctx) => { const [{ previous: prev }] = ctx.items; return prev; }'],
+      ['a nested callback parameter', '(ctx) => ctx.items.map(({ previous: prev }) => prev.n)'],
+      ['a catch-clause pattern', '(ctx) => { try { ctx.run(); } catch ({ message: msg }) { ctx.log = msg; } }'],
+      // No key at all: elements are bound by position, so nothing to mistake.
+      ['an array pattern', '(ctx) => { const [first, second] = ctx.items; return first + second; }'],
+    ];
+    for (const [label, source] of lowerable) {
+      it(label, () => {
+        const r = detectFreeIdentifiers(source);
+        expect(r.unparsed).toBe(false);
+        expect(r.free).toEqual([]);
+      });
+    }
+
+    // The controls: what the element still READS is still judged. Only the key
+    // stopped counting — a default or a computed key that names something out
+    // of scope is as free as it ever was.
+    it('a defaulted alias whose default is free is still reported', () => {
+      const r = detectFreeIdentifiers('(ctx) => { const { key: alias = FALLBACK } = ctx; return alias; }');
+      expect(r.free).toEqual(['FALLBACK']);
+    });
+
+    it('a shorthand element whose default is free is still reported', () => {
+      const r = detectFreeIdentifiers('(ctx) => { const { previous = FALLBACK } = ctx; return previous; }');
+      expect(r.free).toEqual(['FALLBACK']);
+    });
+
+    it('a computed key that is free is still reported', () => {
+      const r = detectFreeIdentifiers('(ctx) => { const { [KEY]: v } = ctx; return v; }');
+      expect(r.free).toEqual(['KEY']);
+    });
+
+    it('a genuinely free `previous` (no ctx source) is still reported', () => {
+      const r = detectFreeIdentifiers('(ctx) => { const prev = previous; ctx.input.n = prev.n; }');
+      expect(r.free).toEqual(['previous']);
+    });
+  });
+
   describe('real compiled `.toString()` shapes', () => {
     it('does not flag a self-contained closure', () => {
       const handler = (ctx: any) => {
@@ -83,6 +142,15 @@ describe('detectFreeIdentifiers (#1876 — body self-containment)', () => {
       };
       expect(detectFreeIdentifiers(src(handler)).free).toEqual([]);
     });
+
+    it('does not flag a renamed destructuring key after the compiler has had it', () => {
+      const handler = (ctx: any) => {
+        const { event, input, previous: prev } = ctx;
+        if (event === 'beforeUpdate' && prev) input.n = prev.n;
+      };
+      expect(src(handler)).toContain('previous: prev');
+      expect(detectFreeIdentifiers(src(handler)).free).toEqual([]);
+    });
   });
 
   it('never invents free vars for non-handler junk (conservative — caller won\'t block)', () => {

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

@@ -227,8 +227,9 @@ function collectBindings(fn: ts.FunctionLikeDeclarationBase): Set<string> {
 /**
  * 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.
+ * object/class member keys, non-computed destructuring 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>();
@@ -253,6 +254,19 @@ function collectReferences(fn: ts.FunctionLikeDeclarationBase): Set<string> {
       walk(node.initializer);
       return;
     }
+    if (ts.isBindingElement(node)) {
+      // `{ key: alias = d } = ctx` — the same rule as `{ key: value }` above:
+      // `key` names a property READ OFF the source, so it is not a ref (unless
+      // computed). Visit the binding name (an alias is subtracted via
+      // `bindings`; a nested pattern recurses here), a computed key's
+      // expression, and the default — `{ [k]: v = d }` reads `k` and `d`.
+      if (node.propertyName && ts.isComputedPropertyName(node.propertyName)) {
+        walk(node.propertyName.expression);
+      }
+      walk(node.name);
+      if (node.initializer) 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); });