Skip to content

Commit 40bc51f

Browse files
fix(cli): a lowered hook body is read off the handler's parse, not by counting braces (#22860) (#22880)
Fixes #22860 Clause-②: no ## What changed `objectstack build` lowers an inline handler to a metadata `body`, and `os lint` calls the same extractor. That extractor found the body by counting `{` / `}` and `(` / `)` through `String(fn)`. It was blind to strings, templates, regexes and comments, and it took the first parenthesised `=>` anywhere as the handler's header. `peelToBlockBody` in `packages/cli/src/utils/extract-hook-body.ts` now reads the body off the handler's TypeScript parse. The node is `parseFunction`'s, the one the parameter rebind and `detectFreeIdentifiers` already read. - A block body is the text between the block's own braces. - An implicit-return arrow is its expression, taken from the end of `=>` with leading whitespace trimmed and wrapped as `return (…);`, as before. The parse runs once per callable and feeds both the peel and the rebind. `findTopLevelArrow`, `matchBalancedIndex` and `sliceBalanced` are deleted. All three were module-private, and `extractLoweredBody` was the peel's only caller. **Landing point:** `extract-hook-body.ts` only, plus tests and a changeset. - `detect-free-identifiers.ts` and `hook-body.ts` are untouched. `parseFunction` was already exported and its node carries its source file, so nothing had to be exposed. - `compile.ts` and `lower-callables.ts` (#22853's) are untouched. - No `hook-body/*` message is reworded. ## The PM's mechanism assumptions, measured 1. **Confirmed on `69d4218058`.** The three functions count brackets with no string, template, regex or comment skipping. 2. **Reachable, same text.** `extractLoweredBody` hands the same `raw = String(fn)` to the peel, the rebind and the free-identifier pass, so the parse node reads exactly the text the peel received. - A block's node span INCLUDES its braces: `getStart` is the `{` and `end` is one past the `}`. The peel takes the text between them, which is what the scan returned. - For an expression body, `body.pos` is the end of the `=>` token, so a leading comment survives the way the scan kept it. 3. **Reproduced at the real build before the fix**, table below. ## Before and after, at the real build Scratch app, `tsx packages/cli/bin/run-dev.js build`. Each shipped body was run under the L2 wrapper `(async (ctx) => { SOURCE })`. | handler | BASE `69d4218058`, build exit 0 | after, build exit 0 | |---|---|---| | `'a}b'` in a string | SyntaxError | runs, `a}b` | | `'a{b'` in a string | bundled: "could not parse the body" | lowered, runs, `a{b` | | template `t:${…}!` | runs | runs, same bytes | | template with a literal `}` | SyntaxError | runs | | regex `/}/g` | SyntaxError | runs | | `async function` with a statement-level inner arrow | SyntaxError | runs | | `async function ({ input })` | ships `var { input } = ctx; input`: runs, stores nothing | runs, stores `fn:alpha` | | control | runs | runs, same bytes | On BASE, `hook-body-source-unparseable` fired 4 times and "could not be lowered" once. After the fix, neither fires. The balanced template already lowered correctly on BASE. The failing template member is one whose literal text carries an unbalanced brace, so both are pinned. The `function` handler with a pattern parameter is a member the card did not name, and it is the silent one. Same mechanism (the scan took the parameter pattern's `{` for the body), same fix, pinned beside the others. ## Pins - **`packages/cli/test/extract-hook-body.test.ts`: six defect pins.** - Covered: a `}` in a string, a template (balanced, and with a `}`), a regex and a comment, the inner arrow (expression-bodied, block-bodied, method), the pattern parameter, and the opening `{`. The opening-`{` pin goes through `lowerCallables` with zero `bodyExtractionWarnings`. - Each pin runs the in-process handler and its lowered body and compares them on a concrete value, so "both did nothing" cannot pass. - **The same file: a CONTROL table of 12 shapes.** Each expected `source` was MEASURED from BASE's scan over the exact text. `String(fn)` equals that text, which the test asserts. - **`packages/qa/dogfood/test/lowered-body-door.dogfood.test.ts`:** - Hooks: `lbd_brace_string` and `lbd_inner_arrow` (the two shapes the card measured), plus `lbd_fn_pattern`. - Each is driven in-process and lowered, through `hooks.run` and over REST. - An anti-vacuity pin asserts each body is the handler's own block. ## Reverse verification (one-off; nothing left in the tree) 1. **The BASE peel restored**, tree only, by a trap-armed script. - Proof of mutation: blob `a47af067c3`, equal to BASE's. Proof of restore: blob `f78625e024`, equal to HEAD's, and `git diff HEAD` is empty. - Unit file: `6 failed | 50 passed (56)`. The 6 are exactly the new defect pins, so all 12 CONTROL cases are green on BASE. - Dogfood door: `7 failed | 13 passed (20)`. - `lbd_brace_string`: SandboxError "SyntaxError: unexpected end of string". - `lbd_inner_arrow`: SyntaxError "expecting ')'". REST answered 500 where the source boot answered 201. - `lbd_fn_pattern`: stored `null` where in-process stored `pattern:alpha`, with no error at all. 2. **The CONTROL can fail.** Mutations went through `scripts/ablation-replace.mjs`; each anchor hit 1 → 0 and was restored to the HEAD blob. - Block arm, `slice(open + 1, …)` changed to `slice(open, …)`: `15 failed`. That is all 9 block-bodied CONTROL cases plus the 6 defect pins; the 3 expression cases stay green. - Expression arm, `body.pos … trimStart()` changed to `body.getStart(sf)`: exactly `1 failed`, the leading-comment case. The unit tests import `../src/utils/extract-hook-body.js`, and the dogfood door imports `../../../cli/src/utils/lower-callables.js`. So no `dist/` sat between a mutation and its run. ## CONTROL over a corpus (one-off scratch instrument, not committed) Both peels were run over every function text in the esbuild-transformed (`es2022`, `esm`) `.ts` / `.mts` sources of `packages/cli`, `examples` and `packages/qa`. The tree was `b4acbe447f` (BASE plus the fix): 1180 files, 21,262 functions. - **20,739 identical.** - **0** that BASE read and the new peel refuses. - **121** that BASE refused (an unbalanced `{` in a string) now lower. - **402 differ, and every one is a BASE misread:** - 185: an inner block taken as the body (inner arrow, parameter pattern or default); - 9: a block outside the body (a pattern or default parameter); - 98: an inner arrow read as the handler; - 110: a body cut short at a brace in a string, template, regex or comment. - **10 corrected bodies fail my probe for reasons outside the peel.** The probe compiles under `(async (ctx) => …)`: 6 of them declare their own `const ctx`, and 4 use `import.meta`. In each one BASE had peeled an inner function instead. **Example apps**, with configs loaded through `loadConfig` as `os build` loads them: - `app-crm`: 2 of 2 callables identical. - `app-todo`: 3 of 3 identical. - `app-multi-package`: no inline callables. - `app-showcase`: **NOT MEASURED** (its connector dists were not built at that point). Its source text is covered by the corpus above. ## Tests and gates, at `ba8ac397a6` (after merging `origin/main` at `0253192f76`) **Tests and typechecks:** - `pnpm --filter @objectstack/cli exec vitest run --project unit --maxWorkers=2`: 283 files, 4260 tests passed, `VERDICT command-exit 0`. - The `integration` tier is declared to CI. The diff touches no integration-tier file and no spawn entry point. - `pnpm --filter @objectstack/cli typecheck`: exit 0, `check:test-typecheck: OK`. - `tsc --listFilesOnly` puts `test/extract-hook-body.test.ts` in `tsconfig.test.json`'s program and `src/utils/extract-hook-body.ts` in `tsconfig.json`'s. - `pnpm --filter @objectstack/dogfood typecheck`: exit 0. Its program includes the door file. - `pnpm --filter @objectstack/dogfood exec vitest run --maxWorkers=2 test/lowered-body-door.dogfood.test.ts`: 20 passed. **Gates:** - The dispatch's 70-command gate list: every command exit 0. - `check:dual-build-cjs-loads` first exited 3 with PREREQUISITE NOT MET: 8 packages outside this card's closure had no `dist/`. - Re-run after building those 8: exit 0, `✓ check:dual-build-cjs-loads — 107 published require entry point(s) across 66 package(s) load`. - `dispatch-gates --ran`, given a record with an exit code on every line: `✓ dispatch-gates --ran: 69 derived famil(ies) accounted for — 69 run, 0 NOT-MEASURED (a DERIVED zero — all 69 recorded an exit code and none of them is 3)`. - Re-deriving on the final tree added no command. It dropped `pnpm check:dispatcher-error-vocabulary`, which was run anyway: exit 0. **Lint, as a proven narrowing rather than a skipped run:** - `eslint --no-inline-config --format json` over the 3 touched TS files at `ba8ac397a6`: 3 files in the output, 0 errors, 0 warnings, none reported as ignored. - **Population, read from the config.** `eslint.config.mjs`'s `**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` object matches all three. The changeset `.md` matches no `files` glob. - **Invariance.** The config enables no type-aware linting (no `parserOptions.project`, no `projectService`) and loads no plugin with cross-file rules. Its only file reads are its own baseline JSONs, which this diff does not touch. So this diff cannot move any untouched file's verdict. **Size:** 4 files, +362 / −96. ## Acceptance notes - **One refusal is folded into another; none is reworded.** The hook face's separate "could not read the parameter list of …" refusal is gone. The body and the parameter list now come from one parse, so a handler that parse cannot read meets the existing "could not parse the body of …" refusal instead: same `unparseable` kind, same remedy clause. No measured function reached the removed refusal (0 of 21,262). - **An observation, not filed.** A zero-parameter hook handler that declares its own `let` / `const ctx` lowers to a body that redeclares the wrapper's `ctx`, which is a SyntaxError: the rebind's shadowing check runs only when there is a parameter. I read this off the differential and did not measure it at a door, and I found no real producer. - **The sibling.** #22853 had not landed when this was opened, and none of its files are touched. ## Out of scope, reported to the seat (not addressed here) An action `target` whose parameters are not named `(input, ctx)`, for example `async (params, context) => …`, lowers at `objectstack build` exit 0 to a body that throws `ReferenceError: params is not defined`. That is under the runtime's action wrapper, `(async (input, ctx) => { … })` in `packages/runtime/src/sandbox/quickjs-runner.ts`. The `(input, ctx)` control runs. This is the action-face sibling of the hook-face parameter rebind; nothing here changes it. --- _Generated by [Claude Code](https://claude.ai/code/session_01VoSxBQujKLZKPwK2u5ehQ6)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 72b26ed commit 40bc51f

4 files changed

Lines changed: 362 additions & 96 deletions

File tree

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
fix(cli): `objectstack build` reads a lowered hook or action body off the handler's parse, so a `}` in a string, a statement-level inner arrow or a `function` handler with a pattern parameter no longer ships a body that is not the handler's
6+
7+
Clause-②: no
8+
9+
When `objectstack build` lowers an inline handler to a metadata `body`, it ships the handler's statements as `body.source`. It found those statements by counting `{` / `}` and `(` / `)` through the handler's text, with no idea of what a string, a template literal, a regular expression or a comment is, and it took the first parenthesised `=>` anywhere in the text as the handler's header. So ordinary handler code shipped a body that is not the handler's, and the build still exited 0. Measured through a real `objectstack build`:
10+
11+
- **A `}` inside a string, a template literal, a regex or a comment** cut the body off at that brace. `ctx.input.note = 'a}b'` shipped as `ctx.input.note = "a`, a `SyntaxError` on the body's first run.
12+
- **A statement-level inner arrow** in a `function` handler (`const f = (a) => a + '!'`) was read as the handler, and the body was built around it, which is also a `SyntaxError`. With a block-bodied inner arrow, the inner arrow's own statements shipped: a body that runs and does none of the handler's work.
13+
- **A `function` handler whose parameter is a pattern** (`async function ({ input }) { … }`) shipped the pattern as its body. After the parameter rebind that is `var { input } = ctx; input`. It throws nothing and stores nothing, where the in-process handler stores the row.
14+
- **A `{` inside a string** left the count unbalanced, so the handler was refused as unparseable and bundled into the runtime module, with a warning telling the author to rewrite a handler that had nothing wrong with it.
15+
16+
The only signal for the first three was `os lint`'s `hook-body-source-unparseable` warning on the lowered body. It pointed the author at a syntax error their handler does not have, and the pattern-parameter case raised no signal at all.
17+
18+
The body is now read off the TypeScript parse of the handler: the statements between its block's own braces, or the expression of an implicit-return arrow. This is the same parse the parameter rebind and the free-identifier check already read. Every shape above now lowers to the handler's own body and runs as the handler does. The `{`-in-a-string handler is lowered instead of bundled. `hook-body-source-unparseable` no longer fires for any of them.
19+
20+
This applies to both faces, a hook's `handler` and an action's `target`. The action face still gets no parameter rebind.
21+
22+
**Unchanged.** A handler the old count read correctly lowers to the same bytes, comments and layout included. This was measured over 21,262 function texts from `packages/cli`, `examples` and `packages/qa`, and over the callables the `app-crm` and `app-todo` configs load. No refusal message is reworded. The hook face's separate "could not read the parameter list" refusal is gone: the body and the parameter list are now read off one parse, so a handler that parse cannot read meets the existing "could not parse the body" refusal instead. It is the same kind (`unparseable`) with the same remedy, and no measured handler reached the old one. `extractHookBody`'s published signature and refusal kinds are the same. No spec key, export, type or stored shape moves.

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

Lines changed: 68 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,13 @@
109109
* The action face is untouched by this: an action body runs as
110110
* `(async (input, ctx) => { SOURCE })`, a different binding list, and its
111111
* lowering is exactly what it was ({@link LoweringFace}).
112+
*
113+
* The body span (#22860). The statements are read off the same TypeScript
114+
* parse of the handler as the parameter list — the block's own braces, or the
115+
* arrow's expression — and never by counting braces through the text, which
116+
* shipped a cut-off or wrong body at build exit 0 for a `}` in a string,
117+
* template, regex or comment, a statement-level inner arrow, and a `function`
118+
* handler with a pattern parameter. See {@link peelToBlockBody}. Both faces.
112119
*/
113120

114121
import { ts } from 'ts-morph';
@@ -580,8 +587,11 @@ export function extractLoweredBody(
580587

581588
// Strip leading function/arrow header and trailing closing brace so the
582589
// result is a pure block body suitable for `new Function('ctx', body)`.
583-
const block = peelToBlockBody(raw);
584-
if (!block) {
590+
// [#22860] Read off the TypeScript parse of the handler, the same node the
591+
// parameter rebind below reads, never off a scan of the text.
592+
const parsed = parseFunction(raw);
593+
const block = parsed ? peelToBlockBody(parsed) : null;
594+
if (!parsed || !block) {
585595
throw new HookBodyExtractionError(
586596
'unparseable',
587597
originLabel,
@@ -596,7 +606,7 @@ export function extractLoweredBody(
596606
// check below reads the REBOUND source, so a member destructured in the
597607
// parameter list (`({ input, submitted })`) meets the same refusal as one
598608
// destructured in the body.
599-
const rebind = face === 'hook' ? planHookParameterRebind(raw, originLabel) : NO_REBIND;
609+
const rebind = face === 'hook' ? planHookParameterRebind(parsed, raw, originLabel) : NO_REBIND;
600610
const source = rebind.prologue === ''
601611
? block.source
602612
: rebind.prologue + (/^\s/.test(block.source) ? '' : ' ') + block.source;
@@ -769,9 +779,11 @@ function parameterRefusal(originLabel: string, raw: string, reason: string): Hoo
769779
* context. So a handler's ONE parameter is the context under the author's
770780
* name, and re-emitting it as `var <param> = ctx;` ahead of the body runs the
771781
* author's code as written. The parameter is read off the TypeScript parse of
772-
* the handler that {@link detectFreeIdentifiers} already performs (one parse,
773-
* so the free-identifier verdict and this rebind agree on what the parameter
774-
* binds), never off the regex peel.
782+
* the handler ({@link parseFunction}, the function-node reading that
783+
* {@link detectFreeIdentifiers} performs, so the free-identifier verdict and
784+
* this rebind agree on what the parameter binds) — the same node
785+
* {@link peelToBlockBody} reads the body off (#22860), so the prologue and the
786+
* statements it precedes come from one reading of the handler.
775787
*
776788
* `var`, not `const`: a parameter is a mutable, function-scoped binding. A
777789
* handler may reassign it (`input = { ...input }` off a `({ input })`), which a
@@ -788,16 +800,11 @@ function parameterRefusal(originLabel: string, raw: string, reason: string): Hoo
788800
* inference cannot follow. A handler whose parameter is `ctx`, or that takes
789801
* none, gets no prologue: it lowers byte-identically to before.
790802
*/
791-
function planHookParameterRebind(raw: string, originLabel: string): ParameterRebind {
792-
const fn = parseFunction(raw);
793-
if (!fn) {
794-
throw new HookBodyExtractionError(
795-
'unparseable',
796-
originLabel,
797-
`[hook-body-extract] could not read the parameter list of ${originLabel}; ` +
798-
`please rewrite the handler as a single arrow function or named function expression`,
799-
);
800-
}
803+
function planHookParameterRebind(
804+
fn: ts.FunctionLikeDeclarationBase,
805+
raw: string,
806+
originLabel: string,
807+
): ParameterRebind {
801808
const params = fn.parameters;
802809
if (params.length === 0) return NO_REBIND;
803810

@@ -977,82 +984,51 @@ interface PeeledBody {
977984
}
978985

979986
/**
980-
* Remove the parameter list and outermost braces from a function string,
981-
* yielding the bare statements (or expression for shorthand arrows).
987+
* The handler's body as source text: the statements between its block's own
988+
* braces, or, for an implicit-return arrow, its expression wrapped as
989+
* `return (…);`.
990+
*
991+
* Read off the handler's function node — {@link parseFunction}, the parse the
992+
* parameter rebind and {@link detectFreeIdentifiers} read — never off a scan
993+
* of the text (#22860). The scan this replaces counted `{`/`}` and `(`/`)` with
994+
* no idea of what a string, a template, a regex or a comment is, and took the
995+
* first parenthesised `=>` anywhere as the handler's header. So ordinary code
996+
* shipped a body that is not the handler's, at build exit 0:
997+
*
998+
* - a `}` in a string, a template, a regex or a comment closed the body
999+
* early — `ctx.input.note = 'a}b'` shipped as `ctx.input.note = 'a`, a
1000+
* SyntaxError on the body's first run;
1001+
* - a statement-level inner arrow (`const f = (a) => a + '!'`) in a
1002+
* `function` handler was read as the handler, and the body built around
1003+
* it — a SyntaxError;
1004+
* - a `function` handler whose parameter is a pattern (`function ({ input })
1005+
* { … }`) shipped the PATTERN as its body — after the rebind,
1006+
* `var { input } = ctx; input`, which runs, throws nothing and does none
1007+
* of the handler's work;
1008+
* - a `{` in a string left the scan unbalanced, so the handler was refused as
1009+
* unparseable and bundled.
1010+
*
1011+
* The parser knows each of those tokens, so the class closes rather than the
1012+
* members. On every shape the scan read correctly the text is the same bytes:
1013+
* a block's inner text is everything between its braces, comments and layout
1014+
* included, and an expression is taken from the end of `=>` with its leading
1015+
* whitespace trimmed, as the scan took it.
1016+
*
1017+
* Returns `null` when the node has no body, or a block whose braces the parse
1018+
* had to invent (`String(fn)` of a real function never yields one) — the
1019+
* caller refuses that as `unparseable`.
9821020
*/
983-
function peelToBlockBody(raw: string): PeeledBody | null {
984-
// Try arrow forms first since they're the dominant authoring style.
985-
// Match the parameter list followed by `=>` and either `{...}` or expr.
986-
// We rely on a manual brace scan rather than a single regex so braces
987-
// inside string/template literals don't confuse us.
988-
const arrowIdx = findTopLevelArrow(raw);
989-
if (arrowIdx >= 0) {
990-
const after = raw.slice(arrowIdx + 2).trimStart();
991-
if (after.startsWith('{')) {
992-
const body = sliceBalanced(after, '{', '}');
993-
if (body) return { source: body.inner, isExpression: false };
994-
} else {
995-
// Implicit-return arrow — wrap as `return ...;`
996-
const expr = after.replace(/[;\s]+$/g, '');
997-
return { source: `return (${expr});`, isExpression: true };
998-
}
999-
}
1000-
1001-
// function () { ... } / async function () { ... }
1002-
const fnIdx = raw.search(/\bfunction\b/);
1003-
if (fnIdx >= 0) {
1004-
const braceIdx = raw.indexOf('{', fnIdx);
1005-
if (braceIdx > 0) {
1006-
const body = sliceBalanced(raw.slice(braceIdx), '{', '}');
1007-
if (body) return { source: body.inner, isExpression: false };
1008-
}
1009-
}
1010-
1011-
// Method shorthand inside an object literal: `name(ctx) { ... }`.
1012-
const braceIdx = raw.indexOf('{');
1013-
if (braceIdx > 0) {
1014-
const body = sliceBalanced(raw.slice(braceIdx), '{', '}');
1015-
if (body) return { source: body.inner, isExpression: false };
1016-
}
1017-
return null;
1018-
}
1019-
1020-
function findTopLevelArrow(s: string): number {
1021-
// Skip past balanced parameter list `(...)` then expect `=>`.
1022-
// We scan looking for `(` not preceded by an identifier char and
1023-
// immediately matched by a balanced `)` then optional whitespace then `=>`.
1024-
let i = 0;
1025-
const len = s.length;
1026-
while (i < len) {
1027-
if (s[i] === '(') {
1028-
const closing = matchBalancedIndex(s, i, '(', ')');
1029-
if (closing < 0) return -1;
1030-
let j = closing + 1;
1031-
while (j < len && /\s/.test(s[j])) j++;
1032-
if (s[j] === '=' && s[j + 1] === '>') return j;
1033-
i = closing + 1;
1034-
continue;
1035-
}
1036-
i++;
1037-
}
1038-
return -1;
1039-
}
1040-
1041-
function matchBalancedIndex(s: string, start: number, open: string, close: string): number {
1042-
let depth = 0;
1043-
for (let i = start; i < s.length; i++) {
1044-
const c = s[i];
1045-
if (c === open) depth++;
1046-
else if (c === close) {
1047-
depth--;
1048-
if (depth === 0) return i;
1049-
}
1021+
function peelToBlockBody(fn: ts.FunctionLikeDeclarationBase): PeeledBody | null {
1022+
const body = fn.body;
1023+
if (!body) return null;
1024+
const sf = body.getSourceFile();
1025+
const text = sf.text;
1026+
if (ts.isBlock(body)) {
1027+
const open = body.getStart(sf);
1028+
if (text[open] !== '{' || text[body.end - 1] !== '}') return null;
1029+
return { source: text.slice(open + 1, body.end - 1), isExpression: false };
10501030
}
1051-
return -1;
1052-
}
1053-
1054-
function sliceBalanced(s: string, open: string, close: string): { inner: string } | null {
1055-
const end = matchBalancedIndex(s, 0, open, close);
1056-
if (end < 0) return null;
1057-
return { inner: s.slice(1, end) };
1031+
// Implicit-return arrow — wrap as `return (...);`. `body.pos` is the end of
1032+
// the `=>` token, so leading comments survive the way the scan kept them.
1033+
return { source: `return (${text.slice(body.pos, body.end).trimStart()});`, isExpression: true };
10581034
}

0 commit comments

Comments
 (0)