Skip to content

Commit 6d36017

Browse files
fix(cli)!: objectstack build rebinds a hook handler's parameter to the sandbox's ctx, and refuses the parameter lists it cannot rebind (#22837) (#22859)
Fixes #22837 Clause-②: yes (narrowing) ## What changes `objectstack build` lowers a hook handler's statements into `body.source`, and the runtime runs a body as `(async (ctx) => { SOURCE })`, so `ctx` is the only name a body is given. The lowering dropped the handler's parameter list. A handler named `async (hookCtx) => …` or `async ({ input }) => …` lowered at exit 0 and threw `ReferenceError` on every run. - **The rebind.** `planHookParameterRebind` in `packages/cli/src/utils/extract-hook-body.ts`. On the hook face the handler's one parameter is re-emitted as the body's first statement: `var hookCtx = ctx;`, `var { input, previous } = ctx;`. The parameter is read off the TypeScript parse that `detectFreeIdentifiers` already performs, never off the regex peel, so the free-identifier verdict and the rebind agree on what the parameter binds. Nothing is emitted for a parameter named `ctx`, or for no parameter. Those lower byte-identically: pinned by requiring the hook face's output to equal the unchanged action face's over six shapes. - **The residue (A), refused as `forbidden-token`, naming the parameter.** Covered: more than one parameter, a default value, a rest parameter. Also covered: a pattern that binds the name `ctx`, and a body that declares `ctx` (or the parameter's own name) with `let` / `const` / `class` / `function` where the prologue would read it. Last, a destructured `log` / `crypto` / `title` / `api` taken apart further than a plain name, which the capability inference cannot follow. The handler is bundled, as `.sudo(` is. - **Capability inference follows the rebound names.** `CAPABILITY_PATTERNS` spells `ctx.log.` and `ctx.crypto.randomUUID`. `reboundCapabilities` re-spells each with the receiver renamed, and nothing else, for an identifier parameter, an object-rest element, and a destructured `log` / `crypto` / `title`. A renamed handler infers exactly what the same handler named `ctx` infers; seven pairs are pinned. - **The enumeration pin that closes the family.** It lives in `packages/cli/test/lowered-body-face-enumeration.test.ts`. It derives the in-process face from `HookContextSchema`'s source (`packages/spec/src/data/hook.zod.ts`, one level into each `z.object`, tombstones excluded). It derives the body face from `buildSandboxContext`'s source (`packages/runtime/src/sandbox/body-runner.ts`, one level into an object literal or a passthrough). Every member absent from the body must be refused, which is measured behaviourally through `extractHookBody`, or recorded in `DELIBERATE_EXCEPTIONS` with its reason. Stale exceptions red too. Both reads are declared in `scripts/cross-package-test-inputs.mjs` and `turbo.json`. Neither package is edited. - **What the pin found.** It found six absent members. Two were already refused (`dispatch.scope`, `submitted`). Three were unrefused, and this PR now refuses them as the family's own rule prescribes (the VM lacks them): `ctx.provenance`, `ctx.ql`, `ctx.transaction`. They are hook face only, and `ctx.api.transaction(fn)` is exempt. One is a recorded exception: `id`, which none of the five hook-context assembly sites in objectql's `engine.ts` sets, so it is absent on both faces. - **Parameter-list destructuring now meets the member refusals.** `async ({ input, submitted }) => …` and `async ({ dispatch: { scope } }) => …` used to lower, because the scan read a body that had lost its parameter list. The scan now reads the rebound source. - **The face split.** `LoweringFace` is `hook | action`. An action body runs as `(async (input, ctx) => …)`, a different binding list. `lowerCallables` and `os lint`'s `hook-body-lowering` rule pass the face of the slot, so an action target lowers exactly as before. The published `extractHookBody` keeps its signature and is the hook face. - **`parseFunction`'s span check** (`detect-free-identifiers.ts`, an in-place fix: see Deviations). A method shorthand holding exactly one nested function returned the NESTED function as the handler. Measured on the base: the nested arrow's parameter was read as the handler's, a method local it closed over was refused as free, and a module-scope helper outside it lowered unreported. The parse now accepts only the node that spans the whole source. - **The door, the docs, the changeset.** In `lowered-body-door.dogfood.test.ts`, `lbd_param` moves from documenting the `500` to proving the rebind, with a destructured cell beside it and the residue (`lbd_two`) bundled like `lbd_stash`. `content/docs/automation/hook-bodies.mdx` states the rebind and the new refusals. The changeset is BREAKING (narrowing), with before and now per door. ## Premise, re-measured on `origin/main` `1eff3224d` before any code - `peelToBlockBody` keeps the block and drops the header. Through esbuild (`bundle-require`, as `loadConfig` loads a config), `async (hookCtx: HookCtx) => { hookCtx.input.note = 'a' }` became `body.source` = `hookCtx.input.note = "a";` with nothing binding `hookCtx`. - `quickjs-runner.ts`'s L2 wrapper is `(async (ctx) => { SOURCE })` for hook and job origins, and `(async (input, ctx) => …)` for actions. - `lbd_param` asserted `SandboxError` / `500`. ## Mechanism assumptions (order §2), measured 1. Holds, all three. 2. These spellings reach `peelToBlockBody` after esbuild: `async (x) => {}`. Bare `async x =>` is printed with parentheses, and a TypeScript annotation is stripped. Also `async function (x)`, a named function, a method shorthand, a destructuring and a nested pattern, and `async () => {}`. All rebind or refuse as above. The regex peel itself mis-peels some bodies. That is pre-existing and unchanged here; see Acceptance notes. 3. The pin derives, can fail, and the cross-package reads are declared (`check:cross-package-test-inputs` green). Ablations are below. 4. The in-process face is `HookContextSchema` (the declared contract). The producer side is cross-checked by hand: all five `HookContext` literals in objectql's `engine.ts` set `provenance`, `transaction` and `ql`, and none sets `id`. ## Measured at the doors | door | base `1eff3224d` | this PR | |---|---|---| | `bootStack(config, { artifact })`, `lbd_param` (`hookCtx`) | `SandboxError: … ReferenceError: 'hookCtx' is not defined`, REST `500`, nothing stored | both boots store `named:alpha`; REST `201` on both, the same stored note | | same, `lbd_destructure` (`({ input })`) | (new cell) under the ablation below: `ReferenceError: 'input' is not defined`, `500` | both boots store the same row, `201` | | same, `lbd_two` (`(ctx, suffix)`) | lowered | bundled, no body; in-process stores `two:alpha` | | `objectstack build`, a two-parameter hook | lowered at exit 0 | bundled, exit 0, with a warning naming its two parameters, `ctx` and `extra`; a renamed hook in the same app ships `var hookCtx = ctx;` | | `objectstack build --strict-body`, same | exit 0 | exit 1, naming `hook 'two_params'` (measured on a scratch app built through `bin/run-dev.js`) | | `os lint`, same | silent | `hook-body/bundled-fallback` warning at `hooks[i].handler` | | `os lint`, an `(input, ctx)` action target | silent | silent (action face) | **The example corpus**, lowered with base and with this head (`lowerCallables` over `loadConfig` of each `examples/*/objectstack.config.ts`): `app-crm`, `app-multi-package` and `app-showcase` are byte-identical. `app-todo`'s `task_logic` moves from lowered to bundled, refused for `ctx.ql` (it reads the kernel logger off `ctx.ql`). Its lowered body was already a silent no-op: it reads `ctx.input.data`, which the body face does not carry, so `if (!data) return;` returned before the completion stamp. Bundled, it runs in-process again wherever the runtime module is served. ## Tests - `pnpm --filter @objectstack/cli exec vitest run --project unit --maxWorkers=2`: 283 files, 4231 tests passed. This was the full `unit` tier, run on `e006f1a65`. The only delta to `a27cba278` renames a private option and deletes two comment lines, and the targeted re-run below covers it. The `integration` tier is declared to CI: the diff touches no spawn entry or driver boot path. - Targeted re-run on `a27cba278`: eight cli files, 179 tests passed (the rebind, enumeration, extractor, lowering, free-identifier, lint rule, refusal-kind and published-subpath pins). `lowered-body-door.dogfood.test.ts`: 13 passed. - `pnpm --filter @objectstack/cli typecheck` (tsc plus the test layer): green; `check:test-typecheck` holds its ledger unchanged. ## Ablations (committed first, mutated through `scripts/ablation-replace.mjs`, each restored to HEAD's blob with an empty `git diff HEAD`) Each subject resolves from source (relative imports to `packages/cli/src`, and the pin reads the two source files directly), so no `dist/` is in the path. 1. `rx: SUBMITTED_RX,` was replaced with a never-matching `rx`. The enumeration pin went red on exactly `['submitted']`. 2. A fake member `ablationOnlyMember` was added to `HookContextSchema`'s source (`packages/spec`, mutated for the run and restored, never committed). The pin went red on exactly `['ablationOnlyMember']`. A first attempt used an anchor its own replacement contained, and the tool refused it before the test ran (anchor count 1 to 1). That run is void and was redone with a disjoint anchor. 3. The rebind was disabled (`const rebind = NO_REBIND;`). The door went red on 6 of 13 tests, with the card's failure exactly: `ReferenceError: 'hookCtx' is not defined`, `expected 500 to be 201`, `ReferenceError: 'input' is not defined`, and `lbd_two ships no body`. ## Gates All on `a27cba278`. `node scripts/pm/dispatch-gates.mjs --commands`, re-derived from this diff with no paths, gives 113 families: every family derived at dispatch except `pnpm lint` (below), plus the ones the diff adds, such as `check:adr-0087-registration`, `check:changeset-no-major` and `check:empty-changeset`. All 113 exited 0. Reconciled with `--ran`: "113 derived famil(ies) accounted for — 113 run, 0 NOT-MEASURED (a DERIVED zero — all 113 recorded an exit code and none of them is 3)". - Two gates first answered `PREREQUISITE NOT MET` (exit 3) because eight packages had never been built in this worktree: `check:skill-examples` (`client-react`) and `check:dual-build-cjs-loads`. I built those packages (57 of 57 turbo tasks were cache hits) and re-ran both: exit 0. - `check:adr-0087-registration` reads one declared-breaking changeset: `[BREAKING+bang+clause-②-narrowing] not-required (no-migration-prescription)`. - `check:cross-package-test-inputs` reads: "30 package(s) read outside themselves, all declared". - `pnpm --filter @objectstack/cli typecheck` and `pnpm --filter @objectstack/dogfood typecheck` exit 0 on `a27cba278`. `--listFilesOnly` shows each new or edited test inside a program: the two `src/` tests in `tsconfig.json`, the two `test/` files in `tsconfig.test.json`, and the door in dogfood's. **Lint.** I ran a proven narrowing rather than `pnpm lint`, with three pieces of evidence. First, the population: eslint's own config over this branch's 14 files, of which `--format json` reports 11 linted and 3 with no matching configuration (the changeset, the `.mdx`, `turbo.json`). Second, the result: 0 errors and 0 warnings. Third, invariance: `eslint.config.mjs` enables no type-aware linting (no `parserOptions.project` anywhere, as its own header states), so no verdict on an untouched file can move with this diff. The repo-wide `pnpm lint` run belongs to CI. ## Deviations from the order and the ruling text - **`var`, not the ruling's literal `const`.** A parameter is a mutable, function-scoped binding. `const` would TypeError on a handler that reassigns its parameter, which runs fine in-process (pinned in "lets the body reassign its parameter"). A `let` or `const` would also SyntaxError against a body's own `var` of the same name. `var` is the one spelling with a parameter's semantics. - **Three member refusals the card did not name** (`provenance`, `ql`, `transaction`). They are the enumeration pin's classification under the ruling's either/or: none of the three has a reason to be a deliberate exception. - **`parseFunction`'s span check is an in-place fix** in `detect-free-identifiers.ts`, under the bounded exemption. It is the same family; the rebind depends on it, because without it a method shorthand with one inner arrow gets that arrow's parameter rebound; the fix is mechanical (one span comparison); no other claim holds the file; and the gates are the same. - **Files beyond the claim's named surface:** `detect-free-identifiers.ts` (+ test), `lint/hook-body-lowering.ts` (+ test), `hook-body.ts` (docblock only), `scripts/cross-package-test-inputs.mjs`, `turbo.json`. Each is required by the face split, the span fix or the pin's declared reads. No `packages/runtime` or `packages/spec` file is edited. - **A measured widening, for the seat's Clause-② call.** The span fix makes one shape lower that was refused: a method shorthand with exactly one nested function closing over a method local. `--strict-body` and `os lint` therefore accept that shape where they refused it. The line above is the claim's `no (narrowing)`, copied as ordered; by `scripts/pm/clause2-line.mjs`'s arms the honest reading may be `yes (narrowing)`. - **Overlap with #22849 (#22839)** on `packages/cli/src/utils/lower-callables.ts`. Both PRs add a parameter on the same four lines: `tryExtractBody`'s signature, the hook call site, `lowerActionCallable`'s `tryExtract` type, and the action call site. That PR adds `ref`, this one adds `face`. The resolution is mechanical (`tryExtractBody(fn, originLabel, ref, face)`). Nothing here touches the counting or `bodylessCallables` code. ## Acceptance notes - **Out of scope, reported for the seat: the regex peel mis-peels some bodies, at exit 0.** Measured through a real `objectstack build` of a scratch app on this branch; the peel is unchanged from base. Two cases ship a `body.source` that is not valid JS (`SyntaxError` under `new Function`). The first is a `}` inside a string, template or regex literal: `ctx.input.note = 'a}b'` ships as `ctx.input.note = "a`. The second is a function or method form with a statement-level parenthesised inner arrow, which ships as `return (a + "!"; …});`. The build's only signal is `hook-body-source-unparseable`, which tells the author to fix a syntax error their handler does not have. A third case, a `{` inside a string, is refused as `unparseable` (bundled, the safe direction). This is the same family. The TypeScript node this PR already reads holds the right body span. - The action face has the same parameter question: a target written `(params, context)` loses both names. No example or package ships an inline action target, the in-process action calling convention was not measured here, and the action face's lowering is unchanged by this PR. - `os lint`'s write-set extractor keys on the literal `ctx.input.FIELD`. A rebound body writes `hookCtx.input.FIELD`, which it does not follow, exactly as it does not follow an in-body `const c = ctx`. This is a coverage gap, not a regression. - The `ctx.input` envelope split (`input.data` exists in-process and not in the body) is a documented, deliberately undecided divergence on `HookContextSchema.input`. The pin treats `input` as opaque. --- _Generated by [Claude Code](https://claude.ai/code/session_01B5CHJNXuuqzChM4w6hkTN4)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 55382dc commit 6d36017

14 files changed

Lines changed: 1373 additions & 104 deletions
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
---
2+
"@objectstack/cli": minor
3+
---
4+
5+
fix(cli)!: `objectstack build` rebinds a hook handler's parameter to the sandbox's `ctx` instead of dropping it, and refuses the parameter lists it cannot rebind (#22837)
6+
7+
Clause-②: yes (narrowing)
8+
9+
The lowering ships a hook handler's statements as `body.source`, and the runtime runs a body as `(async (ctx) => { SOURCE })`, so `ctx` is the only name a body is given. The handler's parameter list was dropped. A handler written `async (hookCtx) => { hookCtx.input.note = … }`, or `async ({ input }) => { input.note = … }`, lowered at exit 0 with no warning, and its body threw `ReferenceError: 'hookCtx' is not defined` on every run. Measured through `bootStack`'s artifact door: REST answered `500 INTERNAL_ERROR` and stored nothing, while the same handler in-process answered `201`.
10+
11+
The hook body extractor (`extractHookBody`) now re-emits the handler's one parameter as the body's first statement, `var hookCtx = ctx;` or `var { input } = ctx;`, so the handler's own code runs as written. Both boots of the door above now store the same row. `os lint` calls the same extractor, so it agrees.
12+
13+
**BREAKING: what moves for consumers.**
14+
15+
- **A hook handler whose parameter is not named `ctx`.** It was lowered to a body that failed in the sandbox. It is now lowered to a body that runs. A handler whose parameter is `ctx`, or that takes none, lowers byte-for-byte as before.
16+
- **A hook handler whose parameter list cannot be rebound.** This covers more than one parameter, a default value or a rest parameter on it, a destructuring pattern that binds the name `ctx`, a body that declares its own `ctx` with `let` / `const` / `class` / `function`, and a destructured `log` / `crypto` / `title` / `api` taken apart further than one plain name (capability inference cannot follow that). Such a handler was lowered to a body that failed, or that ran with a parameter the sandbox never bound. It is now bundled into the runtime module (`objectstack-runtime.*.mjs`), as `.sudo(` and `fetch(` already are, with a warning that names the hook and the parameter. `objectstack build` still exits 0, `objectstack build --strict-body` now fails on it (exit 1), and `os lint` reports `hook-body/bundled-fallback` at `hooks[i].handler`. A handler taking `(ctx, extra)` is refused even when `extra` is unused: an in-process hook handler is only ever called with the context.
17+
- **A hook handler reaching `ctx.provenance`, `ctx.ql` or `ctx.transaction`.** These are the rest of the in-process `HookContext` that the sandbox does not carry. A body reads `ctx.provenance` as `undefined`, so a guard on `ctx.provenance.flowRunId` evaluated as if the write had no origin. `ctx.ql` (the engine) and `ctx.transaction` (a driver handle) are live host objects the sandbox cannot hold. Such a handler is now bundled, with the same warning, build exit code, `--strict-body` failure and lint rule as above. `ctx.api.transaction(fn)`, `ctx.api.object(name)` and a field merely named `provenance`, `ql` or `transaction` read through `ctx.input` or `ctx.previous` are not refused.
18+
- **A member destructured in the parameter list now meets the refusal the body spelling meets.** `async ({ input, submitted }) => …` and `async ({ dispatch: { scope } }) => …` were lowered (and failed). They are now refused like `const { submitted } = ctx`.
19+
- **A method-shorthand handler with exactly one nested function** (`async handler(ctx) { … rows.map((r) => r.id) … }`) is now judged on the method rather than on the nested function. A method-body name that nested function closes over was reported as a free identifier, and the handler was bundled. It now lowers. A module-scope helper called outside the nested function went unreported, and the handler lowered and threw `ReferenceError`. It is now refused and bundled, like any other module-scope reference.
20+
- **The published `extractHookBody` (`@objectstack/cli/hook-body`) is the hook face.** Its signature is unchanged. An action's `(input, ctx)` target passed to it is refused for its second parameter, where `objectstack build` lowers it unchanged: an action body runs in its own `(input, ctx)` wrapper and gets no rebind.
21+
22+
**The one-line fix**, for a refused handler you want shipped as a body: take the context as the one parameter (under any name, or destructured), drop the default, and read the member you need through `ctx.api` or `ctx.previous`. A handler you are content to run bundled needs no change.
23+
24+
**Unchanged.** No spec key, export, type or stored shape moves, and `extractHookBody`'s published signature and refusal kinds are the same. The sandbox still marshals exactly what it did. An action target's lowering is unchanged. An in-process handler, a top-level `functions:` entry referenced by name, and a hook that carries an explicit `body` are not lowered here and are not judged.
25+
26+
<!-- adr-0087: not-required (no-migration-prescription) a build-time verdict about handler code: the rebind makes a handler that failed in the sandbox run as written, and every refusal is of handler code the sandbox cannot run as written or of a parameter list the rebind cannot re-emit faithfully. No authorable key, spelling, export or stored shape moves, and no stored row is read, rewritten or converted. A refused handler keeps running bundled, so no consumer has to edit anything for its build to succeed, and the edit that makes it a body again is a change to handler code, which no ledger entry can derive. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id covers the lowering (not already-registered); and the change is a build verdict, not a declaration (not runtime-interface-only or type-surface-only). -->

‎content/docs/automation/hook-bodies.mdx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,8 +169,9 @@ The CLI builder **rejects** any source that uses:
169169
- references to identifiers from value-only top-level imports
170170
- `.sudo(` and `.create(` — members that are real on the **in-process** `ScopedContext` / `ObjectRepository` and absent from the VM's `ctx.api`, so lowering them would ship a body that `TypeError`s on its first run. Write `.insert({ ... })` instead of `.create({ ... })` — it is the only insert verb the `IScopedObjectRepository` contract declares — and reach for [`runAs: 'system'`](/docs/automation/hooks#elevation--runas) instead of `.sudo()`. `Object.create()` is a real sandbox global and is **not** affected.
171171
- `ctx.dispatch.scope` and `ctx.submitted` — context members the **in-process** `HookContext` carries and the body face deliberately does not: a body's `ctx.dispatch` is `{ mode, index }` only, and it has no `ctx.submitted`. Lowered, a write through `scope` `TypeError`s on its first run and a read of `submitted` sees `undefined`, so a guard on it evaluates as if nothing were submitted. For cross-dispatch work, do it once at `ctx.dispatch.index === 0` or keep the state in the record through `ctx.api`; to derive from a read-only field, read the stored row on `ctx.previous`. `ctx.dispatch.mode` / `ctx.dispatch.index`, and a field that merely happens to be named `scope` or `submitted` (`ctx.input.scope`), are **not** affected.
172+
- `ctx.provenance`, `ctx.ql` and `ctx.transaction` (hook handlers) — the rest of the in-process `HookContext` the body face does not carry. A body reads `ctx.provenance` as `undefined`, so a guard on `ctx.provenance.flowRunId` evaluates as if the write had no origin; `ctx.ql` (the engine) and `ctx.transaction` (a driver handle) are live host objects no copy into the sandbox can carry. Reach other objects through `ctx.api.object(name)`, and group writes with `ctx.api.transaction(fn)`, which is **not** affected. The enumeration behind this list is derived, not remembered: a test compares every member `HookContextSchema` declares against every member the sandbox marshals, and fails on one that is neither refused here nor recorded as a deliberate exception.
172173

173-
Like `.sudo(` and `.create(`, these two refuse the **body**, not the handler: `objectstack build` bundles the handler into its runtime module (`objectstack-runtime.*.mjs`) instead and still exits 0 (with a warning naming the hook), `os lint` reports it as `hook-body/bundled-fallback`, and `objectstack build --strict-body` fails on it. The bundled handler runs in-process wherever that module is served beside `objectstack.json`; a deployment that serves the artifact alone has no function to bind the hook to, so it logs the hook as refused at boot and the hook never fires — build with `--strict-body` when the artifact must be body-only.
174+
Like `.sudo(` and `.create(`, these refuse the **body**, not the handler: `objectstack build` bundles the handler into its runtime module (`objectstack-runtime.*.mjs`) instead and still exits 0 (with a warning naming the hook), `os lint` reports it as `hook-body/bundled-fallback`, and `objectstack build --strict-body` fails on it. The bundled handler runs in-process wherever that module is served beside `objectstack.json`; a deployment that serves the artifact alone has no function to bind the hook to, so it logs the hook as refused at boot and the hook never fires — build with `--strict-body` when the artifact must be body-only.
174175

175176
Need outbound HTTP? Define a **Connector recipe** as metadata and call it via `ctx.connector(...)`. (Connector spec is tracked separately and ships after L1+L2 stabilises.)
176177

@@ -235,6 +236,8 @@ Because the existence check is advisory, and every write-side check here is lite
235236

236237
Hooks mutate `ctx.input`/`ctx.result`; actions return their output value explicitly.
237238

239+
A hook handler's parameter does not have to be named `ctx`. The sandbox binds only `ctx`, so the build re-emits the handler's one parameter as the body's first statement: `async (hookCtx) => { … }` ships as `var hookCtx = ctx; …`, and `async ({ input, previous }) => { … }` as `var { input, previous } = ctx; …`. A handler whose parameter already is `ctx`, or that takes none, ships unchanged. What cannot be re-emitted that way is refused like a forbidden token — the handler is bundled, with a warning naming the parameter: more than one parameter (an in-process hook handler is only ever called with the context), a default value or a rest parameter on it, a pattern that binds the name `ctx` itself, a body that declares its own `ctx`, and a destructured `log` / `crypto` / `title` / `api` taken apart further than one plain name (capability inference cannot follow it). An action's `(input, ctx)` parameters are its own wrapper's and are left as they are.
240+
238241
An action's `ctx` is **not** a hook's. `ctx.input` is the action's **params bag** — validated against its declared `params`, not a record. `ctx.record` is the record the dispatcher pre-fetched, and it is **read-only in effect**: the sandbox receives a plain snapshot and the runtime never writes it back, so `ctx.record.<field> = …` is discarded even for a perfectly valid field name. There is exactly one way an action body persists anything:
239242

240243
```js

‎packages/cli/src/hook-body.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,22 @@
55
*
66
* ## Why this entry exists (#15325)
77
*
8-
* `extractHookBody` decides whether a hook or script action is still shippable
8+
* `extractHookBody` decides whether a hook handler is still shippable
99
* **body-only**: it peels the handler to its statements, refuses the forbidden
1010
* tokens, infers capabilities, and throws a `HookBodyExtractionError` carrying
1111
* `kind` / `freeIdentifiers` / `nodeOnlyIdentifiers`. `os build` applies it to
1212
* lower a handler, `os lint` calls the same function so its verdict cannot
13-
* drift from the build's. An app that wants to assert "my hooks are still
13+
* drift from the build's.
14+
*
15+
* It is the HOOK face (#22837): the handler's one parameter is re-emitted as
16+
* `var <param> = ctx;` ahead of the body, because a hook body runs with `ctx`
17+
* bound and nothing else, and a parameter list that cannot be re-emitted that
18+
* way (two parameters, a default, a rest parameter) is refused. A script
19+
* action's `target` runs in a different wrapper, `(input, ctx)`, and the build
20+
* lowers it without that rebind, so an `(input, ctx)` target passed here is
21+
* refused for its second parameter where `os build` lowers it.
22+
*
23+
* An app that wants to assert "my hooks are still
1424
* metadata-only" — and to RUN the lowered `source` through the real QuickJS
1525
* runner in a test — needs this exact function, not a lookalike: a local
1626
* reimplementation passes its own tests while diverging from the rule the

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,34 @@ describe('checkHookBodyLowering', () => {
364364
});
365365
});
366366

367+
// [#22837] The build lowers a hook handler and an action target in different
368+
// faces: a hook's one parameter is rebound to the sandbox's `ctx`, an
369+
// action's `(input, ctx)` is its own wrapper's binding list. Lint must judge
370+
// each by the face the build uses, or it reports what the build never does.
371+
describe('judges each callable in the face the build lowers it in (#22837)', () => {
372+
it('says nothing about a hook handler whose parameter is renamed: the build rebinds it', () => {
373+
const issues = checkHookBodyLowering({
374+
hooks: [{ name: 'named', object: 'o', events: ['beforeInsert'], handler: async (hookCtx: any) => { hookCtx.input.n = 1; } }],
375+
});
376+
expect(issues).toEqual([]);
377+
});
378+
379+
it('reports a two-parameter hook handler on the bundled-fallback rule, naming the parameters', () => {
380+
const issues = checkHookBodyLowering({
381+
hooks: [{ name: 'two', object: 'o', events: ['beforeInsert'], handler: async (ctx: any, extra: any) => { ctx.input.n = extra; } }],
382+
});
383+
expect(issues.map((i) => [i.rule, i.severity, i.path])).toEqual([[BUNDLED_FALLBACK_RULE, 'warning', 'hooks[0].handler']]);
384+
expect(issues[0].message).toContain('(`ctx`, `extra`)');
385+
});
386+
387+
it('says nothing about an `(input, ctx)` action target, which the action face lowers unchanged', () => {
388+
const issues = checkHookBodyLowering({
389+
actions: [{ name: 'a', target: async (input: any, ctx: any) => { await ctx.api.object('t').insert({ n: input.n }); } }],
390+
});
391+
expect(issues).toEqual([]);
392+
});
393+
});
394+
367395
/**
368396
* The #3782 class: two surfaces disagreeing about what an author is told.
369397
* This rule and the build both call `extractHookBody` on the same normalized

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

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -89,14 +89,15 @@
8989
*
9090
* ## Parity
9191
*
92-
* This rule calls the SAME `extractHookBody` that `lowerCallables` calls, on
93-
* the same normalized input. It therefore cannot drift from what `os build`
94-
* would do to the same handler — the #3782 class (two surfaces disagreeing
95-
* about what an author is told) is closed by construction here, not by a
96-
* second implementation kept in sync by hand.
92+
* This rule calls the SAME `extractLoweredBody` that `lowerCallables` calls, on
93+
* the same normalized input and with the same face (`hook` for a hook handler,
94+
* `action` for an action target — #22837 made the two lower differently). It
95+
* therefore cannot drift from what `os build` would do to the same handler — the
96+
* #3782 class (two surfaces disagreeing about what an author is told) is closed
97+
* by construction here, not by a second implementation kept in sync by hand.
9798
*/
9899

99-
import { extractHookBody, HookBodyExtractionError } from '../utils/extract-hook-body.js';
100+
import { extractLoweredBody, HookBodyExtractionError, type LoweringFace } from '../utils/extract-hook-body.js';
100101

101102
/** Mirrors `LintIssue` in `../commands/lint.ts` (structurally compatible). */
102103
export interface HookBodyLintIssue {
@@ -127,13 +128,13 @@ const isPlainObject = (v: unknown): v is Record<string, unknown> =>
127128
type AnyFn = (...args: unknown[]) => unknown;
128129

129130
/**
130-
* Run `extractHookBody` over one callable and turn a refusal into an issue.
131+
* Run `extractLoweredBody` over one callable and turn a refusal into an issue.
131132
* Returns `null` when the body extracts cleanly — i.e. the callable really does
132133
* ship as metadata.
133134
*/
134-
function judge(fn: AnyFn, originLabel: string, path: string): HookBodyLintIssue | null {
135+
function judge(fn: AnyFn, originLabel: string, path: string, face: LoweringFace): HookBodyLintIssue | null {
135136
try {
136-
extractHookBody(fn, originLabel);
137+
extractLoweredBody(fn, originLabel, face);
137138
return null;
138139
} catch (err: unknown) {
139140
const kind = err instanceof HookBodyExtractionError ? err.kind : 'unknown';
@@ -240,7 +241,7 @@ export function checkHookBodyLowering(config: Record<string, unknown>): HookBody
240241
if (raw.body) return; // author supplied the body themselves
241242
const name =
242243
typeof raw.name === 'string' && raw.name.length > 0 ? raw.name : 'anon_hook';
243-
const issue = judge(raw.handler as AnyFn, `hook '${name}'`, `hooks[${i}].handler`);
244+
const issue = judge(raw.handler as AnyFn, `hook '${name}'`, `hooks[${i}].handler`, 'hook');
244245
if (issue) issues.push(issue);
245246
});
246247
}
@@ -258,6 +259,7 @@ export function checkHookBodyLowering(config: Record<string, unknown>): HookBody
258259
raw.target as AnyFn,
259260
`action '${baseName}'`,
260261
`${pathPrefix}[${i}].target`,
262+
'action',
261263
);
262264
if (issue) issues.push(issue);
263265
});

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { describe, it, expect } from 'vitest';
44
import {
55
detectFreeIdentifiers,
66
NODE_ONLY_GLOBALS,
7+
parseFunction,
78
SANDBOX_GLOBALS,
89
} from './detect-free-identifiers.js';
910

@@ -153,6 +154,33 @@ describe('detectFreeIdentifiers (#1876 — body self-containment)', () => {
153154
});
154155
});
155156

157+
// [#22837] A method shorthand is not a statement, so the first wrap parsed it
158+
// as a call and a block; with exactly ONE function nested in that block, the
159+
// nested function was the only function-like node and was returned as the
160+
// handler. Measured before the span check: both verdicts below were wrong.
161+
describe('reads the handler itself, never a function nested in it (#22837)', () => {
162+
const method = 'async h(ctx) { const k = 2; ctx.input.x = [1].map((i) => i * k); }';
163+
164+
it('a method local that one nested function closes over is bound, not free', () => {
165+
expect(detectFreeIdentifiers(method).free).toEqual([]);
166+
});
167+
168+
it('a module-scope helper called outside that nested function is free', () => {
169+
const r = detectFreeIdentifiers('async h(ctx) { ctx.input.x = helper(ctx.input.n); [1].map((i) => i); }');
170+
expect(r.free).toEqual(['helper']);
171+
});
172+
173+
it('the node returned is the method, whose parameter list is the handler\'s', () => {
174+
const fn = parseFunction(method);
175+
expect(fn && fn.parameters.map((p) => p.getText())).toEqual(['ctx']);
176+
});
177+
178+
it('CONTROL: an arrow and a function expression with one nested function were never affected', () => {
179+
expect(parseFunction('async (ctx) => { [1].map((i) => i); }')?.parameters.map((p) => p.getText())).toEqual(['ctx']);
180+
expect(parseFunction('async function (h) { [1].map((i) => i); }')?.parameters.map((p) => p.getText())).toEqual(['h']);
181+
});
182+
});
183+
156184
it('never invents free vars for non-handler junk (conservative — caller won\'t block)', () => {
157185
// Whether or not TS error-recovers a node, the safe outcome is no free vars
158186
// so extraction is never blocked on garbage. (peelToBlockBody rejects such

0 commit comments

Comments
 (0)